DevTools: use explicit IPC messages for enabling/disabling tracing instead of intercepting protocol…

DevTools: use explicit IPC messages for enabling/disabling tracing instead of intercepting protocol events

Added IPC plumbing that allows InspectorTracingAgent to enable/disable  browser-wide tracing. It was implemented by intercepting Tracing.started/Tracing.stopped events in the browser. Tracing.stopped event in the DevTools protocol was used by the renderer to notify the browser that it should stop trace event recording and it didn't make much sense for the client as there is already Tracing.tracingComplete event in the protocol.

BUG=398787
Committed: https://chromium.googlesource.com/chromium/src/+/a449adae35631e4791547bd0b14403d1766f9ddf

Committed: https://crrev.com/16f8dbcecc8131d862e4f16d848612ac00910f97
Cr-Commit-Position: refs/heads/master@{#292578}

[yurys, 2014-08-27 13:27:30]

yurys@chromium.org changed reviewers:
+ caseq@chromium.org, loislo@chromium.org

[caseq, 2014-08-27 13:45:50]

lgtm

[yurys, 2014-08-27 14:06:23]

yurys@chromium.org changed reviewers:
+ dcheng@chromium.org

[yurys, 2014-08-27 14:06:23]

@dcheng: please review changes in content/common/devtools_messages.h

[pfeldman, 2014-08-27 14:26:41]

From the issue description, it reads as this is a design improvement, but in
fact it is the opposite.

We are going the other way around whereever possible. It is no longer
"intercepting" events, it is now "handling" commands on the browser level and
tracing should be entirely handled there if at all possible.

This case looks legit though since we need to drive tracing from the renderer
for console.timeline and there is no way other than IPC to achiebve that. So I
guess only issue description is what is wrong here.

[yurys, 2014-08-27 14:34:32]

yurys@chromium.org changed reviewers:
+ vsevik@chromium.org

[yurys, 2014-08-27 14:42:32]

On 2014/08/27 14:26:41, pfeldman-OOO wrote:
> From the issue description, it reads as this is a design improvement, but in
> fact it is the opposite.
> 
> We are going the other way around whereever possible. It is no longer
> "intercepting" events, it is now "handling" commands on the browser level and
> tracing should be entirely handled there if at all possible.
> 
Correct. But we never "handle" events send from the render process except this
particular case. The only reason why Tracing.stopped was introduced in the
protocol is to send notification from renderer to the browser that there was
console.timelineEnd() call and the browser handler should stop recording trace
events and send collected data to the user(via existing events). This is wrong
as it is pure implementation detail and should not be exposed in the protocol.

> This case looks legit though since we need to drive tracing from the renderer
> for console.timeline and there is no way other than IPC to achiebve that. So I
> guess only issue description is what is wrong here.

How would you suggest to modify the description?

[dcheng, 2014-08-27 16:43:31]

https://codereview.chromium.org/511873002/diff/20001/content/browser/devtools...
File content/browser/devtools/devtools_tracing_handler.cc (right):

https://codereview.chromium.org/511873002/diff/20001/content/browser/devtools...
content/browser/devtools/devtools_tracing_handler.cc:276:
base::debug::CategoryFilter(category_filter),
I'm not super familiar with inspector. From the CL description, it sounds like
the renderer could already pass arbitrary category_filter strings to the
browser, via 'protocol events' (I am guessing this is what
OnDispatchOnInspectorFrontend is?) Is this true?

[yurys, 2014-08-28 04:20:39]

On 2014/08/27 16:43:31, dcheng (OOO) wrote:
>
https://codereview.chromium.org/511873002/diff/20001/content/browser/devtools...
> File content/browser/devtools/devtools_tracing_handler.cc (right):
> 
>
https://codereview.chromium.org/511873002/diff/20001/content/browser/devtools...
> content/browser/devtools/devtools_tracing_handler.cc:276:
> base::debug::CategoryFilter(category_filter),
> I'm not super familiar with inspector. From the CL description, it sounds like
> the renderer could already pass arbitrary category_filter strings to the
> browser, via 'protocol events' (I am guessing this is what
> OnDispatchOnInspectorFrontend is?) Is this true?

Yes, that's correct. This CL just makes it more obvious.

[dcheng, 2014-08-28 04:50:17]

On 2014/08/28 at 04:20:39, yurys wrote:
> On 2014/08/27 16:43:31, dcheng (OOO) wrote:
> >
https://codereview.chromium.org/511873002/diff/20001/content/browser/devtools...
> > File content/browser/devtools/devtools_tracing_handler.cc (right):
> > 
> >
https://codereview.chromium.org/511873002/diff/20001/content/browser/devtools...
> > content/browser/devtools/devtools_tracing_handler.cc:276:
> > base::debug::CategoryFilter(category_filter),
> > I'm not super familiar with inspector. From the CL description, it sounds
like
> > the renderer could already pass arbitrary category_filter strings to the
> > browser, via 'protocol events' (I am guessing this is what
> > OnDispatchOnInspectorFrontend is?) Is this true?
> 
> Yes, that's correct. This CL just makes it more obvious.

OK. I took a look at CategoryFilter and it seems to be safe to deserialize it
from what's sent over IPC. However, I think it would be good if there was an
explicit review of that code to confirm that (if there hasn't been one already).
A comment noting that CategoryFilter is security-sensitive because it's
deserialized from IPC data would be good too. It's fine to do this separately,
since we're already doing this--it just makes it easier for IPC reviewers like
me.

content/common/devtools_messages.h LGTM

[yurys, 2014-08-28 13:28:04]

The CQ bit was checked by yurys@chromium.org

[commit-bot: I haz the power, 2014-08-28 13:29:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yurys@chromium.org/511873002/40001

[commit-bot: I haz the power, 2014-08-28 14:29:13]

Committed patchset #3 (id:40001) as a449adae35631e4791547bd0b14403d1766f9ddf

[mlamouri (slow - plz ping), 2014-08-28 15:40:52]

A revert of this CL (patchset #3) has been created in
https://codereview.chromium.org/516963003/ by mlamouri@chromium.org.

The reason for reverting is: Creates timeout failures on a lot of inspector/
tests in Blink:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20%28deps....

[yurys, 2014-08-28 16:06:21]

On 2014/08/28 15:40:52, Mounir Lamouri wrote:
> A revert of this CL (patchset #3) has been created in
> https://codereview.chromium.org/516963003/ by mailto:mlamouri@chromium.org.
> 
> The reason for reverting is: Creates timeout failures on a lot of inspector/
> tests in Blink:
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20%28deps....

Oops, this CL depends on
https://src.chromium.org/viewvc/blink?revision=181029&view=revision. I should
have waited until it is rolled.

[yurys, 2014-08-29 06:10:14]

The CQ bit was checked by yurys@chromium.org

[commit-bot: I haz the power, 2014-08-29 06:11:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yurys@chromium.org/511873002/40001

[commit-bot: I haz the power, 2014-08-29 06:11:43]

Committed patchset #3 (id:40001) as c7c2443ab48b35ec4accdd7c1e3b6c1434b6b86f

[commit-bot: I haz the power, 2014-09-10 02:59:28]

Patchset 3 (id:??) landed as
https://crrev.com/97c48c6f7a1848ec11de959811a1c31581478d97
Cr-Commit-Position: refs/heads/master@{#292379}

[commit-bot: I haz the power, 2014-09-10 03:06:38]

Patchset 3 (id:??) landed as
https://crrev.com/16f8dbcecc8131d862e4f16d848612ac00910f97
Cr-Commit-Position: refs/heads/master@{#292578}