PlzNavigate: renderer-initiated navigations create a pending NavigationEntry

content/browser/frame_host/navigator_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/navigator_impl.h"
 
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
 #include "base/time/time.h"
 #include "content/browser/frame_host/frame_tree.h"
@@ skipping 107 matching lines @@
 void NavigatorImpl::DidStartProvisionalLoad(
     RenderFrameHostImpl* render_frame_host,
     const GURL& url) {
   bool is_error_page = (url.spec() == kUnreachableWebDataURL);
   bool is_iframe_srcdoc = (url.spec() == kAboutSrcDocURL);
   GURL validated_url(url);
   RenderProcessHost* render_process_host = render_frame_host->GetProcess();
   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.
     delegate_->DidStartProvisionalLoad(
         render_frame_host, validated_url, is_error_page, is_iframe_srcdoc);
   }
 }
 
 
 void NavigatorImpl::DidFailProvisionalLoadWithError(
@@ skipping 491 matching lines @@
 
   // In all other cases the current navigation, if any, is canceled and a new
   // NavigationRequest is created for the node.
   scoped_ptr<NavigationRequest> navigation_request =
       NavigationRequest::CreateRendererInitiated(
           frame_tree_node, common_params, begin_params, body,
           controller_->GetLastCommittedEntryIndex(),
           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);
 }
 
 // PlzNavigate
 void NavigatorImpl::CommitNavigation(FrameTreeNode* frame_tree_node,
                                      ResourceResponse* response,
                                      scoped_ptr<StreamHandle> body) {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
@@ skipping 222 matching lines @@
     UMA_HISTOGRAM_TIMES(
         "Navigation.TimeToCommit_ExistingRenderer_BeforeUnloadDiscounted",
         time_to_commit);
     UMA_HISTOGRAM_TIMES(
         "Navigation.TimeToURLJobStart_ExistingRenderer_BeforeUnloadDiscounted",
         time_to_network);
   }
   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