Fix NavigationHandle being thrown out and recreated (without all the state) after rapid navigations.

Fix NavigationHandle being thrown out and recreated (without all the state) after rapid navigations.

This only happens with PlzNavigate as the NavigationHandle is created when the renderer sends back the FrameHostMsg_BeforeUnload_ACK IPC. If there was a previous navigation in the renderer, it could end up generating a FrameHostMsg_DidStopLoading or FrameHostMsg_DidCommitProvisionalLoad IPCs which would reset the new navigation's NavigationHandle in RenderFrameHost. Once the new navigation receives FrameHostMsg_DidCommitProvisionalLoad, it would recreate a NavigationHandle in RenderFrameHostImpl::TakeNavigationHandleForCommit. However that NavigationHandle would be missing the SSLStatus, which is why rapid back-to-back navigations sometimes causes the SSL lock icon to disappear. Similarly, RenderFrameHostImpl::OnDidStopLoading() resets the NavigationHandle leading to the same outcome.

This doesn't happen without PlzNavigate because the NavigationHandle is created later, when the FrameHostMsg_DidStartProvisionalLoad IPC is sent. This ensures that IPCs related to previous navigations have already been flushed.

The fix is to call WebFrame::StopLoading when the renderer receives the FrameMsg_BeforeUnload IPC. This ensures that IPCs related to stopping/committing a previous navigations are flushed from the pipe before the new NavigationHandle is created. To handle stray IPCs related to loading finishing on a scheduled task, RenderFrameImpl::DidStopLoading also has to avoid send IPCs inbetween the time that beforeunload handler proceeded and the next commit.

BUG=738177, 727892
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

[jam, 2017-07-06 20:31:12]

Description was changed from

==========
Add tests for race conditions where a NavigationHandle for a new navigation
would be thrown out because of a previous navigation's commti message.

BUG=738177,727892
==========

to

==========
Add tests for race conditions where a NavigationHandle for a new navigation
would be thrown out because of a previous navigation's commti message.

BUG=738177,727892
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-07-06 20:31:16]

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

[commit-bot: I haz the power, 2017-07-06 20:31:33]

Dry run: 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-07-06 21:03:51]

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

[commit-bot: I haz the power, 2017-07-06 21:03:52]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[jam, 2017-07-06 22:40:55]

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

[commit-bot: I haz the power, 2017-07-06 22:41:13]

Dry run: 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-07-06 22:45:40]

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

[commit-bot: I haz the power, 2017-07-06 22:45:40]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[jam, 2017-07-06 22:47:59]

Description was changed from

==========
Add tests for race conditions where a NavigationHandle for a new navigation
would be thrown out because of a previous navigation's commti message.

BUG=738177,727892
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add test for race conditions where a NavigationHandle for a new navigation would
be thrown out because of a previous navigation's commit or stoploading IPC.

This only happens with PlzNavigate as the NavigationHandle is created when the
renderer sends back the FrameHostMsg_BeforeUnload_ACK IPC. If there was a
previous navigation in the renderer, it could end up generating a
FrameHostMsg_DidStopLoading or FrameHostMsg_DidCommitProvisionalLoad IPCs which
would reset the new navigation's NavigationHandle in RenderFrameHost. Once the
new navigation receives FrameHostMsg_DidCommitProvisionalLoad, it would recreate
a NavigationHandle in RenderFrameHostImpl::TakeNavigationHandleForCommit.
However that NavigationHandle would be missing the SSLStatus, which is why rapid
back-to-back navigations sometimes causes the SSL lock icon to disappear.

This doesn't happen without PlzNavigate because the NavigationHandle is created
later, when the FrameHostMsg_DidStartProvisionalLoad IPC is sent. This ensures
that IPCs related to previous navigations have already been flushed.

The fix is to call WebFrame::StopLoading when the renderer receives the
FrameMsg_BeforeUnload IPC. This ensures that IPCs related to stopping/committing
a previous navigations are flushed from the pipe before the new NavigationHandle
is created.

BUG=738177,727892
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-07-06 22:48:56]

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

[commit-bot: I haz the power, 2017-07-06 22:49:08]

Dry run: 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-07-06 22:54:41]

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

[commit-bot: I haz the power, 2017-07-06 22:54:41]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[jam, 2017-07-06 23:14:03]

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

[commit-bot: I haz the power, 2017-07-06 23:14:09]

Dry run: 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-07-06 23:44:28]

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

[commit-bot: I haz the power, 2017-07-06 23:44:29]

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

[jam, 2017-07-07 23:48:22]

Patchset #3 (id:40001) has been deleted

[jam, 2017-07-07 23:48:55]

Patchset #2 (id:20001) has been deleted

[jam, 2017-07-07 23:49:01]

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

[commit-bot: I haz the power, 2017-07-07 23:49:17]

Dry run: CQ is trying da patch.

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

[jam, 2017-07-07 23:52:34]

Description was changed from

==========
Add test for race conditions where a NavigationHandle for a new navigation would
be thrown out because of a previous navigation's commit or stoploading IPC.

This only happens with PlzNavigate as the NavigationHandle is created when the
renderer sends back the FrameHostMsg_BeforeUnload_ACK IPC. If there was a
previous navigation in the renderer, it could end up generating a
FrameHostMsg_DidStopLoading or FrameHostMsg_DidCommitProvisionalLoad IPCs which
would reset the new navigation's NavigationHandle in RenderFrameHost. Once the
new navigation receives FrameHostMsg_DidCommitProvisionalLoad, it would recreate
a NavigationHandle in RenderFrameHostImpl::TakeNavigationHandleForCommit.
However that NavigationHandle would be missing the SSLStatus, which is why rapid
back-to-back navigations sometimes causes the SSL lock icon to disappear.

This doesn't happen without PlzNavigate because the NavigationHandle is created
later, when the FrameHostMsg_DidStartProvisionalLoad IPC is sent. This ensures
that IPCs related to previous navigations have already been flushed.

The fix is to call WebFrame::StopLoading when the renderer receives the
FrameMsg_BeforeUnload IPC. This ensures that IPCs related to stopping/committing
a previous navigations are flushed from the pipe before the new NavigationHandle
is created.

BUG=738177,727892
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add test for race conditions where a NavigationHandle for a new navigation would
be thrown out because of a previous navigation's commit or stoploading IPC.

This only happens with PlzNavigate as the NavigationHandle is created when the
renderer sends back the FrameHostMsg_BeforeUnload_ACK IPC. If there was a
previous navigation in the renderer, it could end up generating a
FrameHostMsg_DidStopLoading or FrameHostMsg_DidCommitProvisionalLoad IPCs which
would reset the new navigation's NavigationHandle in RenderFrameHost. Once the
new navigation receives FrameHostMsg_DidCommitProvisionalLoad, it would recreate
a NavigationHandle in RenderFrameHostImpl::TakeNavigationHandleForCommit.
However that NavigationHandle would be missing the SSLStatus, which is why rapid
back-to-back navigations sometimes causes the SSL lock icon to disappear.

This doesn't happen without PlzNavigate because the NavigationHandle is created
later, when the FrameHostMsg_DidStartProvisionalLoad IPC is sent. This ensures
that IPCs related to previous navigations have already been flushed.

The fix is to call WebFrame::StopLoading when the renderer receives the
FrameMsg_BeforeUnload IPC. This ensures that IPCs related to stopping/committing
a previous navigations are flushed from the pipe before the new NavigationHandle
is created. To handle stray IPCs related to loading finishing on a scheduled
task, RenderFrameImpl::DidStopLoading also has to avoid send IPCs inbetween the
time that beforeunload handler proceeded and the next commit.

BUG=738177,727892
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-07-07 23:55:45]

The CQ bit was unchecked by jam@chromium.org

[jam, 2017-07-08 00:14:56]

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

[jam, 2017-07-08 00:15:09]

The CQ bit was unchecked by jam@chromium.org

[commit-bot: I haz the power, 2017-07-08 00:15:11]

Dry run: CQ is trying da patch.

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

[jam, 2017-07-08 00:15:29]

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

[commit-bot: I haz the power, 2017-07-08 00:15:50]

Dry run: CQ is trying da patch.

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

[jam, 2017-07-08 01:04:42]

Patchset #2 (id:60001) has been deleted

[jam, 2017-07-08 01:31:41]

Description was changed from

==========
Add test for race conditions where a NavigationHandle for a new navigation would
be thrown out because of a previous navigation's commit or stoploading IPC.

This only happens with PlzNavigate as the NavigationHandle is created when the
renderer sends back the FrameHostMsg_BeforeUnload_ACK IPC. If there was a
previous navigation in the renderer, it could end up generating a
FrameHostMsg_DidStopLoading or FrameHostMsg_DidCommitProvisionalLoad IPCs which
would reset the new navigation's NavigationHandle in RenderFrameHost. Once the
new navigation receives FrameHostMsg_DidCommitProvisionalLoad, it would recreate
a NavigationHandle in RenderFrameHostImpl::TakeNavigationHandleForCommit.
However that NavigationHandle would be missing the SSLStatus, which is why rapid
back-to-back navigations sometimes causes the SSL lock icon to disappear.

This doesn't happen without PlzNavigate because the NavigationHandle is created
later, when the FrameHostMsg_DidStartProvisionalLoad IPC is sent. This ensures
that IPCs related to previous navigations have already been flushed.

The fix is to call WebFrame::StopLoading when the renderer receives the
FrameMsg_BeforeUnload IPC. This ensures that IPCs related to stopping/committing
a previous navigations are flushed from the pipe before the new NavigationHandle
is created. To handle stray IPCs related to loading finishing on a scheduled
task, RenderFrameImpl::DidStopLoading also has to avoid send IPCs inbetween the
time that beforeunload handler proceeded and the next commit.

BUG=738177,727892
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix NavigationHandle being thrown out and recreated (without all the state)
after rapid navigations.

This only happens with PlzNavigate as the NavigationHandle is created when the
renderer sends back the FrameHostMsg_BeforeUnload_ACK IPC. If there was a
previous navigation in the renderer, it could end up generating a
FrameHostMsg_DidStopLoading or FrameHostMsg_DidCommitProvisionalLoad IPCs which
would reset the new navigation's NavigationHandle in RenderFrameHost. Once the
new navigation receives FrameHostMsg_DidCommitProvisionalLoad, it would recreate
a NavigationHandle in RenderFrameHostImpl::TakeNavigationHandleForCommit.
However that NavigationHandle would be missing the SSLStatus, which is why rapid
back-to-back navigations sometimes causes the SSL lock icon to disappear.

This doesn't happen without PlzNavigate because the NavigationHandle is created
later, when the FrameHostMsg_DidStartProvisionalLoad IPC is sent. This ensures
that IPCs related to previous navigations have already been flushed.

The fix is to call WebFrame::StopLoading when the renderer receives the
FrameMsg_BeforeUnload IPC. This ensures that IPCs related to stopping/committing
a previous navigations are flushed from the pipe before the new NavigationHandle
is created. To handle stray IPCs related to loading finishing on a scheduled
task, RenderFrameImpl::DidStopLoading also has to avoid send IPCs inbetween the
time that beforeunload handler proceeded and the next commit.

BUG=738177,727892
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-07-08 01:31:41]

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

[jam, 2017-07-08 01:32:49]

https://codereview.chromium.org/2974553002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2974553002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:1772: frame_->StopLoading();
this prevents stray FrameHostMsg_DidCommitProvisionalLoad and sometimes
FrameHostMsg_DidStopLoading

https://codereview.chromium.org/2974553002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:5090: return;
this prevents the other case of stray FrameHostMsg_DidStopLoading being sent
(when the loading was scheduled later)

[commit-bot: I haz the power, 2017-07-08 01:36:08]

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

[commit-bot: I haz the power, 2017-07-08 01:36:09]

Dry run: This issue passed the CQ dry run.

[jam, 2017-07-10 05:49:30]

Description was changed from

==========
Fix NavigationHandle being thrown out and recreated (without all the state)
after rapid navigations.

This only happens with PlzNavigate as the NavigationHandle is created when the
renderer sends back the FrameHostMsg_BeforeUnload_ACK IPC. If there was a
previous navigation in the renderer, it could end up generating a
FrameHostMsg_DidStopLoading or FrameHostMsg_DidCommitProvisionalLoad IPCs which
would reset the new navigation's NavigationHandle in RenderFrameHost. Once the
new navigation receives FrameHostMsg_DidCommitProvisionalLoad, it would recreate
a NavigationHandle in RenderFrameHostImpl::TakeNavigationHandleForCommit.
However that NavigationHandle would be missing the SSLStatus, which is why rapid
back-to-back navigations sometimes causes the SSL lock icon to disappear.

This doesn't happen without PlzNavigate because the NavigationHandle is created
later, when the FrameHostMsg_DidStartProvisionalLoad IPC is sent. This ensures
that IPCs related to previous navigations have already been flushed.

The fix is to call WebFrame::StopLoading when the renderer receives the
FrameMsg_BeforeUnload IPC. This ensures that IPCs related to stopping/committing
a previous navigations are flushed from the pipe before the new NavigationHandle
is created. To handle stray IPCs related to loading finishing on a scheduled
task, RenderFrameImpl::DidStopLoading also has to avoid send IPCs inbetween the
time that beforeunload handler proceeded and the next commit.

BUG=738177,727892
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix NavigationHandle being thrown out and recreated (without all the state)
after rapid navigations.

This only happens with PlzNavigate as the NavigationHandle is created when the
renderer sends back the FrameHostMsg_BeforeUnload_ACK IPC. If there was a
previous navigation in the renderer, it could end up generating a
FrameHostMsg_DidStopLoading or FrameHostMsg_DidCommitProvisionalLoad IPCs which
would reset the new navigation's NavigationHandle in RenderFrameHost. Once the
new navigation receives FrameHostMsg_DidCommitProvisionalLoad, it would recreate
a NavigationHandle in RenderFrameHostImpl::TakeNavigationHandleForCommit.
However that NavigationHandle would be missing the SSLStatus, which is why rapid
back-to-back navigations sometimes causes the SSL lock icon to disappear.
Similarly, RenderFrameHostImpl::OnDidStopLoading() resets the NavigationHandle
leading to the same outcome.

This doesn't happen without PlzNavigate because the NavigationHandle is created
later, when the FrameHostMsg_DidStartProvisionalLoad IPC is sent. This ensures
that IPCs related to previous navigations have already been flushed.

The fix is to call WebFrame::StopLoading when the renderer receives the
FrameMsg_BeforeUnload IPC. This ensures that IPCs related to stopping/committing
a previous navigations are flushed from the pipe before the new NavigationHandle
is created. To handle stray IPCs related to loading finishing on a scheduled
task, RenderFrameImpl::DidStopLoading also has to avoid send IPCs inbetween the
time that beforeunload handler proceeded and the next commit.

BUG=738177,727892
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2017-07-10 13:02:05]

Thanks! I'm not sure the approach in that patch is what we want though. Nasko
and I have been thinking that having DidStopLoading reset the NavigationHandle
in RenderFrameHostImpl is wrong when PlzNavigate is enabled. Maybe you could
look into that direction? Note: I'm having ahemery@ experiment with that as part
of skipping waiting for BeforeUnload when it's not needed. Also as part of
refactoring NavigationRequest so that we can send OnResponseComplete to the
renderer when we have a Mojo datapipe, I'm thinking of storing all
NavigationHandles that we ask the renderer to commit instead of just the last
one. This could also help with those race conditions.

https://codereview.chromium.org/2974553002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2974553002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:1772: frame_->StopLoading();
On 2017/07/08 01:32:49, jam wrote:
> this prevents stray FrameHostMsg_DidCommitProvisionalLoad and sometimes
> FrameHostMsg_DidStopLoading

That seems wrong to me. If we're trying to load a page and the navigation
doesn't go through (eg if it's cancelled), the user will be stuck with a
half-loaded page. I don't think this is what we want.

[jam, 2017-07-10 14:43:45]

(Camille and I chatted, here's the summary)

Camille pointed out that calling stop is wrong since only beforeunload ran, and
not unload. Combined with the other points about other changes that will fix
this issue, I'm going to close this change. I will try to do an isolated change
just for the SSL state lost bug.

On 2017/07/10 13:02:05, clamy wrote:
> Thanks! I'm not sure the approach in that patch is what we want though. Nasko
> and I have been thinking that having DidStopLoading reset the NavigationHandle
> in RenderFrameHostImpl is wrong when PlzNavigate is enabled. Maybe you could
> look into that direction? Note: I'm having ahemery@ experiment with that as
part
> of skipping waiting for BeforeUnload when it's not needed. Also as part of
> refactoring NavigationRequest so that we can send OnResponseComplete to the
> renderer when we have a Mojo datapipe, I'm thinking of storing all
> NavigationHandles that we ask the renderer to commit instead of just the last
> one. This could also help with those race conditions.
> 
>
https://codereview.chromium.org/2974553002/diff/80001/content/renderer/render...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/2974553002/diff/80001/content/renderer/render...
> content/renderer/render_frame_impl.cc:1772: frame_->StopLoading();
> On 2017/07/08 01:32:49, jam wrote:
> > this prevents stray FrameHostMsg_DidCommitProvisionalLoad and sometimes
> > FrameHostMsg_DidStopLoading
> 
> That seems wrong to me. If we're trying to load a page and the navigation
> doesn't go through (eg if it's cancelled), the user will be stuck with a
> half-loaded page. I don't think this is what we want.