OOPIF: History navigations for new child frames.

OOPIF: History navigations for new child frames.

When a subframe is created during a history navigation, we now make an
OpenURL call to the browser process that looks up which
FrameNavigationEntry matches the new frame (based on Blink's unique name
approach).  If no matching entry is found, we will fall back to the
original URL for the frame.

BUG=502317
TEST=See bug for repro steps.

Committed: https://crrev.com/e18ce07ccb40aa6097764dc73ca0ebc3cf624c03
Cr-Commit-Position: refs/heads/master@{#362579}

[Charlie Reis, 2015-11-23 22:56:35]

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

[Charlie Reis, 2015-11-23 22:56:36]

Nasko, can you take a look?

The main TODOs for followup CLs are handling the fallback case (if no FNE is
found for the new subframe) and getting the history load type correct.  Those
seemed like good incremental steps to take after this core part lands.

Once this is in, we can fix session restore as well.

https://codereview.chromium.org/1422333009/diff/100001/testing/buildbot/chrom...
File testing/buildbot/chromium.fyi.json (right):

https://codereview.chromium.org/1422333009/diff/100001/testing/buildbot/chrom...
testing/buildbot/chromium.fyi.json:5452: "Site Isolation Linux": {
We're now "working" enough that SessionRestoreTest.RestoreWebUISettings has
started failing, which is expected until
https://codereview.chromium.org/1415603018/ is finished.

Conversely, the PreventSpoofFromSubframeAndReplace test is passing again, now
that we can recreate FTNs and navigate them.

https://codereview.chromium.org/1422333009/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1422333009/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2007:
frame->tree().setName(name);
This will go away when Daniel fixes unique names with FrameTreeHandle (which I
expect will land first).

[Charlie Reis, 2015-11-24 18:53:41]

creis@chromium.org changed reviewers:
+ avi@chromium.org, dcheng@chromium.org
- nasko@chromium.org

[Charlie Reis, 2015-11-24 18:53:42]

Sounds like Nasko is busy.  Avi, can you take a look?  (I left some intro
comments in the first message.)

Also adding Daniel for the Blink parts.  (Not sure if there's a better way to
update the ProgressTracker, but that's the right point in time.)

[Avi (use Gerrit), 2015-11-24 22:09:47]

lgtm

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

https://codereview.chromium.org/1422333009/diff/100001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:404:
controller_->GetEntryWithUniqueID(render_frame_host->nav_entry_id());
Boy, this "last commit" nav entry id is coming in handy...

So the way I'm understanding it, we get a request to navigate a new child frame,
so we're assuming that we need to work on the last commit. Can that id ever be
stale?

https://codereview.chromium.org/1422333009/diff/100001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:413: // Update the FNE's FTN ID,
since the frame will have a new one.
I didn't understand this comment the first time I read it. Perhaps:

// Update the FNE's FTN ID (which is currently the ID of the old FTN that no
longer exists) to be the ID of the newly-created frame.

... or something like that.

[Charlie Reis, 2015-11-24 23:05:54]

Thanks!

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

https://codereview.chromium.org/1422333009/diff/100001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:404:
controller_->GetEntryWithUniqueID(render_frame_host->nav_entry_id());
On 2015/11/24 22:09:47, Avi wrote:
> Boy, this "last commit" nav entry id is coming in handy...
> 
> So the way I'm understanding it, we get a request to navigate a new child
frame,
> so we're assuming that we need to work on the last commit.

Right.  The renderer sends this IPC when it knows a back/forward is in progress
(i.e., we haven't gotten to DidStopLoading yet during a history navigation in
its parent).  That means we should use the NavigationEntry that the newly
created RenderFrame thinks it's on.  (And that nav_entry_id is inherited from
its parent, as of https://codereview.chromium.org/1437643004.)

> Can that id ever be stale?

Good question.  I think maybe yes, if the RenderFrame is about to be destroyed. 
In that case, we'll hit the early return below and it should be fine.  For
example:
1) Visit page A, then page B (with iframe B1), then C, each in a different
process.
2) Go back from C to B.
3) Quickly go back from B to A, then commit a new page D (pruning B and C from
history).
4) In the meantime, B creates its child frame and we get NavigateNewChildFrame
after D has committed and the NavEntry is gone.

Then again, that might not be possible in practice-- I think we'll delete the
FTN and RFH for B's iframe when going back to A in step 3, so the
NavigateNewChildFrame IPC won't be handled anywhere.

https://codereview.chromium.org/1422333009/diff/100001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:413: // Update the FNE's FTN ID,
since the frame will have a new one.
On 2015/11/24 22:09:47, Avi wrote:
> I didn't understand this comment the first time I read it. Perhaps:
> 
> // Update the FNE's FTN ID (which is currently the ID of the old FTN that no
> longer exists) to be the ID of the newly-created frame.
> 
> ... or something like that.

Done.

[Avi (use Gerrit), 2015-11-25 03:39:14]

SLGTM

[dcheng, 2015-11-25 06:18:55]

https://codereview.chromium.org/1422333009/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1422333009/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1770:
child->loader().progress().progressStarted();
I feel a bit sad about this: we added NavigationPolicyHandledByClient to try to
handle this use case. I guess there's no way to use that here?

[Charlie Reis, 2015-11-25 18:41:16]

https://codereview.chromium.org/1422333009/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1422333009/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1770:
child->loader().progress().progressStarted();
On 2015/11/25 06:18:55, dcheng wrote:
> I feel a bit sad about this: we added NavigationPolicyHandledByClient to try
to
> handle this use case. I guess there's no way to use that here?

Hmm.  That's in shouldContinueForNavigationPolicy, after
decidePolicyForNavigation.  We don't get that far because we don't know what the
destination item is.

There's a chance we could restructure this to make the fallback case easier to
handle as well, but I'm less sure about the implications.  Suppose (in OOPIF
modes) we always load |request| here instead of looking for a history item for
the new child frame.  When we get to decidePolicyForNavigation, we punt it to
the browser process via OpenURL (hitting the NavigationPolicyHandledByClient
case you mention in FrameLoader).  The browser process would then look for a
history item to use instead.  If it doesn't find one, it can navigate to the
fallback URL using the rest of the OpenURL params.

There's a risk that starting to navigate to |request| will affect some state in
Blink, even though we aren't actually going to go it.  (For example, I was
worried that about:blank might just commit directly without going through
decidePolicyForNavigation, but thankfully that's not the case.)

Do you think we should tackle this now or as a separate attempt to fix the
fallback case?  (I'm noticing that we already call
ProgressTracker::progressStarted from
ScriptController::executeScriptIfJavaScriptURL, so it's not entirely hidden
within Frameloader.)

[Charlie Reis, 2015-11-30 23:43:19]

Patchset #9 (id:160001) has been deleted

[Charlie Reis, 2015-12-01 00:12:11]

Description was changed from

==========
OOPIF: History navigations for new child frames.

This adds an IPC to the browser process when a subframe is created during
a history navigation.  The browser looks up which FrameNavigationEntry
matches the new frame (based on Blink's unique name approach) and tells
the frame to navigate.

BUG=502317
TEST=See bug for repro steps.
==========

to

==========
OOPIF: History navigations for new child frames.

When a subframe is created during a history navigation, we now make an
OpenURL call to the browser process that looks up which
FrameNavigationEntry matches the new frame (based on Blink's unique name
approach).  If no matching entry is found, we will fall back to the
original URL for the frame.

BUG=502317
TEST=See bug for repro steps.
==========

[Charlie Reis, 2015-12-01 00:17:22]

Ok, I've restructured this CL to go through decidePolicyForNavigation instead of
sending a new IPC.  We can return NavigationPolicyHandledByClient from
decidePolicyForNavigation to get the correct ProgressTracker behavior.

This CL is marginally bigger as a result, but on the plus side the fallback case
works now!  (I even added a test for it.)  The key diff is between patch sets 8
and 9; patch 10 is just a rebase.

Daniel, how does the rest of the Blink change look?  If you're happy (and if the
try jobs pass) I'll have Avi take another look at the content/ side.

[Avi (use Gerrit), 2015-12-01 15:04:39]

I'm still happy with the CL.

[dcheng, 2015-12-01 19:47:53]

LGTM

https://codereview.chromium.org/1422333009/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/1422333009/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:586: bool
isHistoryNavigationInNewChildFrame = m_webFrame->parent()
Do you mind adding a comment that describes the logic behind these various
checks? It's not completely clear to me how these various checks relate to this
being a history navigation in a new child frame.

[Charlie Reis, 2015-12-01 22:11:51]

https://codereview.chromium.org/1422333009/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/1422333009/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:586: bool
isHistoryNavigationInNewChildFrame = m_webFrame->parent()
On 2015/12/01 19:47:53, dcheng wrote:
> Do you mind adding a comment that describes the logic behind these various
> checks? It's not completely clear to me how these various checks relate to
this
> being a history navigation in a new child frame.

Done.  I also noticed that I should have been using m_webFrame->parent() on the
last line to be consistent with WebLocalFrameImpl::createChildFrame.  Fixed.

[Charlie Reis, 2015-12-01 22:11:59]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2015-12-01 22:12:00]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1422333009/#ps240001
(title: "Rebase")

[commit-bot: I haz the power, 2015-12-01 22:16:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1422333009/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1422333009/240001

[Charlie Reis, 2015-12-01 22:16:33]

The CQ bit was unchecked by creis@chromium.org

[Charlie Reis, 2015-12-01 22:19:52]

Improved comment in patch 13, per offline request.

[Charlie Reis, 2015-12-01 22:19:58]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2015-12-01 22:19:59]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1422333009/#ps260001
(title: "Improve comment.")

[commit-bot: I haz the power, 2015-12-01 22:24:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1422333009/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1422333009/260001

[commit-bot: I haz the power, 2015-12-02 00:08:18]

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

[commit-bot: I haz the power, 2015-12-02 00:08:19]

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

[Charlie Reis, 2015-12-02 00:10:20]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2015-12-02 00:12:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1422333009/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1422333009/260001

[commit-bot: I haz the power, 2015-12-02 02:00:11]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2015-12-02 02:01:00]

Description was changed from

==========
OOPIF: History navigations for new child frames.

When a subframe is created during a history navigation, we now make an
OpenURL call to the browser process that looks up which
FrameNavigationEntry matches the new frame (based on Blink's unique name
approach).  If no matching entry is found, we will fall back to the
original URL for the frame.

BUG=502317
TEST=See bug for repro steps.
==========

to

==========
OOPIF: History navigations for new child frames.

When a subframe is created during a history navigation, we now make an
OpenURL call to the browser process that looks up which
FrameNavigationEntry matches the new frame (based on Blink's unique name
approach).  If no matching entry is found, we will fall back to the
original URL for the frame.

BUG=502317
TEST=See bug for repro steps.

Committed: https://crrev.com/e18ce07ccb40aa6097764dc73ca0ebc3cf624c03
Cr-Commit-Position: refs/heads/master@{#362579}
==========

[commit-bot: I haz the power, 2015-12-02 02:01:00]

Patchset 13 (id:??) landed as
https://crrev.com/e18ce07ccb40aa6097764dc73ca0ebc3cf624c03
Cr-Commit-Position: refs/heads/master@{#362579}