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

content/browser/frame_host/render_frame_host_manager.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/render_frame_host_manager.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/logging.h"
@@ skipping 697 matching lines @@
   // (see crbug.com/440266).
   if (current_site_instance == dest_site_instance.get() ||
       (!frame_tree_node_->IsMainFrame() &&
        !base::CommandLine::ForCurrentProcess()->HasSwitch(
            switches::kSitePerProcess))) {
     // Reuse the current RFH if its SiteInstance matches the the navigation's
     // or if this is a subframe navigation. We only swap RFHs for subframes when
     // --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
     // this new SiteInstance.
     if (!speculative_render_frame_host_ ||
         speculative_render_frame_host_->GetSiteInstance() !=
             dest_site_instance.get()) {
       CleanUpNavigation();
       bool success = CreateSpeculativeRenderFrameHost(
           request.common_params().url, current_site_instance,
@@ skipping 28 matching lines @@
       return nullptr;
     }
   }
 
   cross_navigation_pending_ = navigation_rfh != render_frame_host_.get();
   return navigation_rfh;
 }
 
 // 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());
 }
 
 // PlzNavigate
 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();
 }
 
 void RenderFrameHostManager::OnDidStartLoading() {
   for (const auto& pair : proxy_hosts_) {
     pair.second->Send(
         new FrameMsg_DidStartLoading(pair.second->GetRoutingID()));
   }
 }
@@ skipping 469 matching lines @@
   return render_frame_host.Pass();
 }
 
 // PlzNavigate
 bool RenderFrameHostManager::CreateSpeculativeRenderFrameHost(
     const GURL& url,
     SiteInstance* old_instance,
     SiteInstance* new_instance,
     int bindings) {
   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 =
       CreateOpenerRenderViewsIfNeeded(old_instance, new_instance,
                                       &create_render_frame_flags);
 
   if (frame_tree_node_->IsMainFrame())
     create_render_frame_flags |= CREATE_RF_FOR_MAIN_FRAME_NAVIGATION;
   if (delegate_->IsHidden())
     create_render_frame_flags |= CREATE_RF_HIDDEN;
   speculative_render_frame_host_ =
       CreateRenderFrame(new_instance, speculative_web_ui_.get(),
                         opener_route_id, create_render_frame_flags, nullptr);
 
   if (!speculative_render_frame_host_) {
-    should_reuse_web_ui_ = false;
     speculative_web_ui_.reset();
     return false;
   }
   return true;
 }
 
 scoped_ptr<RenderFrameHostImpl> RenderFrameHostManager::CreateRenderFrame(
     SiteInstance* instance,
     WebUIImpl* web_ui,
     int opener_route_id,
@@ skipping 412 matching lines @@
   }
 
   SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
   scoped_refptr<SiteInstance> new_instance = GetSiteInstanceForNavigation(
       dest_url, source_instance, dest_instance, transition,
       dest_is_restore, dest_is_view_source_mode);
 
   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",
         "RenderFrameHostManager::UpdateStateForNavigate:New SiteInstance",
         TRACE_EVENT_SCOPE_THREAD,
         "current_instance id", current_instance->GetId(),
         "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
     // must happen before the new renderer is created below so it will get
     // bindings. It must also happen after the above conditional call to
     // CancelPending(), otherwise CancelPending may clear the pending_web_ui_
     // and the page will not have its bindings set appropriately.
     SetPendingWebUI(dest_url, bindings);
     CreatePendingRenderFrameHost(current_instance, new_instance.get(),
                                  frame_tree_node_->IsMainFrame());
@@ skipping 48 matching lines @@
       // old render view to run its beforeunload handler, since it doesn't
       // otherwise know that the cross-site request is happening.  This will
       // trigger a call to OnBeforeUnloadACK with the reply.
       render_frame_host_->DispatchBeforeUnload(true);
     }
 
     return pending_render_frame_host_.get();
   }
 
   // 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
   // original site).  In that case, we have a proxy for the current RFH but
   // haven't deleted it yet.  The new navigation will swap it back in, so we can
   // delete the proxy.
   DeleteRenderFrameProxyHost(new_instance.get());
 
   if (ShouldReuseWebUI(current_entry, dest_url)) {
     pending_web_ui_.reset();
@@ skipping 120 matching lines @@
 void RenderFrameHostManager::DeleteRenderFrameProxyHost(
     SiteInstance* instance) {
   RenderFrameProxyHostMap::iterator iter = proxy_hosts_.find(instance->GetId());
   if (iter != proxy_hosts_.end()) {
     delete iter->second;
     proxy_hosts_.erase(iter);
   }
 }
 
 }  // namespace content