Implement Reactive Configuration in Background Tracing

content/browser/tracing/background_tracing_manager_impl.cc

 // Copyright 2015 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.
 
 #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"
 
 namespace content {
 
 namespace {
 
 base::LazyInstance<BackgroundTracingManagerImpl>::Leaky g_controller =
     LAZY_INSTANCE_INITIALIZER;
@@ skipping 13 matching lines @@
 void BackgroundTracingManagerImpl::TraceDataEndpointWrapper::
     ReceiveTraceFinalContents(const std::string& file_contents) {
   std::string tmp = file_contents;
   scoped_refptr<base::RefCountedString> contents_ptr =
       base::RefCountedString::TakeString(&tmp);
 
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                           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();
 }
 
 BackgroundTracingManagerImpl* BackgroundTracingManagerImpl::GetInstance() {
   return g_controller.Pointer();
 }
 
 BackgroundTracingManagerImpl::BackgroundTracingManagerImpl()
     : is_gathering_(false),
@@ skipping 14 matching lines @@
   CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   idle_callback_ = idle_callback;
 }
 
 bool BackgroundTracingManagerImpl::IsSupportedConfig(
     BackgroundTracingConfig* config) {
   // No config is just fine, we just don't do anything.
   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;
 }
 
 bool BackgroundTracingManagerImpl::SetActiveScenario(
     scoped_ptr<BackgroundTracingConfig> config,
     const BackgroundTracingManager::ReceiveCallback& receive_callback,
     bool requires_anonymized_data) {
   CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
@@ skipping 18 matching lines @@
 
 void BackgroundTracingManagerImpl::EnableRecordingIfConfigNeedsIt() {
   if (!config_)
     return;
 
   if (config_->mode == BackgroundTracingConfig::PREEMPTIVE_TRACING_MODE) {
     EnableRecording(GetCategoryFilterForCategoryPreset(
         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.

   }
 }
 
 bool BackgroundTracingManagerImpl::IsAbleToTriggerTracing(
     TriggerHandle handle) const {
   if (!config_)
     return false;
 
   // If the last trace is still uploading, we don't allow a new one to trigger.
   if (is_gathering_)
@@ skipping 15 matching lines @@
     for (size_t i = 0; i < configs.size(); ++i) {
       if (configs[i].type != BackgroundTracingPreemptiveConfig::
                                  MONITOR_AND_DUMP_WHEN_TRIGGER_NAMED)
         continue;
 
       if (trigger_name == configs[i].named_trigger_info.trigger_name) {
         return true;
       }
     }
   } 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;
 }
 
 void BackgroundTracingManagerImpl::TriggerNamedEvent(
     BackgroundTracingManagerImpl::TriggerHandle handle,
     StartedFinalizingCallback callback) {
   if (!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
     content::BrowserThread::PostTask(
         content::BrowserThread::UI, FROM_HERE,
         base::Bind(&BackgroundTracingManagerImpl::TriggerNamedEvent,
                    base::Unretained(this), handle, callback));
     return;
   }
 
   if (!IsAbleToTriggerTracing(handle)) {
     if (!callback.is_null())
       callback.Run(false);
     return;
   }
 
   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;
+        }
+      }
+    }
   }
 }
 
 BackgroundTracingManagerImpl::TriggerHandle
 BackgroundTracingManagerImpl::RegisterTriggerType(const char* trigger_name) {
   CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
   trigger_handle_ids_ += 1;
 
   trigger_handles_.insert(
@@ skipping 101 matching lines @@
   return NULL;
 }
 
 void BackgroundTracingConfig::IntoDict(const BackgroundTracingConfig* config,
                                        base::DictionaryValue* dict) {
   // TODO(simonhatch): Implement this.
   CHECK(false);
 }
 
 }  // namspace content