PlzNavigate: stop navigations when opening a document for write

PlzNavigate: stop navigations when opening a document for write

This CL ensures that we stop renderer-initiated navigations when
attempting to do a document write. In order to do this in PlzNavigate,
we introduce a new IPC where the RenderFrame asks the browser process to
drop its renderer-initiated request. The CL also fixes an issue with the
cancellation of renderer-initiated navigations in PlzNavigate. We issued
a Stop IPC that would stop all loaders. Instead, we now have a new IPC
that will only cancel the dummy provisional DocumentLoader that keeps
track of navigations handled by the browser process.

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

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

[clamy, 2017-03-07 17:35:50]

Description was changed from

==========
PlzNavigate: stop navigations when opening a document for write

This CL ensures that we stop renderer-initiated navigations when
attempting to do a document write. In order to do this in PlzNavigate,
we introduce a new IPC where the RenderFrame asks the browser process to
drop its renderer-initiated request. The CL also fixes an issue with the
cancellation of renderer-initiated navigations in PlzNavigate. We issued
a Stop IPC that would stop all loaders. Instead, we now have a new IPC
that will only cancel the dummy provisional DocumentLoader that keeps
track of navigations handled by the browser process.

BUG=576261
==========

to

==========
PlzNavigate: stop navigations when opening a document for write

This CL ensures that we stop renderer-initiated navigations when
attempting to do a document write. In order to do this in PlzNavigate,
we introduce a new IPC where the RenderFrame asks the browser process to
drop its renderer-initiated request. The CL also fixes an issue with the
cancellation of renderer-initiated navigations in PlzNavigate. We issued
a Stop IPC that would stop all loaders. Instead, we now have a new IPC
that will only cancel the dummy provisional DocumentLoader that keeps
track of navigations handled by the browser process.

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

[clamy, 2017-03-07 17:35:56]

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

[clamy, 2017-03-07 17:36:23]

Description was changed from

==========
PlzNavigate: stop navigations when opening a document for write

This CL ensures that we stop renderer-initiated navigations when
attempting to do a document write. In order to do this in PlzNavigate,
we introduce a new IPC where the RenderFrame asks the browser process to
drop its renderer-initiated request. The CL also fixes an issue with the
cancellation of renderer-initiated navigations in PlzNavigate. We issued
a Stop IPC that would stop all loaders. Instead, we now have a new IPC
that will only cancel the dummy provisional DocumentLoader that keeps
track of navigations handled by the browser process.

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

to

==========
PlzNavigate: stop navigations when opening a document for write

This CL ensures that we stop renderer-initiated navigations when
attempting to do a document write. In order to do this in PlzNavigate,
we introduce a new IPC where the RenderFrame asks the browser process to
drop its renderer-initiated request. The CL also fixes an issue with the
cancellation of renderer-initiated navigations in PlzNavigate. We issued
a Stop IPC that would stop all loaders. Instead, we now have a new IPC
that will only cancel the dummy provisional DocumentLoader that keeps
track of navigations handled by the browser process.

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

[clamy, 2017-03-07 17:36:23]

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

[clamy, 2017-03-07 17:36:36]

@nasko, japhet: PTAL

[commit-bot: I haz the power, 2017-03-07 17:36:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nate Chapin, 2017-03-07 18:39:08]

https://codereview.chromium.org/2735183003/diff/1/content/common/frame_messag...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2735183003/diff/1/content/common/frame_messag...
content/common/frame_messages.h:892:
IPC_MESSAGE_ROUTED0(FrameMsg_DroppedNavigation)
Are there any race conditions that could lead to dropping a subsequent
navigation instead of the intended one? I don't know how much to worry about
that case.

https://codereview.chromium.org/2735183003/diff/1/content/shell/test_runner/t...
File content/shell/test_runner/test_runner.cc (right):

https://codereview.chromium.org/2735183003/diff/1/content/shell/test_runner/t...
content/shell/test_runner/test_runner.cc:643: LOG(ERROR) << "Notifying done";
?

https://codereview.chromium.org/2735183003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/http/tests/misc/window-open-then-write.html
(right):

https://codereview.chromium.org/2735183003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/misc/window-open-then-write.html:11:
w.document.write('<img src="resources/compass.jpg" onload="if
(window.testRunner) setTimeout(\'testRunner.notifyDone()\', 500);">');
What's the reason for extending this timeout? Is there any way we can avoid
extended it, or even better, get rid of it entirely?

[commit-bot: I haz the power, 2017-03-07 20:47:19]

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

[commit-bot: I haz the power, 2017-03-07 20:47:20]

Dry run: This issue passed the CQ dry run.

[nasko, 2017-03-09 05:19:28]

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

https://codereview.chromium.org/2735183003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_node.cc:429: new
FrameMsg_DroppedNavigation(current_frame_host()->GetRoutingID()));
Hmm, why use the new IPC here? The navigation could have been reset for other
reasons than the one this CL is trying to fix.

https://codereview.chromium.org/2735183003/diff/1/content/common/frame_messag...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2735183003/diff/1/content/common/frame_messag...
content/common/frame_messages.h:892:
IPC_MESSAGE_ROUTED0(FrameMsg_DroppedNavigation)
On 2017/03/07 18:39:08, Nate Chapin wrote:
> Are there any race conditions that could lead to dropping a subsequent
> navigation instead of the intended one? I don't know how much to worry about
> that case.

Would it help if we add the URL of the navigation the renderer wanted to cancel?
This gives us an extra piece of data to ensure we are cancelling the intended
navigation.

[clamy, 2017-03-13 13:55:57]

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

https://codereview.chromium.org/2735183003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_node.cc:429: new
FrameMsg_DroppedNavigation(current_frame_host()->GetRoutingID()));
On 2017/03/09 05:19:28, nasko (slow) wrote:
> Hmm, why use the new IPC here? The navigation could have been reset for other
> reasons than the one this CL is trying to fix.

Yes. But I don't think that the cancellation of one renderer-initiation on the
browser side should lead to all loads stopping in the renderer. In particular we
should not stop an ongoing load, but just inform the renderer that it should not
expect the navigation it sent to the browser to continue. This is what the new
FrameMsg_DroppedNavigation is meant to do (I mean to introduce this IPC as
something more general than this particular issue).

https://codereview.chromium.org/2735183003/diff/1/content/common/frame_messag...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2735183003/diff/1/content/common/frame_messag...
content/common/frame_messages.h:892:
IPC_MESSAGE_ROUTED0(FrameMsg_DroppedNavigation)
On 2017/03/09 05:19:28, nasko (slow) wrote:
> On 2017/03/07 18:39:08, Nate Chapin wrote:
> > Are there any race conditions that could lead to dropping a subsequent
> > navigation instead of the intended one? I don't know how much to worry about
> > that case.
> 
> Would it help if we add the URL of the navigation the renderer wanted to
cancel?
> This gives us an extra piece of data to ensure we are cancelling the intended
> navigation.

We could still have race condition though if we try to request a navigation to
the same url twice. Short of introducing an ID, there's not much to do to
prevent a race from happening.

https://codereview.chromium.org/2735183003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/http/tests/misc/window-open-then-write.html
(right):

https://codereview.chromium.org/2735183003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/misc/window-open-then-write.html:11:
w.document.write('<img src="resources/compass.jpg" onload="if
(window.testRunner) setTimeout(\'testRunner.notifyDone()\', 500);">');
On 2017/03/07 18:39:08, Nate Chapin wrote:
> What's the reason for extending this timeout? Is there any way we can avoid
> extended it, or even better, get rid of it entirely?

Sorry this was for testing. I needed to trigger a race condition, and it worked
a lot better with a long timer. Will remove it in the next patchset.

[clamy, 2017-03-14 13:59:33]

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

[commit-bot: I haz the power, 2017-03-14 13:59:45]

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-03-14 14:01:59]

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

[commit-bot: I haz the power, 2017-03-14 14:02:00]

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...)

[clamy, 2017-03-14 14:05:26]

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

[commit-bot: I haz the power, 2017-03-14 14:05:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[clamy, 2017-03-14 16:40:20]

@nasko: I have uploaded a version with an id to avoid race conditions. Wdyt?

https://codereview.chromium.org/2735183003/diff/1/content/shell/test_runner/t...
File content/shell/test_runner/test_runner.cc (right):

https://codereview.chromium.org/2735183003/diff/1/content/shell/test_runner/t...
content/shell/test_runner/test_runner.cc:643: LOG(ERROR) << "Notifying done";
On 2017/03/07 18:39:08, Nate Chapin wrote:
> ?

Sorry, leftover from debugging. Removed.

[commit-bot: I haz the power, 2017-03-14 16:46:27]

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

[commit-bot: I haz the power, 2017-03-14 16:46:28]

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

[Nate Chapin, 2017-03-14 16:51:02]

blink lgtm

[clamy, 2017-03-16 15:08:35]

@nasko: if you have time, could you take a brief look to see if this approach is
ok? I still have a few issues to fix, and I'd like to know if you agree with the
general direction before doing it.

[nasko, 2017-03-16 21:23:13]

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

[nasko, 2017-03-16 21:23:15]

The main issue is that this reminds me a bit about page_id, which we were very
happy to get rid of :). I think this usage is different and doesn't have the
drawbacks of page_id, but I'd like to make sure that Charlie is also ok with it,
so adding him to the reviewers list.

https://codereview.chromium.org/2735183003/diff/40001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2735183003/diff/40001/content/common/navigati...
content/common/navigation_params.h:183: // -1 is this navigation wasn't
renderer-initiated.
nit: "Value of -1 means this ..."

Why not define a constant in this header, something like kInvalidNavigationId,
so it can be used in the rest of the codebase? It is more meaningful to read the
constant name than -1 and less error prone.

https://codereview.chromium.org/2735183003/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2735183003/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.h:1431: int navigation_id_ = 0;
I'd like to hear what Charlie thinks about this approach. We used to have
page_id, which we removed and this kinda feels like this.

nit: unsigned?

[Charlie Reis, 2017-03-17 00:06:21]

On 2017/03/16 21:23:15, nasko (slow) wrote:
> The main issue is that this reminds me a bit about page_id, which we were very
> happy to get rid of :). I think this usage is different and doesn't have the
> drawbacks of page_id, but I'd like to make sure that Charlie is also ok with
it,
> so adding him to the reviewers list.

Thanks Nasko.  I didn't get a chance to review this today, but I would like to
take a close look.  (Any navigation state in the renderer makes me a bit nervous
since we've seen so many bugs where it can get out of sync, but it's possible
there are reasons it's safe.)

[clamy, 2017-03-17 14:55:13]

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

[commit-bot: I haz the power, 2017-03-17 14:55:32]

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-03-17 14:58:47]

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

[commit-bot: I haz the power, 2017-03-17 14:58:49]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[clamy, 2017-03-17 15:18:38]

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

[commit-bot: I haz the power, 2017-03-17 15:18:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[clamy, 2017-03-17 15:19:12]

Thanks!

https://codereview.chromium.org/2735183003/diff/40001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2735183003/diff/40001/content/common/navigati...
content/common/navigation_params.h:183: // -1 is this navigation wasn't
renderer-initiated.
On 2017/03/16 21:23:14, nasko (slow) wrote:
> nit: "Value of -1 means this ..."
> 
> Why not define a constant in this header, something like kInvalidNavigationId,
> so it can be used in the rest of the codebase? It is more meaningful to read
the
> constant name than -1 and less error prone.

Done.

https://codereview.chromium.org/2735183003/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2735183003/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.h:1431: int navigation_id_ = 0;
On 2017/03/16 21:23:14, nasko (slow) wrote:
> I'd like to hear what Charlie thinks about this approach. We used to have
> page_id, which we removed and this kinda feels like this.
> 
> nit: unsigned?

TBH I'm not super happy about introducing an id, but I don't really see a better
way. The only thing is that it's absolutely not used on the browser side, and is
really meant for book keeping in the renderer process. When we switch to mojo, I
hope that with the services we can have a more reliable way of knowing that the
request we asked the browser to do was cancelled, and this id can go away.

For making the int unsigned, I'm going to transmit it as an int (given that -1
is a possible value), and I'd rather avoid the unsigned/signed conversion if
possible.

[commit-bot: I haz the power, 2017-03-17 16:29:08]

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

[commit-bot: I haz the power, 2017-03-17 16:29:10]

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

[Charlie Reis, 2017-03-20 06:34:44]

Thanks for clarifying.  I'd still really prefer not to add this ID if possible,
since extra state like this tends to be (mis)used in future CLs and makes
navigation logic harder to follow on the whole.

I've left a question below to see if we can get by without it, as well as some
questions about additional races we should think about. 

(This is certainly a tricky area where we've added some asynchronous behavior--
we hit some similar problems last fall with going back/forward in iframes when
we made session history asynchronous.)

https://codereview.chromium.org/2735183003/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2735183003/diff/100001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:1051: DCHECK(frame_tree_node);
nit: This seems unnecessary.  It doesn't seem likely to have a null
FrameTreeNode passed in, and we'd find out on the next line anyway.

https://codereview.chromium.org/2735183003/diff/100001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:1055: if
(!ongoing_navigation_request ||
What happens if we've already sent the CommitNavigation IPC to the renderer by
the time we get this?  (It's naturally a race.)

Will ongoing_navigation_request still exist at that point?  What happens if we
cancel it after sending CommitNavigation and the renderer then sends up a
DidCommitProvisionalLoad?  (Will that be a problem when trying to match up the
commit with a NavigationHandle?)

More importantly, what happens in the renderer process if the CommitNavigation
IPC arrives and the renderer has already done its document.open, expecting the
load to be aborted?  Will that lead to problems, or will it safely overwrite
whatever the document.open started?

https://codereview.chromium.org/2735183003/diff/100001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:1056:
ongoing_navigation_request->browser_initiated()) {
The case we're looking at seems really similar to the logic in
OnBeginNavigation, which also checks for whether there's a user gesture.  Should
a document.write be able to cancel a link click (i.e., a renderer-initiated
navigation with a user gesture), for example?

Just wondering if we should be sharing the same policy (and possibly code)
instead of ending up with another variation here.

https://codereview.chromium.org/2735183003/diff/100001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2735183003/diff/100001/content/common/frame_m...
content/common/frame_messages.h:911:
IPC_MESSAGE_ROUTED1(FrameMsg_DroppedNavigation,
We seem to be a bit inconsistent with calling this dropped, stopped, ignored, or
canceled.  (Also aborted, but maybe that's always the other direction?)  Since
we know it needs to be differentiated from failed, maybe we should pick one term
and use it consistently?

https://codereview.chromium.org/2735183003/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2735183003/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5287: frame_->clientDroppedNavigation();
Why does this code need to wait until we hear back from the browser process? 
Could we just do this at the time of abortClientNavigation, knowing that the
navigation will be canceled (or already has been canceled in favor of a
browser-initiated navigation)?

I'm wondering if we can skip the extra IPC back to the renderer, which seems
like it would eliminate any need for navigation_id_ and cut out some of the
races being introduced here.

[clamy, 2017-03-20 14:56:22]

As I said before, I'm really not happy about adding an id, as there is a
potential for misuse. However, if we want to avoid the races described below in
the comments, I really don't see another way.

https://codereview.chromium.org/2735183003/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2735183003/diff/100001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:1051: DCHECK(frame_tree_node);
On 2017/03/20 06:34:44, Charlie Reis (slow) wrote:
> nit: This seems unnecessary.  It doesn't seem likely to have a null
> FrameTreeNode passed in, and we'd find out on the next line anyway.

Done.

https://codereview.chromium.org/2735183003/diff/100001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:1055: if
(!ongoing_navigation_request ||
On 2017/03/20 06:34:44, Charlie Reis (slow) wrote:
> What happens if we've already sent the CommitNavigation IPC to the renderer by
> the time we get this?  (It's naturally a race.)
> 
> Will ongoing_navigation_request still exist at that point?  What happens if we
> cancel it after sending CommitNavigation and the renderer then sends up a
> DidCommitProvisionalLoad?  (Will that be a problem when trying to match up the
> commit with a NavigationHandle?)
> 
> More importantly, what happens in the renderer process if the CommitNavigation
> IPC arrives and the renderer has already done its document.open, expecting the
> load to be aborted?  Will that lead to problems, or will it safely overwrite
> whatever the document.open started?

The ongoing navigation request will no longer exist after we've sent the
CommitNavigation IPC. There is still a possibility for a race, if the
CommitNavigation and DropNavigation are emitted at the same time and cross each
other. I don't think I can do much to prevent it, and it seems it just leads to
the navigation being committed. I'd like to point out that this kind of race is
already present: if we start a browser-initiated navigation in the current
renderer while it is doing the document.open, the navigation will go on.
Something similar happens in that case.

https://codereview.chromium.org/2735183003/diff/100001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:1056:
ongoing_navigation_request->browser_initiated()) {
On 2017/03/20 06:34:44, Charlie Reis (slow) wrote:
> The case we're looking at seems really similar to the logic in
> OnBeginNavigation, which also checks for whether there's a user gesture. 
Should
> a document.write be able to cancel a link click (i.e., a renderer-initiated
> navigation with a user gesture), for example?
> 
> Just wondering if we should be sharing the same policy (and possibly code)
> instead of ending up with another variation here.

The existing behavior is that document.write will cancel a link click. I'm not
fully following the current behavior though, as currently we would cancel any
ongoing navigation in the frame of the renderer process, which includes
browser-initiated navigations. I thought restricting to only renderer-initiated
navigations made more sense (and emulating the current behavior in Plznavigate
just looks weird). If you think we should also restrict to non-user initiated
navigations, this can be changed.

https://codereview.chromium.org/2735183003/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2735183003/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5287: frame_->clientDroppedNavigation();
On 2017/03/20 06:34:44, Charlie Reis (slow) wrote:
> Why does this code need to wait until we hear back from the browser process? 
> Could we just do this at the time of abortClientNavigation, knowing that the
> navigation will be canceled (or already has been canceled in favor of a
> browser-initiated navigation)?
> 
> I'm wondering if we can skip the extra IPC back to the renderer, which seems
> like it would eliminate any need for navigation_id_ and cut out some of the
> races being introduced here.

The patch is fixing two issues: the first one is cancelling any ongoing
navigation when we do a document.open. This is what the IPC from the renderer to
the browser is doing. This exposed another more general issue: what happens when
the browser stops processing a navigation that the renderer sent, and that the
renderer is waiting for? This IPC here tackles this second problem, and is not
limited to the document.open case.

When the browser decided not to proceed with a renderer-initiated navigation, we
used to send a Stop IPC. This had the side effect of cancelling any ongoing
load. In particular, if the renderer was not done with the load of the current
page, the load would stop. Not proceeding with a navigation should not cause the
current load to stop, so I had to introduce this new IPC, DroppedNavigation.
However, it was pointed that this could lead to potential races, if the renderer
had issued several navigations.

Let's imagine the following scenario:
- the renderer wants to navigate to foo.com
- the browser decides to drop the navigation
- the renderer wants to navigate to bar.com
- the renderer receives the DroppedNavigation IPC from the navigation to foo.com
and thinks the navigation to bar.com is no longer ongoing
- the browser did in fact go on with the navigation to bar.com

This may lead to bad behavior, such as the load event being fired in the parent
frame when it should not.

In order to avoid this kind of race, Nasko proposed differentiating based on the
urls. However, if we take the example above and replace the second navigation to
bar.com with one to foo.com, we still have the same issue. This is why I
proposed introducing an id, since with the id the race condition goes away.

It may be easier if I split this patch in two: one part to fix the issue with
cancelled renderer-initiated navigations, and one part to fix the issue with
document.open.

[Charlie Reis, 2017-03-20 23:02:50]

Thanks for the explanations!  I do appreciate your goals, and I'm just hoping to
have fewer phases of back-and-forth IPCs to reason about, if we can get by
without them.  Some replies below.

https://codereview.chromium.org/2735183003/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2735183003/diff/100001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:1055: if
(!ongoing_navigation_request ||
On 2017/03/20 14:56:22, clamy wrote:
> On 2017/03/20 06:34:44, Charlie Reis (slow) wrote:
> > What happens if we've already sent the CommitNavigation IPC to the renderer
by
> > the time we get this?  (It's naturally a race.)
> > 
> > Will ongoing_navigation_request still exist at that point?  What happens if
we
> > cancel it after sending CommitNavigation and the renderer then sends up a
> > DidCommitProvisionalLoad?  (Will that be a problem when trying to match up
the
> > commit with a NavigationHandle?)
> > 
> > More importantly, what happens in the renderer process if the
CommitNavigation
> > IPC arrives and the renderer has already done its document.open, expecting
the
> > load to be aborted?  Will that lead to problems, or will it safely overwrite
> > whatever the document.open started?
> 
> The ongoing navigation request will no longer exist after we've sent the
> CommitNavigation IPC.

Great, that's a bit simpler.

> There is still a possibility for a race, if the
> CommitNavigation and DropNavigation are emitted at the same time and cross
each
> other. I don't think I can do much to prevent it, and it seems it just leads
to
> the navigation being committed. I'd like to point out that this kind of race
is
> already present: if we start a browser-initiated navigation in the current
> renderer while it is doing the document.open, the navigation will go on.
> Something similar happens in that case.

Yes, I think it's fine if the navigation gets committed over whatever
document.open created.  (The same thing would also happen if document.open were
on a timer and the new commit just happened before it fired in the first place.)

I was concerned that something in the renderer might break if we've already
discarded renderer-side state, but it sounds like it works ok.

https://codereview.chromium.org/2735183003/diff/100001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:1056:
ongoing_navigation_request->browser_initiated()) {
On 2017/03/20 14:56:22, clamy wrote:
> On 2017/03/20 06:34:44, Charlie Reis (slow) wrote:
> > The case we're looking at seems really similar to the logic in
> > OnBeginNavigation, which also checks for whether there's a user gesture. 
> Should
> > a document.write be able to cancel a link click (i.e., a renderer-initiated
> > navigation with a user gesture), for example?
> > 
> > Just wondering if we should be sharing the same policy (and possibly code)
> > instead of ending up with another variation here.
> 
> The existing behavior is that document.write will cancel a link click. I'm not
> fully following the current behavior though, as currently we would cancel any
> ongoing navigation in the frame of the renderer process, which includes
> browser-initiated navigations. I thought restricting to only
renderer-initiated
> navigations made more sense (and emulating the current behavior in Plznavigate
> just looks weird). If you think we should also restrict to non-user initiated
> navigations, this can be changed.

It only bothered me in the number of different policies we have for different
cases.  Changing it to ignore user-initiated navigations does pose some
compatibility risk (e.g., if a page depended on doing a document.write in
response to link clicks (yuck!)), so it's probably safer to leave it as you have
it.

(I don't see as big a regression risk with excluding browser-initiated
navigations from this.)

https://codereview.chromium.org/2735183003/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2735183003/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5287: frame_->clientDroppedNavigation();
On 2017/03/20 14:56:22, clamy wrote:
> On 2017/03/20 06:34:44, Charlie Reis (slow) wrote:
> > Why does this code need to wait until we hear back from the browser process?

> > Could we just do this at the time of abortClientNavigation, knowing that the
> > navigation will be canceled (or already has been canceled in favor of a
> > browser-initiated navigation)?
> > 
> > I'm wondering if we can skip the extra IPC back to the renderer, which seems
> > like it would eliminate any need for navigation_id_ and cut out some of the
> > races being introduced here.
> 
> The patch is fixing two issues: the first one is cancelling any ongoing
> navigation when we do a document.open. This is what the IPC from the renderer
to
> the browser is doing. This exposed another more general issue: what happens
when
> the browser stops processing a navigation that the renderer sent, and that the
> renderer is waiting for? This IPC here tackles this second problem, and is not
> limited to the document.open case.

Right.

> 
> When the browser decided not to proceed with a renderer-initiated navigation,
we
> used to send a Stop IPC. This had the side effect of cancelling any ongoing
> load. In particular, if the renderer was not done with the load of the current
> page, the load would stop. Not proceeding with a navigation should not cause
the
> current load to stop, so I had to introduce this new IPC, DroppedNavigation.

Yes, I do like that you're trying to add something here which avoids stopping
all loaders.

> However, it was pointed that this could lead to potential races, if the
renderer
> had issued several navigations.
> 
> Let's imagine the following scenario:
> - the renderer wants to navigate to http://foo.com
> - the browser decides to drop the navigation
> - the renderer wants to navigate to http://bar.com
> - the renderer receives the DroppedNavigation IPC from the navigation to
http://foo.com
> and thinks the navigation to http://bar.com is no longer ongoing
> - the browser did in fact go on with the navigation to http://bar.com
> 
> This may lead to bad behavior, such as the load event being fired in the
parent
> frame when it should not.

In my suggestion, I was hoping that we could skip the DroppedNavigation IPC in
the document.open case because the renderer could assume the navigation would be
canceled and preemptively clear its navigation state.  That way there's no race
with the renderer subsequently starting a new navigation, potentially making
things much simpler to reason about.

I suppose that suggestion won't help in the other cases that the browser
independently decides to drop the navigation, though.

> 
> In order to avoid this kind of race, Nasko proposed differentiating based on
the
> urls. However, if we take the example above and replace the second navigation
to
> http://bar.com with one to http://foo.com, we still have the same issue. This
is why I
> proposed introducing an id, since with the id the race condition goes away.
> 
> It may be easier if I split this patch in two: one part to fix the issue with
> cancelled renderer-initiated navigations, and one part to fix the issue with
> document.open.

How bad is the issue with the FrameMsg_Stop IPC today?  While I do like the idea
of fixing that, I'm not sure it it's worth adding a navigation ID for.

For your "split the patch" idea, what if we landed the document.open fix first,
doing the preemptive cancel of the renderer's navigation state rather than
having the browser send back an IPC?  Then we could separately decide whether
the issues with the Stop IPC are big enough to justify the ID.  (And you may be
able to convince me that it's worthwhile!  But at least that way it's
independent of the document.open fix.)

[clamy, 2017-03-22 14:26:45]

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

[commit-bot: I haz the power, 2017-03-22 14:27:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[clamy, 2017-03-22 14:28:12]

I've split the patchset, which now contains only the fix for the document.open
case. Note that I had to add a paremeter to the function on the cancellation
path to ensure that we don't send the stop IPC in that case.

[commit-bot: I haz the power, 2017-03-22 15:11:00]

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

[commit-bot: I haz the power, 2017-03-22 15:11:01]

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

[Charlie Reis, 2017-03-22 23:54:41]

Thanks, LGTM now that we don't have the extra reply coming back.  We can discuss
the idea of the navigation ID for the stop cases on the other CL.

On 2017/03/22 14:28:12, clamy wrote:
> I've split the patchset, which now contains only the fix for the document.open
> case. Note that I had to add a paremeter to the function on the cancellation
> path to ensure that we don't send the stop IPC in that case.

Yeah, that's a bit unfortunate given how many places it affected, but I don't
see a good way to avoid it.  (The default behavior is to send it, so it's not
like splitting the IPC out of ResetNavigationRequest into a different method
would help.)

https://codereview.chromium.org/2735183003/diff/160001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/2735183003/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:424: // informed that the
navigation stopped.
nit: Update comment, maybe giving an example of why inform_renderer would be
false.

https://codereview.chromium.org/2735183003/diff/160001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/2735183003/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:264: // If |keep_state| is false,
teh request is renderer-initiated and
nit: s/teh/the/

nit: ..."renderer-initiated. If"
(That is, I think it might be clearer to split the sentences.  Or are the two
parameters somehow dependent?)

https://codereview.chromium.org/2735183003/diff/160001/content/browser/frame_...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/2735183003/diff/160001/content/browser/frame_...
content/browser/frame_host/navigator.h:166: // the renderer process to inform it
that the navigation it request was
nit: requested

[clamy, 2017-03-23 13:17:52]

Thanks!

https://codereview.chromium.org/2735183003/diff/160001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/2735183003/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:424: // informed that the
navigation stopped.
On 2017/03/22 23:54:41, Charlie Reis (OOO soon) wrote:
> nit: Update comment, maybe giving an example of why inform_renderer would be
> false.

Done.

https://codereview.chromium.org/2735183003/diff/160001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/2735183003/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:264: // If |keep_state| is false,
teh request is renderer-initiated and
On 2017/03/22 23:54:41, Charlie Reis (OOO soon) wrote:
> nit: s/teh/the/

Done

> 
> nit: ..."renderer-initiated. If"
> (That is, I think it might be clearer to split the sentences.  Or are the two
> parameters somehow dependent?)
Yes they all need to be present. I've replaced the first comma by an "and" to
make it clearer.

https://codereview.chromium.org/2735183003/diff/160001/content/browser/frame_...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/2735183003/diff/160001/content/browser/frame_...
content/browser/frame_host/navigator.h:166: // the renderer process to inform it
that the navigation it request was
On 2017/03/22 23:54:41, Charlie Reis (OOO soon) wrote:
> nit: requested

Done.

[clamy, 2017-03-23 13:18:26]

The CQ bit was checked by clamy@chromium.org

[clamy, 2017-03-23 13:18:26]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2735183003/#ps200001
(title: "Addressed commemts")

[commit-bot: I haz the power, 2017-03-23 13:18:43]

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-03-23 13:25:24]

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

[commit-bot: I haz the power, 2017-03-23 13:25:26]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[nasko, 2017-03-23 13:27:04]

IPC LGTM

[nasko, 2017-03-23 13:27:16]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2017-03-23 13:27:41]

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-03-23 14:45:53]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1490275636749460,
"parent_rev": "bc8f3c98286adb9520d7d856b20465a49402eef1", "commit_rev":
"a86695ba7e713a92a517631681c549f74871a747"}

[commit-bot: I haz the power, 2017-03-23 14:46:34]

Description was changed from

==========
PlzNavigate: stop navigations when opening a document for write

This CL ensures that we stop renderer-initiated navigations when
attempting to do a document write. In order to do this in PlzNavigate,
we introduce a new IPC where the RenderFrame asks the browser process to
drop its renderer-initiated request. The CL also fixes an issue with the
cancellation of renderer-initiated navigations in PlzNavigate. We issued
a Stop IPC that would stop all loaders. Instead, we now have a new IPC
that will only cancel the dummy provisional DocumentLoader that keeps
track of navigations handled by the browser process.

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

to

==========
PlzNavigate: stop navigations when opening a document for write

This CL ensures that we stop renderer-initiated navigations when
attempting to do a document write. In order to do this in PlzNavigate,
we introduce a new IPC where the RenderFrame asks the browser process to
drop its renderer-initiated request. The CL also fixes an issue with the
cancellation of renderer-initiated navigations in PlzNavigate. We issued
a Stop IPC that would stop all loaders. Instead, we now have a new IPC
that will only cancel the dummy provisional DocumentLoader that keeps
track of navigations handled by the browser process.

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

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

[commit-bot: I haz the power, 2017-03-23 14:46:43]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/a86695ba7e713a92a517631681c5...