Tracing: Remove the Coordinator::StartTracing callback

This callback was previously called when all of the
currently connected tracing agents had ACKed the StartTracing
call, plus any new agents that connected while the controller
was still waiting for any existing agents to ACK. Since agents
can dynamically be added/removed at any time, this particular
callback is not particularly useful; in practice it only got used
for one purpose: For tests and devtools to know when they could
start emitting trace events that'll make it into the trace, after
a StartTracing call.

This CL removes the callback from the tracing::Coordinator mojo
interface, and instead adds a TraceLog observer to the
TracingCoordinatorImpl that lets us more reliably and directly
let tracing clients know when they can start emitting events.

Perfetto doesn't currently have a concept of
"let me know when the Consumers are notified to begin
tracing". so this saves some future implementation work in order to make
the switch.

TBR=yusukes@chromium.org
Bug: 914579

Change-Id: I3fc4bf4a06b93938e053bb13ab098881c485b239
Reviewed-on: https://chromium-review.googlesource.com/c/1439082
Commit-Queue: oysteine <oysteine@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Eric Seckler <eseckler@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#627423}(cherry picked from commit 3d8e0ad7f87d2c3b91cfe528ab9948ce4299a3dd)
Reviewed-on: https://chromium-review.googlesource.com/c/1452365
Reviewed-by: oysteine <oysteine@chromium.org>
Cr-Commit-Position: refs/branch-heads/3683@{#165}
Cr-Branched-From: e51029943e0a38dd794b73caaf6373d5496ae783-refs/heads/master@{#625896}
diff --git a/chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc b/chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc
index 6b2adab..63535a3 100644
--- a/chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc
+++ b/chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc
@@ -344,7 +344,8 @@
 
   if (state_ != State::kDisabled) {
     DLOG(WARNING) << "Cannot start tracing, it is already enabled.";
-    std::move(callback).Run(false /*success*/);
+    if (callback)
+      std::move(callback).Run(false /*success*/);
     return;
   }
   state_ = State::kStarting;
@@ -391,7 +392,8 @@
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK_EQ(State::kStarting, state_);
   state_ = success ? State::kEnabled : State::kDisabled;
-  std::move(callback).Run(success);
+  if (callback)
+    std::move(callback).Run(success);
 }
 
 void ArcTracingBridge::StopAndFlush(TraceDataCallback callback) {
@@ -462,10 +464,9 @@
 
 void ArcTracingBridge::ArcTracingAgent::StartTracing(
     const std::string& config,
-    base::TimeTicks coordinator_time,
-    Agent::StartTracingCallback callback) {
+    base::TimeTicks coordinator_time) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  bridge_->StartTracing(config, std::move(callback));
+  bridge_->StartTracing(config, SuccessCallback());
 }
 
 void ArcTracingBridge::ArcTracingAgent::StopAndFlush(
diff --git a/chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.h b/chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.h
index dc321c2..e77f8a4 100644
--- a/chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.h
+++ b/chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.h
@@ -78,8 +78,7 @@
 
     // tracing::mojom::Agent.
     void StartTracing(const std::string& config,
-                      base::TimeTicks coordinator_time,
-                      Agent::StartTracingCallback callback) override;
+                      base::TimeTicks coordinator_time) override;
     void StopAndFlush(tracing::mojom::RecorderPtr recorder) override;
 
     void OnTraceData(const std::string& data);
diff --git a/content/browser/tracing/background_tracing_manager_impl.cc b/content/browser/tracing/background_tracing_manager_impl.cc
index de8d6c5d..0e18cf6 100644
--- a/content/browser/tracing/background_tracing_manager_impl.cc
+++ b/content/browser/tracing/background_tracing_manager_impl.cc
@@ -508,10 +508,6 @@
     config.SetTraceBufferSizeInEvents(20000);
 #endif
 
-  is_tracing_ = TracingControllerImpl::GetInstance()->StartTracing(
-      config, base::BindOnce(&BackgroundTracingManagerImpl::OnStartTracingDone,
-                             base::Unretained(this), preset));
-
   // Activate the categories immediately. StartTracing eventually does this
   // itself, but asynchronously via PostTask, and in the meantime events will be
   // dropped. This ensures that we start recording events for those categories
@@ -523,6 +519,10 @@
     base::trace_event::TraceLog::GetInstance()->SetEnabled(config, modes);
   }
 
+  is_tracing_ = TracingControllerImpl::GetInstance()->StartTracing(
+      config, base::BindOnce(&BackgroundTracingManagerImpl::OnStartTracingDone,
+                             base::Unretained(this), preset));
+
   RecordBackgroundTracingMetric(RECORDING_ENABLED);
 }
 
diff --git a/content/browser/tracing/cast_tracing_agent.cc b/content/browser/tracing/cast_tracing_agent.cc
index 597c377..7a263c6 100644
--- a/content/browser/tracing/cast_tracing_agent.cc
+++ b/content/browser/tracing/cast_tracing_agent.cc
@@ -279,14 +279,13 @@
 
 // tracing::mojom::Agent. Called by Mojo internals on the UI thread.
 void CastTracingAgent::StartTracing(const std::string& config,
-                                    base::TimeTicks coordinator_time,
-                                    Agent::StartTracingCallback callback) {
+                                    base::TimeTicks coordinator_time) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!session_);
   session_ = std::make_unique<CastSystemTracingSession>(worker_task_runner_);
   session_->StartTracing(
       config, base::BindOnce(&CastTracingAgent::StartTracingCallbackProxy,
-                             base::Unretained(this), std::move(callback)));
+                             base::Unretained(this)));
 }
 
 void CastTracingAgent::StopAndFlush(tracing::mojom::RecorderPtr recorder) {
@@ -306,12 +305,10 @@
 }
 
 void CastTracingAgent::StartTracingCallbackProxy(
-    Agent::StartTracingCallback callback,
     bool success) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (!success)
     session_.reset();
-  std::move(callback).Run(success);
 }
 
 void CastTracingAgent::HandleTraceData(chromecast::SystemTracer::Status status,
diff --git a/content/browser/tracing/cast_tracing_agent.h b/content/browser/tracing/cast_tracing_agent.h
index 7b90f2a..8d59d1c 100644
--- a/content/browser/tracing/cast_tracing_agent.h
+++ b/content/browser/tracing/cast_tracing_agent.h
@@ -33,12 +33,10 @@
 
   // tracing::mojom::Agent. Called by Mojo internals on the UI thread.
   void StartTracing(const std::string& config,
-                    base::TimeTicks coordinator_time,
-                    Agent::StartTracingCallback callback) override;
+                    base::TimeTicks coordinator_time) override;
   void StopAndFlush(tracing::mojom::RecorderPtr recorder) override;
 
-  void StartTracingCallbackProxy(Agent::StartTracingCallback callback,
-                                 bool success);
+  void StartTracingCallbackProxy(bool success);
   void HandleTraceData(chromecast::SystemTracer::Status status,
                        std::string trace_data);
 
diff --git a/content/browser/tracing/cros_tracing_agent.cc b/content/browser/tracing/cros_tracing_agent.cc
index d9e25fe..4f45515 100644
--- a/content/browser/tracing/cros_tracing_agent.cc
+++ b/content/browser/tracing/cros_tracing_agent.cc
@@ -228,13 +228,12 @@
 
 // tracing::mojom::Agent. Called by Mojo internals on the UI thread.
 void CrOSTracingAgent::StartTracing(const std::string& config,
-                                    base::TimeTicks coordinator_time,
-                                    Agent::StartTracingCallback callback) {
+                                    base::TimeTicks coordinator_time) {
   DCHECK(!session_);
   session_ = std::make_unique<CrOSSystemTracingSession>();
   session_->StartTracing(
       config, base::BindOnce(&CrOSTracingAgent::StartTracingCallbackProxy,
-                             base::Unretained(this), std::move(callback)));
+                             base::Unretained(this)));
 }
 
 void CrOSTracingAgent::StopAndFlush(tracing::mojom::RecorderPtr recorder) {
@@ -247,11 +246,9 @@
 }
 
 void CrOSTracingAgent::StartTracingCallbackProxy(
-    Agent::StartTracingCallback callback,
     bool success) {
   if (!success)
     session_.reset();
-  std::move(callback).Run(success);
 }
 
 void CrOSTracingAgent::RecorderProxy(
diff --git a/content/browser/tracing/cros_tracing_agent.h b/content/browser/tracing/cros_tracing_agent.h
index 289db7c..91ae13c 100644
--- a/content/browser/tracing/cros_tracing_agent.h
+++ b/content/browser/tracing/cros_tracing_agent.h
@@ -32,12 +32,10 @@
 
   // tracing::mojom::Agent. Called by Mojo internals on the UI thread.
   void StartTracing(const std::string& config,
-                    base::TimeTicks coordinator_time,
-                    Agent::StartTracingCallback callback) override;
+                    base::TimeTicks coordinator_time) override;
   void StopAndFlush(tracing::mojom::RecorderPtr recorder) override;
 
-  void StartTracingCallbackProxy(Agent::StartTracingCallback callback,
-                                 bool success);
+  void StartTracingCallbackProxy(bool success);
   void RecorderProxy(const scoped_refptr<base::RefCountedString>& events);
 
   std::unique_ptr<CrOSSystemTracingSession> session_;
diff --git a/content/browser/tracing/tracing_controller_browsertest.cc b/content/browser/tracing/tracing_controller_browsertest.cc
index 3abe338..d59699d 100644
--- a/content/browser/tracing/tracing_controller_browsertest.cc
+++ b/content/browser/tracing/tracing_controller_browsertest.cc
@@ -129,29 +129,6 @@
   }
 };
 
-class WaitForTraceLogEnabled
-    : public base::trace_event::TraceLog::EnabledStateObserver {
- public:
-  WaitForTraceLogEnabled() {
-    base::trace_event::TraceLog::GetInstance()->AddEnabledStateObserver(this);
-  }
-
-  ~WaitForTraceLogEnabled() override {
-    base::trace_event::TraceLog::GetInstance()->RemoveEnabledStateObserver(
-        this);
-  }
-
-  void Wait() { wait_for_tracelog_.Run(); }
-
-  // TraceLog::EnabledStateObserver overrides:
-  void OnTraceLogEnabled() override { wait_for_tracelog_.Quit(); }
-
-  void OnTraceLogDisabled() override {}
-
- private:
-  base::RunLoop wait_for_tracelog_;
-};
-
 class TracingControllerTest : public ContentBrowserTest {
  public:
   TracingControllerTest() {}
@@ -245,11 +222,9 @@
       TraceConfig config;
       if (enable_systrace)
         config.EnableSystrace();
-      WaitForTraceLogEnabled wait_for_tracelog;
       bool result = controller->StartTracing(config, std::move(callback));
       ASSERT_TRUE(result);
       run_loop.Run();
-      wait_for_tracelog.Wait();
       EXPECT_EQ(enable_recording_done_callback_count(), 1);
     }
 
@@ -291,11 +266,9 @@
       TraceConfig config = TraceConfig();
       config.EnableArgumentFilter();
 
-      WaitForTraceLogEnabled wait_for_tracelog;
       bool result = controller->StartTracing(config, std::move(callback));
       ASSERT_TRUE(result);
       run_loop.Run();
-      wait_for_tracelog.Wait();
       EXPECT_EQ(enable_recording_done_callback_count(), 1);
     }
 
@@ -332,12 +305,10 @@
       TracingController::StartTracingDoneCallback callback =
           base::BindOnce(&TracingControllerTest::StartTracingDoneCallbackTest,
                          base::Unretained(this), run_loop.QuitClosure());
-      WaitForTraceLogEnabled wait_for_tracelog;
       bool result =
           controller->StartTracing(TraceConfig(), std::move(callback));
       ASSERT_TRUE(result);
       run_loop.Run();
-      wait_for_tracelog.Wait();
       EXPECT_EQ(enable_recording_done_callback_count(), 1);
     }
 
@@ -369,12 +340,10 @@
       TracingController::StartTracingDoneCallback callback =
           base::BindOnce(&TracingControllerTest::StartTracingDoneCallbackTest,
                          base::Unretained(this), run_loop.QuitClosure());
-      WaitForTraceLogEnabled wait_for_tracelog;
       bool result =
           controller->StartTracing(TraceConfig(), std::move(callback));
       ASSERT_TRUE(result);
       run_loop.Run();
-      wait_for_tracelog.Wait();
       EXPECT_EQ(enable_recording_done_callback_count(), 1);
     }
 
diff --git a/content/browser/tracing/tracing_controller_impl.cc b/content/browser/tracing/tracing_controller_impl.cc
index bfc85215..beed93a 100644
--- a/content/browser/tracing/tracing_controller_impl.cc
+++ b/content/browser/tracing/tracing_controller_impl.cc
@@ -116,16 +116,22 @@
 }
 
 TracingControllerImpl::TracingControllerImpl()
-    : delegate_(GetContentClient()->browser()->GetTracingDelegate()) {
+    : delegate_(GetContentClient()->browser()->GetTracingDelegate()),
+      weak_ptr_factory_(this) {
   DCHECK(!g_tracing_controller);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   // Deliberately leaked, like this class.
   base::FileTracing::SetProvider(new FileTracingProviderImpl);
   AddAgents();
+  base::trace_event::TraceLog::GetInstance()->AddAsyncEnabledStateObserver(
+      weak_ptr_factory_.GetWeakPtr());
   g_tracing_controller = this;
 }
 
-TracingControllerImpl::~TracingControllerImpl() = default;
+TracingControllerImpl::~TracingControllerImpl() {
+  base::trace_event::TraceLog::GetInstance()->RemoveAsyncEnabledStateObserver(
+      this);
+}
 
 void TracingControllerImpl::AddAgents() {
   tracing::TracedProcessImpl::GetInstance()->SetTaskRunner(
@@ -327,20 +333,32 @@
   trace_config_ =
       std::make_unique<base::trace_event::TraceConfig>(trace_config);
 
+  start_tracing_done_ = std::move(callback);
   ConnectToServiceIfNeeded();
-  coordinator_->StartTracing(
-      trace_config.ToString(),
-      base::BindOnce(
-          [](StartTracingDoneCallback callback, bool success) {
-            if (!callback.is_null())
-              std::move(callback).Run();
-          },
-          std::move(callback)));
+  coordinator_->StartTracing(trace_config.ToString());
+
+  if (start_tracing_done_ &&
+      (base::trace_event::TraceLog::GetInstance()->IsEnabled() ||
+       !trace_config.process_filter_config().IsEnabled(
+           base::Process::Current().Pid()))) {
+    // If we're already tracing, or if the current process is excluded from the
+    // process filter, we'll never receive a callback from the TraceLog, so then
+    // we just run the callback right away.
+    std::move(start_tracing_done_).Run();
+  }
+
   // TODO(chiniforooshan): The actual success value should be sent by the
   // callback asynchronously.
   return true;
 }
 
+void TracingControllerImpl::OnTraceLogEnabled() {
+  if (start_tracing_done_)
+    std::move(start_tracing_done_).Run();
+}
+
+void TracingControllerImpl::OnTraceLogDisabled() {}
+
 bool TracingControllerImpl::StopTracing(
     const scoped_refptr<TraceDataEndpoint>& trace_data_endpoint) {
   return StopTracing(std::move(trace_data_endpoint), "");
diff --git a/content/browser/tracing/tracing_controller_impl.h b/content/browser/tracing/tracing_controller_impl.h
index dbb0b86..5a6f9e6 100644
--- a/content/browser/tracing/tracing_controller_impl.h
+++ b/content/browser/tracing/tracing_controller_impl.h
@@ -12,6 +12,8 @@
 
 #include "base/callback_forward.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "base/trace_event/trace_log.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/tracing_controller.h"
 #include "mojo/public/cpp/system/data_pipe_drainer.h"
@@ -36,8 +38,10 @@
 class TracingDelegate;
 class TracingUI;
 
-class TracingControllerImpl : public TracingController,
-                              public mojo::DataPipeDrainer::Client {
+class TracingControllerImpl
+    : public TracingController,
+      public mojo::DataPipeDrainer::Client,
+      public base::trace_event::TraceLog::AsyncEnabledStateObserver {
  public:
   // Create an endpoint for dumping the trace data to a callback.
   CONTENT_EXPORT static scoped_refptr<TraceDataEndpoint> CreateCallbackEndpoint(
@@ -83,6 +87,10 @@
   void OnDataAvailable(const void* data, size_t num_bytes) override;
   void OnDataComplete() override;
 
+  // base::trace_event::TraceLog::AsyncEnabledStateObserver
+  void OnTraceLogEnabled() override;
+  void OnTraceLogDisabled() override;
+
   void OnMetadataAvailable(base::Value metadata);
 
   void CompleteFlush();
@@ -91,6 +99,7 @@
   std::vector<std::unique_ptr<tracing::BaseAgent>> agents_;
   std::unique_ptr<TracingDelegate> delegate_;
   std::unique_ptr<base::trace_event::TraceConfig> trace_config_;
+  StartTracingDoneCallback start_tracing_done_;
   std::unique_ptr<mojo::DataPipeDrainer> drainer_;
   scoped_refptr<TraceDataEndpoint> trace_data_endpoint_;
   std::unique_ptr<base::DictionaryValue> filtered_metadata_;
@@ -98,6 +107,8 @@
   bool is_data_complete_ = false;
   bool is_metadata_available_ = false;
 
+  // NOTE: Weak pointers must be invalidated before all other member variables.
+  base::WeakPtrFactory<TracingControllerImpl> weak_ptr_factory_;
   DISALLOW_COPY_AND_ASSIGN(TracingControllerImpl);
 };
 
diff --git a/content/public/browser/tracing_controller.h b/content/public/browser/tracing_controller.h
index c2e3e98d..5115143 100644
--- a/content/public/browser/tracing_controller.h
+++ b/content/public/browser/tracing_controller.h
@@ -75,8 +75,9 @@
   // Tracing begins immediately locally, and asynchronously on child processes
   // as soon as they receive the StartTracing request.
   //
-  // Once all child processes have acked to the StartTracing request,
-  // StartTracingDoneCallback will be called back.
+  // Once tracing is enabled in the current process and trace events can be
+  // emitted (unless excluded from the config), StartTracingDoneCallback will
+  // be called back.
   //
   // |category_filter| is a filter to control what category groups should be
   // traced. A filter can have an optional '-' prefix to exclude category groups
diff --git a/services/tracing/coordinator.cc b/services/tracing/coordinator.cc
index e3fad08..fc9f4f15 100644
--- a/services/tracing/coordinator.cc
+++ b/services/tracing/coordinator.cc
@@ -37,7 +37,6 @@
 const char kMetadataTraceLabel[] = "metadata";
 
 const char kRequestBufferUsageClosureName[] = "RequestBufferUsageClosure";
-const char kStartTracingClosureName[] = "StartTracingClosure";
 
 }  // namespace
 
@@ -299,8 +298,6 @@
     base::ResetAndReturn(&stop_and_flush_callback_)
         .Run(base::Value(base::Value::Type::DICTIONARY));
   }
-  if (!start_tracing_callback_.is_null())
-    base::ResetAndReturn(&start_tracing_callback_).Run(false);
   if (!request_buffer_usage_callback_.is_null())
     base::ResetAndReturn(&request_buffer_usage_callback_).Run(false, 0, 0);
 
@@ -329,11 +326,8 @@
       &Coordinator::OnClientConnectionError, base::Unretained(this)));
 }
 
-void Coordinator::StartTracing(const std::string& config,
-                               StartTracingCallback callback) {
-  bool is_initializing = !start_tracing_callback_.is_null();
-  if (is_initializing || (is_tracing_ && config == config_)) {
-    std::move(callback).Run(config == config_);
+void Coordinator::StartTracing(const std::string& config) {
+  if ((is_tracing_ && config == config_)) {
     return;
   }
 
@@ -344,43 +338,13 @@
       base::BindRepeating(&Coordinator::SendStartTracingToAgent,
                           weak_ptr_factory_.GetWeakPtr()),
       false /* call_on_new_agents_only */);
-
-  // We specifically don't check for the case where there's
-  // no existing connected agents; meaning in that case we
-  // assume at least one *will* connect and we'll wait for
-  // it to do so and start tracing before calling the callback
-  // so that the trace will contain data from at least one agent.
-  start_tracing_callback_ = std::move(callback);
 }
 
 void Coordinator::SendStartTracingToAgent(
     AgentRegistry::AgentEntry* agent_entry) {
-  if (agent_entry->HasDisconnectClosure(&kStartTracingClosureName))
-    return;
   if (!parsed_config_.process_filter_config().IsEnabled(agent_entry->pid()))
     return;
-  agent_entry->AddDisconnectClosure(
-      &kStartTracingClosureName,
-      base::BindOnce(&Coordinator::OnTracingStarted,
-                     weak_ptr_factory_.GetWeakPtr(),
-                     base::Unretained(agent_entry), false));
-  agent_entry->agent()->StartTracing(
-      config_, TRACE_TIME_TICKS_NOW(),
-      base::BindRepeating(&Coordinator::OnTracingStarted,
-                          weak_ptr_factory_.GetWeakPtr(),
-                          base::Unretained(agent_entry)));
-}
-
-void Coordinator::OnTracingStarted(AgentRegistry::AgentEntry* agent_entry,
-                                   bool success) {
-  bool removed =
-      agent_entry->RemoveDisconnectClosure(&kStartTracingClosureName);
-  DCHECK(removed);
-
-  if (!agent_registry_->HasDisconnectClosure(&kStartTracingClosureName) &&
-      !start_tracing_callback_.is_null()) {
-    std::move(start_tracing_callback_).Run(true);
-  }
+  agent_entry->agent()->StartTracing(config_, TRACE_TIME_TICKS_NOW());
 }
 
 void Coordinator::StopAndFlush(mojo::ScopedDataPipeProducerHandle stream,
@@ -408,18 +372,6 @@
 }
 
 void Coordinator::StopAndFlushInternal() {
-  if (agent_registry_->HasDisconnectClosure(&kStartTracingClosureName)) {
-    // We received a |StopAndFlush| command before receiving |StartTracing| acks
-    // from all agents. Let's retry after a delay.
-    task_runner_->PostDelayedTask(
-        FROM_HERE,
-        base::BindRepeating(&Coordinator::StopAndFlushInternal,
-                            weak_ptr_factory_.GetWeakPtr()),
-        base::TimeDelta::FromMilliseconds(
-            mojom::kStopTracingRetryTimeMilliseconds));
-    return;
-  }
-
   size_t num_initialized_agents =
       agent_registry_->SetAgentInitializationCallback(
           base::BindRepeating(&Coordinator::SendStopTracingToAgent,
diff --git a/services/tracing/coordinator.h b/services/tracing/coordinator.h
index eecae55..9eec908 100644
--- a/services/tracing/coordinator.h
+++ b/services/tracing/coordinator.h
@@ -64,8 +64,7 @@
   void Reset();
 
   // mojom::Coordinator
-  void StartTracing(const std::string& config,
-                    StartTracingCallback callback) override;
+  void StartTracing(const std::string& config) override;
   void StopAndFlush(mojo::ScopedDataPipeProducerHandle stream,
                     StopAndFlushCallback callback) override;
   void StopAndFlushAgent(mojo::ScopedDataPipeProducerHandle stream,
@@ -76,7 +75,6 @@
 
   // Internal methods for collecting events from agents.
   void SendStartTracingToAgent(AgentRegistry::AgentEntry* agent_entry);
-  void OnTracingStarted(AgentRegistry::AgentEntry* agent_entry, bool success);
   void StopAndFlushInternal();
   void SendStopTracingToAgent(AgentRegistry::AgentEntry* agent_entry);
   void SendStopTracingWithNoOpRecorderToAgent(
@@ -99,7 +97,6 @@
   bool is_tracing_ = false;
 
   std::unique_ptr<TraceStreamer> trace_streamer_;
-  StartTracingCallback start_tracing_callback_;
   StopAndFlushCallback stop_and_flush_callback_;
 
   // For computing trace buffer usage.
diff --git a/services/tracing/coordinator_unittest.cc b/services/tracing/coordinator_unittest.cc
index f986892..e6e2655 100644
--- a/services/tracing/coordinator_unittest.cc
+++ b/services/tracing/coordinator_unittest.cc
@@ -77,29 +77,7 @@
     return agents_.back().get();
   }
 
-  void StartTracing(std::string config,
-                    bool expected_response,
-                    bool stop_and_flush) {
-    base::RepeatingClosure closure;
-    if (stop_and_flush) {
-      closure = base::BindRepeating(&CoordinatorTest::StopAndFlush,
-                                    base::Unretained(this));
-    }
-
-    coordinator_->StartTracing(
-        config,
-        base::BindRepeating(
-            [](bool expected, base::RepeatingClosure closure, bool actual) {
-              EXPECT_EQ(expected, actual);
-              if (!closure.is_null())
-                closure.Run();
-            },
-            expected_response, closure));
-  }
-
-  void StartTracing(std::string config, bool expected_response) {
-    StartTracing(config, expected_response, false);
-  }
+  void StartTracing(std::string config) { coordinator_->StartTracing(config); }
 
   void StopAndFlush() {
     mojo::DataPipe data_pipe;
@@ -151,7 +129,7 @@
 TEST_F(CoordinatorTest, StartTracingSimple) {
   base::RunLoop run_loop;
   auto* agent = AddArrayAgent();
-  StartTracing("*", true);
+  StartTracing("*");
   run_loop.RunUntilIdle();
 
   // The agent should have received exactly one call from the coordinator.
@@ -162,7 +140,7 @@
 TEST_F(CoordinatorTest, StartTracingTwoAgents) {
   base::RunLoop run_loop;
   auto* agent1 = AddArrayAgent();
-  StartTracing("*", true);
+  StartTracing("*");
   auto* agent2 = AddStringAgent();
   run_loop.RunUntilIdle();
 
@@ -177,13 +155,13 @@
   base::RunLoop run_loop1;
   auto* agent1 = AddArrayAgent(static_cast<base::ProcessId>(1));
   auto* agent2 = AddArrayAgent(static_cast<base::ProcessId>(2));
-  StartTracing("{\"included_process_ids\":[2,4]}", true);
+  StartTracing("{\"included_process_ids\":[2,4]}");
   run_loop1.RunUntilIdle();
 
   base::RunLoop run_loop2;
   auto* agent3 = AddArrayAgent(static_cast<base::ProcessId>(3));
   auto* agent4 = AddArrayAgent(static_cast<base::ProcessId>(4));
-  StartTracing("{\"included_process_ids\":[4,6]}", true);
+  StartTracing("{\"included_process_ids\":[4,6]}");
   run_loop2.RunUntilIdle();
 
   base::RunLoop run_loop3;
@@ -192,38 +170,27 @@
   run_loop3.RunUntilIdle();
 
   // StartTracing should only be received by agents 2, 4, and 6.
+  // Agent 4 should receive StartTracing twice, as it's
+  // included in both configs.
   EXPECT_EQ(0u, agent1->call_stat().size());
   EXPECT_EQ(1u, agent2->call_stat().size());
   EXPECT_EQ("StartTracing", agent2->call_stat()[0]);
   EXPECT_EQ(0u, agent3->call_stat().size());
-  EXPECT_EQ(1u, agent4->call_stat().size());
+  EXPECT_EQ(2u, agent4->call_stat().size());
   EXPECT_EQ("StartTracing", agent4->call_stat()[0]);
+  EXPECT_EQ("StartTracing", agent4->call_stat()[1]);
   EXPECT_EQ(0u, agent5->call_stat().size());
   EXPECT_EQ(1u, agent6->call_stat().size());
   EXPECT_EQ("StartTracing", agent6->call_stat()[0]);
 }
 
-TEST_F(CoordinatorTest, StartTracingWithDifferentConfigs) {
-  base::RunLoop run_loop;
-  auto* agent = AddArrayAgent();
-  StartTracing("config 1", true);
-  // The 2nd |StartTracing| should return false.
-  StartTracing("config 2", false);
-  run_loop.RunUntilIdle();
-
-  // The agent should have received exactly one call from the coordinator
-  // because the 2nd |StartTracing| was aborted.
-  EXPECT_EQ(1u, agent->call_stat().size());
-  EXPECT_EQ("StartTracing", agent->call_stat()[0]);
-}
-
 TEST_F(CoordinatorTest, StartTracingWithSameConfigs) {
   base::RunLoop run_loop;
   auto* agent = AddArrayAgent();
-  StartTracing("config", true);
-  // The 2nd |StartTracing| should return true when we are not trying to change
+  StartTracing("config");
+  // The 2nd |StartTracing| should succeed when we are not trying to change
   // the config.
-  StartTracing("config", true);
+  StartTracing("config");
   run_loop.RunUntilIdle();
 
   // The agent should have received exactly one call from the coordinator
@@ -240,7 +207,8 @@
   agent->data_.push_back("\"content\":{\"a\":1}");
   agent->data_.push_back("\"name\":\"etw\"");
 
-  StartTracing("config", true, true);
+  StartTracing("config");
+  StopAndFlush();
   if (!quit_closure_.is_null())
     run_loop.Run();
 
@@ -265,7 +233,8 @@
   agent2->data_.push_back("e3");
   agent2->data_.push_back("e4");
 
-  StartTracing("config", true, true);
+  StartTracing("config");
+  StopAndFlush();
   if (!quit_closure_.is_null())
     run_loop.Run();
 
@@ -303,7 +272,8 @@
   agent2->data_.push_back("e3");
   agent2->data_.push_back("e4");
 
-  StartTracing("config", true, true);
+  StartTracing("config");
+  StopAndFlush();
   if (!quit_closure_.is_null())
     run_loop.Run();
 
@@ -328,7 +298,8 @@
   agent->data_.push_back("event");
   agent->metadata_.SetString("key", "value");
 
-  StartTracing("config", true, true);
+  StartTracing("config");
+  StopAndFlush();
   if (!quit_closure_.is_null())
     run_loop.Run();
 
@@ -343,7 +314,7 @@
 TEST_F(CoordinatorTest, IsTracing) {
   base::RunLoop run_loop;
   AddArrayAgent();
-  StartTracing("config", true);
+  StartTracing("config");
   IsTracing(true);
   run_loop.RunUntilIdle();
 }
@@ -392,7 +363,8 @@
   base::RunLoop run_loop1;
   quit_closure_ = run_loop1.QuitClosure();
   auto* agent1 = AddArrayAgent();
-  StartTracing("config", true, true);
+  StartTracing("config");
+  StopAndFlush();
   if (!quit_closure_.is_null())
     run_loop1.Run();
 
diff --git a/services/tracing/perfetto/perfetto_tracing_coordinator.cc b/services/tracing/perfetto/perfetto_tracing_coordinator.cc
index 1916281..83d60e4f 100644
--- a/services/tracing/perfetto/perfetto_tracing_coordinator.cc
+++ b/services/tracing/perfetto/perfetto_tracing_coordinator.cc
@@ -105,13 +105,11 @@
                      base::Unretained(this)));
 }
 
-void PerfettoTracingCoordinator::StartTracing(const std::string& config,
-                                              StartTracingCallback callback) {
+void PerfettoTracingCoordinator::StartTracing(const std::string& config) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   tracing_session_ = std::make_unique<TracingSession>(
       config, base::BindOnce(&PerfettoTracingCoordinator::OnTracingOverCallback,
                              weak_factory_.GetWeakPtr()));
-  std::move(callback).Run(true);
 }
 
 void PerfettoTracingCoordinator::OnTracingOverCallback() {
diff --git a/services/tracing/perfetto/perfetto_tracing_coordinator.h b/services/tracing/perfetto/perfetto_tracing_coordinator.h
index 26b53ed..29ff7ad 100644
--- a/services/tracing/perfetto/perfetto_tracing_coordinator.h
+++ b/services/tracing/perfetto/perfetto_tracing_coordinator.h
@@ -37,8 +37,7 @@
 
   // mojom::Coordinator implementation.
   // Called by the tracing controller.
-  void StartTracing(const std::string& config,
-                    StartTracingCallback callback) override;
+  void StartTracing(const std::string& config) override;
   void StopAndFlush(mojo::ScopedDataPipeProducerHandle stream,
                     StopAndFlushCallback callback) override;
   void StopAndFlushAgent(mojo::ScopedDataPipeProducerHandle stream,
diff --git a/services/tracing/public/cpp/base_agent.cc b/services/tracing/public/cpp/base_agent.cc
index 4c16162..99540d2 100644
--- a/services/tracing/public/cpp/base_agent.cc
+++ b/services/tracing/public/cpp/base_agent.cc
@@ -38,10 +38,7 @@
 }
 
 void BaseAgent::StartTracing(const std::string& config,
-                             base::TimeTicks coordinator_time,
-                             Agent::StartTracingCallback callback) {
-  std::move(callback).Run(true /* success */);
-}
+                             base::TimeTicks coordinator_time) {}
 
 void BaseAgent::StopAndFlush(tracing::mojom::RecorderPtr recorder) {}
 
diff --git a/services/tracing/public/cpp/base_agent.h b/services/tracing/public/cpp/base_agent.h
index 8be2cf5..dc1325d 100644
--- a/services/tracing/public/cpp/base_agent.h
+++ b/services/tracing/public/cpp/base_agent.h
@@ -35,8 +35,7 @@
 
   // tracing::mojom::Agent:
   void StartTracing(const std::string& config,
-                    base::TimeTicks coordinator_time,
-                    Agent::StartTracingCallback callback) override;
+                    base::TimeTicks coordinator_time) override;
   void StopAndFlush(tracing::mojom::RecorderPtr recorder) override;
   void RequestBufferStatus(
       Agent::RequestBufferStatusCallback callback) override;
diff --git a/services/tracing/public/cpp/trace_event_agent.cc b/services/tracing/public/cpp/trace_event_agent.cc
index 94cc70a..88e25d8 100644
--- a/services/tracing/public/cpp/trace_event_agent.cc
+++ b/services/tracing/public/cpp/trace_event_agent.cc
@@ -72,8 +72,7 @@
 }
 
 void TraceEventAgent::StartTracing(const std::string& config,
-                                   base::TimeTicks coordinator_time,
-                                   StartTracingCallback callback) {
+                                   base::TimeTicks coordinator_time) {
   DCHECK(!recorder_);
 #if defined(__native_client__)
   // NaCl and system times are offset by a bit, so subtract some time from
@@ -88,7 +87,6 @@
     enabled_tracing_modes_ |= base::trace_event::TraceLog::FILTERING_MODE;
   base::trace_event::TraceLog::GetInstance()->SetEnabled(
       trace_config, enabled_tracing_modes_);
-  std::move(callback).Run(true);
 }
 
 void TraceEventAgent::StopAndFlush(mojom::RecorderPtr recorder) {
diff --git a/services/tracing/public/cpp/trace_event_agent.h b/services/tracing/public/cpp/trace_event_agent.h
index 70063d2..1a7c891 100644
--- a/services/tracing/public/cpp/trace_event_agent.h
+++ b/services/tracing/public/cpp/trace_event_agent.h
@@ -49,8 +49,7 @@
 
   // mojom::Agent
   void StartTracing(const std::string& config,
-                    base::TimeTicks coordinator_time,
-                    StartTracingCallback callback) override;
+                    base::TimeTicks coordinator_time) override;
   void StopAndFlush(mojom::RecorderPtr recorder) override;
 
   void RequestBufferStatus(RequestBufferStatusCallback callback) override;
diff --git a/services/tracing/public/cpp/trace_event_agent_unittest.cc b/services/tracing/public/cpp/trace_event_agent_unittest.cc
index 9749aac..443dd11 100644
--- a/services/tracing/public/cpp/trace_event_agent_unittest.cc
+++ b/services/tracing/public/cpp/trace_event_agent_unittest.cc
@@ -90,8 +90,7 @@
   void StartTracing(const std::string& categories) {
     TraceEventAgent::GetInstance()->StartTracing(
         base::trace_event::TraceConfig(categories, "").ToString(),
-        base::TimeTicks::Now(),
-        base::BindRepeating([](bool success) { EXPECT_TRUE(success); }));
+        base::TimeTicks::Now());
   }
 
   void StopAndFlush(base::Closure quit_closure) {
diff --git a/services/tracing/public/mojom/constants.mojom b/services/tracing/public/mojom/constants.mojom
index f62a072..d258406 100644
--- a/services/tracing/public/mojom/constants.mojom
+++ b/services/tracing/public/mojom/constants.mojom
@@ -4,8 +4,6 @@
 
 module tracing.mojom;
 
-const uint32 kStopTracingRetryTimeMilliseconds = 100;
-
 const string kServiceName = "tracing";
 
 // The label of agents that provide trace data of the format explained in
diff --git a/services/tracing/public/mojom/tracing.mojom b/services/tracing/public/mojom/tracing.mojom
index 99d6cfd..41d170d 100644
--- a/services/tracing/public/mojom/tracing.mojom
+++ b/services/tracing/public/mojom/tracing.mojom
@@ -51,8 +51,7 @@
 // close the recorder connection to signal the tracing service that no more data
 // will be sent.
 interface Agent {
-  StartTracing(string config, mojo_base.mojom.TimeTicks coordinator_time)
-      => (bool success);
+  StartTracing(string config, mojo_base.mojom.TimeTicks coordinator_time);
   StopAndFlush(Recorder recorder);
   RequestBufferStatus() => (uint32 capacity, uint32 count);
 };
@@ -73,7 +72,7 @@
   // The return value is false if tracing is already enabled with a different
   // config. Otherwise, true is returned as soon as the service receives acks
   // from all existing agents and agents that connect during |StartTracing|.
-  StartTracing(string config) => (bool success);
+  StartTracing(string config);
   StopAndFlush(handle<data_pipe_producer> stream)
       => (mojo_base.mojom.DictionaryValue metadata);
   // Same as |StopAndFlush| but only write data from a certain |agent_label| to
diff --git a/services/tracing/test_util.cc b/services/tracing/test_util.cc
index 1bce1291..e514302 100644
--- a/services/tracing/test_util.cc
+++ b/services/tracing/test_util.cc
@@ -21,10 +21,8 @@
 }
 
 void MockAgent::StartTracing(const std::string& config,
-                             base::TimeTicks coordinator_time,
-                             StartTracingCallback cb) {
+                             base::TimeTicks coordinator_time) {
   call_stat_.push_back("StartTracing");
-  std::move(cb).Run(true);
 }
 
 void MockAgent::StopAndFlush(mojom::RecorderPtr recorder) {
diff --git a/services/tracing/test_util.h b/services/tracing/test_util.h
index 821a7eb..948f30c 100644
--- a/services/tracing/test_util.h
+++ b/services/tracing/test_util.h
@@ -37,8 +37,7 @@
  private:
   // mojom::Agent
   void StartTracing(const std::string& config,
-                    base::TimeTicks coordinator_time,
-                    StartTracingCallback cb) override;
+                    base::TimeTicks coordinator_time) override;
   void StopAndFlush(mojom::RecorderPtr recorder) override;
   void RequestBufferStatus(RequestBufferStatusCallback cb) override;