Implement Reactive Configuration in Background Tracing

This CL implements the Reactive configuration, a configuration that allows triggering a trace manually,
and the trace is either concluded after a timeout or on
a second trigger.

BUG=495187
Committed: https://crrev.com/743fcdfd45d0a18c263e897e38eb2ddfae37f7c9
Cr-Commit-Position: refs/heads/master@{#332739}

[shatch, 2015-05-21 15:59:05]

simonhatch@chromium.org changed reviewers:
+ simonhatch@chromium.org

[shatch, 2015-05-21 15:59:06]

https://codereview.chromium.org/1148393003/diff/1/content/browser/tracing/bac...
File content/browser/tracing/background_tracing_manager_browsertest.cc (right):

https://codereview.chromium.org/1148393003/diff/1/content/browser/tracing/bac...
content/browser/tracing/background_tracing_manager_browsertest.cc:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
Lotsa tests.

https://codereview.chromium.org/1148393003/diff/1/content/browser/tracing/bac...
File content/browser/tracing/background_tracing_manager_impl.cc (right):

https://codereview.chromium.org/1148393003/diff/1/content/browser/tracing/bac...
content/browser/tracing/background_tracing_manager_impl.cc:54: void
BackgroundTracingManagerImpl::TracingTimer::CancelTimer() {
This isn't referenced anywhere.

https://codereview.chromium.org/1148393003/diff/1/content/browser/tracing/bac...
content/browser/tracing/background_tracing_manager_impl.cc:59:
BackgroundTracingManagerImpl::GetInstance()->BeginFinalizing(callback_);
Both TriggerNamedEvent and this can trigger a BeginFinalizing call, if you get
this sequence:

BackgroundTracingManager::TriggerNamedEvent("foo"); <- EnableRecording
BackgroundTracingManager::TriggerNamedEvent("foo"); <- BeginFinalizing
BackgroundTracingManager::TracingTimer::TracingTimerFired -> BeginFinalizing
again?

You call BeginFinalizing twice, think you need to kill the timer.

https://codereview.chromium.org/1148393003/diff/1/content/browser/tracing/bac...
content/browser/tracing/background_tracing_manager_impl.cc:236:
BeginFinalizing(callback);
Just wondering, does deep reports need to know or care about whether a call to
TriggerNamedEvent triggers an EnableRecording or BeginFinalizing?

https://codereview.chromium.org/1148393003/diff/1/content/browser/tracing/bac...
content/browser/tracing/background_tracing_manager_impl.cc:242:
GetCategoryFilterForCategoryPreset(
BackgroundTracingReactiveConfig::TracingRule has a category_preset.

https://codereview.chromium.org/1148393003/diff/1/content/browser/tracing/bac...
content/browser/tracing/background_tracing_manager_impl.cc:245: TracingTimer*
timer = new TracingTimer(callback);
Maybe change this to a scoped_ptr member?

https://codereview.chromium.org/1148393003/diff/1/content/browser/tracing/bac...
content/browser/tracing/background_tracing_manager_impl.cc:305: void
BackgroundTracingManagerImpl::OnTracingTimeout() {
Is this reference somewhere? What does it do?

[fmeawad, 2015-05-21 16:09:39]

Thank you for the quick feedback, will send another version later today.
Some replies.

https://codereview.chromium.org/1148393003/diff/1/content/browser/tracing/bac...
File content/browser/tracing/background_tracing_manager_impl.cc (right):

https://codereview.chromium.org/1148393003/diff/1/content/browser/tracing/bac...
content/browser/tracing/background_tracing_manager_impl.cc:236:
BeginFinalizing(callback);
On 2015/05/21 15:59:06, shatch wrote:
> Just wondering, does deep reports need to know or care about whether a call to
> TriggerNamedEvent triggers an EnableRecording or BeginFinalizing?

No. The idea is to collect as many as we can, but feedback is not needed.

https://codereview.chromium.org/1148393003/diff/1/content/browser/tracing/bac...
content/browser/tracing/background_tracing_manager_impl.cc:242:
GetCategoryFilterForCategoryPreset(
On 2015/05/21 15:59:06, shatch wrote:
> BackgroundTracingReactiveConfig::TracingRule has a category_preset.

Yes, see todo above.

https://codereview.chromium.org/1148393003/diff/1/content/browser/tracing/bac...
content/browser/tracing/background_tracing_manager_impl.cc:245: TracingTimer*
timer = new TracingTimer(callback);
On 2015/05/21 15:59:06, shatch wrote:
> Maybe change this to a scoped_ptr member?

Yes, that is the plan.

[fmeawad, 2015-05-30 00:38:50]

fmeawad@chromium.org changed reviewers:
+ oysteine@chromium.org

[fmeawad, 2015-05-30 00:38:50]

Simon, can you take another look?

[shatch, 2015-06-01 15:33:34]

lg2m

https://codereview.chromium.org/1148393003/diff/120001/content/browser/tracin...
File content/browser/tracing/background_tracing_manager_browsertest.cc (right):

https://codereview.chromium.org/1148393003/diff/120001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_browsertest.cc:407:
ReactiveThridTriggerTimeout) {
nit: thrid->third

https://codereview.chromium.org/1148393003/diff/120001/content/browser/tracin...
File content/browser/tracing/background_tracing_manager_impl.cc (right):

https://codereview.chromium.org/1148393003/diff/120001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:50: 
nit: Remove blank line.

https://codereview.chromium.org/1148393003/diff/120001/content/public/browser...
File content/public/browser/background_tracing_reactive_config.h (right):

https://codereview.chromium.org/1148393003/diff/120001/content/public/browser...
content/public/browser/background_tracing_reactive_config.h:23: enum RuleType {
TRACE_UNTIL_10S_OR_TRIGGER_OR_FULL };
Thanks for shortening that.

[fmeawad, 2015-06-01 18:15:27]

https://codereview.chromium.org/1148393003/diff/120001/content/browser/tracin...
File content/browser/tracing/background_tracing_manager_browsertest.cc (right):

https://codereview.chromium.org/1148393003/diff/120001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_browsertest.cc:407:
ReactiveThridTriggerTimeout) {
On 2015/06/01 15:33:33, shatch wrote:
> nit: thrid->third

Done.

https://codereview.chromium.org/1148393003/diff/120001/content/browser/tracin...
File content/browser/tracing/background_tracing_manager_impl.cc (right):

https://codereview.chromium.org/1148393003/diff/120001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:50: 
On 2015/06/01 15:33:33, shatch wrote:
> nit: Remove blank line.

Done.

https://codereview.chromium.org/1148393003/diff/120001/content/public/browser...
File content/public/browser/background_tracing_reactive_config.h (right):

https://codereview.chromium.org/1148393003/diff/120001/content/public/browser...
content/public/browser/background_tracing_reactive_config.h:23: enum RuleType {
TRACE_UNTIL_10S_OR_TRIGGER_OR_FULL };
On 2015/06/01 15:33:33, shatch wrote:
> Thanks for shortening that.

Acknowledged.

[fmeawad, 2015-06-01 18:18:04]

fmeawad@chromium.org changed reviewers:
+ dsinclair@chromium.org
- oysteine@chromium.org, simonhatch@chromium.org

[fmeawad, 2015-06-01 18:18:05]

Expanding to a wider audience, Dan, Can you take a look?

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

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
File content/browser/tracing/background_tracing_manager_browsertest.cc (right):

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_browsertest.cc:360:
BackgroundTracingManager::GetInstance()->TriggerNamedEvent(
What is the timeout this is using? How long will this test require to timeout?

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_browsertest.cc:375: 
nit: extra blank line.

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_browsertest.cc:396:
BackgroundTracingManager::GetInstance()->TriggerNamedEvent(
How do we know this terminated because of the second event and not because of
timeout?

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_browsertest.cc:405: // This
tests that reactive mode multiple still triggers gathers once.
I'm not sure what this is trying to say? triggers gathering once?

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_browsertest.cc:411: 
nit: remove blank link.

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_browsertest.cc:436: handle,
base::Bind(&StartedFinalizingCallback, base::Closure(), false));
How do we know this failed?

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
File content/browser/tracing/background_tracing_manager_impl.cc (right):

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:43:
TracingTimer(StartedFinalizingCallback callback) : callback_(callback) {
I think it's more common to wrap at the params then at the method name?

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:112: const
std::vector<BackgroundTracingReactiveConfig::TracingRule>&
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

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:158: // There is
nothing to do in case of reactive tracing.
Let's remove the else in that case. Just put the comment in.

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:235:
BeginFinalizing(callback);
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?

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:237: } else {
The above returned, so don't need the else.

https://codereview.chromium.org/1148393003/diff/140001/content/public/browser...
File content/public/browser/background_tracing_reactive_config.h (right):

https://codereview.chromium.org/1148393003/diff/140001/content/public/browser...
content/public/browser/background_tracing_reactive_config.h:23: enum RuleType {
TRACE_UNTIL_10S_OR_TRIGGER_OR_FULL };
TRACE_FOR_10S_OR_TRIGGER_OR_FULL ?

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

Thank you for the prompt review, PTAL.

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
File content/browser/tracing/background_tracing_manager_browsertest.cc (right):

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_browsertest.cc:360:
BackgroundTracingManager::GetInstance()->TriggerNamedEvent(
On 2015/06/01 19:14:52, dsinclair wrote:
> What is the timeout this is using? How long will this test require to timeout?

I added a Test fire to make it instant, it used to be 10s, another approach
would be to use a MockTimer but would require exposing the timer to the test.

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_browsertest.cc:375: 
On 2015/06/01 19:14:52, dsinclair wrote:
> nit: extra blank line.

Done.

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_browsertest.cc:396:
BackgroundTracingManager::GetInstance()->TriggerNamedEvent(
On 2015/06/01 19:14:52, dsinclair wrote:
> How do we know this terminated because of the second event and not because of
> timeout?

The timeout is 10 seconds. It is not guaranteed, but I would doubt it gets
de-scheduled for that long.

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_browsertest.cc:405: // This
tests that reactive mode multiple still triggers gathers once.
On 2015/06/01 19:14:52, dsinclair wrote:
> I'm not sure what this is trying to say? triggers gathering once?

Fixed.

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_browsertest.cc:411: 
On 2015/06/01 19:14:52, dsinclair wrote:
> nit: remove blank link.

Done.

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_browsertest.cc:436: handle,
base::Bind(&StartedFinalizingCallback, base::Closure(), false));
On 2015/06/01 19:14:52, dsinclair wrote:
> How do we know this failed?

The callback (StartedFinalizingCallback) is bound using the expected value
(false here) and get the actual value comes from the callback.Run(false).
(called from TiggerNamedEvent).

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
File content/browser/tracing/background_tracing_manager_impl.cc (right):

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:43:
TracingTimer(StartedFinalizingCallback callback) : callback_(callback) {
On 2015/06/01 19:14:52, dsinclair wrote:
> I think it's more common to wrap at the params then at the method name?

Done.

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:112: const
std::vector<BackgroundTracingReactiveConfig::TracingRule>&
On 2015/06/01 19:14:53, dsinclair wrote:
> 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

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.

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:158: // There is
nothing to do in case of reactive tracing.
On 2015/06/01 19:14:53, dsinclair wrote:
> Let's remove the else in that case. Just put the comment in.

Done.

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:235:
BeginFinalizing(callback);
On 2015/06/01 19:14:52, dsinclair wrote:
> 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?

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.

https://codereview.chromium.org/1148393003/diff/140001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:237: } else {
On 2015/06/01 19:14:52, dsinclair wrote:
> The above returned, so don't need the else.

Done.

https://codereview.chromium.org/1148393003/diff/140001/content/public/browser...
File content/public/browser/background_tracing_reactive_config.h (right):

https://codereview.chromium.org/1148393003/diff/140001/content/public/browser...
content/public/browser/background_tracing_reactive_config.h:23: enum RuleType {
TRACE_UNTIL_10S_OR_TRIGGER_OR_FULL };
On 2015/06/01 19:14:53, dsinclair wrote:
> TRACE_FOR_10S_OR_TRIGGER_OR_FULL ?

Done.

[dsinclair, 2015-06-03 14:15:30]

lgtm w/ nits.

https://codereview.chromium.org/1148393003/diff/200001/content/browser/tracin...
File content/browser/tracing/background_tracing_manager_impl.cc (right):

https://codereview.chromium.org/1148393003/diff/200001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:50:
tracing_timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(10), this,
nit: Let's put the 10 into a constant.

https://codereview.chromium.org/1148393003/diff/200001/content/browser/tracin...
File content/browser/tracing/background_tracing_manager_impl.h (right):

https://codereview.chromium.org/1148393003/diff/200001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.h:70:
TracingTimer(StartedFinalizingCallback);
nit: explicit TracingTimer(StartedFinalizingCallback);

https://codereview.chromium.org/1148393003/diff/200001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.h:82:
BackgroundTracingManager::StartedFinalizingCallback callback_;
nit: Why does this need the BackgroundTracingManager:: qualification but the
constructor didn't? Can we drop it here?

[fmeawad, 2015-06-03 18:00:18]

https://codereview.chromium.org/1148393003/diff/200001/content/browser/tracin...
File content/browser/tracing/background_tracing_manager_impl.cc (right):

https://codereview.chromium.org/1148393003/diff/200001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:50:
tracing_timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(10), this,
On 2015/06/03 14:15:30, dsinclair wrote:
> nit: Let's put the 10 into a constant.

Done.

https://codereview.chromium.org/1148393003/diff/200001/content/browser/tracin...
File content/browser/tracing/background_tracing_manager_impl.h (right):

https://codereview.chromium.org/1148393003/diff/200001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.h:70:
TracingTimer(StartedFinalizingCallback);
On 2015/06/03 14:15:30, dsinclair wrote:
> nit: explicit TracingTimer(StartedFinalizingCallback);

Done.

https://codereview.chromium.org/1148393003/diff/200001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.h:82:
BackgroundTracingManager::StartedFinalizingCallback callback_;
On 2015/06/03 14:15:30, dsinclair wrote:
> nit: Why does this need the BackgroundTracingManager:: qualification but the
> constructor didn't? Can we drop it here?

Done.

[fmeawad, 2015-06-03 18:00:38]

The CQ bit was checked by fmeawad@chromium.org to run a CQ dry run

[fmeawad, 2015-06-03 18:00:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org
Link to the patchset: https://codereview.chromium.org/1148393003/#ps220001
(title: "Address Comments")

[commit-bot: I haz the power, 2015-06-03 18:01:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1148393003/220001

[fmeawad, 2015-06-03 18:02:32]

fmeawad@chromium.org changed reviewers:
+ sievers@chromium.org

[fmeawad, 2015-06-03 18:03:00]

+sievers for content/browser/

[no sievers, 2015-06-03 19:26:56]

lgtm stamp

[commit-bot: I haz the power, 2015-06-03 19:33:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-03 19:33:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fmeawad, 2015-06-03 21:50:48]

The CQ bit was checked by fmeawad@chromium.org

[fmeawad, 2015-06-03 21:50:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1148393003/#ps240001
(title: "Rebase files depending on reactive config")

[commit-bot: I haz the power, 2015-06-03 21:51:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1148393003/240001

[commit-bot: I haz the power, 2015-06-03 23:55:40]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-06-03 23:57:27]

Patchset 13 (id:??) landed as
https://crrev.com/743fcdfd45d0a18c263e897e38eb2ddfae37f7c9
Cr-Commit-Position: refs/heads/master@{#332739}