Don't load subframe history items if a client redirect occurs during load.

Don't load subframe history items if a client redirect occurs during load.

Javascript navigations during load should be able to interrupt loading
the history item.  This change updates the UseSubframeNavigationEntries
path to handle this similarly to the default navigation path.

(In both cases, there's some raciness involved, but it tends to allow
the JS navigation to win.)

BUG=585194
TEST=Visit http://csreis.github.io/tests/dynamic-iframe.html,
     navigate away and come back.  Should get a different number.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/935b99db3e8295407308c052b7835eb18e2a43d4
Cr-Commit-Position: refs/heads/master@{#409811}

[Charlie Reis, 2016-08-04 00:11:28]

Description was changed from

==========
Don't load subframe history items if a client redirect occurs during load.

Javascript navigations during load should be able to interrupt loading
the history item.  This change updates the UseSubframeNavigationEntries
path to handle this similarly to the default navigation path.

(In both cases, there's some raciness involved, but it tends to allow
the JS navigation to win.)

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

to

==========
Don't load subframe history items if a client redirect occurs during load.

Javascript navigations during load should be able to interrupt loading
the history item.  This change updates the UseSubframeNavigationEntries
path to handle this similarly to the default navigation path.

(In both cases, there's some raciness involved, but it tends to allow
the JS navigation to win.)

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

[Charlie Reis, 2016-08-04 00:11:40]

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

[commit-bot: I haz the power, 2016-08-04 00:12:02]

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, 2016-08-04 00:21:23]

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

[commit-bot: I haz the power, 2016-08-04 00:21:24]

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

[Charlie Reis, 2016-08-04 05:44:05]

Description was changed from

==========
Don't load subframe history items if a client redirect occurs during load.

Javascript navigations during load should be able to interrupt loading
the history item.  This change updates the UseSubframeNavigationEntries
path to handle this similarly to the default navigation path.

(In both cases, there's some raciness involved, but it tends to allow
the JS navigation to win.)

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

to

==========
Don't load subframe history items if a client redirect occurs during load.

Javascript navigations during load should be able to interrupt loading
the history item.  This change updates the UseSubframeNavigationEntries
path to handle this similarly to the default navigation path.

(In both cases, there's some raciness involved, but it tends to allow
the JS navigation to win.)

BUG=585194
TEST=Visit http://csreis.github.io/tests/dynamic-iframe.html,
     navigate away and come back.  Should get a different number.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-08-04 05:48:19]

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

[commit-bot: I haz the power, 2016-08-04 05:48:36]

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, 2016-08-04 07:03:01]

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

[commit-bot: I haz the power, 2016-08-04 07:03:02]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-08-04 14:48:45]

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

[Charlie Reis, 2016-08-04 14:48:46]

Nate, what do you think of the fix in render_frame_impl.cc?  I think it should
do more or less the same thing as your NavigationScheduler change in
https://codereview.chromium.org/296533004, by attempting to cancel/ignore the
initial history navigation if we get a later JS navigation during load.

Nasko, can you review the rest?  I'll see if I can put together an OOPIF test,
but maybe I'll do that in a second CL, since the existing layout test covers the
same-process case that we need for the new navigation path.

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

https://codereview.chromium.org/2208933002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4963: if (!info.isClientRedirect) {
Before, we were punting both the original iframe load and the subsequent JS
navigation (which is treated as a client redirect) to the browser via OpenURL in
the new navigation path.  Now we avoid punting the client redirect and let it
proceed, and we'll try to ignore the history item when we get it in
NavigateInternal below.

https://codereview.chromium.org/2208933002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4974: Send(new
FrameHostMsg_CancelInitialHistoryLoad(routing_id_));
This is only necessary if the history item was being sent to a different
process.  We'll try to cancel it before it commits (if possible).

[nasko, 2016-08-04 16:00:38]

content/ LGTM

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

https://codereview.chromium.org/2208933002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4962: // JavaScript on the page is trying
to interrupt the history navigation.
nit: s/on the page/in a different frame/ to be more precise that it isn't this
specific frame and in general I'd like us to avoid "page" as we are now in frame
granularity almost everywhere.

[Nate Chapin, 2016-08-04 16:40:42]

Yeah, that looks reasonable. LGTM

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

https://codereview.chromium.org/2208933002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4963: if (!info.isClientRedirect) {
On 2016/08/04 14:48:46, Charlie Reis (OOO til 8-8) wrote:
> Before, we were punting both the original iframe load and the subsequent JS
> navigation (which is treated as a client redirect) to the browser via OpenURL
in
> the new navigation path.  Now we avoid punting the client redirect and let it
> proceed, and we'll try to ignore the history item when we get it in
> NavigateInternal below.

It surprised me that info.isHistoryNavigationInNewChildFrame is true for the
client redirect. I see why, but that name fooled me :/

[Charlie Reis, 2016-08-04 16:46:27]

Thanks!

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

https://codereview.chromium.org/2208933002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4962: // JavaScript on the page is trying
to interrupt the history navigation.
On 2016/08/04 16:00:38, nasko (Out until 8-8) wrote:
> nit: s/on the page/in a different frame/ to be more precise that it isn't this
> specific frame and in general I'd like us to avoid "page" as we are now in
frame
> granularity almost everywhere.

Will fix in the followup CL with OOPIF test, as you suggested to me offline.

https://codereview.chromium.org/2208933002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4963: if (!info.isClientRedirect) {
On 2016/08/04 16:40:42, Nate Chapin wrote:
> On 2016/08/04 14:48:46, Charlie Reis (OOO til 8-8) wrote:
> > Before, we were punting both the original iframe load and the subsequent JS
> > navigation (which is treated as a client redirect) to the browser via
OpenURL
> in
> > the new navigation path.  Now we avoid punting the client redirect and let
it
> > proceed, and we'll try to ignore the history item when we get it in
> > NavigateInternal below.
> 
> It surprised me that info.isHistoryNavigationInNewChildFrame is true for the
> client redirect. I see why, but that name fooled me :/

Yeah, I was surprised at first, too.  I suppose we could use the
isClientRedirect hint in FrameLoaderClientImpl::decidePolicyForNavigation to
avoid setting that to true, but then we wouldn't know to send the cancel IPC
below.

I'll think about ways to clarify that for the followup.

[Charlie Reis, 2016-08-04 16:46:39]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2016-08-04 16:46:51]

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, 2016-08-04 16:50:47]

Description was changed from

==========
Don't load subframe history items if a client redirect occurs during load.

Javascript navigations during load should be able to interrupt loading
the history item.  This change updates the UseSubframeNavigationEntries
path to handle this similarly to the default navigation path.

(In both cases, there's some raciness involved, but it tends to allow
the JS navigation to win.)

BUG=585194
TEST=Visit http://csreis.github.io/tests/dynamic-iframe.html,
     navigate away and come back.  Should get a different number.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't load subframe history items if a client redirect occurs during load.

Javascript navigations during load should be able to interrupt loading
the history item.  This change updates the UseSubframeNavigationEntries
path to handle this similarly to the default navigation path.

(In both cases, there's some raciness involved, but it tends to allow
the JS navigation to win.)

BUG=585194
TEST=Visit http://csreis.github.io/tests/dynamic-iframe.html,
     navigate away and come back.  Should get a different number.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-08-04 16:50:48]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-04 16:54:19]

Description was changed from

==========
Don't load subframe history items if a client redirect occurs during load.

Javascript navigations during load should be able to interrupt loading
the history item.  This change updates the UseSubframeNavigationEntries
path to handle this similarly to the default navigation path.

(In both cases, there's some raciness involved, but it tends to allow
the JS navigation to win.)

BUG=585194
TEST=Visit http://csreis.github.io/tests/dynamic-iframe.html,
     navigate away and come back.  Should get a different number.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don't load subframe history items if a client redirect occurs during load.

Javascript navigations during load should be able to interrupt loading
the history item.  This change updates the UseSubframeNavigationEntries
path to handle this similarly to the default navigation path.

(In both cases, there's some raciness involved, but it tends to allow
the JS navigation to win.)

BUG=585194
TEST=Visit http://csreis.github.io/tests/dynamic-iframe.html,
     navigate away and come back.  Should get a different number.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/935b99db3e8295407308c052b7835eb18e2a43d4
Cr-Commit-Position: refs/heads/master@{#409811}
==========

[commit-bot: I haz the power, 2016-08-04 16:54:20]

Patchset 4 (id:??) landed as
https://crrev.com/935b99db3e8295407308c052b7835eb18e2a43d4
Cr-Commit-Position: refs/heads/master@{#409811}