This CL is basically a 3 fixes in 1. I started off fixing the fact that

WebUIs were not properly being created when reusing the current
RenderFrameHost. But while doing that I found an issue in
RenderFrameHostManager::CreateSpeculativeRenderFrameHost where instead
of setting an instance member I was mistakenly shadowing it with a
same-named function member.

This last fix caused another test to crash because the WebUI reuse logic
was incorrect. Instead of checking if it should be reused when the
navigation SiteInstance matched the current one, it was being done when
they were different (what doesn't even makes sense).

Due to these changes I also had to update how RFHM::CleanUpNavigation
and UnsetSpeculativeRenderFrameHost work,so that the speculative WebUI
member is always cleaned up even if a speculative RFH does not exist.

Also renamed the speculative WebUI getter as it's not test-only anymore.

With these changes these tests were fixed when PlzNavigate is enabled:
    RenderFrameHostManagerTest.EnableWebUIWithSwappedOutOpener
    RenderFrameHostManagerTest.WebUIInNewTab

BUG=439423

Review URL: https://codereview.chromium.org/877343010

Cr-Commit-Position: refs/heads/master@{#315006}

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 SiteInstances are the same, check if the WebUI should be reused.
+    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();
 }
@@ -1270,14 +1288,11 @@ bool RenderFrameHostManager::CreateSpeculativeRenderFrameHost(
     int bindings) {
   CHECK(new_instance);
   CHECK_NE(old_instance, new_instance);
+  CHECK(!should_reuse_web_ui_);
 
-  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_)
+  // 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;
@@ -1294,7 +1309,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;
   }
@@ -1727,6 +1741,8 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
   const NavigationEntry* current_entry =
       delegate_->GetLastCommittedNavigationEntryForRenderManager();
 
+  DCHECK(!cross_navigation_pending_);
+
   if (new_instance.get() != current_instance) {
     TRACE_EVENT_INSTANT2(
         "navigation",
@@ -1736,7 +1752,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
@@ -1805,7 +1820,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

content/browser/frame_host/render_frame_host_manager.h

@@ -237,7 +237,7 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
   // PlzNavigate
   // Returns the speculative WebUI for the navigation (a newly created one or
   // the current one if it should be reused). If none is set returns nullptr.
-  WebUIImpl* speculative_web_ui_for_testing() const {
+  WebUIImpl* speculative_web_ui() const {
     return should_reuse_web_ui_ ? web_ui_.get() : speculative_web_ui_.get();
   }
 

content/browser/frame_host/render_frame_host_manager_unittest.cc

@@ -973,7 +973,7 @@ TEST_F(RenderFrameHostManagerTest, WebUI) {
   // used yet. UpdateStateForNavigate() took the short cut path.
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableBrowserSideNavigation)) {
-    EXPECT_FALSE(manager->speculative_web_ui_for_testing());
+    EXPECT_FALSE(manager->speculative_web_ui());
   } else {
     EXPECT_FALSE(manager->pending_web_ui());
   }
@@ -1033,6 +1033,7 @@ TEST_F(RenderFrameHostManagerTest, WebUIInNewTab) {
   // RenderWidgetHost::Init when opening a new tab from a link.
   manager2->current_host()->CreateRenderView(
       base::string16(), -1, MSG_ROUTING_NONE, -1, false);
+  EXPECT_TRUE(manager2->current_host()->IsRenderViewLive());
 
   const GURL kUrl2("chrome://foo/bar");
   NavigationEntryImpl entry2(NULL /* instance */, -1 /* page_id */, kUrl2,
@@ -1044,6 +1045,12 @@ TEST_F(RenderFrameHostManagerTest, WebUIInNewTab) {
   // No cross-process transition happens because we are already in the right
   // SiteInstance.  We should grant bindings immediately.
   EXPECT_EQ(host2, manager2->current_frame_host());
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableBrowserSideNavigation)) {
+    EXPECT_TRUE(manager2->speculative_web_ui());
+  } else {
+    EXPECT_TRUE(manager2->pending_web_ui());
+  }
   EXPECT_TRUE(
       host2->render_view_host()->GetEnabledBindings() & BINDINGS_POLICY_WEB_UI);