Add more navigation tracing to RenderFrameHost and NavigationRequest.

Add more navigation tracing to RenderFrameHost and NavigationRequest.

This is a follow up CL to https://codereview.chromium.org/2830353003/ to
add more trace events and allow for navigation to be easily debugged
only through capturing a trace.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2846983002
Cr-Commit-Position: refs/heads/master@{#468034}
Committed: https://chromium.googlesource.com/chromium/src/+/ea44a038e5c5d78b5756e9b1db213c505c260c93

[nasko, 2017-04-27 22:10:01]

Description was changed from

==========
Add more navigation tracing to RenderFrameHost and NavigationRequest.

This is a follow up CL to https://codereview.chromium.org/2830353003/ to
add more trace events and allow for navigation to be easily debugged
only through capturing a trace.

BUG=
==========

to

==========
Add more navigation tracing to RenderFrameHost and NavigationRequest.

This is a follow up CL to https://codereview.chromium.org/2830353003/ to
add more trace events and allow for navigation to be easily debugged
only through capturing a trace.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2017-04-27 22:15:19]

nasko@chromium.org changed reviewers:
+ clamy@chromium.org

[nasko, 2017-04-27 22:15:19]

Hey Camille,
Can you review this CL for me? It adds more tracing to navigations and should be
helpful in debugging.
Thanks in advance!
Nasko

[nasko, 2017-04-27 22:15:26]

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

[commit-bot: I haz the power, 2017-04-27 22:30:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nasko, 2017-04-28 00:09:32]

Also, if the CL looks fine, feel free to check the CQ box.

[Charlie Reis, 2017-04-28 00:30:52]

Cool-- I look forward to using!  I'll let clamy@ review the actual code; just
one drive-by nit as I was seeing what it looked like.

https://codereview.chromium.org/2846983002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/2846983002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.h:141: state_ =
WAITING_FOR_RENDERER_RESPONSE;
Minor nit: This implementation probably belongs in the .cc file at this point,
with the extra code.  (It's already not hacker_style(), so it seems a bit odd to
have it inlined anyway.)

[commit-bot: I haz the power, 2017-04-28 02:58:01]

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

[commit-bot: I haz the power, 2017-04-28 02:58:02]

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

[nasko, 2017-04-28 14:01:53]

https://codereview.chromium.org/2846983002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/2846983002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.h:141: state_ =
WAITING_FOR_RENDERER_RESPONSE;
On 2017/04/28 00:30:52, Charlie Reis wrote:
> Minor nit: This implementation probably belongs in the .cc file at this point,
> with the extra code.  (It's already not hacker_style(), so it seems a bit odd
to
> have it inlined anyway.)

Done.

[clamy, 2017-04-28 14:07:15]

Thanks, that seems really helpful! Lgtm. I have also added some suggestions for
additional traces, though that doesn't need to happen in this patch.

https://codereview.chromium.org/2846983002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2846983002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:682:
NavigationThrottle::ThrottleCheckResult result) {
How about adding some traces in the On*ChecksComplete methods here and below?
These are the ones that will actually do stuff such as sending the request to
the net stack or committing it.

https://codereview.chromium.org/2846983002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/2846983002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:140: void
set_associated_site_instance_type(AssociatedSiteInstanceType type) {
I'm wondering if we should also trace this, possibly with the RFH/RPH ids of the
speculative RFH. Same when we commit.

https://codereview.chromium.org/2846983002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2846983002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2091:
TRACE_EVENT2("navigation", "RenderFrameHostImpl::OnBeforeNavigation",
nit: s/OnBeforeNavigation/OnBeginNavigation ?

[nasko, 2017-04-28 14:18:57]

https://codereview.chromium.org/2846983002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2846983002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:682:
NavigationThrottle::ThrottleCheckResult result) {
On 2017/04/28 14:07:15, clamy wrote:
> How about adding some traces in the On*ChecksComplete methods here and below?
> These are the ones that will actually do stuff such as sending the request to
> the net stack or committing it.

All NavigationThrottle checks are already instrumented in NavigationHandle. If
we get to this method, we already have a trace of what happened with UI thread
throttles and otherwise we will proceed on sending the request to the network
stack. My approach was to add trace events at the places where I see state_
changes. Is there anything else specific that you care about that I should add?

https://codereview.chromium.org/2846983002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/2846983002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:140: void
set_associated_site_instance_type(AssociatedSiteInstanceType type) {
On 2017/04/28 14:07:15, clamy wrote:
> I'm wondering if we should also trace this, possibly with the RFH/RPH ids of
the
> speculative RFH. Same when we commit.

Sure. I'll try to add this.

[clamy, 2017-04-28 14:21:01]

https://codereview.chromium.org/2846983002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2846983002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:682:
NavigationThrottle::ThrottleCheckResult result) {
On 2017/04/28 14:18:56, nasko wrote:
> On 2017/04/28 14:07:15, clamy wrote:
> > How about adding some traces in the On*ChecksComplete methods here and
below?
> > These are the ones that will actually do stuff such as sending the request
to
> > the net stack or committing it.
> 
> All NavigationThrottle checks are already instrumented in NavigationHandle. If
> we get to this method, we already have a trace of what happened with UI thread
> throttles and otherwise we will proceed on sending the request to the network
> stack. My approach was to add trace events at the places where I see state_
> changes. Is there anything else specific that you care about that I should
add?

Acknowledged. Seems we should be covered enough.

[nasko, 2017-04-28 15:58:56]

https://codereview.chromium.org/2846983002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/2846983002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:140: void
set_associated_site_instance_type(AssociatedSiteInstanceType type) {
On 2017/04/28 14:18:56, nasko wrote:
> On 2017/04/28 14:07:15, clamy wrote:
> > I'm wondering if we should also trace this, possibly with the RFH/RPH ids of
> the
> > speculative RFH. Same when we commit.
> 
> Sure. I'll try to add this.

I looked at this and it seems more appropriate to trace RFH/RPH from the caller
of this method, which is RFHM. There is probably other interesting state to
trace in RFHM, so I'll do it in a separate CL altogether.

[nasko, 2017-04-28 15:59:01]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2017-04-28 15:59:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 17:07:32]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493395141931480,
"parent_rev": "4c4914e6e841e18cfbc6d13ebb2ba38276aaff5f", "commit_rev":
"ea44a038e5c5d78b5756e9b1db213c505c260c93"}

[commit-bot: I haz the power, 2017-04-28 17:07:45]

Description was changed from

==========
Add more navigation tracing to RenderFrameHost and NavigationRequest.

This is a follow up CL to https://codereview.chromium.org/2830353003/ to
add more trace events and allow for navigation to be easily debugged
only through capturing a trace.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add more navigation tracing to RenderFrameHost and NavigationRequest.

This is a follow up CL to https://codereview.chromium.org/2830353003/ to
add more trace events and allow for navigation to be easily debugged
only through capturing a trace.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2846983002
Cr-Commit-Position: refs/heads/master@{#468034}
Committed:
https://chromium.googlesource.com/chromium/src/+/ea44a038e5c5d78b5756e9b1db21...
==========

[commit-bot: I haz the power, 2017-04-28 17:07:47]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/ea44a038e5c5d78b5756e9b1db21...