Slow Reports - Add message filter to trigger background traces from child processes.

Slow Reports - Add message filter to trigger background traces from child processes.

At the moment, we want to trigger background traces from UMA callbacks, but you can only set a callback in the same process. This CL adds a message filter for the background tracing manager, which allows it to install UMA callbacks in child processes, and trigger background tracing from there.

BUG=504889
Committed: https://crrev.com/cebf0f7018da1787d11bed7b9a406e29a84e5d79
Cr-Commit-Position: refs/heads/master@{#340544}

[shatch, 2015-07-20 19:44:20]

Patchset #1 (id:1) has been deleted

[shatch, 2015-07-20 19:44:39]

Patchset #1 (id:1) has been deleted

[shatch, 2015-07-21 15:09:46]

Patchset #1 (id:20001) has been deleted

[shatch, 2015-07-21 15:30:14]

simonhatch@chromium.org changed reviewers:
+ oysteine@google.com

[shatch, 2015-07-21 15:34:43]

Patchset #2 (id:60001) has been deleted

[shatch, 2015-07-21 15:34:49]

Patchset #1 (id:40001) has been deleted

[fmeawad, 2015-07-21 16:14:22]

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

[fmeawad, 2015-07-21 16:14:23]

https://codereview.chromium.org/1243743002/diff/80001/components/tracing/chil...
File components/tracing/child_trace_message_filter.h (right):

https://codereview.chromium.org/1243743002/diff/80001/components/tracing/chil...
components/tracing/child_trace_message_filter.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
nit: 2012

[shatch, 2015-07-21 16:19:04]

Patchset #2 (id:100001) has been deleted

[shatch, 2015-07-21 16:19:48]

https://codereview.chromium.org/1243743002/diff/80001/components/tracing/chil...
File components/tracing/child_trace_message_filter.h (right):

https://codereview.chromium.org/1243743002/diff/80001/components/tracing/chil...
components/tracing/child_trace_message_filter.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/07/21 16:14:22, fmeawad wrote:
> nit: 2012

Oops changed the wrong file. Fixed + changed new files to 2015.

[shatch, 2015-07-21 19:49:35]

Patchset #3 (id:140001) has been deleted

[shatch, 2015-07-21 19:50:23]

Patchset #3 (id:160001) has been deleted

[shatch, 2015-07-21 19:53:45]

Patchset #3 (id:180001) has been deleted

[oystein (OOO til 10th of July), 2015-07-21 20:52:45]

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

[oystein (OOO til 10th of July), 2015-07-21 20:52:45]

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

https://codereview.chromium.org/1243743002/diff/200001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:100: // first access
to the BackgroundTracingManagerImpl is from another thread
When does this happen? Can we be sure there's no race conditions here, if the
BackgroundTracingManagerImpl can be lazily instantiated on multiple threads?

https://codereview.chromium.org/1243743002/diff/200001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:289: if
(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
Which threads can this be called on? Wouldn't this be consistent?

https://codereview.chromium.org/1243743002/diff/200001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:303: if
(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
Same thread question as AddFilter

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

https://codereview.chromium.org/1243743002/diff/200001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.h:99: FilterSet
filters_;
Could we just use the list in TraceControllerImpl, instead of tracking this
separately?

[shatch, 2015-07-22 17:36:38]

Patchset #4 (id:220001) has been deleted

[shatch, 2015-07-22 17:44:13]

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

https://codereview.chromium.org/1243743002/diff/200001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:100: // first access
to the BackgroundTracingManagerImpl is from another thread
On 2015/07/21 20:52:45, Oystein wrote:
> When does this happen? Can we be sure there's no race conditions here, if the
> BackgroundTracingManagerImpl can be lazily instantiated on multiple threads?

This happened in a few tests where the message filter called in to add a filter,
but I think this is unnecessary now if we're piggybacking on the trace message
filter, which does some checks before adding the filter.

LazyInstance guarantees thread safety for creation.

https://codereview.chromium.org/1243743002/diff/200001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:289: if
(!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
On 2015/07/21 20:52:45, Oystein wrote:
> Which threads can this be called on? Wouldn't this be consistent?

Called via OnChildSupportsTracing from the TraceMessageFtiler on the IO thread.
Moot if we reuse the filters from TracingControllerImpl.

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

https://codereview.chromium.org/1243743002/diff/200001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.h:99: FilterSet
filters_;
On 2015/07/21 20:52:45, Oystein wrote:
> Could we just use the list in TraceControllerImpl, instead of tracking this
> separately?

Thought about this a bit more, didn't get any great ideas on a cleaner way. To
reuse the list in TraceControllerImpl, we need to expose both a GetFilters()
method as well as some way to get notified when a filter is added so that we can
send the appropriate IPC's to setup callbacks. Put up a version that does that,
tell me what you think.

[oystein (OOO til 10th of July), 2015-07-22 18:21:41]

lgtm

[shatch, 2015-07-22 18:23:50]

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

[shatch, 2015-07-22 18:24:58]

Hey Dan, ptal

[shatch, 2015-07-23 19:22:55]

On 2015/07/22 18:24:58, shatch wrote:
> Hey Dan, ptal

ping dsinclair

[shatch, 2015-07-23 20:04:53]

simonhatch@chromium.org changed reviewers:
+ dcheng@chromium.org, sievers@chromium.org
- dsinclair@chromium.org

[shatch, 2015-07-23 20:06:48]

simonhatch@chromium.org changed reviewers:
- sievers@chromium.org

[shatch, 2015-07-23 20:12:40]

+dcheng for components/tracing/tracing_messages.h

[dcheng, 2015-07-23 21:11:33]

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

https://codereview.chromium.org/1243743002/diff/250001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:94:
base::Unretained(this)));
Why is it safe to use Unretained here? This callback isn't cleared in the dtor,
for instance: what keeps it from getting called after this is destroyed?

https://codereview.chromium.org/1243743002/diff/250001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:198: for (size_t i =
0; i < configs.size(); ++i) {
for (const auto& config : preemptive_config->configs)?

https://codereview.chromium.org/1243743002/diff/250001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:287: for (auto it =
filters.begin(); it != filters.end(); ++it)
for (auto& filter : filters)

https://codereview.chromium.org/1243743002/diff/250001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:304: for (size_t i =
0; i < configs.size(); ++i) {
Ditto: use a for each.

https://codereview.chromium.org/1243743002/diff/250001/content/browser/tracin...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/1243743002/diff/250001/content/browser/tracin...
content/browser/tracing/tracing_controller_impl.cc:839:
std::copy(trace_message_filters_.begin(), trace_message_filters_.end(),
filters->insert(trace_message_filters_.begin(), trace_message_filters_.end())?

[shatch, 2015-07-23 22:39:27]

Patchset #6 (id:270001) has been deleted

[shatch, 2015-07-23 23:06:52]

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

https://codereview.chromium.org/1243743002/diff/250001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:94:
base::Unretained(this)));
On 2015/07/23 21:11:33, dcheng wrote:
> Why is it safe to use Unretained here? This callback isn't cleared in the
dtor,
> for instance: what keeps it from getting called after this is destroyed?

This is a leaky instance, we shouldn't hit the dtor.

https://codereview.chromium.org/1243743002/diff/250001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:198: for (size_t i =
0; i < configs.size(); ++i) {
On 2015/07/23 21:11:33, dcheng wrote:
> for (const auto& config : preemptive_config->configs)?

Oh neat, didn't realize this was recommended now, thanks!

https://codereview.chromium.org/1243743002/diff/250001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:287: for (auto it =
filters.begin(); it != filters.end(); ++it)
On 2015/07/23 21:11:33, dcheng wrote:
> for (auto& filter : filters)

Done.

https://codereview.chromium.org/1243743002/diff/250001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:304: for (size_t i =
0; i < configs.size(); ++i) {
On 2015/07/23 21:11:33, dcheng wrote:
> Ditto: use a for each.

Done.

https://codereview.chromium.org/1243743002/diff/250001/content/browser/tracin...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/1243743002/diff/250001/content/browser/tracin...
content/browser/tracing/tracing_controller_impl.cc:839:
std::copy(trace_message_filters_.begin(), trace_message_filters_.end(),
On 2015/07/23 21:11:33, dcheng wrote:
> filters->insert(trace_message_filters_.begin(), trace_message_filters_.end())?

Done.

[shatch, 2015-07-27 13:15:15]

On 2015/07/23 23:06:52, shatch wrote:
>
https://codereview.chromium.org/1243743002/diff/250001/content/browser/tracin...
> File content/browser/tracing/background_tracing_manager_impl.cc (right):
> 
>
https://codereview.chromium.org/1243743002/diff/250001/content/browser/tracin...
> content/browser/tracing/background_tracing_manager_impl.cc:94:
> base::Unretained(this)));
> On 2015/07/23 21:11:33, dcheng wrote:
> > Why is it safe to use Unretained here? This callback isn't cleared in the
> dtor,
> > for instance: what keeps it from getting called after this is destroyed?
> 
> This is a leaky instance, we shouldn't hit the dtor.
> 
>
https://codereview.chromium.org/1243743002/diff/250001/content/browser/tracin...
> content/browser/tracing/background_tracing_manager_impl.cc:198: for (size_t i
=
> 0; i < configs.size(); ++i) {
> On 2015/07/23 21:11:33, dcheng wrote:
> > for (const auto& config : preemptive_config->configs)?
> 
> Oh neat, didn't realize this was recommended now, thanks!
> 
>
https://codereview.chromium.org/1243743002/diff/250001/content/browser/tracin...
> content/browser/tracing/background_tracing_manager_impl.cc:287: for (auto it =
> filters.begin(); it != filters.end(); ++it)
> On 2015/07/23 21:11:33, dcheng wrote:
> > for (auto& filter : filters)
> 
> Done.
> 
>
https://codereview.chromium.org/1243743002/diff/250001/content/browser/tracin...
> content/browser/tracing/background_tracing_manager_impl.cc:304: for (size_t i
=
> 0; i < configs.size(); ++i) {
> On 2015/07/23 21:11:33, dcheng wrote:
> > Ditto: use a for each.
> 
> Done.
> 
>
https://codereview.chromium.org/1243743002/diff/250001/content/browser/tracin...
> File content/browser/tracing/tracing_controller_impl.cc (right):
> 
>
https://codereview.chromium.org/1243743002/diff/250001/content/browser/tracin...
> content/browser/tracing/tracing_controller_impl.cc:839:
> std::copy(trace_message_filters_.begin(), trace_message_filters_.end(),
> On 2015/07/23 21:11:33, dcheng wrote:
> > filters->insert(trace_message_filters_.begin(),
trace_message_filters_.end())?
> 
> Done.

dcheng: friendly ping!

[dcheng, 2015-07-27 17:19:58]

lgtm

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

https://codereview.chromium.org/1243743002/diff/290001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:94:
base::Unretained(this)));
Personally, I think it's better to annotate uses of base::Unretained() to
document why it's safe.

https://codereview.chromium.org/1243743002/diff/290001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:299: configs =
preemptive_config->configs;
You don't technically need this line anymore: just
s/configs/preemptive_config->configs/ on line 301

[shatch, 2015-07-27 19:08:04]

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

https://codereview.chromium.org/1243743002/diff/290001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:94:
base::Unretained(this)));
On 2015/07/27 17:19:58, dcheng wrote:
> Personally, I think it's better to annotate uses of base::Unretained() to
> document why it's safe.

Yeah that's fair enough, added some explanation.

https://codereview.chromium.org/1243743002/diff/290001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:299: configs =
preemptive_config->configs;
On 2015/07/27 17:19:58, dcheng wrote:
> You don't technically need this line anymore: just
> s/configs/preemptive_config->configs/ on line 301

Done.

[shatch, 2015-07-27 19:08:09]

The CQ bit was checked by simonhatch@chromium.org

[shatch, 2015-07-27 19:08:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
oysteine@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1243743002/#ps310001
(title: "Addressed dcheng's comments.")

[commit-bot: I haz the power, 2015-07-27 19:08:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1243743002/310001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1243743002/310001

[commit-bot: I haz the power, 2015-07-27 20:13:26]

Committed patchset #7 (id:310001)

[commit-bot: I haz the power, 2015-07-27 20:14:20]

Patchset 7 (id:??) landed as
https://crrev.com/cebf0f7018da1787d11bed7b9a406e29a84e5d79
Cr-Commit-Position: refs/heads/master@{#340544}