tracing: Forbid system tracing while local startup tracing is active

Makes ProducerClient::IsTracingActive() also consider active local
startup tracing sessions, and adds a test that verifies that system
tracing doesn't start until after local startup tracing is complete.

Bug: 1058461
Change-Id: I59a488e663ad9ac2aaa41ae4fef8990f3268b695
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2087410
Commit-Queue: Eric Seckler <eseckler@chromium.org>
Reviewed-by: Stephen Nusko <nuskos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#747257}

content/browser/tracing/background_tracing_active_scenario.cc

@@ -82,7 +82,8 @@ class PerfettoTracingSession
         raw_data_(std::make_unique<std::string>()) {
 #if !defined(OS_ANDROID)
     // TODO(crbug.com/941318): Re-enable startup tracing for Android once all
-    // Perfetto-related deadlocks are resolved.
+    // Perfetto-related deadlocks are resolved and we also handle concurrent
+    // system tracing for startup tracing.
     if (!TracingControllerImpl::GetInstance()->IsTracing()) {
       did_setup_startup_tracing_ = tracing::SetupStartupTracingForProcess(
           /*privacy_filtering_enabled=*/true,
@@ -199,7 +200,8 @@ class LegacyTracingSession
       : parent_scenario_(parent_scenario) {
 #if !defined(OS_ANDROID)
     // TODO(crbug.com/941318): Re-enable startup tracing for Android once all
-    // Perfetto-related deadlocks are resolved.
+    // Perfetto-related deadlocks are resolved and we also handle concurrent
+    // system tracing for startup tracing.
     if (!TracingControllerImpl::GetInstance()->IsTracing()) {
       did_setup_startup_tracing_ = tracing::SetupStartupTracingForProcess(
           /*privacy_filtering_enabled=*/false,

services/tracing/perfetto/system_perfetto_unittest.cc

@@ -26,6 +26,7 @@
 #include "services/tracing/perfetto/test_utils.h"
 #include "services/tracing/public/cpp/perfetto/dummy_producer.h"
 #include "services/tracing/public/cpp/perfetto/producer_client.h"
+#include "services/tracing/public/cpp/trace_startup.h"
 #include "services/tracing/public/cpp/tracing_features.h"
 #include "testing/gtest/include/gtest/gtest-death-test.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -698,6 +699,108 @@ TEST_F(SystemPerfettoTest, MultipleSystemAndLocalSourcesLocalFirst) {
   PerfettoProducer::DeleteSoonForTesting(std::move(system_producer));
 }
 
+// Attempts to start a system trace while a local startup trace is active. The
+// system trace should only be started after the local trace is completed.
+TEST_F(SystemPerfettoTest, SystemTraceWhileLocalStartupTracing) {
+  // We're using mojom::kTraceEventDataSourceName for the local producer to
+  // emulate starting the real TraceEventDataSource which owns startup tracing.
+  auto mock_trace_event_ds = TestDataSource::CreateAndRegisterDataSource(
+      mojom::kTraceEventDataSourceName, 2);
+
+  // Wait for data source to register.
+  RunUntilIdle();
+
+  auto system_service = CreateMockSystemService();
+
+  // Create local producer.
+  base::RunLoop local_data_source_enabled_runloop;
+  base::RunLoop local_data_source_disabled_runloop;
+  auto local_producer = std::make_unique<MockProducerClient>(
+      /* num_data_sources = */ 1,
+      local_data_source_enabled_runloop.QuitClosure(),
+      local_data_source_disabled_runloop.QuitClosure());
+
+  // Setup startup tracing for local producer.
+  CHECK(local_producer->SetupStartupTracing());
+
+  // Attempt to start a system tracing session. Because startup tracing is
+  // already active, the system producer shouldn't activate yet.
+  base::RunLoop system_no_more_packets_runloop;
+  MockConsumer system_consumer(
+      {kPerfettoTestDataSourceName}, system_service->GetService(),
+      [&system_no_more_packets_runloop](bool has_more) {
+        if (!has_more) {
+          system_no_more_packets_runloop.Quit();
+        }
+      });
+
+  base::RunLoop system_data_source_enabled_runloop;
+  base::RunLoop system_data_source_disabled_runloop;
+  auto system_producer = CreateMockPosixSystemProducer(
+      system_service.get(),
+      /* num_data_sources = */ 1, &system_data_source_enabled_runloop,
+      &system_data_source_disabled_runloop);
+
+  RunUntilIdle();
+
+  // Now connect the local ProducerHost & consumer, taking over startup tracing.
+  base::RunLoop local_no_more_packets_runloop;
+  std::unique_ptr<MockConsumer> local_consumer(new MockConsumer(
+      {mojom::kTraceEventDataSourceName}, local_service()->GetService(),
+      [&local_no_more_packets_runloop](bool has_more) {
+        if (!has_more) {
+          local_no_more_packets_runloop.Quit();
+        }
+      }));
+  auto local_producer_host = std::make_unique<MockProducerHost>(
+      kPerfettoProducerName, mojom::kTraceEventDataSourceName, local_service(),
+      local_producer.get());
+  local_data_source_enabled_runloop.Run();
+  local_consumer->WaitForAllDataSourcesStarted();
+
+  // Ensures that the Trace data gets written and committed.
+  RunUntilIdle();
+
+  // Stop local session. This should reconnect the system producer & start the
+  // system session.
+  local_consumer->StopTracing();
+  local_data_source_disabled_runloop.Run();
+  local_consumer->WaitForAllDataSourcesStopped();
+  local_no_more_packets_runloop.Run();
+  // Local consumer should have received 2 packets from |mock_trace_event_ds|.
+  EXPECT_EQ(2u, local_consumer->received_test_packets());
+
+  // Wait for system producer to get enabled. Wait have to wait for the
+  // individual data source to write data, because it might be queued until the
+  // local trace has fully finished.
+  base::RunLoop system_data_source_wrote_data_runloop;
+  data_sources_[0]->set_start_tracing_callback(
+      system_data_source_wrote_data_runloop.QuitClosure());
+
+  system_data_source_enabled_runloop.Run();
+  system_data_source_wrote_data_runloop.Run();
+  system_consumer.WaitForAllDataSourcesStarted();
+
+  // Stop the trace on the correct sequence to ensure everything is committed.
+  base::RunLoop stop_tracing;
+  PerfettoTracedProcess::GetTaskRunner()->PostTask(
+      [&system_consumer, &stop_tracing]() {
+        system_consumer.StopTracing();
+        stop_tracing.Quit();
+      });
+  stop_tracing.Run();
+
+  system_data_source_disabled_runloop.Run();
+  system_consumer.WaitForAllDataSourcesStopped();
+  system_no_more_packets_runloop.Run();
+
+  // Local consumer should have received 1 packet from the |data_sources_[0]|.
+  EXPECT_EQ(1u, system_consumer.received_test_packets());
+
+  PerfettoProducer::DeleteSoonForTesting(std::move(local_producer));
+  PerfettoProducer::DeleteSoonForTesting(std::move(system_producer));
+}
+
 #if defined(OS_ANDROID)
 TEST_F(SystemPerfettoTest, SystemToLowAPILevel) {
   if (base::android::BuildInfo::GetInstance()->sdk_int() >=

services/tracing/public/cpp/perfetto/producer_client.cc

@@ -45,7 +45,7 @@ void ProducerClient::Connect(
 
   mojo::ScopedSharedBufferHandle shm;
   {
-    base::AutoLock lock(shared_memory_lock_);
+    base::AutoLock lock(lock_);
     shm = shared_memory_->Clone();
   }
   CHECK(shm.is_valid());
@@ -73,7 +73,7 @@ void ProducerClient::BindInProcessSharedMemoryArbiter(
 
   perfetto::SharedMemoryArbiter* arbiter;
   {
-    base::AutoLock lock(shared_memory_lock_);
+    base::AutoLock lock(lock_);
     // Shared memory should have been created before connecting to ProducerHost.
     DCHECK(shared_memory_arbiter_);
     // |shared_memory_arbiter_| is never destroyed, thus OK to call
@@ -84,7 +84,12 @@ void ProducerClient::BindInProcessSharedMemoryArbiter(
 }
 
 bool ProducerClient::SetupStartupTracing() {
-  return InitSharedMemoryIfNeeded();
+  base::AutoLock lock(lock_);
+  if (!InitSharedMemoryIfNeededLocked()) {
+    return false;
+  }
+  startup_tracing_active_ = true;
+  return true;
 }
 
 void ProducerClient::BindStartupTargetBuffer(
@@ -106,7 +111,7 @@ void ProducerClient::BindStartupTargetBuffer(
 }
 
 perfetto::SharedMemoryArbiter* ProducerClient::MaybeSharedMemoryArbiter() {
-  base::AutoLock lock(shared_memory_lock_);
+  base::AutoLock lock(lock_);
   // |shared_memory_arbiter_| is never destroyed, thus OK to return a
   // reference here.
   return shared_memory_arbiter_.get();
@@ -141,7 +146,8 @@ void ProducerClient::NewDataSourceAdded(
 
 bool ProducerClient::IsTracingActive() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return data_sources_tracing_ > 0;
+  base::AutoLock lock(lock_);
+  return data_sources_tracing_ > 0 || startup_tracing_active_;
 }
 
 void ProducerClient::OnTracingStart() {
@@ -153,7 +159,7 @@ void ProducerClient::OnTracingStart() {
   if (!in_process_arbiter_task_runner_) {
     perfetto::SharedMemoryArbiter* arbiter;
     {
-      base::AutoLock lock(shared_memory_lock_);
+      base::AutoLock lock(lock_);
       // |shared_memory_arbiter_| is never destroyed, thus OK to call
       // BindToProducerEndpoint() without holding the lock.
       arbiter = shared_memory_arbiter_.get();
@@ -186,9 +192,17 @@ void ProducerClient::StartDataSource(
                 if (!weak_ptr) {
                   return;
                 }
+
                 DCHECK_CALLED_ON_VALID_SEQUENCE(weak_ptr->sequence_checker_);
                 data_source->StartTracingWithID(id, weak_ptr.get(),
                                                 data_source_config);
+
+                // Starting TraceEventDataSource takes over startup tracing.
+                if (data_source->name() == mojom::kTraceEventDataSourceName) {
+                  base::AutoLock lock(weak_ptr->lock_);
+                  weak_ptr->startup_tracing_active_ = false;
+                }
+
                 // TODO(eseckler): Consider plumbing this callback through
                 // |data_source|.
                 std::move(callback).Run();
@@ -346,14 +360,18 @@ void ProducerClient::ResetSequenceForTesting() {
 }
 
 perfetto::SharedMemory* ProducerClient::shared_memory_for_testing() {
-  base::AutoLock lock(shared_memory_lock_);
+  base::AutoLock lock(lock_);
   // |shared_memory_| is never destroyed except in tests, thus OK to return a
   // reference here.
   return shared_memory_.get();
 }
 
 bool ProducerClient::InitSharedMemoryIfNeeded() {
-  base::AutoLock lock(shared_memory_lock_);
+  base::AutoLock lock(lock_);
+  return InitSharedMemoryIfNeededLocked();
+}
+
+bool ProducerClient::InitSharedMemoryIfNeededLocked() {
   if (shared_memory_) {
     return true;
   }

services/tracing/public/cpp/perfetto/producer_client.h

@@ -121,6 +121,8 @@ class COMPONENT_EXPORT(TRACING_CPP) ProducerClient
   // Called after a data source has completed a flush.
   void NotifyDataSourceFlushComplete(perfetto::FlushRequestID id);
 
+  bool InitSharedMemoryIfNeededLocked() EXCLUSIVE_LOCKS_REQUIRED(lock_);
+
   uint32_t data_sources_tracing_ = 0;
   std::unique_ptr<mojo::Receiver<mojom::ProducerClient>> receiver_;
   mojo::Remote<mojom::ProducerHost> producer_host_;
@@ -129,14 +131,15 @@ class COMPONENT_EXPORT(TRACING_CPP) ProducerClient
   // replies we're still waiting for.
   std::pair<uint64_t, size_t> pending_replies_for_latest_flush_;
 
-  // Guards initialization of |shared_memory_| and |shared_memory_arbiter_|.
-  // TODO(eseckler): Consider accessing these without locks after setup was
-  // completed, since we never destroy or unset them.
-  base::Lock shared_memory_lock_;
-  std::unique_ptr<MojoSharedMemory> shared_memory_
-      GUARDED_BY(shared_memory_lock_);
+  // Guards initialization of SMB and startup tracing.
+  base::Lock lock_;
+  // TODO(eseckler): Consider accessing |shared_memory_| and
+  // |shared_memory_arbiter_| without locks after setup was completed, since we
+  // never destroy or unset them.
+  std::unique_ptr<MojoSharedMemory> shared_memory_ GUARDED_BY(lock_);
   std::unique_ptr<perfetto::SharedMemoryArbiter> shared_memory_arbiter_
-      GUARDED_BY(shared_memory_lock_);
+      GUARDED_BY(lock_);
+  bool startup_tracing_active_ GUARDED_BY(lock_) = false;
 
   SEQUENCE_CHECKER(sequence_checker_);