Implement Reactive Configuration in Background Tracing

content/browser/tracing/background_tracing_manager_impl.cc

Index: content/browser/tracing/background_tracing_manager_impl.cc
diff --git a/content/browser/tracing/background_tracing_manager_impl.cc b/content/browser/tracing/background_tracing_manager_impl.cc
index 64326d95a73feef83100e9bc721f356b1445517f..d453b6ffead66cd697113c2636d5f4598dff69f2 100644
--- a/content/browser/tracing/background_tracing_manager_impl.cc
+++ b/content/browser/tracing/background_tracing_manager_impl.cc
@@ -5,6 +5,7 @@
 #include "content/browser/tracing/background_tracing_manager_impl.h"
 
 #include "base/macros.h"
+#include "base/time/time.h"
 #include "content/public/browser/background_tracing_preemptive_config.h"
 #include "content/public/browser/background_tracing_reactive_config.h"
 #include "content/public/browser/browser_thread.h"
@@ -38,6 +39,26 @@ void BackgroundTracingManagerImpl::TraceDataEndpointWrapper::
                           base::Bind(done_callback_, contents_ptr));
 }
 
+BackgroundTracingManagerImpl::TracingTimer::
+    TracingTimer(StartedFinalizingCallback callback) : callback_(callback) {

[dsinclair, 2015/06/01 19:14:52]

I think it's more common to wrap at the params then at the method name?

[fmeawad, 2015/06/02 21:15:28]

Done.

+}
+
+BackgroundTracingManagerImpl::TracingTimer::~TracingTimer() {
+}
+
+void BackgroundTracingManagerImpl::TracingTimer::StartTimer() {
+  tracing_timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(10), this,
+      &BackgroundTracingManagerImpl::TracingTimer::TracingTimerFired);
+}
+
+void BackgroundTracingManagerImpl::TracingTimer::CancelTimer() {
+  tracing_timer_.Stop();
+}
+
+void BackgroundTracingManagerImpl::TracingTimer::TracingTimerFired() {
+  BackgroundTracingManagerImpl::GetInstance()->BeginFinalizing(callback_);
+}
+
 BackgroundTracingManager* BackgroundTracingManager::GetInstance() {
   return BackgroundTracingManagerImpl::GetInstance();
 }
@@ -72,19 +93,30 @@ bool BackgroundTracingManagerImpl::IsSupportedConfig(
   if (!config)
     return true;
 
-  // TODO(simonhatch): Implement reactive tracing path.
-  if (config->mode != BackgroundTracingConfig::PREEMPTIVE_TRACING_MODE)
-    return false;
+  if (config->mode == BackgroundTracingConfig::PREEMPTIVE_TRACING_MODE) {
+    BackgroundTracingPreemptiveConfig* preemptive_config =
+        static_cast<BackgroundTracingPreemptiveConfig*>(config);
+    const std::vector<BackgroundTracingPreemptiveConfig::MonitoringRule>&
+        configs = preemptive_config->configs;
+    for (size_t i = 0; i < configs.size(); ++i) {
+      if (configs[i].type !=
+          BackgroundTracingPreemptiveConfig::
+              MONITOR_AND_DUMP_WHEN_TRIGGER_NAMED)
+        return false;
+    }
+  }
 
-  // TODO(fmeawad): Implement uma triggers.
-  BackgroundTracingPreemptiveConfig* preemptive_config =
-      static_cast<BackgroundTracingPreemptiveConfig*>(config);
-  const std::vector<BackgroundTracingPreemptiveConfig::MonitoringRule>&
-      configs = preemptive_config->configs;
-  for (size_t i = 0; i < configs.size(); ++i) {
-    if (configs[i].type !=
-        BackgroundTracingPreemptiveConfig::MONITOR_AND_DUMP_WHEN_TRIGGER_NAMED)
-      return false;
+  if (config->mode == BackgroundTracingConfig::REACTIVE_TRACING_MODE) {
+    BackgroundTracingReactiveConfig* reactive_config =
+        static_cast<BackgroundTracingReactiveConfig*>(config);
+    const std::vector<BackgroundTracingReactiveConfig::TracingRule>&

[dsinclair, 2015/06/01 19:14:53]

What's the difference between monitoringRules and tracingRules? This block is
very similar to the old one just the rule types are different and the check is
slightly differnet

[fmeawad, 2015/06/02 21:15:28]

It is more of an explanation using variable names, monitoring is always on, once
a trigger happens, it dumps the trace. While tracing only starts at the trigger
point. (before that the system was not tracing).

Given that, a monitoring rule does not include the categories, as the categories
are fixed while having multiple trigger points. While a tracing rule contains
the categories as the categories can change per trace trigger.

+        configs = reactive_config->configs;
+    for (size_t i = 0; i < configs.size(); ++i) {
+      if (configs[i].type !=
+          BackgroundTracingReactiveConfig::
+              TRACE_UNTIL_10S_OR_TRIGGER_OR_FULL)
+        return false;
+    }
   }
 
   return true;
@@ -123,8 +155,7 @@ void BackgroundTracingManagerImpl::EnableRecordingIfConfigNeedsIt() {
         static_cast<BackgroundTracingPreemptiveConfig*>(config_.get())
             ->category_preset));
   } else {
-    // TODO(simonhatch): Implement reactive tracing path.
-    NOTREACHED();
+    // There is nothing to do in case of reactive tracing.

[dsinclair, 2015/06/01 19:14:53]

Let's remove the else in that case. Just put the comment in.

[fmeawad, 2015/06/02 21:15:28]

Done.

   }
 }
 
@@ -160,10 +191,22 @@ bool BackgroundTracingManagerImpl::IsAbleToTriggerTracing(
       }
     }
   } else {
-    // TODO(simonhatch): Implement reactive path.
-    NOTREACHED();
-  }
+    BackgroundTracingReactiveConfig* reactive_config =
+        static_cast<BackgroundTracingReactiveConfig*>(config_.get());
+
+    const std::vector<BackgroundTracingReactiveConfig::TracingRule>&
+        configs = reactive_config->configs;
 
+    for (size_t i = 0; i < configs.size(); ++i) {
+      if (configs[i].type !=
+              BackgroundTracingReactiveConfig::
+                  TRACE_UNTIL_10S_OR_TRIGGER_OR_FULL)
+        continue;
+      if (trigger_name == configs[i].trigger_name) {
+        return true;
+      }
+    }
+  }
   return false;
 }
 
@@ -187,8 +230,26 @@ void BackgroundTracingManagerImpl::TriggerNamedEvent(
   if (config_->mode == BackgroundTracingConfig::PREEMPTIVE_TRACING_MODE) {
     BeginFinalizing(callback);
   } else {
-    // TODO(simonhatch): Implement reactive tracing path.
-    NOTREACHED();
+    if (is_tracing_) {
+      tracing_timer_->CancelTimer();
+      BeginFinalizing(callback);

[dsinclair, 2015/06/01 19:14:52]

Is there a race condition in here? The BeginFinalizing sets is_tracing_ to false
and calls to DisableRecording which is a PostTask. The code below calls to
EnableRecording which is also a PostTask.

So, what happens if we disable, post the task but before it's processed we come
in here and Enable. Will we still write out the correct results for the first
disable before we re-enable and potentially change the trace buffer?

[fmeawad, 2015/06/02 21:15:28]

This code will not be reached on the second trigger since is_gathering_ will be
true which is checked in the IsAbleToTriggerTracing (the last test case covers
that scenario). And is_gathering_ is set from BeginFinalizing and not from the
posted task.

+      return;
+    } else {

[dsinclair, 2015/06/01 19:14:52]

The above returned, so don't need the else.

[fmeawad, 2015/06/02 21:15:28]

Done.

+      BackgroundTracingReactiveConfig* reactive_config =
+          static_cast<BackgroundTracingReactiveConfig*>(config_.get());
+      const std::vector<BackgroundTracingReactiveConfig::TracingRule>&
+          configs = reactive_config->configs;
+      std::string trigger_name = GetTriggerNameFromHandle(handle);
+      for (size_t i = 0; i < configs.size(); ++i) {
+        if (configs[i].trigger_name == trigger_name) {
+          EnableRecording(
+              GetCategoryFilterForCategoryPreset(configs[i].category_preset));
+          tracing_timer_.reset(new TracingTimer(callback));
+          tracing_timer_->StartTimer();
+          break;
+        }
+      }
+    }
   }
 }