Issue AboutToNavigateRenderFrame notification after beforeunload handling.

Issue AboutToNavigateRenderFrame notification after beforeunload handling.

Debugger can stop the renderer in beforeunload handling, and navigation
will not proceed. Thus, moving the notification after beforeunload ack
is received seems logical. This allows DevTools to correctly handle
beforeunload breakpoint case.

BUG=131371

[dgozman, 2015-09-22 18:21:35]

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

[commit-bot: I haz the power, 2015-09-22 18:22:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1358153002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1358153002/1

[dgozman, 2015-09-22 18:23:33]

dgozman@chromium.org changed reviewers:
+ nasko@chromium.org, yurys@chromium.org

[dgozman, 2015-09-22 18:23:33]

Hi Nasko,

Could you please take a look? I think this is conceptually right, but I'm not
sure how to implement it best.

Basically, when navigation is suspended (because of beforeunload handling), we
don't send AboutToNavigateRenderFrame, and only issue it after navigation may
actually proceed.

Thanks,
Dmitry

[commit-bot: I haz the power, 2015-09-22 18:53:46]

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

[commit-bot: I haz the power, 2015-09-22 18:53:47]

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_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yurys, 2015-09-22 19:31:43]

This approach should fix the bug but I'd rather have a single place where
FrameMsg_Navigate message is sent and call AboutToNavigateRenderFrame right
before sending the message. WDYT?

https://codereview.chromium.org/1358153002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1358153002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:318:
delegate_->AboutToNavigateRenderFrame(frame_tree_node->current_frame_host(),
Should we move this call closer to the two places where  "Send(new
FrameMsg_Navigate" is callsed?

[nasko, 2015-09-22 20:46:41]

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

[nasko, 2015-09-22 20:46:42]

Hey Charlie,
Can you take a look at this? I won't be able to look at this soon and don't want
to be in the blocking path.
Thanks!

[dgozman, 2015-09-22 20:51:31]

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

[commit-bot: I haz the power, 2015-09-22 20:52:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1358153002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1358153002/20001

[dgozman, 2015-09-22 20:54:30]

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

https://codereview.chromium.org/1358153002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:562:
pending_render_frame_host_->ShouldDispatchBeforeUnload()) {
This unfortunate check is here to not issue notification twice for subframes (we
immediately cancel beforeunload handling for them).

[commit-bot: I haz the power, 2015-09-22 21:21:53]

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

[commit-bot: I haz the power, 2015-09-22 21:21:54]

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

[Charlie Reis, 2015-09-22 23:12:10]

Hmm, I don't understand what impact this change has.  Maybe you can clarify in
the questions below?


On 2015/09/22 19:31:43, yurys wrote:
> This approach should fix the bug but I'd rather have a single place where
> FrameMsg_Navigate message is sent and call AboutToNavigateRenderFrame right
> before sending the message. WDYT?

You could abstract the Send call into a helper function in RenderFrameHostImpl
to make that happen.  See my other questions about this change below, though.

https://codereview.chromium.org/1358153002/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right):

https://codereview.chromium.org/1358153002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_browsertest.cc:42:
waiting_for_command_result_id_(0) {
What are these test changes?  I don't see a test for the actual bug, and I'm not
sure how these changes are related to the AboutToNavigateRenderFrame change.

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

https://codereview.chromium.org/1358153002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:318:
delegate_->AboutToNavigateRenderFrame(frame_tree_node->current_frame_host(),
What's happening in AboutToNavigateRenderFrame that must be delayed until after
the beforeunload handler?

I'm confused, because even after this change, the browser process will time out
and allow the navigation to proceed if there's a breakpoint in beforeunload. 
What's different about firing AboutToNavigateRenderFrame later?

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

https://codereview.chromium.org/1358153002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:555: // already made by
ShouldCloseTabOnUnresponsiveRenderer.  In that case, it
What about this case (on line 533)?  If the unresponsiveness timer fires, we'll
call SetNavigationsSuspended up there and allow the navigation to proceed.

https://codereview.chromium.org/1358153002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:562:
pending_render_frame_host_->ShouldDispatchBeforeUnload()) {
On 2015/09/22 20:54:30, dgozman wrote:
> This unfortunate check is here to not issue notification twice for subframes
(we
> immediately cancel beforeunload handling for them).

This is almost certainly not what you want to do.  Why would it matter whether
the pending (destination) RFH has a beforeunload handler?  In most cases, we've
just created that RFH and it hasn't loaded a page yet.

I also don't understand how it relates to the subframe case you describe.

https://codereview.chromium.org/1358153002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:563:
navigator_delegate->AboutToNavigateRenderFrame(
What if we moved AboutToNavigateRenderFrame to RenderFrameHostDelegate (which is
WebContents, just like NavigatorDelegate) and moved all of the calls inside
RenderFrameHostImpl?  We could even use a helper function to do the call and the
Send, as yurys@ suggests.

That would avoid the problem above with ShouldCloseTabOnUnresponsiveRenderer.

[dgozman, 2015-09-22 23:27:56]

> Hmm, I don't understand what impact this change has.  Maybe you can clarify in
the questions below?
We send AboutToNavigateRenderFrame later, potentially after round-trip to the
renderer for beforeunload. This makes DevTools think that navigation has not
started yet and we can still pause on breakpoint in beforeunload handler and do
other debug stuff.

This should be pretty safe, as we are the only client of
AboutToNavigateRenderFrame. Perhaps, we can event remove it from observer and
use a private call, similar to
RenderFrameDevToolsAgentHost::OnCancelPendingNavigation?

https://codereview.chromium.org/1358153002/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right):

https://codereview.chromium.org/1358153002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_browsertest.cc:42:
waiting_for_command_result_id_(0) {
On 2015/09/22 23:12:10, Charlie Reis wrote:
> What are these test changes?  I don't see a test for the actual bug, and I'm
not
> sure how these changes are related to the AboutToNavigateRenderFrame change.

This is a side-effect of different timings between synchronous calls (one of
them is AboutToNavigateRenderFrame). We just happened to be lucky and waited for
the first command ack, instead of the asked one. Now this properly waits for the
actual command we were asked to wait for.

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

https://codereview.chromium.org/1358153002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:318:
delegate_->AboutToNavigateRenderFrame(frame_tree_node->current_frame_host(),
On 2015/09/22 23:12:10, Charlie Reis wrote:
> What's happening in AboutToNavigateRenderFrame that must be delayed until
after
> the beforeunload handler?

It's called from another place.

> 
> I'm confused, because even after this change, the browser process will time
out
> and allow the navigation to proceed if there's a breakpoint in beforeunload. 
> What's different about firing AboutToNavigateRenderFrame later?

I didn't know about the timeout. How long is that? I didn't notice it while
testing locally.

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

https://codereview.chromium.org/1358153002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:562:
pending_render_frame_host_->ShouldDispatchBeforeUnload()) {
On 2015/09/22 23:12:10, Charlie Reis wrote:
> On 2015/09/22 20:54:30, dgozman wrote:
> > This unfortunate check is here to not issue notification twice for subframes
> (we
> > immediately cancel beforeunload handling for them).
> 
> This is almost certainly not what you want to do.  Why would it matter whether
> the pending (destination) RFH has a beforeunload handler?  In most cases,
we've
> just created that RFH and it hasn't loaded a page yet.
> 
> I also don't understand how it relates to the subframe case you describe.

See
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

https://codereview.chromium.org/1358153002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:563:
navigator_delegate->AboutToNavigateRenderFrame(
On 2015/09/22 23:12:10, Charlie Reis wrote:
> What if we moved AboutToNavigateRenderFrame to RenderFrameHostDelegate (which
is
> WebContents, just like NavigatorDelegate) and moved all of the calls inside
> RenderFrameHostImpl?  We could even use a helper function to do the call and
the
> Send, as yurys@ suggests.
> 
> That would avoid the problem above with ShouldCloseTabOnUnresponsiveRenderer.

I was hesitant about moving notifications between delegates. But if it's ok, I
will surely do that. Just needed a blessing from content owner.

[Charlie Reis, 2015-09-22 23:56:43]

On 2015/09/22 23:27:56, dgozman wrote:
> > Hmm, I don't understand what impact this change has.  Maybe you can clarify
in
> the questions below?
> We send AboutToNavigateRenderFrame later, potentially after round-trip to the
> renderer for beforeunload. This makes DevTools think that navigation has not
> started yet and we can still pause on breakpoint in beforeunload handler and
do
> other debug stuff.

See my comments on the timeout below, since I'm not sure this solves the
problem.

> This should be pretty safe, as we are the only client of
> AboutToNavigateRenderFrame. Perhaps, we can event remove it from observer and
> use a private call, similar to
> RenderFrameDevToolsAgentHost::OnCancelPendingNavigation?

Yes-- I'd love to have AboutToNavigateRenderFrame moved off WebContentsObserver!

https://codereview.chromium.org/1358153002/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right):

https://codereview.chromium.org/1358153002/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_browsertest.cc:42:
waiting_for_command_result_id_(0) {
On 2015/09/22 23:27:55, dgozman wrote:
> On 2015/09/22 23:12:10, Charlie Reis wrote:
> > What are these test changes?  I don't see a test for the actual bug, and I'm
> not
> > sure how these changes are related to the AboutToNavigateRenderFrame change.
> 
> This is a side-effect of different timings between synchronous calls (one of
> them is AboutToNavigateRenderFrame). We just happened to be lucky and waited
for
> the first command ack, instead of the asked one. Now this properly waits for
the
> actual command we were asked to wait for.

Ok, thanks.  It'd probably be worthwhile to include a test for the change once
we settle on the right approach, if possible.

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

https://codereview.chromium.org/1358153002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:318:
delegate_->AboutToNavigateRenderFrame(frame_tree_node->current_frame_host(),
On 2015/09/22 23:27:55, dgozman wrote:
> On 2015/09/22 23:12:10, Charlie Reis wrote:
> > What's happening in AboutToNavigateRenderFrame that must be delayed until
> after
> > the beforeunload handler?
> 
> It's called from another place.

That didn't really answer my question, though.

What is DevTools doing in AboutToNavigateRenderFrame that needs to be delayed?

> 
> > 
> > I'm confused, because even after this change, the browser process will time
> out
> > and allow the navigation to proceed if there's a breakpoint in beforeunload.

> > What's different about firing AboutToNavigateRenderFrame later?
> 
> I didn't know about the timeout. How long is that? I didn't notice it while
> testing locally.

RenderFrameHostImpl::DispatchBeforeUnload sets a timer for kUnloadTimeoutMS,
which is only 1 second.

That means that the navigation will likely try to proceed while the breakpoint
is hit, which seems wrong-- we'll be calling AboutToNavigateRenderFrame 1 second
later than before, but still during the breakpoint.  (I imagine the timeout is
part of the problem in today's behavior as well, though I'm not sure how that
leads to the hang.)

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

https://codereview.chromium.org/1358153002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:562:
pending_render_frame_host_->ShouldDispatchBeforeUnload()) {
On 2015/09/22 23:27:55, dgozman wrote:
> On 2015/09/22 23:12:10, Charlie Reis wrote:
> > On 2015/09/22 20:54:30, dgozman wrote:
> > > This unfortunate check is here to not issue notification twice for
subframes
> > (we
> > > immediately cancel beforeunload handling for them).
> > 
> > This is almost certainly not what you want to do.  Why would it matter
whether
> > the pending (destination) RFH has a beforeunload handler?  In most cases,
> we've
> > just created that RFH and it hasn't loaded a page yet.
> > 
> > I also don't understand how it relates to the subframe case you describe.
> 
> See
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

The current implementation of ShouldDispatchBeforeUnload just temporary code
until we support beforeunload on subframes.  If you're trying to check whether
this is a live subframe, calling ShouldDispatchBeforeUnload is not a good way to
do it.  Just call frame_tree_node_->IsMainFrame() and IsRenderFrameLive()
directly if that's the condition you're after.

However, I don't understand why that check is needed, especially on the pending
RFH.  You might want to take a closer look at why the notification is happening
twice in some cases, to help us understand how to resolve it.

https://codereview.chromium.org/1358153002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:563:
navigator_delegate->AboutToNavigateRenderFrame(
On 2015/09/22 23:27:56, dgozman wrote:
> On 2015/09/22 23:12:10, Charlie Reis wrote:
> > What if we moved AboutToNavigateRenderFrame to RenderFrameHostDelegate
(which
> is
> > WebContents, just like NavigatorDelegate) and moved all of the calls inside
> > RenderFrameHostImpl?  We could even use a helper function to do the call and
> the
> > Send, as yurys@ suggests.
> > 
> > That would avoid the problem above with
ShouldCloseTabOnUnresponsiveRenderer.
> 
> I was hesitant about moving notifications between delegates. But if it's ok, I
> will surely do that. Just needed a blessing from content owner.

Yep, it's fine.  And even better if we can remove the method from
WebContentsObserver, as you suggest.

[dgozman, 2015-09-23 00:05:21]

> > This should be pretty safe, as we are the only client of
> > AboutToNavigateRenderFrame. Perhaps, we can event remove it from observer
and
> > use a private call, similar to
> > RenderFrameDevToolsAgentHost::OnCancelPendingNavigation?
> 
> Yes-- I'd love to have AboutToNavigateRenderFrame moved off
WebContentsObserver!

Ok, I will try to do this.

> Ok, thanks.  It'd probably be worthwhile to include a test for the change once
> we settle on the right approach, if possible.

Sure! Just didn't want to work on test until the path is clear.

> 
>
https://codereview.chromium.org/1358153002/diff/20001/content/browser/frame_h...
> File content/browser/frame_host/navigator_impl.cc (right):
> 
>
https://codereview.chromium.org/1358153002/diff/20001/content/browser/frame_h...
> content/browser/frame_host/navigator_impl.cc:318:
> delegate_->AboutToNavigateRenderFrame(frame_tree_node->current_frame_host(),
> On 2015/09/22 23:27:55, dgozman wrote:
> > On 2015/09/22 23:12:10, Charlie Reis wrote:
> > > What's happening in AboutToNavigateRenderFrame that must be delayed until
> > after
> > > the beforeunload handler?
> > 
> > It's called from another place.
> 
> That didn't really answer my question, though.

Sorry, misunderstood the question. The problem is we ignore messages from the
renderer once navigation has started, including the "stopped on breakpoint" one.

> 
> What is DevTools doing in AboutToNavigateRenderFrame that needs to be delayed?
> 
> > 
> > > 
> > > I'm confused, because even after this change, the browser process will
time
> > out
> > > and allow the navigation to proceed if there's a breakpoint in
beforeunload.
> 
> > > What's different about firing AboutToNavigateRenderFrame later?
> > 
> > I didn't know about the timeout. How long is that? I didn't notice it while
> > testing locally.
> 
> RenderFrameHostImpl::DispatchBeforeUnload sets a timer for kUnloadTimeoutMS,
> which is only 1 second.
> 
> That means that the navigation will likely try to proceed while the breakpoint
> is hit, which seems wrong-- we'll be calling AboutToNavigateRenderFrame 1
second
> later than before, but still during the breakpoint.  (I imagine the timeout is
> part of the problem in today's behavior as well, though I'm not sure how that
> leads to the hang.)

Hmm... With this patch, I'm able to step over and continue way after 1 second.
I'll dig more to see what happens.

> > > This is almost certainly not what you want to do.  Why would it matter
> whether
> > > the pending (destination) RFH has a beforeunload handler?  In most cases,
> > we've
> > > just created that RFH and it hasn't loaded a page yet.
> > > 
> > > I also don't understand how it relates to the subframe case you describe.

Well, we immediately ACK for subframes (as I understand it). This happens
synchronously, so the code in NavigatorImpl doesn't have a chance to see that
beforeunload was kind of handled. See
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr....

> > I was hesitant about moving notifications between delegates. But if it's ok,
I
> > will surely do that. Just needed a blessing from content owner.
> 
> Yep, it's fine.  And even better if we can remove the method from
> WebContentsObserver, as you suggest.

I will use this approach and update the patch.

[Charlie Reis, 2015-09-23 00:09:54]

On Tue, Sep 22, 2015 at 5:05 PM, <dgozman@chromium.org> wrote:

> > This should be pretty safe, as we are the only client of
>> > AboutToNavigateRenderFrame. Perhaps, we can event remove it from
>> observer
>>
> and
>
>> > use a private call, similar to
>> > RenderFrameDevToolsAgentHost::OnCancelPendingNavigation?
>>
>
> Yes-- I'd love to have AboutToNavigateRenderFrame moved off
>>
> WebContentsObserver!
>
> Ok, I will try to do this.
>
> Ok, thanks.  It'd probably be worthwhile to include a test for the change
>> once
>> we settle on the right approach, if possible.
>>
>
> Sure! Just didn't want to work on test until the path is clear.
>
>
>
>
>
https://codereview.chromium.org/1358153002/diff/20001/content/browser/frame_h...
>
>> File content/browser/frame_host/navigator_impl.cc (right):
>>
>
>
>
>
https://codereview.chromium.org/1358153002/diff/20001/content/browser/frame_h...
>
>> content/browser/frame_host/navigator_impl.cc:318:
>>
>> delegate_->AboutToNavigateRenderFrame(frame_tree_node->current_frame_host(),
>> On 2015/09/22 23:27:55, dgozman wrote:
>> > On 2015/09/22 23:12:10, Charlie Reis wrote:
>> > > What's happening in AboutToNavigateRenderFrame that must be delayed
>> until
>> > after
>> > > the beforeunload handler?
>> >
>> > It's called from another place.
>>
>
> That didn't really answer my question, though.
>>
>
> Sorry, misunderstood the question. The problem is we ignore messages from
> the
> renderer once navigation has started, including the "stopped on
> breakpoint" one.
>
>
> What is DevTools doing in AboutToNavigateRenderFrame that needs to be
>> delayed?
>>
>
> >
>> > >
>> > > I'm confused, because even after this change, the browser process will
>>
> time
>
>> > out
>> > > and allow the navigation to proceed if there's a breakpoint in
>>
> beforeunload.
>
> > > What's different about firing AboutToNavigateRenderFrame later?
>> >
>> > I didn't know about the timeout. How long is that? I didn't notice it
>> while
>> > testing locally.
>>
>
> RenderFrameHostImpl::DispatchBeforeUnload sets a timer for
>> kUnloadTimeoutMS,
>> which is only 1 second.
>>
>
> That means that the navigation will likely try to proceed while the
>> breakpoint
>> is hit, which seems wrong-- we'll be calling AboutToNavigateRenderFrame 1
>>
> second
>
>> later than before, but still during the breakpoint.  (I imagine the
>> timeout is
>> part of the problem in today's behavior as well, though I'm not sure how
>> that
>> leads to the hang.)
>>
>
> Hmm... With this patch, I'm able to step over and continue way after 1
> second.
> I'll dig more to see what happens.


Maybe that's because we aren't calling AboutToNavigateRenderFrame from
ShouldCloseTabOnUnresponsiveRenderer?  That would probably hide this
problem but cause other problems if the timeout fires in practice without
having a breakpoint in beforeunload.

Anyway, gotta run but I can discuss more tomorrow.


>
> > > This is almost certainly not what you want to do.  Why would it matter
>> whether
>> > > the pending (destination) RFH has a beforeunload handler?  In most
>> cases,
>> > we've
>> > > just created that RFH and it hasn't loaded a page yet.
>> > >
>> > > I also don't understand how it relates to the subframe case you
>> describe.
>>
>
> Well, we immediately ACK for subframes (as I understand it). This happens
> synchronously, so the code in NavigatorImpl doesn't have a chance to see
> that
> beforeunload was kind of handled. See
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> .
>
> > I was hesitant about moving notifications between delegates. But if it's
>> ok,
>>
> I
>
>> > will surely do that. Just needed a blessing from content owner.
>>
>
> Yep, it's fine.  And even better if we can remove the method from
>> WebContentsObserver, as you suggest.
>>
>
> I will use this approach and update the patch.
>
> https://codereview.chromium.org/1358153002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.