tracing: reland the coordinator implementation

The original CL (crrev.com/3a1994e) was reverted in crrev.com/3a8e71c,
because the tests were failing on Win7:

https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%281%29/builds/68107

I could not reproduce the failure on a Win7 machine. I have some
speculative fixes here and a little bit more logging so that if the
tests fail again, maybe I have a little bit more information.

Summary of changes:
- The tracing service clears the callback before running it. This way,
  the callback can call service methods successfully. Before, we would
  clear the callback after the callback was finished and so we could
  not call service methods during the callback because the service
  would think the previous call is not finalized yet.

- StopAndFlush* tests check if flushing is already done before running
  the RunLoop. This is for the hypothetical case that mojo calls do
  not generate tasks and everything is running directly. Then, by the
  time StopAndFlush is finished the events are already flushed and the
  task queue is empty. So, running the RunLoop will time out.

- RequestBufferUsage verifies that all disconnect closures are indeed
  cleared up after the RunLoop quits. I added this so that if the test
  fails again at least I know whether RequestBufferUsage was actually
  finalized or there is a pending disconnect closure for some reason.

Original CL description:

The coordinator is the main part of the tracing service. It provides
an API for trace collection to be used by the tracing UI, devtools,
etc. It coordinates tracing agents through the agent registry.

A prototype of the final product (a little bit stale and incomplete):
https://codereview.chromium.org/2833873003/

Issuing clock sync markers is not implemented in this CL. I think it
deserves to be sent and reviewed in a separate CL.

Design doc (use @chromium.org account):
https://docs.google.com/document/d/1osLqctK_rTiioKFOG9wDLkrCQ9DLJZmm4j-S335u-vM

BUG=640235

Change-Id: I5904946171851da0a90313c3a8164134fb275c01
Reviewed-on: https://chromium-review.googlesource.com/541556
Commit-Queue: Ehsan Chiniforooshan <chiniforooshan@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Primiano Tucci <primiano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#484653}

services/resource_coordinator/BUILD.gn

@@ -79,7 +79,10 @@ source_set("tests") {
     "public/cpp/memory_instrumentation/process_metrics_memory_dump_provider_unittest.cc",
     "public/cpp/tracing/chrome_trace_event_agent_unittest.cc",
     "tracing/agent_registry_unittest.cc",
+    "tracing/coordinator_unittest.cc",
     "tracing/recorder_unittest.cc",
+    "tracing/test_util.cc",
+    "tracing/test_util.h",
   ]
 
   if (!is_android) {

services/resource_coordinator/public/cpp/tracing/chrome_trace_event_agent.cc

@@ -30,9 +30,7 @@ ChromeTraceEventAgent::ChromeTraceEventAgent(
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK(!g_chrome_trace_event_agent);
   g_chrome_trace_event_agent = this;
-  // agent_registry can be null in tests. The constructor is private and cannot
-  // be directly used in non-test scenarios. GetOrCreateInstance makes sure that
-  // the constructor is called with a non-null agent_registry.
+  // agent_registry can be null in tests.
   if (!agent_registry)
     return;
 
@@ -55,10 +53,8 @@ void ChromeTraceEventAgent::AddMetadataGeneratorFunction(
 }
 
 void ChromeTraceEventAgent::StartTracing(const std::string& config,
-                                         mojom::RecorderPtr recorder,
                                          const StartTracingCallback& callback) {
   DCHECK(!recorder_);
-  recorder_ = std::move(recorder);
   if (!base::trace_event::TraceLog::GetInstance()->IsEnabled()) {
     base::trace_event::TraceLog::GetInstance()->SetEnabled(
         base::trace_event::TraceConfig(config),
@@ -67,10 +63,10 @@ void ChromeTraceEventAgent::StartTracing(const std::string& config,
   callback.Run();
 }
 
-void ChromeTraceEventAgent::StopAndFlush() {
+void ChromeTraceEventAgent::StopAndFlush(mojom::RecorderPtr recorder) {
+  DCHECK(!recorder_);
+  recorder_ = std::move(recorder);
   base::trace_event::TraceLog::GetInstance()->SetDisabled();
-  if (!recorder_)
-    return;
   for (const auto& generator : metadata_generator_functions_) {
     auto metadata = generator.Run();
     if (metadata)

services/resource_coordinator/public/cpp/tracing/chrome_trace_event_agent.h

@@ -27,20 +27,20 @@ class SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_EXPORT ChromeTraceEventAgent
 
   static ChromeTraceEventAgent* GetInstance();
 
+  explicit ChromeTraceEventAgent(mojom::AgentRegistryPtr agent_registry);
+
   void AddMetadataGeneratorFunction(MetadataGeneratorFunction generator);
 
  private:
   friend std::default_delete<ChromeTraceEventAgent>;  // For Testing
   friend class ChromeTraceEventAgentTest;             // For Testing
 
-  explicit ChromeTraceEventAgent(mojom::AgentRegistryPtr agent_registry);
   ~ChromeTraceEventAgent() override;
 
   // mojom::Agent
   void StartTracing(const std::string& config,
-                    mojom::RecorderPtr recorder,
                     const StartTracingCallback& callback) override;
-  void StopAndFlush() override;
+  void StopAndFlush(mojom::RecorderPtr recorder) override;
   void RequestClockSyncMarker(
       const std::string& sync_id,
       const RequestClockSyncMarkerCallback& callback) override;

services/resource_coordinator/public/cpp/tracing/chrome_trace_event_agent_unittest.cc

@@ -89,16 +89,18 @@ class ChromeTraceEventAgentTest : public testing::Test {
     message_loop_.reset();
   }
 
-  void StartTracing(const std::string& categories, base::Closure quit_closure) {
-    mojom::RecorderPtr recorder_ptr;
-    recorder_.reset(new MockRecorder(MakeRequest(&recorder_ptr)));
-    recorder_->set_quit_closure(quit_closure);
+  void StartTracing(const std::string& categories) {
     agent_->StartTracing(
         base::trace_event::TraceConfig(categories, "").ToString(),
-        std::move(recorder_ptr), base::BindRepeating([] {}));
+        base::BindRepeating([] {}));
   }
 
-  void StopAndFlush() { agent_->StopAndFlush(); }
+  void StopAndFlush(base::Closure quit_closure) {
+    mojom::RecorderPtr recorder_ptr;
+    recorder_.reset(new MockRecorder(MakeRequest(&recorder_ptr)));
+    recorder_->set_quit_closure(quit_closure);
+    agent_->StopAndFlush(std::move(recorder_ptr));
+  }
 
   void AddMetadataGeneratorFunction(
       ChromeTraceEventAgent::MetadataGeneratorFunction generator) {
@@ -130,19 +132,19 @@ class ChromeTraceEventAgentTest : public testing::Test {
 TEST_F(ChromeTraceEventAgentTest, StartTracing) {
   EXPECT_FALSE(base::trace_event::TraceLog::GetInstance()->IsEnabled());
   base::RunLoop run_loop;
-  StartTracing("*", run_loop.QuitClosure());
+  StartTracing("*");
   EXPECT_TRUE(base::trace_event::TraceLog::GetInstance()->IsEnabled());
-  StopAndFlush();
+  StopAndFlush(run_loop.QuitClosure());
   run_loop.Run();
 }
 
 TEST_F(ChromeTraceEventAgentTest, StopAndFlushEvents) {
   EXPECT_FALSE(base::trace_event::TraceLog::GetInstance()->IsEnabled());
   base::RunLoop run_loop;
-  StartTracing(kTestCategory, run_loop.QuitClosure());
+  StartTracing(kTestCategory);
   TRACE_EVENT_INSTANT0(kTestCategory, "event1", TRACE_EVENT_SCOPE_THREAD);
   TRACE_EVENT_INSTANT0(kTestCategory, "event2", TRACE_EVENT_SCOPE_THREAD);
-  StopAndFlush();
+  StopAndFlush(run_loop.QuitClosure());
   run_loop.Run();
 
   auto* mock_recorder = recorder();
@@ -167,8 +169,8 @@ TEST_F(ChromeTraceEventAgentTest, StopAndFlushMetadata) {
     metadata_dict->SetString(kTestMetadataKey, "test metadata");
     return metadata_dict;
   }));
-  StartTracing(kTestCategory, run_loop.QuitClosure());
-  StopAndFlush();
+  StartTracing(kTestCategory);
+  StopAndFlush(run_loop.QuitClosure());
   run_loop.Run();
 
   auto* mock_recorder = recorder();

services/resource_coordinator/public/interfaces/BUILD.gn

@@ -16,6 +16,7 @@ mojom("interfaces_internal") {
     "service_callbacks.mojom",
     "service_constants.mojom",
     "tracing/tracing.mojom",
+    "tracing/tracing_constants.mojom",
   ]
 
   public_deps = [

services/resource_coordinator/public/interfaces/tracing/tracing.mojom

@@ -31,12 +31,12 @@ interface AgentRegistry {
 };
 
 // When the tracing service calls |StopAndFlush| on an agent, the agent begins
-// serializing data into the recorder that was given in the |StartTracing| call.
-// When finished, the agent should close the recorder connection to signal the
-// tracing service that no more data will be sent.
+// serializing data into the given recorder.  When finished, the agent should
+// close the recorder connection to signal the tracing service that no more data
+// will be sent.
 interface Agent {
-  StartTracing(string config, Recorder recorder) => ();
-  StopAndFlush();
+  StartTracing(string config) => ();
+  StopAndFlush(Recorder recorder);
   RequestClockSyncMarker(string sync_id) => (
       mojo.common.mojom.TimeTicks issue_ts,
       mojo.common.mojom.TimeTicks issue_end_ts);
@@ -57,10 +57,13 @@ interface Recorder {
 // from all registered agents. At any given time, there should be at most one
 // connected controller.
 interface Coordinator {
-  StartTracing(handle<data_pipe_producer> stream, string config);
-  StopAndFlush();
+  // 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);
+  StopAndFlush(handle<data_pipe_producer> stream);
   IsTracing() => (bool is_tracing);
   RequestBufferUsage() => (bool success, float percent_full,
                            uint32 approximate_count);
-  GetCategories() => (string categories);
+  GetCategories() => (bool success, string categories);
 };

services/resource_coordinator/public/interfaces/tracing/tracing_constants.mojom

@@ -0,0 +1,7 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+module tracing.mojom;
+
+const uint32 kStopTracingRetryTimeMilliseconds = 100;

services/resource_coordinator/tracing/BUILD.gn

@@ -6,6 +6,8 @@ source_set("lib") {
   sources = [
     "agent_registry.cc",
     "agent_registry.h",
+    "coordinator.cc",
+    "coordinator.h",
     "recorder.cc",
     "recorder.h",
   ]

services/resource_coordinator/tracing/agent_registry.cc

@@ -37,25 +37,30 @@ AgentRegistry::AgentEntry::AgentEntry(size_t id,
 
 AgentRegistry::AgentEntry::~AgentEntry() = default;
 
-void AgentRegistry::AgentEntry::SetDisconnectClosure(
+void AgentRegistry::AgentEntry::AddDisconnectClosure(
+    const void* closure_name,
     base::OnceClosure closure) {
-  DCHECK(closure_.is_null());
-  closure_ = std::move(closure);
+  DCHECK_EQ(0u, closures_.count(closure_name));
+  closures_[closure_name] = std::move(closure);
 }
 
-bool AgentRegistry::AgentEntry::RemoveDisconnectClosure() {
-  bool closure_was_set = !closure_.is_null();
-  closure_.Reset();
-  return closure_was_set;
+bool AgentRegistry::AgentEntry::RemoveDisconnectClosure(
+    const void* closure_name) {
+  return closures_.erase(closure_name) > 0;
+}
+
+bool AgentRegistry::AgentEntry::HasDisconnectClosure(const void* closure_name) {
+  return closures_.count(closure_name) > 0;
 }
 
 void AgentRegistry::AgentEntry::OnConnectionError() {
-  // Run the disconnect closure if it is set. We should mark |closure_| as
-  // movable so that the version of |Run| that takes an rvalue reference is
+  // Run disconnect closures if there is any. We should mark |key_value.second|
+  // as movable so that the version of |Run| that takes an rvalue reference is
   // selected not the version that takes a const reference. The former is for
   // once callbacks and the latter is for repeating callbacks.
-  if (!closure_.is_null())
-    std::move(closure_).Run();
+  for (auto& key_value : closures_) {
+    std::move(key_value.second).Run();
+  }
   agent_registry_->UnregisterAgent(id_);
 }
 
@@ -95,6 +100,15 @@ void AgentRegistry::RemoveAgentInitializationCallback() {
   agent_initialization_callback_.Reset();
 }
 
+bool AgentRegistry::HasDisconnectClosure(const void* closure_name) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  for (const auto& key_value : agents_) {
+    if (key_value.second->HasDisconnectClosure(closure_name))
+      return true;
+  }
+  return false;
+}
+
 void AgentRegistry::RegisterAgent(mojom::AgentPtr agent,
                                   const std::string& label,
                                   mojom::TraceDataType type,

services/resource_coordinator/tracing/agent_registry.h

@@ -30,11 +30,13 @@ class AgentRegistry : public mojom::AgentRegistry {
                bool supports_explicit_clock_sync);
     ~AgentEntry();
 
-    // Currently, at most one callback when the tracing agent is disconnected is
-    // enough. We can generalize this later if several parts of the service need
-    // to get notified when an agent disconnects.
-    void SetDisconnectClosure(base::OnceClosure closure);
-    bool RemoveDisconnectClosure();
+    void AddDisconnectClosure(const void* closure_name,
+                              base::OnceClosure closure);
+    bool RemoveDisconnectClosure(const void* closure_name);
+    bool HasDisconnectClosure(const void* closure_name);
+    size_t num_disconnect_closures_for_testing() const {
+      return closures_.size();
+    }
 
     mojom::Agent* agent() const { return agent_.get(); }
     const std::string& label() const { return label_; }
@@ -52,7 +54,7 @@ class AgentRegistry : public mojom::AgentRegistry {
     const std::string label_;
     const mojom::TraceDataType type_;
     const bool supports_explicit_clock_sync_;
-    base::OnceClosure closure_;
+    std::map<const void*, base::OnceClosure> closures_;
 
     DISALLOW_COPY_AND_ASSIGN(AgentEntry);
   };
@@ -71,6 +73,7 @@ class AgentRegistry : public mojom::AgentRegistry {
   void SetAgentInitializationCallback(
       const AgentInitializationCallback& callback);
   void RemoveAgentInitializationCallback();
+  bool HasDisconnectClosure(const void* closure_name);
 
   template <typename FunctionType>
   void ForAllAgents(FunctionType function) {
@@ -83,6 +86,7 @@ class AgentRegistry : public mojom::AgentRegistry {
  private:
   friend std::default_delete<AgentRegistry>;
   friend class AgentRegistryTest;  // For testing.
+  friend class CoordinatorTest;    // For testing.
 
   ~AgentRegistry() override;
 

services/resource_coordinator/tracing/agent_registry_unittest.cc

@@ -10,38 +10,12 @@
 
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
-#include "mojo/public/cpp/bindings/binding.h"
 #include "services/resource_coordinator/public/interfaces/tracing/tracing.mojom.h"
+#include "services/resource_coordinator/tracing/test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace tracing {
 
-class MockAgent : public mojom::Agent {
- public:
-  MockAgent() : binding_(this) {}
-
-  mojom::AgentPtr CreateAgentPtr() {
-    mojom::AgentPtr agent;
-    binding_.Bind(mojo::MakeRequest(&agent));
-    return agent;
-  }
-
- private:
-  // mojom::Agent
-  void StartTracing(const std::string& config,
-                    mojom::RecorderPtr recorder,
-                    const StartTracingCallback& callback) override {}
-  void StopAndFlush() override {}
-  void RequestClockSyncMarker(
-      const std::string& sync_id,
-      const RequestClockSyncMarkerCallback& callback) override {}
-  void RequestBufferStatus(
-      const RequestBufferStatusCallback& callback) override {}
-  void GetCategories(const GetCategoriesCallback& callback) override {}
-
-  mojo::Binding<mojom::Agent> binding_;
-};
-
 class AgentRegistryTest : public testing::Test {
  public:
   void SetUp() override {