PlzNavigate: renderer-initiated navigations create a pending NavigationEntry

PlzNavigate: renderer-initiated navigations create a pending NavigationEntry

This CL creates a pending NavigationEntry in the NavigationController at the
beginning of a renderer-initiated navigation when browser-side navigation is
enabled (this is done currently upon reception of the DidStartProvisionalLoad
IPC). In particular, this allows to display a url in a new window created by
clicking on a targeted-link
(RenderFrameHostManagerTest.ShowLoadingURLUntilSpoof test).

BUG=475027
Committed: https://crrev.com/fa0939fd8a3f28642852025737ccb807afd3d204
Cr-Commit-Position: refs/heads/master@{#332798}

[clamy, 2015-06-02 16:47:18]

clamy@chromium.org changed reviewers:
+ avi@chromium.org, carlosk@chromium.org, creis@chromium.org,
fdegans@chromium.org

[clamy, 2015-06-02 16:47:38]

@avi, creis: PTAL
@carlosk, fdegans: FYI

[Charlie Reis, 2015-06-02 17:36:18]

Ah, https://crbug.com/9682.  Yes, this seems reasonable to share.

A few nits and questions below.

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

https://codereview.chromium.org/1167693007/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:642: if
(frame_tree_node->IsMainFrame()) {
Do we not get here for error pages?

https://codereview.chromium.org/1167693007/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:644: // RenderFrameHost will not be
swapped. Therefore it is safe to call
This first sentence isn't true in general.  Renderer-initiated main frame
navigations can be cross-process, for things like links to the Chrome Web Store
or rel=noreferrer target=_blank links.  That happens via
DecidePolicyForNavigation and RequestOpenURL.

Do those cases not get here?  (If not, let's clarify the comment.)

https://codereview.chromium.org/1167693007/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:648: common_params.url,
I just noticed that we're not validating the URL with FilterURL.  That's really
important for security, so that renderers can't lie about which URLs they're
allowed to navigate to.

We should add a block at the top of OnBeginNavigation that filters the URLs in
the params structs.  We'll probably need a separate mutable copy of the structs,
so that we can pass them to NavigationRequest::CreateRendererInitiated as well.

(It's fine to do this in a separate CL.)

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

https://codereview.chromium.org/1167693007/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.h:128: // Called when a navigation
started in a main frame, to update the pending
nit: has started

https://codereview.chromium.org/1167693007/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.h:131: void
MainFrameNavigationStarted(const GURL& url,
DidStartMainFrameNavigation

[clamy, 2015-06-03 09:11:11]

Thanks!

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

https://codereview.chromium.org/1167693007/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:642: if
(frame_tree_node->IsMainFrame()) {
On 2015/06/02 17:36:18, Charlie Reis wrote:
> Do we not get here for error pages?

No this is the beginning of a normal navigation.

https://codereview.chromium.org/1167693007/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:644: // RenderFrameHost will not be
swapped. Therefore it is safe to call
On 2015/06/02 17:36:18, Charlie Reis wrote:
> This first sentence isn't true in general.  Renderer-initiated main frame
> navigations can be cross-process, for things like links to the Chrome Web
Store
> or rel=noreferrer target=_blank links.  That happens via
> DecidePolicyForNavigation and RequestOpenURL.
> 
> Do those cases not get here?  (If not, let's clarify the comment.)

No they don't, edited the comment to precise that.

https://codereview.chromium.org/1167693007/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:648: common_params.url,
On 2015/06/02 17:36:18, Charlie Reis wrote:
> I just noticed that we're not validating the URL with FilterURL.  That's
really
> important for security, so that renderers can't lie about which URLs they're
> allowed to navigate to.
> 
> We should add a block at the top of OnBeginNavigation that filters the URLs in
> the params structs.  We'll probably need a separate mutable copy of the
structs,
> so that we can pass them to NavigationRequest::CreateRendererInitiated as
well.
> 
> (It's fine to do this in a separate CL.)

Acknowledged. Added a TODO at the beginning of the function.

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

https://codereview.chromium.org/1167693007/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.h:128: // Called when a navigation
started in a main frame, to update the pending
On 2015/06/02 17:36:18, Charlie Reis wrote:
> nit: has started

Done.

https://codereview.chromium.org/1167693007/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.h:131: void
MainFrameNavigationStarted(const GURL& url,
On 2015/06/02 17:36:18, Charlie Reis wrote:
> DidStartMainFrameNavigation

Done.

[Charlie Reis, 2015-06-03 20:26:04]

Thanks!  LGTM.

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

https://codereview.chromium.org/1167693007/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:644: // RenderFrameHost will not be
swapped. Therefore it is safe to call
On 2015/06/03 09:11:10, clamy wrote:
> On 2015/06/02 17:36:18, Charlie Reis wrote:
> > This first sentence isn't true in general.  Renderer-initiated main frame
> > navigations can be cross-process, for things like links to the Chrome Web
> Store
> > or rel=noreferrer target=_blank links.  That happens via
> > DecidePolicyForNavigation and RequestOpenURL.
> > 
> > Do those cases not get here?  (If not, let's clarify the comment.)
> 
> No they don't, edited the comment to precise that.

Acknowledged.

[Charlie Reis, 2015-06-03 20:26:59]

(I'll run a CQ dry run so it's quick to land when you're ready.)

[Charlie Reis, 2015-06-03 20:27:27]

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

[commit-bot: I haz the power, 2015-06-03 20:28:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1167693007/40001

[commit-bot: I haz the power, 2015-06-03 21:23:15]

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

[commit-bot: I haz the power, 2015-06-03 21:23:16]

Dry run: This issue passed the CQ dry run.

[clamy, 2015-06-04 08:35:13]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2015-06-04 08:35:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1167693007/40001

[commit-bot: I haz the power, 2015-06-04 08:39:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-06-04 08:40:14]

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