service: fix --save-for-bugreport am: 341d3b9c5f Original change: https://googleplex-android-review.googlesource.com/c/platform/external/perfetto/+/15059760 Change-Id: I2f17e99093db9b5bab5621dba2de92ac39ad9f27
diff --git a/include/perfetto/ext/tracing/core/tracing_service.h b/include/perfetto/ext/tracing/core/tracing_service.h index 20382a1..db54dbb 100644 --- a/include/perfetto/ext/tracing/core/tracing_service.h +++ b/include/perfetto/ext/tracing/core/tracing_service.h
@@ -42,7 +42,7 @@ class TraceWriter; // Exposed for testing. -extern const char* kBugreportTracePath; +std::string GetBugreportPath(); // TODO: for the moment this assumes that all the calls happen on the same // thread/sequence. Not sure this will be the case long term in Chrome.
diff --git a/protos/perfetto/ipc/consumer_port.proto b/protos/perfetto/ipc/consumer_port.proto index 41242e7..9f6d533 100644 --- a/protos/perfetto/ipc/consumer_port.proto +++ b/protos/perfetto/ipc/consumer_port.proto
@@ -261,7 +261,7 @@ // or something failed. message SaveTraceForBugreportResponse { // If true, an eligible the trace was saved into a known location (on Android - // /data/misc/perfetto-traces, see kBugreportTracePath). + // /data/misc/perfetto-traces, see GetBugreportPath()). // If false no trace with bugreport_score > 0 was found or an error occurred. // see |msg| in that case for details about the failure. optional bool success = 1;
diff --git a/src/tracing/core/tracing_service_impl.cc b/src/tracing/core/tracing_service_impl.cc index 472bb31..1764a0b 100644 --- a/src/tracing/core/tracing_service_impl.cc +++ b/src/tracing/core/tracing_service_impl.cc
@@ -56,6 +56,7 @@ #include "perfetto/ext/base/file_utils.h" #include "perfetto/ext/base/metatrace.h" #include "perfetto/ext/base/string_utils.h" +#include "perfetto/ext/base/temp_file.h" #include "perfetto/ext/base/utils.h" #include "perfetto/ext/base/watchdog.h" #include "perfetto/ext/tracing/core/basic_types.h" @@ -100,16 +101,11 @@ #if PERFETTO_BUILDFLAG(PERFETTO_OS_ANDROID) && \ PERFETTO_BUILDFLAG(PERFETTO_ANDROID_BUILD) -// This is the only SELinux approved dir for trace files that are created +// These are the only SELinux approved dir for trace files that are created // directly by traced. const char* kTraceDirBasePath = "/data/misc/perfetto-traces/"; -#endif - -#if PERFETTO_BUILDFLAG(PERFETTO_OS_ANDROID) -const char* kBugreportTracePath = +const char* kAndroidProductionBugreportTracePath = "/data/misc/perfetto-traces/bugreport/systrace.pftrace"; -#else -const char* kBugreportTracePath = "/tmp/bugreport.pftrace"; #endif namespace { @@ -273,7 +269,7 @@ } std::string GetBugreportTmpPath() { - return std::string(kBugreportTracePath) + ".tmp"; + return GetBugreportPath() + ".tmp"; } bool ShouldLogEvent(const TraceConfig& cfg) { @@ -299,6 +295,16 @@ constexpr uint32_t TracingServiceImpl::kDataSourceStopTimeoutMs; constexpr uint8_t TracingServiceImpl::kSyncMarker[]; +std::string GetBugreportPath() { +#if PERFETTO_BUILDFLAG(PERFETTO_OS_ANDROID) && \ + PERFETTO_BUILDFLAG(PERFETTO_ANDROID_BUILD) + return kAndroidProductionBugreportTracePath; +#else + // Only for tests, SaveTraceForBugreport is not used on other OSes. + return base::GetSysTempDir() + "/bugreport.pftrace"; +#endif +} + // static std::unique_ptr<TracingService> TracingService::CreateInstance( std::unique_ptr<SharedMemory::Factory> shm_factory, @@ -1926,7 +1932,8 @@ // packets like TraceConfig and Clock snapshots. So we bail out early and let // the consumer know there is no data. if (!tracing_session->config.trigger_config().triggers().empty() && - tracing_session->received_triggers.empty()) { + tracing_session->received_triggers.empty() && + !tracing_session->seized_for_bugreport) { PERFETTO_DLOG( "ReadBuffers(): tracing session has not received a trigger yet."); return false; @@ -3528,12 +3535,12 @@ SaveTraceForBugreportCallback consumer_callback) { PERFETTO_DCHECK_THREAD(thread_checker_); auto on_complete_callback = [consumer_callback] { - if (rename(GetBugreportTmpPath().c_str(), kBugreportTracePath)) { + if (rename(GetBugreportTmpPath().c_str(), GetBugreportPath().c_str())) { consumer_callback(false, "rename(" + GetBugreportTmpPath() + ", " + - kBugreportTracePath + ") failed (" + + GetBugreportPath() + ") failed (" + strerror(errno) + ")"); } else { - consumer_callback(true, kBugreportTracePath); + consumer_callback(true, GetBugreportPath()); } }; if (!service_->MaybeSaveTraceForBugreport(std::move(on_complete_callback))) {
diff --git a/test/end_to_end_integrationtest.cc b/test/end_to_end_integrationtest.cc index 64a7c80..6333641 100644 --- a/test/end_to_end_integrationtest.cc +++ b/test/end_to_end_integrationtest.cc
@@ -270,7 +270,7 @@ // Read the trace written in the fixed location (/data/misc/perfetto-traces/ // on Android, /tmp/ on Linux/Mac) and make sure it has the right contents. std::string trace_str; - base::ReadFile(kBugreportTracePath, &trace_str); + base::ReadFile(GetBugreportPath(), &trace_str); ASSERT_FALSE(trace_str.empty()); protos::gen::Trace trace; ASSERT_TRUE(trace.ParseFromString(trace_str)); @@ -1076,6 +1076,47 @@ } } +// Tests that SaveTraceForBugreport() works also if the trace has triggers +// defined and those triggers have not been hit. This is a regression test for +// b/188008375 . +TEST_F(PerfettoTest, SaveForBugreport_Triggers) { + base::TestTaskRunner task_runner; + + TestHelper helper(&task_runner); + helper.StartServiceIfRequired(); + helper.ConnectFakeProducer(); + helper.ConnectConsumer(); + helper.WaitForConsumerConnect(); + + TraceConfig trace_config; + SetTraceConfigForBugreportTest(&trace_config); + trace_config.set_duration_ms(0); // set_trigger_timeout_ms is used instead. + auto* trigger_config = trace_config.mutable_trigger_config(); + trigger_config->set_trigger_timeout_ms(8.64e+7); + trigger_config->set_trigger_mode(TraceConfig::TriggerConfig::STOP_TRACING); + auto* trigger = trigger_config->add_triggers(); + trigger->set_name("trigger_name"); + trigger->set_stop_delay_ms(1); + + helper.StartTracing(trace_config); + helper.WaitForProducerEnabled(); + + EXPECT_TRUE(helper.SaveTraceForBugreportAndWait()); + helper.WaitForTracingDisabled(); + + VerifyBugreportTraceContents(); + + // Now read the original trace. + helper.ReadData(); + helper.WaitForReadData(); + const auto& packets = helper.full_trace(); + ASSERT_EQ(packets.size(), 1u); + for (const auto& p : packets) { + ASSERT_TRUE(p.has_service_event()); + ASSERT_TRUE(p.service_event().seized_for_bugreport()); + } +} + // Disable cmdline tests on sanitizets because they use fork() and that messes // up leak / races detections, which has been fixed only recently (see // https://github.com/google/sanitizers/issues/836 ).