Reland of Background tracing: Added config option for repeated trigger...

Reland of Background tracing: Added config option for repeated trigger behavior (patchset #1 id:1 of https://codereview.chromium.org/2265423002/ )

Reason for revert:
Re-landing with fix

Original issue's description:
> Revert of Background tracing: Added config option for repeated trigger behavior (patchset #1 id:1 of https://codereview.chromium.org/2247033005/ )
>
> Reason for revert:
> Test is flaky on builder "Android Tests"
>
> Builder: https://build.chromium.org/p/chromium.linux/builders/Android%20Tests
>
> Failure:
> https://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/30899
>
> Output:
> BackgroundTracingManagerBrowserTest.ReactiveSecondTriggerIgnored (run #1):
> [ RUN      ] BackgroundTracingManagerBrowserTest.ReactiveSecondTriggerIgnored
> [WARNING:dns_config_service_posix.cc(316)] Failed to read DnsConfig.
> [ERROR:devtools_http_handler.cc(228)] Cannot start http server for devtools. Stop devtools.
> ../../content/browser/tracing/background_tracing_manager_browsertest.cc:1210: Failure
> Value of: BackgroundTracingManagerImpl::GetInstance()->IsTracingForTesting()
>   Actual: false
> Expected: true
> ../../content/browser/tracing/background_tracing_manager_browsertest.cc:95: Failure
> Value of: value
>   Actual: true
> Expected: expected
> Which is: false
> [  FAILED  ] BackgroundTracingManagerBrowserTest.ReactiveSecondTriggerIgnored, where TypeParam =  and GetParam() =  (188 ms)
> [----------] 1 test from BackgroundTracingManagerBrowserTest (188 ms total)
>
> Original issue's description:
> > Background tracing: Add config option to control whether or not to stop reactive tracing when a repeated trigger occurs
> >
> > R=simonhatch@chromium.org
> > BUG=637129
> >
> > Committed: https://crrev.com/fba230c24383ae08ea6d924401bdff81b46c7108
> > Cr-Commit-Position: refs/heads/master@{#412577}
>
> TBR=simonhatch@chromium.org,ssid@chromium.org,petrcermak@chromium.org,oysteine@chromium.org
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG=637129
>
> Committed: https://crrev.com/ccb0f2af13bf3cc4ebdcdd7a08e19b281d788897
> Cr-Commit-Position: refs/heads/master@{#413627}

TBR=simonhatch@chromium.org,ssid@chromium.org,petrcermak@chromium.org,megjablon@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=637129

Review-Url: https://codereview.chromium.org/2285853002
Cr-Commit-Position: refs/heads/master@{#414867}

chrome/browser/tracing/navigation_tracing.cc

@@ -67,6 +67,7 @@ void SetupNavigationTracing() {
         new base::DictionaryValue());
     rules_dict->SetString("rule", "TRACE_ON_NAVIGATION_UNTIL_TRIGGER_OR_FULL");
     rules_dict->SetString("trigger_name", kNavigationTracingConfig);
+    rules_dict->SetBoolean("stop_tracing_on_repeated_reactive", true);
     rules_dict->SetString("category", "BENCHMARK_DEEP");
     rules_list->Append(std::move(rules_dict));
   }

content/browser/tracing/background_tracing_config_unittest.cc

@@ -381,14 +381,17 @@ TEST_F(BackgroundTracingConfigTest, ReactiveConfigFromValidString) {
             "\"trigger_delay\":30,\"trigger_name\":\"foo2\"}");
   config = ReadFromJSONString(
       "{\"mode\":\"REACTIVE_TRACING_MODE\",\"configs\": [{\"rule\": "
-      "\"TRACE_AT_RANDOM_INTERVALS\",\"category\": \"BENCHMARK_DEEP\","
+      "\"TRACE_AT_RANDOM_INTERVALS\","
+      "\"stop_tracing_on_repeated_reactive\": true,"
+      "\"category\": \"BENCHMARK_DEEP\","
       "\"timeout_min\":10, \"timeout_max\":20}]}");
   EXPECT_TRUE(config);
   EXPECT_EQ(config->tracing_mode(), BackgroundTracingConfig::REACTIVE);
   EXPECT_EQ(config->rules().size(), 1u);
   EXPECT_EQ(RuleToString(config->rules()[0]),
             "{\"category\":\"BENCHMARK_DEEP\",\"rule\":\"TRACE_AT_RANDOM_"
-            "INTERVALS\",\"timeout_max\":20,\"timeout_min\":10}");
+            "INTERVALS\",\"stop_tracing_on_repeated_reactive\":true,"
+            "\"timeout_max\":20,\"timeout_min\":10}");
 }
 
 TEST_F(BackgroundTracingConfigTest, ValidPreemptiveConfigToString) {

content/browser/tracing/background_tracing_manager_browsertest.cc

@@ -131,6 +131,7 @@ std::unique_ptr<BackgroundTracingConfig> CreateReactiveConfig() {
         new base::DictionaryValue());
     rules_dict->SetString("rule", "TRACE_ON_NAVIGATION_UNTIL_TRIGGER_OR_FULL");
     rules_dict->SetString("trigger_name", "reactive_test");
+    rules_dict->SetBoolean("stop_tracing_on_repeated_reactive", true);
     rules_dict->SetString("category", "BENCHMARK");
     rules_list->Append(std::move(rules_dict));
   }
@@ -1286,6 +1287,8 @@ IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
       rules_dict->SetString("rule",
                             "TRACE_ON_NAVIGATION_UNTIL_TRIGGER_OR_FULL");
       rules_dict->SetString("trigger_name", "reactive_test1");
+      rules_dict->SetBoolean("stop_tracing_on_repeated_reactive", true);
+      rules_dict->SetInteger("trigger_delay", 10);
       rules_dict->SetString("category", "BENCHMARK");
       rules_list->Append(std::move(rules_dict));
     }
@@ -1295,6 +1298,8 @@ IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
       rules_dict->SetString("rule",
                             "TRACE_ON_NAVIGATION_UNTIL_TRIGGER_OR_FULL");
       rules_dict->SetString("trigger_name", "reactive_test2");
+      rules_dict->SetBoolean("stop_tracing_on_repeated_reactive", true);
+      rules_dict->SetInteger("trigger_delay", 10);
       rules_dict->SetString("category", "BENCHMARK");
       rules_list->Append(std::move(rules_dict));
     }
@@ -1380,4 +1385,79 @@ IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
   }
 }
 
+// This tests that reactive mode only terminates with a repeated trigger
+// if the config specifies that it should.
+IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
+                       ReactiveSecondTriggerIgnored) {
+  {
+    SetupBackgroundTracingManager();
+
+    base::RunLoop run_loop;
+    BackgroundTracingManagerUploadConfigWrapper upload_config_wrapper(
+        run_loop.QuitClosure());
+
+    base::DictionaryValue dict;
+    dict.SetString("mode", "REACTIVE_TRACING_MODE");
+
+    std::unique_ptr<base::ListValue> rules_list(new base::ListValue());
+    {
+      std::unique_ptr<base::DictionaryValue> rules_dict(
+          new base::DictionaryValue());
+      rules_dict->SetString("rule",
+                            "TRACE_ON_NAVIGATION_UNTIL_TRIGGER_OR_FULL");
+      rules_dict->SetString("trigger_name", "reactive_test");
+      rules_dict->SetBoolean("stop_tracing_on_repeated_reactive", false);
+      rules_dict->SetInteger("trigger_delay", 10);
+      rules_dict->SetString("category", "BENCHMARK");
+      rules_list->Append(std::move(rules_dict));
+    }
+    dict.Set("configs", std::move(rules_list));
+
+    std::unique_ptr<BackgroundTracingConfig> config(
+        BackgroundTracingConfigImpl::FromDict(&dict));
+
+    BackgroundTracingManager::TriggerHandle trigger_handle =
+        BackgroundTracingManager::GetInstance()->RegisterTriggerType(
+            "reactive_test");
+
+    EXPECT_TRUE(BackgroundTracingManager::GetInstance()->SetActiveScenario(
+        std::move(config), upload_config_wrapper.get_receive_callback(),
+        BackgroundTracingManager::NO_DATA_FILTERING));
+
+    BackgroundTracingManager::GetInstance()->WhenIdle(
+        base::Bind(&DisableScenarioWhenIdle));
+
+    base::RunLoop wait_for_tracing_enabled;
+    static_cast<BackgroundTracingManagerImpl*>(
+        BackgroundTracingManager::GetInstance())
+        ->SetTracingEnabledCallbackForTesting(
+            wait_for_tracing_enabled.QuitClosure());
+
+    BackgroundTracingManager::GetInstance()->TriggerNamedEvent(
+        trigger_handle,
+        base::Bind(&StartedFinalizingCallback, base::Closure(), true));
+
+    wait_for_tracing_enabled.Run();
+
+    // This is expected to fail since we already triggered.
+    BackgroundTracingManager::GetInstance()->TriggerNamedEvent(
+        trigger_handle,
+        base::Bind(&StartedFinalizingCallback, base::Closure(), false));
+
+    // Since we specified a delay in the scenario, we should still be tracing
+    // at this point.
+    EXPECT_TRUE(
+        BackgroundTracingManagerImpl::GetInstance()->IsTracingForTesting());
+
+    BackgroundTracingManager::GetInstance()->FireTimerForTesting();
+
+    EXPECT_FALSE(
+        BackgroundTracingManagerImpl::GetInstance()->IsTracingForTesting());
+
+    run_loop.Run();
+
+    EXPECT_TRUE(upload_config_wrapper.get_receive_count() == 1);
+  }
+}
+
 }  // namespace content

content/browser/tracing/background_tracing_manager_impl.cc

@@ -329,9 +329,16 @@ void BackgroundTracingManagerImpl::OnRuleTriggered(
       StartTracing(triggered_rule->category_preset(),
                    base::trace_event::RECORD_UNTIL_FULL);
     } else {
-      // Reactive configs that trigger again while tracing should just
+      // Some reactive configs that trigger again while tracing should just
       // end right away (to not capture multiple navigations, for example).
-      trace_delay = -1;
+      // For others we just want to ignore the repeated trigger.
+      if (triggered_rule->stop_tracing_on_repeated_reactive()) {
+        trace_delay = -1;
+      } else {
+        if (!callback.is_null())
+          callback.Run(false);
+        return;
+      }
     }
   } else {
     // In preemptive mode, a trigger starts finalizing a trace if one is

content/browser/tracing/background_tracing_rule.cc

@@ -23,6 +23,8 @@ const char kConfigCategoryKey[] = "category";
 const char kConfigRuleTriggerNameKey[] = "trigger_name";
 const char kConfigRuleTriggerDelay[] = "trigger_delay";
 const char kConfigRuleTriggerChance[] = "trigger_chance";
+const char kConfigRuleStopTracingOnRepeatedReactive[] =
+    "stop_tracing_on_repeated_reactive";
 
 const char kConfigRuleHistogramNameKey[] = "histogram_name";
 const char kConfigRuleHistogramValueOldKey[] = "histogram_value";
@@ -59,11 +61,13 @@ namespace content {
 BackgroundTracingRule::BackgroundTracingRule()
     : trigger_chance_(1.0),
       trigger_delay_(-1),
+      stop_tracing_on_repeated_reactive_(false),
       category_preset_(BackgroundTracingConfigImpl::CATEGORY_PRESET_UNSET) {}
 
 BackgroundTracingRule::BackgroundTracingRule(int trigger_delay)
     : trigger_chance_(1.0),
       trigger_delay_(trigger_delay),
+      stop_tracing_on_repeated_reactive_(false),
       category_preset_(BackgroundTracingConfigImpl::CATEGORY_PRESET_UNSET) {}
 
 BackgroundTracingRule::~BackgroundTracingRule() {}
@@ -85,6 +89,11 @@ void BackgroundTracingRule::IntoDict(base::DictionaryValue* dict) const {
   if (trigger_delay_ != -1)
     dict->SetInteger(kConfigRuleTriggerDelay, trigger_delay_);
 
+  if (stop_tracing_on_repeated_reactive_) {
+    dict->SetBoolean(kConfigRuleStopTracingOnRepeatedReactive,
+                     stop_tracing_on_repeated_reactive_);
+  }
+
   if (category_preset_ != BackgroundTracingConfigImpl::CATEGORY_PRESET_UNSET) {
     dict->SetString(
         kConfigCategoryKey,
@@ -95,6 +104,8 @@ void BackgroundTracingRule::IntoDict(base::DictionaryValue* dict) const {
 void BackgroundTracingRule::Setup(const base::DictionaryValue* dict) {
   dict->GetDouble(kConfigRuleTriggerChance, &trigger_chance_);
   dict->GetInteger(kConfigRuleTriggerDelay, &trigger_delay_);
+  dict->GetBoolean(kConfigRuleStopTracingOnRepeatedReactive,
+                   &stop_tracing_on_repeated_reactive_);
 }
 
 namespace {

content/browser/tracing/background_tracing_rule.h

@@ -42,6 +42,10 @@ class CONTENT_EXPORT BackgroundTracingRule {
   // Probability that we should allow a tigger to  happen.
   double trigger_chance() const { return trigger_chance_; }
 
+  bool stop_tracing_on_repeated_reactive() const {
+    return stop_tracing_on_repeated_reactive_;
+  }
+
   static std::unique_ptr<BackgroundTracingRule> CreateRuleFromDict(
       const base::DictionaryValue* dict);
 
@@ -50,6 +54,7 @@ class CONTENT_EXPORT BackgroundTracingRule {
 
   double trigger_chance_;
   int trigger_delay_;
+  bool stop_tracing_on_repeated_reactive_;
   BackgroundTracingConfigImpl::CategoryPreset category_preset_;
 };