PlzNavigate: renderer-initiated navigations create a pending NavigationEntry

content/browser/frame_host/navigator_impl.cc

Index: content/browser/frame_host/navigator_impl.cc
diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc
index 888a2b7e41ae0fa86ef664bc880bfa37254ab2e1..7fb8459e6a14da298573c43c8788d8ba534874ad 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -125,37 +125,9 @@ void NavigatorImpl::DidStartProvisionalLoad(
   render_process_host->FilterURL(false, &validated_url);
 
   bool is_main_frame = render_frame_host->frame_tree_node()->IsMainFrame();
-  NavigationEntryImpl* pending_entry = controller_->GetPendingEntry();
-  if (is_main_frame) {
-    // If there is no browser-initiated pending entry for this navigation and it
-    // is not for the error URL, create a pending entry using the current
-    // SiteInstance, and ensure the address bar updates accordingly.  We don't
-    // know the referrer or extra headers at this point, but the referrer will
-    // be set properly upon commit.
-    bool has_browser_initiated_pending_entry = pending_entry &&
-        !pending_entry->is_renderer_initiated();
-    if (!has_browser_initiated_pending_entry && !is_error_page) {
-      NavigationEntryImpl* entry = NavigationEntryImpl::FromNavigationEntry(
-          controller_->CreateNavigationEntry(validated_url,
-                                             content::Referrer(),
-                                             ui::PAGE_TRANSITION_LINK,
-                                             true /* is_renderer_initiated */,
-                                             std::string(),
-                                             controller_->GetBrowserContext()));
-      entry->set_site_instance(render_frame_host->GetSiteInstance());
-      // TODO(creis): If there's a pending entry already, find a safe way to
-      // update it instead of replacing it and copying over things like this.
-      if (pending_entry) {
-        entry->set_transferred_global_request_id(
-            pending_entry->transferred_global_request_id());
-        entry->set_should_replace_entry(pending_entry->should_replace_entry());
-        entry->SetRedirectChain(pending_entry->GetRedirectChain());
-      }
-      controller_->SetPendingEntry(entry);
-      if (delegate_)
-        delegate_->NotifyChangedNavigationState(content::INVALIDATE_TYPE_URL);
-    }
-  }
+  if (is_main_frame && !is_error_page)
+    MainFrameNavigationStarted(validated_url,
+                               render_frame_host->GetSiteInstance());
 
   if (delegate_) {
     // Notify the observer about the start of the provisional load.
@@ -667,8 +639,16 @@ void NavigatorImpl::OnBeginNavigation(
           controller_->GetEntryCount());
   frame_tree_node->SetNavigationRequest(navigation_request.Pass());
 
-  if (frame_tree_node->IsMainFrame())
+  if (frame_tree_node->IsMainFrame()) {

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

Do we not get here for error pages?

[clamy, 2015/06/03 09:11:10]

No this is the beginning of a normal navigation.

+    // In the case of a renderer-initiated main frame navigation, the
+    // RenderFrameHost will not be swapped. Therefore it is safe to call

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

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

[clamy, 2015/06/03 09:11:10]

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

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

Acknowledged.

+    // MainFrameNavigationStarted with the SiteInstance from the current
+    // RenderFrameHost.
+    MainFrameNavigationStarted(
+        common_params.url,

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

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

[clamy, 2015/06/03 09:11:10]

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

+        frame_tree_node->current_frame_host()->GetSiteInstance());
     navigation_data_.reset();
+  }
 
   BeginNavigation(frame_tree_node);
 }
@@ -911,4 +891,36 @@ void NavigatorImpl::RecordNavigationMetrics(
   navigation_data_.reset();
 }
 
+void NavigatorImpl::MainFrameNavigationStarted(
+    const GURL& url,
+    SiteInstanceImpl* site_instance) {
+  // If there is no browser-initiated pending entry for this navigation and it
+  // is not for the error URL, create a pending entry using the current
+  // SiteInstance, and ensure the address bar updates accordingly.  We don't
+  // know the referrer or extra headers at this point, but the referrer will
+  // be set properly upon commit.
+  NavigationEntryImpl* pending_entry = controller_->GetPendingEntry();
+  bool has_browser_initiated_pending_entry =
+      pending_entry && !pending_entry->is_renderer_initiated();
+  if (!has_browser_initiated_pending_entry) {
+    NavigationEntryImpl* entry = NavigationEntryImpl::FromNavigationEntry(
+        controller_->CreateNavigationEntry(
+            url, content::Referrer(), ui::PAGE_TRANSITION_LINK,
+            true /* is_renderer_initiated */, std::string(),
+            controller_->GetBrowserContext()));
+    entry->set_site_instance(site_instance);
+    // TODO(creis): If there's a pending entry already, find a safe way to
+    // update it instead of replacing it and copying over things like this.
+    if (pending_entry) {
+      entry->set_transferred_global_request_id(
+          pending_entry->transferred_global_request_id());
+      entry->set_should_replace_entry(pending_entry->should_replace_entry());
+      entry->SetRedirectChain(pending_entry->GetRedirectChain());
+    }
+    controller_->SetPendingEntry(entry);
+    if (delegate_)
+      delegate_->NotifyChangedNavigationState(content::INVALIDATE_TYPE_URL);
+  }
+}
+
 }  // namespace content