Revert "Perfetto: Fix ProducerClient threading/sequencing issues"

This reverts commit 2c2411fb.

Reason for revert: Suspected cause of Issue 846609

Original change's description:
> Perfetto: Fix ProducerClient threading/sequencing issues
> 
> * TraceWriters will no longer be destroyed when their TLS slot is cleaned
>   up on thread shutdown as this can trigger a CommitData() call from
>   Perfetto and cause asserts when Mojo does a TaskRunnerHandle::Get()
>   for a PostTask; instead we leave the deletion to the taskrunner
>   Perfetto is running on (if it's getting shut down, the task should
>   get dropped).
> * Run ProducerClients on their own sequence.
> 
> Bug: 844379
> Change-Id: Ie45b36a4e463674122555dbec2105953a882f37c
> Reviewed-on: https://chromium-review.googlesource.com/1069528
> Reviewed-by: Primiano Tucci <primiano@chromium.org>
> Commit-Queue: oysteine <oysteine@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#561356}

TBR=primiano@chromium.org,oysteine@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 844379
Change-Id: I82eb4e335521ef8f39119bdd2a984f4dc81ac88c
Reviewed-on: https://chromium-review.googlesource.com/1073247Reviewed-by: Olga Sharonova <olka@chromium.org>
Commit-Queue: Olga Sharonova <olka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561823}

base/trace_event/trace_log.cc

@@ -30,7 +30,6 @@
 #include "base/sys_info.h"
 #include "base/task_scheduler/post_task.h"
 #include "base/threading/platform_thread.h"
-#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/thread_id_name_manager.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
@@ -864,8 +863,8 @@ void TraceLog::FlushInternal(const TraceLog::OutputCallback& cb,
   {
     AutoLock lock(lock_);
     DCHECK(!flush_task_runner_);
-    flush_task_runner_ = SequencedTaskRunnerHandle::IsSet()
-                             ? SequencedTaskRunnerHandle::Get()
+    flush_task_runner_ = ThreadTaskRunnerHandle::IsSet()
+                             ? ThreadTaskRunnerHandle::Get()
                              : nullptr;
     DCHECK(thread_message_loops_.empty() || flush_task_runner_);
     flush_output_callback_ = cb;
@@ -993,7 +992,7 @@ void TraceLog::FlushCurrentThread(int generation, bool discard_events) {
   // to acquiring a tracing lock. Given that posting a task requires grabbing
   // a scheduler lock, we need to post this task outside tracing lock to avoid
   // deadlocks.
-  scoped_refptr<SequencedTaskRunner> cached_flush_task_runner;
+  scoped_refptr<SingleThreadTaskRunner> cached_flush_task_runner;
   {
     AutoLock lock(lock_);
     cached_flush_task_runner = flush_task_runner_;

base/trace_event/trace_log.h

@@ -511,7 +511,7 @@ class BASE_EXPORT TraceLog : public MemoryDumpProvider {
 
   // Set when asynchronous Flush is in progress.
   OutputCallback flush_output_callback_;
-  scoped_refptr<SequencedTaskRunner> flush_task_runner_;
+  scoped_refptr<SingleThreadTaskRunner> flush_task_runner_;
   ArgumentFilterPredicate argument_filter_predicate_;
   subtle::AtomicWord generation_;
   bool use_worker_thread_;

services/tracing/perfetto/perfetto_integration_unittest.cc

@@ -36,21 +36,16 @@ const char kPerfettoTestString[] = "d00df00d";
 
 class PerfettoIntegrationTest : public testing::Test {
  public:
-  void SetUp() override {
+  PerfettoIntegrationTest() {
     perfetto_service_ = std::make_unique<PerfettoService>(
         base::SequencedTaskRunnerHandle::Get());
     // The actual Perfetto service is created async on the given task_runner;
     // wait until that's done.
-    RunUntilIdle();
-    ProducerClient::ResetTaskRunnerForTesting();
+    base::RunLoop().RunUntilIdle();
   }
 
-  void TearDown() override { perfetto_service_.reset(); }
-
   PerfettoService* perfetto_service() const { return perfetto_service_.get(); }
 
-  void RunUntilIdle() { scoped_task_environment_.RunUntilIdle(); }
-
  private:
   std::unique_ptr<PerfettoService> perfetto_service_;
   base::test::ScopedTaskEnvironment scoped_task_environment_;
@@ -131,16 +126,11 @@ class MockProducerClient : public ProducerClient {
 
   void CommitData(const perfetto::CommitDataRequest& commit,
                   CommitDataCallback callback = {}) override {
-    // Only write out commits that have actual data in it; Perfetto
-    // might send two commits from different threads (one always empty),
-    // which causes TSan to complain.
-    if (commit.chunks_to_patch_size() || commit.chunks_to_move_size()) {
-      perfetto::protos::CommitDataRequest proto;
-      commit.ToProto(&proto);
-      std::string proto_string;
-      CHECK(proto.SerializeToString(&proto_string));
-      all_client_commit_data_requests_ += proto_string;
-    }
+    perfetto::protos::CommitDataRequest proto;
+    commit.ToProto(&proto);
+    std::string proto_string;
+    CHECK(proto.SerializeToString(&proto_string));
+    all_client_commit_data_requests_ += proto_string;
     ProducerClient::CommitData(commit, callback);
   }
 
@@ -152,6 +142,13 @@ class MockProducerClient : public ProducerClient {
     client_disabled_callback_ = std::move(client_disabled_callback);
   }
 
+  void FlushTaskRunner() {
+    base::RunLoop wait_for_client_task_runner;
+    GetTaskRunner()->PostTask(FROM_HERE,
+                              wait_for_client_task_runner.QuitClosure());
+    wait_for_client_task_runner.Run();
+  }
+
   const std::string& all_client_commit_data_requests() const {
     return all_client_commit_data_requests_;
   }
@@ -247,11 +244,6 @@ class MockProducer : public ProducerHost {
   }
 
   void OnCommit(const perfetto::CommitDataRequest& commit_data_request) {
-    if (!commit_data_request.chunks_to_patch_size() &&
-        !commit_data_request.chunks_to_move_size()) {
-      return;
-    }
-
     perfetto::protos::CommitDataRequest proto;
     commit_data_request.ToProto(&proto);
     std::string proto_string;
@@ -259,14 +251,6 @@ class MockProducer : public ProducerHost {
     all_host_commit_data_requests_ += proto_string;
   }
 
-  void OnMessagepipesReadyCallback(
-      perfetto::Service* perfetto_service,
-      mojom::ProducerClientPtr producer_client_pipe,
-      mojom::ProducerHostRequest producer_host_pipe) {
-    Initialize(std::move(producer_client_pipe), std::move(producer_host_pipe),
-               perfetto_service, kPerfettoProducerName);
-  }
-
   const std::string& all_host_commit_data_requests() const {
     return all_host_commit_data_requests_;
   }
@@ -284,11 +268,10 @@ TEST_F(PerfettoIntegrationTest, ProducerDatasourceInitialized) {
   base::RunLoop producer_initialized_runloop;
   auto new_producer = std::make_unique<MockProducer>(
       kPerfettoTestDataSourceName, producer_initialized_runloop.QuitClosure());
-  dummy_client->CreateMojoMessagepipes(
-      base::BindOnce(&MockProducer::OnMessagepipesReadyCallback,
-                     base::Unretained(new_producer.get()),
-                     base::Unretained(perfetto_service()->GetService())));
-
+  new_producer->Initialize(dummy_client->CreateAndBindProducerClient(),
+                           dummy_client->CreateProducerHostRequest(),
+                           perfetto_service()->GetService(),
+                           kPerfettoProducerName);
   producer_initialized_runloop.Run();
 
   ProducerClient::DeleteSoon(std::move(dummy_client));
@@ -310,14 +293,14 @@ TEST_F(PerfettoIntegrationTest, ClientEnabledAndDisabled) {
       client_disabled_callback.QuitClosure());
 
   auto producer = std::make_unique<MockProducer>(kPerfettoTestDataSourceName);
-  client->CreateMojoMessagepipes(
-      base::BindOnce(&MockProducer::OnMessagepipesReadyCallback,
-                     base::Unretained(producer.get()),
-                     base::Unretained(perfetto_service()->GetService())));
+  producer->Initialize(client->CreateAndBindProducerClient(),
+                       client->CreateProducerHostRequest(),
+                       perfetto_service()->GetService(), kPerfettoProducerName);
 
   client_enabled_callback.Run();
 
-  RunUntilIdle();
+  // Wait until all Mojo messages have been sent.
+  base::RunLoop().RunUntilIdle();
 
   consumer.StopTracing();
 
@@ -339,10 +322,9 @@ TEST_F(PerfettoIntegrationTest, PacketsEndToEndProducerFirst) {
       client_disabled_callback.QuitClosure());
 
   auto producer = std::make_unique<MockProducer>(kPerfettoTestDataSourceName);
-  client->CreateMojoMessagepipes(
-      base::BindOnce(&MockProducer::OnMessagepipesReadyCallback,
-                     base::Unretained(producer.get()),
-                     base::Unretained(perfetto_service()->GetService())));
+  producer->Initialize(client->CreateAndBindProducerClient(),
+                       client->CreateProducerHostRequest(),
+                       perfetto_service()->GetService(), kPerfettoProducerName);
 
   base::RunLoop no_more_packets_runloop;
   MockConsumer consumer(perfetto_service()->GetService(),
@@ -355,7 +337,8 @@ TEST_F(PerfettoIntegrationTest, PacketsEndToEndProducerFirst) {
 
   client_enabled_callback.Run();
 
-  RunUntilIdle();
+  // Wait until all Mojo messages have been sent.
+  base::RunLoop().RunUntilIdle();
 
   consumer.StopTracing();
   client_disabled_callback.Run();
@@ -385,14 +368,15 @@ TEST_F(PerfettoIntegrationTest, PacketsEndToEndConsumerFirst) {
 
   auto new_producer =
       std::make_unique<MockProducer>(kPerfettoTestDataSourceName);
-  client->CreateMojoMessagepipes(
-      base::BindOnce(&MockProducer::OnMessagepipesReadyCallback,
-                     base::Unretained(new_producer.get()),
-                     base::Unretained(perfetto_service()->GetService())));
+  new_producer->Initialize(client->CreateAndBindProducerClient(),
+                           client->CreateProducerHostRequest(),
+                           perfetto_service()->GetService(),
+                           kPerfettoProducerName);
 
   client_enabled_callback.Run();
 
-  RunUntilIdle();
+  // Wait until all Mojo messages have been sent.
+  base::RunLoop().RunUntilIdle();
 
   consumer.StopTracing();
 
@@ -425,16 +409,19 @@ TEST_F(PerfettoIntegrationTest, MAYBE_CommitDataRequestIsMaybeComplete) {
       kNumPackets, client_enabled_callback.QuitClosure());
   auto new_producer =
       std::make_unique<MockProducer>(kPerfettoTestDataSourceName);
-  client->CreateMojoMessagepipes(
-      base::BindOnce(&MockProducer::OnMessagepipesReadyCallback,
-                     base::Unretained(new_producer.get()),
-                     base::Unretained(perfetto_service()->GetService())));
+  new_producer->Initialize(client->CreateAndBindProducerClient(),
+                           client->CreateProducerHostRequest(),
+                           perfetto_service()->GetService(),
+                           kPerfettoProducerName);
 
   client_enabled_callback.Run();
 
   client->data_source()->WritePacketBigly();
 
-  RunUntilIdle();
+  client->FlushTaskRunner();
+
+  // Wait until all Mojo messages have been sent.
+  base::RunLoop().RunUntilIdle();
 
   consumer.StopTracing();
 
@@ -464,14 +451,15 @@ TEST_F(PerfettoIntegrationTest, TracingRestarted) {
 
   auto new_producer =
       std::make_unique<MockProducer>(kPerfettoTestDataSourceName);
-  client->CreateMojoMessagepipes(
-      base::BindOnce(&MockProducer::OnMessagepipesReadyCallback,
-                     base::Unretained(new_producer.get()),
-                     base::Unretained(perfetto_service()->GetService())));
+  new_producer->Initialize(client->CreateAndBindProducerClient(),
+                           client->CreateProducerHostRequest(),
+                           perfetto_service()->GetService(),
+                           kPerfettoProducerName);
 
   client_enabled_callback.Run();
 
-  RunUntilIdle();
+  // Wait until all Mojo messages have been sent.
+  base::RunLoop().RunUntilIdle();
 
   perfetto::SharedMemory* first_session_shm = client->shared_memory();
   consumer.StopTracing();
@@ -487,7 +475,8 @@ TEST_F(PerfettoIntegrationTest, TracingRestarted) {
   consumer.StartTracing();
   client_reenabled_callback.Run();
 
-  RunUntilIdle();
+  // Wait until all Mojo messages have been sent.
+  base::RunLoop().RunUntilIdle();
 
   // We should still be using the same shared memory.
   EXPECT_EQ(first_session_shm, client->shared_memory());
@@ -515,10 +504,10 @@ TEST_F(PerfettoIntegrationTest, NoPacketsReceivedOnWrongSourceName) {
   base::RunLoop producer_initialized_runloop;
   auto new_producer = std::make_unique<MockProducer>(
       "fake", producer_initialized_runloop.QuitClosure());
-  client->CreateMojoMessagepipes(
-      base::BindOnce(&MockProducer::OnMessagepipesReadyCallback,
-                     base::Unretained(new_producer.get()),
-                     base::Unretained(perfetto_service()->GetService())));
+  new_producer->Initialize(client->CreateAndBindProducerClient(),
+                           client->CreateProducerHostRequest(),
+                           perfetto_service()->GetService(),
+                           kPerfettoProducerName);
   producer_initialized_runloop.Run();
 
   base::RunLoop no_more_packets_runloop;
@@ -530,7 +519,8 @@ TEST_F(PerfettoIntegrationTest, NoPacketsReceivedOnWrongSourceName) {
                           }
                         });
 
-  RunUntilIdle();
+  // Wait until all Mojo messages have been sent.
+  base::RunLoop().RunUntilIdle();
 
   consumer.StopTracing();
 
@@ -550,16 +540,16 @@ TEST_F(PerfettoIntegrationTest,
       0 /* send_packet_count */, client2_enabled_callback.QuitClosure());
 
   auto producer1 = std::make_unique<MockProducer>(kPerfettoTestDataSourceName);
-  client1->CreateMojoMessagepipes(
-      base::BindOnce(&MockProducer::OnMessagepipesReadyCallback,
-                     base::Unretained(producer1.get()),
-                     base::Unretained(perfetto_service()->GetService())));
+  producer1->Initialize(client1->CreateAndBindProducerClient(),
+                        client1->CreateProducerHostRequest(),
+                        perfetto_service()->GetService(),
+                        kPerfettoProducerName);
 
   auto producer2 = std::make_unique<MockProducer>(kPerfettoTestDataSourceName);
-  client2->CreateMojoMessagepipes(
-      base::BindOnce(&MockProducer::OnMessagepipesReadyCallback,
-                     base::Unretained(producer2.get()),
-                     base::Unretained(perfetto_service()->GetService())));
+  producer2->Initialize(client2->CreateAndBindProducerClient(),
+                        client2->CreateProducerHostRequest(),
+                        perfetto_service()->GetService(),
+                        kPerfettoProducerName);
 
   MockConsumer consumer(perfetto_service()->GetService(),
                         kPerfettoTestDataSourceName, nullptr);

services/tracing/perfetto/perfetto_service.h

@@ -47,7 +47,7 @@ class PerfettoService : public mojom::PerfettoService {
                              mojom::ProducerHostRequest producer_host) override;
 
   perfetto::Service* GetService() const;
-  scoped_refptr<base::SequencedTaskRunner> task_runner() {
+  base::SequencedTaskRunner* task_runner() {
     return perfetto_task_runner_.task_runner();
   }
 

services/tracing/perfetto/producer_host.cc

@@ -43,13 +43,8 @@ void ProducerHost::OnConnectionError() {
   // Manually reset to prevent any callbacks from the ProducerEndpoint
   // when we're in a half-destructed state.
   producer_endpoint_.reset();
-  // If the ProducerHost is owned by the PerfettoService, let it know
-  // we're disconnected to let this be cleaned up. Tests manage lifespan
-  // themselves.
-  if (connection_error_handler_) {
-    std::move(connection_error_handler_).Run();
-  }
-  // This object *may* be destroyed at this point.
+  std::move(connection_error_handler_).Run();
+  // This object is destroyed at this point.
 }
 
 void ProducerHost::OnConnect() {

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

@@ -6,7 +6,6 @@
 
 #include <utility>
 
-#include "base/no_destructor.h"
 #include "base/task_scheduler/post_task.h"
 #include "services/tracing/public/cpp/perfetto/shared_memory.h"
 #include "services/tracing/public/cpp/perfetto/trace_event_data_source.h"
@@ -16,24 +15,10 @@
 
 namespace tracing {
 
-namespace {
-
-scoped_refptr<base::SequencedTaskRunner> CreateTaskRunner() {
-  return base::CreateSequencedTaskRunnerWithTraits(
-      {base::MayBlock(), base::TaskPriority::BACKGROUND});
-}
-
-// We never destroy the taskrunner as we may need it for cleanup
-// of TraceWriters in TLS, which could happen after the ProducerClient
-// is deleted.
-PerfettoTaskRunner* GetPerfettoTaskRunner() {
-  static base::NoDestructor<PerfettoTaskRunner> task_runner(CreateTaskRunner());
-  return task_runner.get();
-}
-
-}  // namespace
-
-ProducerClient::ProducerClient() {
+// TODO(oysteine): Use a new sequence here once Perfetto handles multi-threading
+// properly.
+ProducerClient::ProducerClient()
+    : perfetto_task_runner_(base::SequencedTaskRunnerHandle::Get()) {
   DETACH_FROM_SEQUENCE(sequence_checker_);
 }
 
@@ -44,48 +29,36 @@ ProducerClient::~ProducerClient() {
 // static
 void ProducerClient::DeleteSoon(
     std::unique_ptr<ProducerClient> producer_client) {
-  GetTaskRunner()->DeleteSoon(FROM_HERE, std::move(producer_client));
+  producer_client->GetTaskRunner()->DeleteSoon(FROM_HERE,
+                                               std::move(producer_client));
 }
 
-// static
 base::SequencedTaskRunner* ProducerClient::GetTaskRunner() {
-  auto* task_runner = GetPerfettoTaskRunner()->task_runner();
-  DCHECK(task_runner);
-  return task_runner;
-}
-
-// static
-void ProducerClient::ResetTaskRunnerForTesting() {
-  GetPerfettoTaskRunner()->ResetTaskRunnerForTesting(CreateTaskRunner());
+  return perfetto_task_runner_.task_runner();
 }
 
-void ProducerClient::CreateMojoMessagepipes(
-    MessagepipesReadyCallback callback) {
-  auto origin_task_runner = base::SequencedTaskRunnerHandle::Get();
-  DCHECK(origin_task_runner);
+// The Mojo binding should run on the same sequence as the one we get
+// callbacks from Perfetto on, to avoid additional PostTasks.
+mojom::ProducerClientPtr ProducerClient::CreateAndBindProducerClient() {
+  DCHECK(!binding_);
   mojom::ProducerClientPtr producer_client;
+
   GetTaskRunner()->PostTask(
       FROM_HERE,
-      base::BindOnce(&ProducerClient::CreateMojoMessagepipesOnSequence,
-                     base::Unretained(this), origin_task_runner,
-                     std::move(callback), mojo::MakeRequest(&producer_client),
-                     std::move(producer_client)));
+      base::BindOnce(&ProducerClient::BindOnSequence, base::Unretained(this),
+                     mojo::MakeRequest(&producer_client)));
+
+  return producer_client;
 }
 
-// The Mojo binding should run on the same sequence as the one we get
-// callbacks from Perfetto on, to avoid additional PostTasks.
-void ProducerClient::CreateMojoMessagepipesOnSequence(
-    scoped_refptr<base::SequencedTaskRunner> origin_task_runner,
-    MessagepipesReadyCallback callback,
-    mojom::ProducerClientRequest producer_client_request,
-    mojom::ProducerClientPtr producer_client) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+mojom::ProducerHostRequest ProducerClient::CreateProducerHostRequest() {
+  return mojo::MakeRequest(&producer_host_);
+}
 
+void ProducerClient::BindOnSequence(mojom::ProducerClientRequest request) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   binding_ = std::make_unique<mojo::Binding<mojom::ProducerClient>>(
-      this, std::move(producer_client_request));
-  origin_task_runner->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), std::move(producer_client),
-                                mojo::MakeRequest(&producer_host_)));
+      this, std::move(request));
 }
 
 void ProducerClient::OnTracingStart(
@@ -101,7 +74,7 @@ void ProducerClient::OnTracingStart(
 
     shared_memory_arbiter_ = perfetto::SharedMemoryArbiter::CreateInstance(
         shared_memory_.get(), kShmemBufferPageSize, this,
-        GetPerfettoTaskRunner());
+        &perfetto_task_runner_);
   } else {
     // TODO(oysteine): This is assuming the SMB is the same, currently. Swapping
     // out SharedMemoryBuffers would require more thread synchronization.

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

@@ -46,17 +46,15 @@ class COMPONENT_EXPORT(TRACING_CPP) ProducerClient
 
   static void DeleteSoon(std::unique_ptr<ProducerClient>);
 
-  // Returns the taskrunner used by Perfetto.
-  static base::SequencedTaskRunner* GetTaskRunner();
-
-  // Create the messagepipes that'll be used to connect
-  // to the service-side ProducerHost, on the correct
-  // sequence. The callback will be called on same sequence
-  // as CreateMojoMessagepipes() got called on.
-  using MessagepipesReadyCallback =
-      base::OnceCallback<void(mojom::ProducerClientPtr,
-                              mojom::ProducerHostRequest)>;
-  void CreateMojoMessagepipes(MessagepipesReadyCallback);
+  // Connect to the service-side ProducerHost which will
+  // let Perfetto know we're ready to start logging
+  // our data.
+  mojom::ProducerClientPtr CreateAndBindProducerClient();
+  mojom::ProducerHostRequest CreateProducerHostRequest();
+
+  bool has_connected_producer_host_for_testing() const {
+    return !!producer_host_;
+  }
 
   // mojom::ProducerClient implementation.
   // Called through Mojo by the ProducerHost on the service-side to control
@@ -89,18 +87,17 @@ class COMPONENT_EXPORT(TRACING_CPP) ProducerClient
   size_t shared_buffer_page_size_kb() const override;
   void NotifyFlushComplete(perfetto::FlushRequestID) override;
 
-  static void ResetTaskRunnerForTesting();
+ protected:
+  base::SequencedTaskRunner* GetTaskRunner();
 
  private:
+  void BindOnSequence(mojom::ProducerClientRequest request);
   void CommitDataOnSequence(mojom::CommitDataRequestPtr request);
-  // The callback will be run on the |origin_task_runner|, meaning
-  // the same sequence as CreateMojoMessagePipes() got called on.
-  void CreateMojoMessagepipesOnSequence(
-      scoped_refptr<base::SequencedTaskRunner> origin_task_runner,
-      MessagepipesReadyCallback,
-      mojom::ProducerClientRequest,
-      mojom::ProducerClientPtr);
 
+  // Keep the TaskRunner first in the member list so it outlives
+  // everything else and no dependent classes will try to use
+  // an invalid taskrunner.
+  PerfettoTaskRunner perfetto_task_runner_;
   std::unique_ptr<mojo::Binding<mojom::ProducerClient>> binding_;
   std::unique_ptr<perfetto::SharedMemoryArbiter> shared_memory_arbiter_;
   mojom::ProducerHostPtr producer_host_;

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

@@ -5,7 +5,6 @@
 #include "services/tracing/public/cpp/perfetto/task_runner.h"
 
 #include <memory>
-#include <utility>
 
 #include "base/threading/sequenced_task_runner_handle.h"
 
@@ -39,9 +38,4 @@ void PerfettoTaskRunner::RemoveFileDescriptorWatch(int fd) {
   NOTREACHED();
 }
 
-void PerfettoTaskRunner::ResetTaskRunnerForTesting(
-    scoped_refptr<base::SequencedTaskRunner> task_runner) {
-  task_runner_ = std::move(task_runner);
-}
-
 }  // namespace tracing

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

@@ -33,11 +33,6 @@ class COMPONENT_EXPORT(TRACING_CPP) PerfettoTaskRunner
 
   base::SequencedTaskRunner* task_runner() { return task_runner_.get(); }
 
-  // Tests will shut down all task runners in between runs, so we need
-  // to re-create any static instances on each SetUp();
-  void ResetTaskRunnerForTesting(
-      scoped_refptr<base::SequencedTaskRunner> task_runner);
-
  private:
   scoped_refptr<base::SequencedTaskRunner> task_runner_;
 

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

@@ -27,16 +27,6 @@ class TraceEventDataSource::ThreadLocalEventSink {
       std::unique_ptr<perfetto::TraceWriter> trace_writer)
       : trace_writer_(std::move(trace_writer)) {}
 
-  ~ThreadLocalEventSink() {
-    // Delete the TraceWriter on the sequence that Perfetto runs on, needed
-    // as the ThreadLocalEventSink gets deleted on thread
-    // shutdown and we can't safely call TaskRunnerHandle::Get() at that point
-    // (which can happen as the TraceWriter destructor might make a Mojo call
-    // and trigger it).
-    ProducerClient::GetTaskRunner()->DeleteSoon(FROM_HERE,
-                                                std::move(trace_writer_));
-  }
-
   void AddTraceEvent(const TraceEvent& trace_event) {
     // TODO(oysteine): Adding trace events to Perfetto will
     // stall in some situations, specifically when we overflow

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

@@ -5,13 +5,11 @@
 #include "services/tracing/public/cpp/perfetto/trace_event_data_source.h"
 
 #include <utility>
-#include <vector>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
-#include "base/test/scoped_task_environment.h"
 #include "base/trace_event/trace_event.h"
 #include "services/tracing/public/mojom/perfetto_service.mojom.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -24,15 +22,10 @@ namespace tracing {
 
 namespace {
 
-const char kCategoryGroup[] = "foo";
-
 class MockProducerClient : public ProducerClient {
  public:
-  explicit MockProducerClient(
-      scoped_refptr<base::SequencedTaskRunner> main_thread_task_runner)
-      : delegate_(perfetto::base::kPageSize),
-        stream_(&delegate_),
-        main_thread_task_runner_(std::move(main_thread_task_runner)) {
+  MockProducerClient()
+      : delegate_(perfetto::base::kPageSize), stream_(&delegate_) {
     trace_packet_.Reset(&stream_);
   }
 
@@ -51,10 +44,7 @@ class MockProducerClient : public ProducerClient {
 
       auto proto = std::make_unique<perfetto::protos::TracePacket>();
       EXPECT_TRUE(proto->ParseFromArray(buffer.begin, message_size));
-      if (proto->has_chrome_events() &&
-          proto->chrome_events().trace_events().size() > 0 &&
-          proto->chrome_events().trace_events()[0].category_group_name() ==
-              kCategoryGroup) {
+      if (proto->has_chrome_events()) {
         finalized_packets_.push_back(std::move(proto));
       }
     }
@@ -89,33 +79,6 @@ class MockProducerClient : public ProducerClient {
   perfetto::protos::pbzero::TracePacket trace_packet_;
   protozero::ScatteredStreamWriterNullDelegate delegate_;
   protozero::ScatteredStreamWriter stream_;
-  scoped_refptr<base::SequencedTaskRunner> main_thread_task_runner_;
-};
-
-// For sequences/threads other than our own, we just want to ignore
-// any events coming in.
-class DummyTraceWriter : public perfetto::TraceWriter {
- public:
-  DummyTraceWriter()
-      : delegate_(perfetto::base::kPageSize), stream_(&delegate_) {}
-
-  perfetto::TraceWriter::TracePacketHandle NewTracePacket() override {
-    stream_.Reset(delegate_.GetNewBuffer());
-    trace_packet_.Reset(&stream_);
-
-    return perfetto::TraceWriter::TracePacketHandle(&trace_packet_);
-  }
-
-  void Flush(std::function<void()> callback = {}) override {}
-
-  perfetto::WriterID writer_id() const override {
-    return perfetto::WriterID(0);
-  }
-
- private:
-  perfetto::protos::pbzero::TracePacket trace_packet_;
-  protozero::ScatteredStreamWriterNullDelegate delegate_;
-  protozero::ScatteredStreamWriter stream_;
 };
 
 class MockTraceWriter : public perfetto::TraceWriter {
@@ -140,19 +103,14 @@ class MockTraceWriter : public perfetto::TraceWriter {
 
 std::unique_ptr<perfetto::TraceWriter> MockProducerClient::CreateTraceWriter(
     perfetto::BufferID target_buffer) {
-  if (main_thread_task_runner_->RunsTasksInCurrentSequence()) {
-    return std::make_unique<MockTraceWriter>(this);
-  } else {
-    return std::make_unique<DummyTraceWriter>();
-  }
+  return std::make_unique<MockTraceWriter>(this);
 }
 
 class TraceEventDataSourceTest : public testing::Test {
  public:
   void SetUp() override {
-    ProducerClient::ResetTaskRunnerForTesting();
-    producer_client_ = std::make_unique<MockProducerClient>(
-        scoped_task_environment_.GetMainThreadTaskRunner());
+    message_loop_ = std::make_unique<base::MessageLoop>();
+    producer_client_ = std::make_unique<MockProducerClient>();
   }
 
   void TearDown() override {
@@ -165,10 +123,13 @@ class TraceEventDataSourceTest : public testing::Test {
     wait_for_tracelog_flush.Run();
 
     // As MockTraceWriter keeps a pointer to our MockProducerClient,
-    // we need to make sure to clean it up from TLS. The other sequences
-    // get DummyTraceWriters that we don't care about.
+    // we need to make sure to clean it up from TLS. This means
+    // these tests can't use a multithreaded task environment that
+    // might add trace events from other threads, as those might
+    // use old TraceWriters writing into invalid memory.
     TraceEventDataSource::GetInstance()->ResetCurrentThreadForTesting();
     producer_client_.reset();
+    message_loop_.reset();
   }
 
   void CreateTraceEventDataSource() {
@@ -181,20 +142,20 @@ class TraceEventDataSourceTest : public testing::Test {
 
  private:
   std::unique_ptr<MockProducerClient> producer_client_;
-  base::test::ScopedTaskEnvironment scoped_task_environment_;
+  std::unique_ptr<base::MessageLoop> message_loop_;
 };
 
 TEST_F(TraceEventDataSourceTest, BasicTraceEvent) {
   CreateTraceEventDataSource();
 
-  TRACE_EVENT_BEGIN0(kCategoryGroup, "bar");
+  TRACE_EVENT_BEGIN0("foo", "bar");
 
   auto trace_events = producer_client()->GetChromeTraceEvents();
   EXPECT_EQ(trace_events.size(), 1);
 
   auto trace_event = trace_events[0];
   EXPECT_EQ("bar", trace_event.name());
-  EXPECT_EQ(kCategoryGroup, trace_event.category_group_name());
+  EXPECT_EQ("foo", trace_event.category_group_name());
   EXPECT_EQ(TRACE_EVENT_PHASE_BEGIN, trace_event.phase());
 }
 
@@ -202,7 +163,7 @@ TEST_F(TraceEventDataSourceTest, TimestampedTraceEvent) {
   CreateTraceEventDataSource();
 
   TRACE_EVENT_BEGIN_WITH_ID_TID_AND_TIMESTAMP0(
-      kCategoryGroup, "bar", 42, 4242,
+      "foo", "bar", 42, 4242,
       base::TimeTicks() + base::TimeDelta::FromMicroseconds(424242));
 
   auto trace_events = producer_client()->GetChromeTraceEvents();
@@ -210,7 +171,7 @@ TEST_F(TraceEventDataSourceTest, TimestampedTraceEvent) {
 
   auto trace_event = trace_events[0];
   EXPECT_EQ("bar", trace_event.name());
-  EXPECT_EQ(kCategoryGroup, trace_event.category_group_name());
+  EXPECT_EQ("foo", trace_event.category_group_name());
   EXPECT_EQ(42u, trace_event.id());
   EXPECT_EQ(4242, trace_event.thread_id());
   EXPECT_EQ(424242, trace_event.timestamp());
@@ -220,14 +181,14 @@ TEST_F(TraceEventDataSourceTest, TimestampedTraceEvent) {
 TEST_F(TraceEventDataSourceTest, InstantTraceEvent) {
   CreateTraceEventDataSource();
 
-  TRACE_EVENT_INSTANT0(kCategoryGroup, "bar", TRACE_EVENT_SCOPE_THREAD);
+  TRACE_EVENT_INSTANT0("foo", "bar", TRACE_EVENT_SCOPE_THREAD);
 
   auto trace_events = producer_client()->GetChromeTraceEvents();
   EXPECT_EQ(trace_events.size(), 1);
 
   auto trace_event = trace_events[0];
   EXPECT_EQ("bar", trace_event.name());
-  EXPECT_EQ(kCategoryGroup, trace_event.category_group_name());
+  EXPECT_EQ("foo", trace_event.category_group_name());
   EXPECT_EQ(TRACE_EVENT_SCOPE_THREAD, trace_event.flags());
   EXPECT_EQ(TRACE_EVENT_PHASE_INSTANT, trace_event.phase());
 }
@@ -235,8 +196,8 @@ TEST_F(TraceEventDataSourceTest, InstantTraceEvent) {
 TEST_F(TraceEventDataSourceTest, EventWithStringArgs) {
   CreateTraceEventDataSource();
 
-  TRACE_EVENT_INSTANT2(kCategoryGroup, "bar", TRACE_EVENT_SCOPE_THREAD,
-                       "arg1_name", "arg1_val", "arg2_name", "arg2_val");
+  TRACE_EVENT_INSTANT2("foo", "bar", TRACE_EVENT_SCOPE_THREAD, "arg1_name",
+                       "arg1_val", "arg2_name", "arg2_val");
 
   auto trace_events = producer_client()->GetChromeTraceEvents();
   EXPECT_EQ(trace_events.size(), 1);
@@ -253,8 +214,8 @@ TEST_F(TraceEventDataSourceTest, EventWithStringArgs) {
 TEST_F(TraceEventDataSourceTest, EventWithUIntArgs) {
   CreateTraceEventDataSource();
 
-  TRACE_EVENT_INSTANT2(kCategoryGroup, "bar", TRACE_EVENT_SCOPE_THREAD, "foo",
-                       42u, "bar", 4242u);
+  TRACE_EVENT_INSTANT2("foo", "bar", TRACE_EVENT_SCOPE_THREAD, "foo", 42u,
+                       "bar", 4242u);
 
   auto trace_events = producer_client()->GetChromeTraceEvents();
   EXPECT_EQ(trace_events.size(), 1);
@@ -269,8 +230,8 @@ TEST_F(TraceEventDataSourceTest, EventWithUIntArgs) {
 TEST_F(TraceEventDataSourceTest, EventWithIntArgs) {
   CreateTraceEventDataSource();
 
-  TRACE_EVENT_INSTANT2(kCategoryGroup, "bar", TRACE_EVENT_SCOPE_THREAD, "foo",
-                       42, "bar", 4242);
+  TRACE_EVENT_INSTANT2("foo", "bar", TRACE_EVENT_SCOPE_THREAD, "foo", 42, "bar",
+                       4242);
 
   auto trace_events = producer_client()->GetChromeTraceEvents();
   EXPECT_EQ(trace_events.size(), 1);
@@ -285,8 +246,8 @@ TEST_F(TraceEventDataSourceTest, EventWithIntArgs) {
 TEST_F(TraceEventDataSourceTest, EventWithBoolArgs) {
   CreateTraceEventDataSource();
 
-  TRACE_EVENT_INSTANT2(kCategoryGroup, "bar", TRACE_EVENT_SCOPE_THREAD, "foo",
-                       true, "bar", false);
+  TRACE_EVENT_INSTANT2("foo", "bar", TRACE_EVENT_SCOPE_THREAD, "foo", true,
+                       "bar", false);
 
   auto trace_events = producer_client()->GetChromeTraceEvents();
   EXPECT_EQ(trace_events.size(), 1);
@@ -303,8 +264,8 @@ TEST_F(TraceEventDataSourceTest, EventWithBoolArgs) {
 TEST_F(TraceEventDataSourceTest, EventWithDoubleArgs) {
   CreateTraceEventDataSource();
 
-  TRACE_EVENT_INSTANT2(kCategoryGroup, "bar", TRACE_EVENT_SCOPE_THREAD, "foo",
-                       42.42, "bar", 4242.42);
+  TRACE_EVENT_INSTANT2("foo", "bar", TRACE_EVENT_SCOPE_THREAD, "foo", 42.42,
+                       "bar", 4242.42);
 
   auto trace_events = producer_client()->GetChromeTraceEvents();
   EXPECT_EQ(trace_events.size(), 1);
@@ -319,7 +280,7 @@ TEST_F(TraceEventDataSourceTest, EventWithDoubleArgs) {
 TEST_F(TraceEventDataSourceTest, EventWithPointerArgs) {
   CreateTraceEventDataSource();
 
-  TRACE_EVENT_INSTANT2(kCategoryGroup, "bar", TRACE_EVENT_SCOPE_THREAD, "foo",
+  TRACE_EVENT_INSTANT2("foo", "bar", TRACE_EVENT_SCOPE_THREAD, "foo",
                        reinterpret_cast<void*>(0xBEEF), "bar",
                        reinterpret_cast<void*>(0xF00D));
 
@@ -334,6 +295,7 @@ TEST_F(TraceEventDataSourceTest, EventWithPointerArgs) {
 }
 
 TEST_F(TraceEventDataSourceTest, CompleteTraceEventsIntoSeparateBeginAndEnd) {
+  static const char kCategoryGroup[] = "foo";
   static const char kEventName[] = "bar";
 
   CreateTraceEventDataSource();

services/tracing/public/cpp/trace_event_agent.cc

@@ -41,14 +41,9 @@ class PerfettoTraceEventAgent : public TraceEventAgent {
     connector->BindInterface(mojom::kServiceName, &perfetto_service);
 
     producer_client_ = std::make_unique<ProducerClient>();
-    producer_client_->CreateMojoMessagepipes(base::BindOnce(
-        [](mojom::PerfettoServicePtr perfetto_service,
-           mojom::ProducerClientPtr producer_client_pipe,
-           mojom::ProducerHostRequest producer_host_pipe) {
-          perfetto_service->ConnectToProducerHost(
-              std::move(producer_client_pipe), std::move(producer_host_pipe));
-        },
-        std::move(perfetto_service)));
+    perfetto_service->ConnectToProducerHost(
+        producer_client_->CreateAndBindProducerClient(),
+        producer_client_->CreateProducerHostRequest());
   }
 
   ~PerfettoTraceEventAgent() override {