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/1069528Reviewed-by: Primiano Tucci <primiano@chromium.org>
Commit-Queue: oysteine <oysteine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561356}

base/trace_event/trace_log.cc

@@ -30,6 +30,7 @@
 #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"
@@ -863,8 +864,8 @@ void TraceLog::FlushInternal(const TraceLog::OutputCallback& cb,
   {
     AutoLock lock(lock_);
     DCHECK(!flush_task_runner_);
-    flush_task_runner_ = ThreadTaskRunnerHandle::IsSet()
-                             ? ThreadTaskRunnerHandle::Get()
+    flush_task_runner_ = SequencedTaskRunnerHandle::IsSet()
+                             ? SequencedTaskRunnerHandle::Get()
                              : nullptr;
     DCHECK(thread_message_loops_.empty() || flush_task_runner_);
     flush_output_callback_ = cb;
@@ -992,7 +993,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<SingleThreadTaskRunner> cached_flush_task_runner;
+  scoped_refptr<SequencedTaskRunner> 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<SingleThreadTaskRunner> flush_task_runner_;
+  scoped_refptr<SequencedTaskRunner> flush_task_runner_;
   ArgumentFilterPredicate argument_filter_predicate_;
   subtle::AtomicWord generation_;
   bool use_worker_thread_;

services/tracing/perfetto/perfetto_service.h

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

services/tracing/perfetto/producer_host.cc

@@ -43,8 +43,13 @@ void ProducerHost::OnConnectionError() {
   // Manually reset to prevent any callbacks from the ProducerEndpoint
   // when we're in a half-destructed state.
   producer_endpoint_.reset();
-  std::move(connection_error_handler_).Run();
-  // This object is destroyed at this point.
+  // 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.
 }
 
 void ProducerHost::OnConnect() {

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

@@ -6,6 +6,7 @@
 
 #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"
@@ -15,10 +16,24 @@
 
 namespace tracing {
 
-// TODO(oysteine): Use a new sequence here once Perfetto handles multi-threading
-// properly.
-ProducerClient::ProducerClient()
-    : perfetto_task_runner_(base::SequencedTaskRunnerHandle::Get()) {
+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() {
   DETACH_FROM_SEQUENCE(sequence_checker_);
 }
 
@@ -29,36 +44,48 @@ ProducerClient::~ProducerClient() {
 // static
 void ProducerClient::DeleteSoon(
     std::unique_ptr<ProducerClient> producer_client) {
-  producer_client->GetTaskRunner()->DeleteSoon(FROM_HERE,
-                                               std::move(producer_client));
+  GetTaskRunner()->DeleteSoon(FROM_HERE, std::move(producer_client));
 }
 
+// static
 base::SequencedTaskRunner* ProducerClient::GetTaskRunner() {
-  return perfetto_task_runner_.task_runner();
+  auto* task_runner = GetPerfettoTaskRunner()->task_runner();
+  DCHECK(task_runner);
+  return 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;
+// static
+void ProducerClient::ResetTaskRunnerForTesting() {
+  GetPerfettoTaskRunner()->ResetTaskRunnerForTesting(CreateTaskRunner());
+}
 
+void ProducerClient::CreateMojoMessagepipes(
+    MessagepipesReadyCallback callback) {
+  auto origin_task_runner = base::SequencedTaskRunnerHandle::Get();
+  DCHECK(origin_task_runner);
+  mojom::ProducerClientPtr producer_client;
   GetTaskRunner()->PostTask(
       FROM_HERE,
-      base::BindOnce(&ProducerClient::BindOnSequence, base::Unretained(this),
-                     mojo::MakeRequest(&producer_client)));
-
-  return producer_client;
+      base::BindOnce(&ProducerClient::CreateMojoMessagepipesOnSequence,
+                     base::Unretained(this), origin_task_runner,
+                     std::move(callback), mojo::MakeRequest(&producer_client),
+                     std::move(producer_client)));
 }
 
-mojom::ProducerHostRequest ProducerClient::CreateProducerHostRequest() {
-  return mojo::MakeRequest(&producer_host_);
-}
-
-void ProducerClient::BindOnSequence(mojom::ProducerClientRequest request) {
+// 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_);
+
   binding_ = std::make_unique<mojo::Binding<mojom::ProducerClient>>(
-      this, std::move(request));
+      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_)));
 }
 
 void ProducerClient::OnTracingStart(
@@ -74,7 +101,7 @@ void ProducerClient::OnTracingStart(
 
     shared_memory_arbiter_ = perfetto::SharedMemoryArbiter::CreateInstance(
         shared_memory_.get(), kShmemBufferPageSize, this,
-        &perfetto_task_runner_);
+        GetPerfettoTaskRunner());
   } 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,15 +46,17 @@ class COMPONENT_EXPORT(TRACING_CPP) ProducerClient
 
   static void DeleteSoon(std::unique_ptr<ProducerClient>);
 
-  // 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_;
-  }
+  // 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);
 
   // mojom::ProducerClient implementation.
   // Called through Mojo by the ProducerHost on the service-side to control
@@ -87,17 +89,18 @@ class COMPONENT_EXPORT(TRACING_CPP) ProducerClient
   size_t shared_buffer_page_size_kb() const override;
   void NotifyFlushComplete(perfetto::FlushRequestID) override;
 
- protected:
-  base::SequencedTaskRunner* GetTaskRunner();
+  static void ResetTaskRunnerForTesting();
 
  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,6 +5,7 @@
 #include "services/tracing/public/cpp/perfetto/task_runner.h"
 
 #include <memory>
+#include <utility>
 
 #include "base/threading/sequenced_task_runner_handle.h"
 
@@ -38,4 +39,9 @@ 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,6 +33,11 @@ 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,6 +27,16 @@ 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,11 +5,13 @@
 #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"
@@ -22,10 +24,15 @@ namespace tracing {
 
 namespace {
 
+const char kCategoryGroup[] = "foo";
+
 class MockProducerClient : public ProducerClient {
  public:
-  MockProducerClient()
-      : delegate_(perfetto::base::kPageSize), stream_(&delegate_) {
+  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)) {
     trace_packet_.Reset(&stream_);
   }
 
@@ -44,7 +51,10 @@ 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()) {
+      if (proto->has_chrome_events() &&
+          proto->chrome_events().trace_events().size() > 0 &&
+          proto->chrome_events().trace_events()[0].category_group_name() ==
+              kCategoryGroup) {
         finalized_packets_.push_back(std::move(proto));
       }
     }
@@ -79,6 +89,33 @@ 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 {
@@ -103,14 +140,19 @@ class MockTraceWriter : public perfetto::TraceWriter {
 
 std::unique_ptr<perfetto::TraceWriter> MockProducerClient::CreateTraceWriter(
     perfetto::BufferID target_buffer) {
-  return std::make_unique<MockTraceWriter>(this);
+  if (main_thread_task_runner_->RunsTasksInCurrentSequence()) {
+    return std::make_unique<MockTraceWriter>(this);
+  } else {
+    return std::make_unique<DummyTraceWriter>();
+  }
 }
 
 class TraceEventDataSourceTest : public testing::Test {
  public:
   void SetUp() override {
-    message_loop_ = std::make_unique<base::MessageLoop>();
-    producer_client_ = std::make_unique<MockProducerClient>();
+    ProducerClient::ResetTaskRunnerForTesting();
+    producer_client_ = std::make_unique<MockProducerClient>(
+        scoped_task_environment_.GetMainThreadTaskRunner());
   }
 
   void TearDown() override {
@@ -123,13 +165,10 @@ 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. 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.
+    // we need to make sure to clean it up from TLS. The other sequences
+    // get DummyTraceWriters that we don't care about.
     TraceEventDataSource::GetInstance()->ResetCurrentThreadForTesting();
     producer_client_.reset();
-    message_loop_.reset();
   }
 
   void CreateTraceEventDataSource() {
@@ -142,20 +181,20 @@ class TraceEventDataSourceTest : public testing::Test {
 
  private:
   std::unique_ptr<MockProducerClient> producer_client_;
-  std::unique_ptr<base::MessageLoop> message_loop_;
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
 };
 
 TEST_F(TraceEventDataSourceTest, BasicTraceEvent) {
   CreateTraceEventDataSource();
 
-  TRACE_EVENT_BEGIN0("foo", "bar");
+  TRACE_EVENT_BEGIN0(kCategoryGroup, "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("foo", trace_event.category_group_name());
+  EXPECT_EQ(kCategoryGroup, trace_event.category_group_name());
   EXPECT_EQ(TRACE_EVENT_PHASE_BEGIN, trace_event.phase());
 }
 
@@ -163,7 +202,7 @@ TEST_F(TraceEventDataSourceTest, TimestampedTraceEvent) {
   CreateTraceEventDataSource();
 
   TRACE_EVENT_BEGIN_WITH_ID_TID_AND_TIMESTAMP0(
-      "foo", "bar", 42, 4242,
+      kCategoryGroup, "bar", 42, 4242,
       base::TimeTicks() + base::TimeDelta::FromMicroseconds(424242));
 
   auto trace_events = producer_client()->GetChromeTraceEvents();
@@ -171,7 +210,7 @@ TEST_F(TraceEventDataSourceTest, TimestampedTraceEvent) {
 
   auto trace_event = trace_events[0];
   EXPECT_EQ("bar", trace_event.name());
-  EXPECT_EQ("foo", trace_event.category_group_name());
+  EXPECT_EQ(kCategoryGroup, trace_event.category_group_name());
   EXPECT_EQ(42u, trace_event.id());
   EXPECT_EQ(4242, trace_event.thread_id());
   EXPECT_EQ(424242, trace_event.timestamp());
@@ -181,14 +220,14 @@ TEST_F(TraceEventDataSourceTest, TimestampedTraceEvent) {
 TEST_F(TraceEventDataSourceTest, InstantTraceEvent) {
   CreateTraceEventDataSource();
 
-  TRACE_EVENT_INSTANT0("foo", "bar", TRACE_EVENT_SCOPE_THREAD);
+  TRACE_EVENT_INSTANT0(kCategoryGroup, "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("foo", trace_event.category_group_name());
+  EXPECT_EQ(kCategoryGroup, trace_event.category_group_name());
   EXPECT_EQ(TRACE_EVENT_SCOPE_THREAD, trace_event.flags());
   EXPECT_EQ(TRACE_EVENT_PHASE_INSTANT, trace_event.phase());
 }
@@ -196,8 +235,8 @@ TEST_F(TraceEventDataSourceTest, InstantTraceEvent) {
 TEST_F(TraceEventDataSourceTest, EventWithStringArgs) {
   CreateTraceEventDataSource();
 
-  TRACE_EVENT_INSTANT2("foo", "bar", TRACE_EVENT_SCOPE_THREAD, "arg1_name",
-                       "arg1_val", "arg2_name", "arg2_val");
+  TRACE_EVENT_INSTANT2(kCategoryGroup, "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);
@@ -214,8 +253,8 @@ TEST_F(TraceEventDataSourceTest, EventWithStringArgs) {
 TEST_F(TraceEventDataSourceTest, EventWithUIntArgs) {
   CreateTraceEventDataSource();
 
-  TRACE_EVENT_INSTANT2("foo", "bar", TRACE_EVENT_SCOPE_THREAD, "foo", 42u,
-                       "bar", 4242u);
+  TRACE_EVENT_INSTANT2(kCategoryGroup, "bar", TRACE_EVENT_SCOPE_THREAD, "foo",
+                       42u, "bar", 4242u);
 
   auto trace_events = producer_client()->GetChromeTraceEvents();
   EXPECT_EQ(trace_events.size(), 1);
@@ -230,8 +269,8 @@ TEST_F(TraceEventDataSourceTest, EventWithUIntArgs) {
 TEST_F(TraceEventDataSourceTest, EventWithIntArgs) {
   CreateTraceEventDataSource();
 
-  TRACE_EVENT_INSTANT2("foo", "bar", TRACE_EVENT_SCOPE_THREAD, "foo", 42, "bar",
-                       4242);
+  TRACE_EVENT_INSTANT2(kCategoryGroup, "bar", TRACE_EVENT_SCOPE_THREAD, "foo",
+                       42, "bar", 4242);
 
   auto trace_events = producer_client()->GetChromeTraceEvents();
   EXPECT_EQ(trace_events.size(), 1);
@@ -246,8 +285,8 @@ TEST_F(TraceEventDataSourceTest, EventWithIntArgs) {
 TEST_F(TraceEventDataSourceTest, EventWithBoolArgs) {
   CreateTraceEventDataSource();
 
-  TRACE_EVENT_INSTANT2("foo", "bar", TRACE_EVENT_SCOPE_THREAD, "foo", true,
-                       "bar", false);
+  TRACE_EVENT_INSTANT2(kCategoryGroup, "bar", TRACE_EVENT_SCOPE_THREAD, "foo",
+                       true, "bar", false);
 
   auto trace_events = producer_client()->GetChromeTraceEvents();
   EXPECT_EQ(trace_events.size(), 1);
@@ -264,8 +303,8 @@ TEST_F(TraceEventDataSourceTest, EventWithBoolArgs) {
 TEST_F(TraceEventDataSourceTest, EventWithDoubleArgs) {
   CreateTraceEventDataSource();
 
-  TRACE_EVENT_INSTANT2("foo", "bar", TRACE_EVENT_SCOPE_THREAD, "foo", 42.42,
-                       "bar", 4242.42);
+  TRACE_EVENT_INSTANT2(kCategoryGroup, "bar", TRACE_EVENT_SCOPE_THREAD, "foo",
+                       42.42, "bar", 4242.42);
 
   auto trace_events = producer_client()->GetChromeTraceEvents();
   EXPECT_EQ(trace_events.size(), 1);
@@ -280,7 +319,7 @@ TEST_F(TraceEventDataSourceTest, EventWithDoubleArgs) {
 TEST_F(TraceEventDataSourceTest, EventWithPointerArgs) {
   CreateTraceEventDataSource();
 
-  TRACE_EVENT_INSTANT2("foo", "bar", TRACE_EVENT_SCOPE_THREAD, "foo",
+  TRACE_EVENT_INSTANT2(kCategoryGroup, "bar", TRACE_EVENT_SCOPE_THREAD, "foo",
                        reinterpret_cast<void*>(0xBEEF), "bar",
                        reinterpret_cast<void*>(0xF00D));
 
@@ -295,7 +334,6 @@ 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,9 +41,14 @@ class PerfettoTraceEventAgent : public TraceEventAgent {
     connector->BindInterface(mojom::kServiceName, &perfetto_service);
 
     producer_client_ = std::make_unique<ProducerClient>();
-    perfetto_service->ConnectToProducerHost(
-        producer_client_->CreateAndBindProducerClient(),
-        producer_client_->CreateProducerHostRequest());
+    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)));
   }
 
   ~PerfettoTraceEventAgent() override {