tracing: mojom changes and clock sync marker impl

- The coordinator requests clock sync markers before stopping tracing
  from all agents that support it.

- The coordinator returns the metadata separately, to be used in crash
  service uploader.

- StopAndFlush now supports flusing a specific agent only. DevTools,
  for example, just cares about chrome trace events and not other
  agents. So it can request to receive events only from agents that
  produce chrome trace events.

- A bug in AgentRegistry is fixed where we could invalidate an
  iterator inside a loop.

BUG=640235

Change-Id: I4a8ba3662a72cafc11b4ef81938e74d094b4c94c
Reviewed-on: https://chromium-review.googlesource.com/610420
Commit-Queue: Ehsan Chiniforooshan <chiniforooshan@chromium.org>
Reviewed-by: Oystein Eftevaag <oysteine@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494909}

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

@@ -4,17 +4,26 @@
 
 #include "services/resource_coordinator/public/cpp/tracing/chrome_trace_event_agent.h"
 
+#include <string>
+#include <utility>
+#include <vector>
+
 #include "base/bind.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/strings/string_util.h"
 #include "base/threading/thread_checker.h"
+#include "base/time/time.h"
 #include "base/trace_event/trace_log.h"
 #include "base/values.h"
 #include "services/service_manager/public/cpp/connector.h"
 
 namespace {
+
+const char kChromeTraceEventLabel[] = "traceEvents";
+
 tracing::ChromeTraceEventAgent* g_chrome_trace_event_agent;
+
 }  // namespace
 
 namespace tracing {
@@ -25,18 +34,23 @@ ChromeTraceEventAgent* ChromeTraceEventAgent::GetInstance() {
 }
 
 ChromeTraceEventAgent::ChromeTraceEventAgent(
-    mojom::AgentRegistryPtr agent_registry)
-    : binding_(this) {
+    service_manager::Connector* connector,
+    const std::string& service_name)
+    : binding_(this), enabled_tracing_modes_(0) {
   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.
-  if (!agent_registry)
+  // |connector| can be null in tests.
+  if (!connector)
     return;
 
+  // Connecto to the agent registry interface.
+  tracing::mojom::AgentRegistryPtr agent_registry;
+  connector->BindInterface(service_name, &agent_registry);
+
   mojom::AgentPtr agent;
   binding_.Bind(mojo::MakeRequest(&agent));
-  agent_registry->RegisterAgent(std::move(agent), "traceEvents",
+  agent_registry->RegisterAgent(std::move(agent), kChromeTraceEventLabel,
                                 mojom::TraceDataType::ARRAY,
                                 false /* supports_explicit_clock_sync */);
 }
@@ -53,20 +67,31 @@ void ChromeTraceEventAgent::AddMetadataGeneratorFunction(
 }
 
 void ChromeTraceEventAgent::StartTracing(const std::string& config,
+                                         base::TimeTicks coordinator_time,
                                          const StartTracingCallback& callback) {
   DCHECK(!recorder_);
-  if (!base::trace_event::TraceLog::GetInstance()->IsEnabled()) {
-    base::trace_event::TraceLog::GetInstance()->SetEnabled(
-        base::trace_event::TraceConfig(config),
-        base::trace_event::TraceLog::RECORDING_MODE);
-  }
-  callback.Run();
+#if defined(__native_client__)
+  // NaCl and system times are offset by a bit, so subtract some time from
+  // the captured timestamps. The value might be off by a bit due to messaging
+  // latency.
+  base::TimeDelta time_offset = base::TimeTicks::Now() - coordinator_time;
+  TraceLog::GetInstance()->SetTimeOffset(time_offset);
+#endif
+  enabled_tracing_modes_ = base::trace_event::TraceLog::RECORDING_MODE;
+  const base::trace_event::TraceConfig trace_config(config);
+  if (!trace_config.event_filters().empty())
+    enabled_tracing_modes_ |= base::trace_event::TraceLog::FILTERING_MODE;
+  base::trace_event::TraceLog::GetInstance()->SetEnabled(
+      trace_config, enabled_tracing_modes_);
+  callback.Run(true);
 }
 
 void ChromeTraceEventAgent::StopAndFlush(mojom::RecorderPtr recorder) {
   DCHECK(!recorder_);
   recorder_ = std::move(recorder);
-  base::trace_event::TraceLog::GetInstance()->SetDisabled();
+  base::trace_event::TraceLog::GetInstance()->SetDisabled(
+      enabled_tracing_modes_);
+  enabled_tracing_modes_ = 0;
   for (const auto& generator : metadata_generator_functions_) {
     auto metadata = generator.Run();
     if (metadata)

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

@@ -5,17 +5,25 @@
 #ifndef SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_TRACING_CHROME_TRACE_EVENT_AGENT_H_
 #define SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_TRACING_CHROME_TRACE_EVENT_AGENT_H_
 
+#include <memory>
+#include <string>
+#include <vector>
+
 #include "base/memory/ref_counted.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/threading/thread_checker.h"
+#include "base/values.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "services/resource_coordinator/public/cpp/resource_coordinator_export.h"
 #include "services/resource_coordinator/public/interfaces/tracing/tracing.mojom.h"
-#include "services/service_manager/public/cpp/connector.h"
 
 namespace base {
-class DictionaryValue;
-}
+class TimeTicks;
+}  // namespace base
+
+namespace service_manager {
+class Connector;
+}  // namespace service_manager
 
 namespace tracing {
 
@@ -27,7 +35,8 @@ class SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_EXPORT ChromeTraceEventAgent
 
   static ChromeTraceEventAgent* GetInstance();
 
-  explicit ChromeTraceEventAgent(mojom::AgentRegistryPtr agent_registry);
+  explicit ChromeTraceEventAgent(service_manager::Connector* connector,
+                                 const std::string& service_name);
 
   void AddMetadataGeneratorFunction(MetadataGeneratorFunction generator);
 
@@ -39,6 +48,7 @@ class SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_EXPORT ChromeTraceEventAgent
 
   // mojom::Agent
   void StartTracing(const std::string& config,
+                    base::TimeTicks coordinator_time,
                     const StartTracingCallback& callback) override;
   void StopAndFlush(mojom::RecorderPtr recorder) override;
   void RequestClockSyncMarker(
@@ -52,6 +62,7 @@ class SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_EXPORT ChromeTraceEventAgent
                        bool has_more_events);
 
   mojo::Binding<mojom::Agent> binding_;
+  uint8_t enabled_tracing_modes_;
   mojom::RecorderPtr recorder_;
   std::vector<MetadataGeneratorFunction> metadata_generator_functions_;
   bool trace_log_needs_me_ = false;

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

@@ -9,6 +9,7 @@
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/test/trace_event_analyzer.h"
+#include "base/time/time.h"
 #include "base/trace_event/trace_config.h"
 #include "base/trace_event/trace_event.h"
 #include "base/trace_event/trace_log.h"
@@ -80,7 +81,7 @@ class ChromeTraceEventAgentTest : public testing::Test {
  public:
   void SetUp() override {
     message_loop_.reset(new base::MessageLoop());
-    agent_.reset(new ChromeTraceEventAgent(nullptr));
+    agent_.reset(new ChromeTraceEventAgent(nullptr, ""));
   }
 
   void TearDown() override {
@@ -93,7 +94,8 @@ class ChromeTraceEventAgentTest : public testing::Test {
   void StartTracing(const std::string& categories) {
     agent_->StartTracing(
         base::trace_event::TraceConfig(categories, "").ToString(),
-        base::BindRepeating([] {}));
+        base::TimeTicks::Now(),
+        base::BindRepeating([](bool success) { EXPECT_TRUE(success); }));
   }
 
   void StopAndFlush(base::Closure quit_closure) {

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

@@ -35,7 +35,8 @@ interface AgentRegistry {
 // close the recorder connection to signal the tracing service that no more data
 // will be sent.
 interface Agent {
-  StartTracing(string config) => ();
+  StartTracing(string config, mojo.common.mojom.TimeTicks coordinator_time) => (
+      bool success);
   StopAndFlush(Recorder recorder);
   RequestClockSyncMarker(string sync_id) => (
       mojo.common.mojom.TimeTicks issue_ts,
@@ -59,9 +60,14 @@ interface Recorder {
 interface Coordinator {
   // 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|. 
+  // from all existing agents and agents that connect during |StartTracing|.
   StartTracing(string config) => (bool success);
-  StopAndFlush(handle<data_pipe_producer> stream);
+  StopAndFlush(handle<data_pipe_producer> stream) => (
+      mojo.common.mojom.DictionaryValue metadata);
+  // Same as |StopAndFlush| but only write data from a certain |agent_label| to
+  // the |stream|.
+  StopAndFlushAgent(handle<data_pipe_producer> stream, string agent_label) => (
+      mojo.common.mojom.DictionaryValue metadata);
   IsTracing() => (bool is_tracing);
   RequestBufferUsage() => (bool success, float percent_full,
                            uint32 approximate_count);

services/resource_coordinator/tracing/agent_registry.cc

@@ -10,6 +10,7 @@
 #include "base/callback_forward.h"
 #include "base/logging.h"
 #include "base/threading/thread_checker.h"
+#include "services/service_manager/public/cpp/bind_source_info.h"
 
 namespace {
 tracing::AgentRegistry* g_agent_registry;
@@ -57,8 +58,13 @@ void AgentRegistry::AgentEntry::OnConnectionError() {
   // 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.
-  for (auto& key_value : closures_) {
-    std::move(key_value.second).Run();
+  while (!closures_.empty()) {
+    auto iterator = closures_.begin();
+    auto callback = std::move(iterator->second);
+    const size_t closures_size = closures_.size();
+    std::move(callback).Run();
+    // Verify that the callback has removed itself.
+    DCHECK_EQ(1u, closures_size - closures_.size());
   }
   agent_registry_->UnregisterAgent(id_);
 }
@@ -79,9 +85,10 @@ AgentRegistry::~AgentRegistry() {
 }
 
 void AgentRegistry::BindAgentRegistryRequest(
-    mojom::AgentRegistryRequest request) {
+    mojom::AgentRegistryRequest request,
+    const service_manager::BindSourceInfo& source_info) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-  bindings_.AddBinding(this, std::move(request));
+  bindings_.AddBinding(this, std::move(request), source_info.identity);
 }
 
 void AgentRegistry::SetAgentInitializationCallback(

services/resource_coordinator/tracing/agent_registry.h

@@ -14,6 +14,11 @@
 #include "base/threading/thread_checker.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
 #include "services/resource_coordinator/public/interfaces/tracing/tracing.mojom.h"
+#include "services/service_manager/public/cpp/identity.h"
+
+namespace service_manager {
+struct BindSourceInfo;
+}  // namespace service_manager
 
 namespace tracing {
 
@@ -66,7 +71,9 @@ class AgentRegistry : public mojom::AgentRegistry {
 
   AgentRegistry();
 
-  void BindAgentRegistryRequest(mojom::AgentRegistryRequest request);
+  void BindAgentRegistryRequest(
+      mojom::AgentRegistryRequest request,
+      const service_manager::BindSourceInfo& source_info);
   void SetAgentInitializationCallback(
       const AgentInitializationCallback& callback);
   void RemoveAgentInitializationCallback();
@@ -95,7 +102,7 @@ class AgentRegistry : public mojom::AgentRegistry {
 
   void UnregisterAgent(size_t agent_id);
 
-  mojo::BindingSet<mojom::AgentRegistry> bindings_;
+  mojo::BindingSet<mojom::AgentRegistry, service_manager::Identity> bindings_;
   size_t next_agent_id_ = 0;
   std::map<size_t, std::unique_ptr<AgentEntry>> agents_;
   AgentInitializationCallback agent_initialization_callback_;