PlzNavigate: Fix WebUI creation logic for both cross and same site navigation.

content/browser/frame_host/render_frame_host_manager.cc

Index: content/browser/frame_host/render_frame_host_manager.cc
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
index 4a9034b51560718045abcf988cde7a79027070ab..4cc0efb63eff450326e946d0764327883c3e85a5 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -715,6 +715,22 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
     // --site-per-process is enabled.
     CleanUpNavigation();
     navigation_rfh = render_frame_host_.get();
+
+    // As site instances are the same, check if the WebUI should be reused

[clamy, 2015/02/05 16:54:27]

nit: add a . at the end of the comment.

[carlosk, 2015/02/05 17:07:33]

Done.

+    const NavigationEntry* current_navigation_entry =
+        delegate_->GetLastCommittedNavigationEntryForRenderManager();
+    bool should_reuse_web_ui_ = ShouldReuseWebUI(current_navigation_entry,
+                                                 request.common_params().url);
+    if (!should_reuse_web_ui_) {
+      speculative_web_ui_ = CreateWebUI(request.common_params().url,
+                                        request.bindings());
+      // Make sure the current RenderViewHost has the right bindings.
+      if (speculative_web_ui() &&
+          !render_frame_host_->GetProcess()->IsIsolatedGuest()) {
+        render_frame_host_->render_view_host()->AllowBindings(
+            speculative_web_ui()->GetBindings());
+      }
+    }
   } else {
     // If the SiteInstance for the final URL doesn't match the one from the
     // speculatively created RenderFrameHost, create a new RenderFrameHost using
@@ -763,6 +779,10 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
 
 // PlzNavigate
 void RenderFrameHostManager::CleanUpNavigation() {
+  CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));
+  speculative_web_ui_.reset();
+  should_reuse_web_ui_ = false;
   if (speculative_render_frame_host_)
     DiscardUnusedFrame(UnsetSpeculativeRenderFrameHost());
 }
@@ -772,8 +792,6 @@ scoped_ptr<RenderFrameHostImpl>
 RenderFrameHostManager::UnsetSpeculativeRenderFrameHost() {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
-  speculative_web_ui_.reset();
-  should_reuse_web_ui_ = false;
   speculative_render_frame_host_->GetProcess()->RemovePendingView();
   return speculative_render_frame_host_.Pass();
 }
@@ -1265,14 +1283,10 @@ bool RenderFrameHostManager::CreateSpeculativeRenderFrameHost(
   CHECK(new_instance);
   CHECK_NE(old_instance, new_instance);

[clamy, 2015/02/05 16:54:27]

Could you add a CHECK(!should_reuse_web_ui_) here? I don't think we want it to
get set to true by mistake.

[carlosk, 2015/02/05 17:07:33]

Done.

 
-  const NavigationEntry* current_navigation_entry =
-      delegate_->GetLastCommittedNavigationEntryForRenderManager();
-  // Note: |should_reuse_web_ui_| and |speculative_web_ui_| must be initialized
-  // before trying to create the |speculative_render_frame_host_|. Otherwise the
-  // WebUI won't be properly initialized.
-  bool should_reuse_web_ui_ = ShouldReuseWebUI(current_navigation_entry, url);
-  if (!should_reuse_web_ui_)
-    speculative_web_ui_ = CreateWebUI(url, bindings);
+  // Note: |speculative_web_ui_| must be initialized before starting the
+  // |speculative_render_frame_host_| creation steps otherwise the WebUI
+  // won't be properly initialized.
+  speculative_web_ui_ = CreateWebUI(url, bindings);
 
   int create_render_frame_flags = 0;
   int opener_route_id =
@@ -1288,7 +1302,6 @@ bool RenderFrameHostManager::CreateSpeculativeRenderFrameHost(
                         opener_route_id, create_render_frame_flags, nullptr);
 
   if (!speculative_render_frame_host_) {
-    should_reuse_web_ui_ = false;
     speculative_web_ui_.reset();
     return false;
   }
@@ -1721,6 +1734,8 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
   const NavigationEntry* current_entry =
       delegate_->GetLastCommittedNavigationEntryForRenderManager();
 
+  DCHECK(!cross_navigation_pending_);
+

[carlosk, 2015/02/05 16:09:06]

I moved the 2 removed DCHECKs up here because it happened either way if the next
if-block was executed or not.

   if (new_instance.get() != current_instance) {
     TRACE_EVENT_INSTANT2(
         "navigation",
@@ -1730,7 +1745,6 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
         "new_instance id", new_instance->GetId());
 
     // New SiteInstance: create a pending RFH to navigate.
-    DCHECK(!cross_navigation_pending_);
 
     // This will possibly create (set to nullptr) a Web UI object for the
     // pending page. We'll use this later to give the page special access. This
@@ -1799,7 +1813,6 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
   }
 
   // Otherwise the same SiteInstance can be used.  Navigate render_frame_host_.
-  DCHECK(!cross_navigation_pending_);
 
   // It's possible to swap out the current RFH and then decide to navigate in it
   // anyway (e.g., a cross-process navigation that redirects back to the