PlzNavigate: Avoid creating WebUI for subframes.

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 <stddef.h>
 
 #include <algorithm>
 #include <string>
@@ skipping 701 matching lines @@
   scoped_refptr<SiteInstance> dest_site_instance = GetSiteInstanceForNavigation(
       request.common_params().url, request.source_site_instance(),
       request.dest_site_instance(), candidate_site_instance,
       request.common_params().transition,
       request.restore_type() != RestoreType::NONE, request.is_view_source(),
       was_server_redirect);
 
   // The appropriate RenderFrameHost to commit the navigation.
   RenderFrameHostImpl* navigation_rfh = nullptr;
 
-  bool notify_webui_of_rf_creation = false;
-
   // Reuse the current RenderFrameHost if its SiteInstance matches the
   // navigation's.
   bool no_renderer_swap = current_site_instance == dest_site_instance.get();
 
   if (frame_tree_node_->IsMainFrame()) {
     // Renderer-initiated main frame navigations that may require a
     // SiteInstance swap are sent to the browser via the OpenURL IPC and are
     // afterwards treated as browser-initiated navigations. NavigationRequests
     // marked as renderer-initiated are created by receiving a BeginNavigation
     // IPC, and will then proceed in the same renderer. In site-per-process
     // mode, it is possible for renderer-intiated navigations to be allowed to
     // go cross-process. Check it first.
     bool can_renderer_initiate_transfer =
         render_frame_host_->IsRenderFrameLive() &&
         ShouldMakeNetworkRequestForURL(request.common_params().url) &&
         IsRendererTransferNeededForNavigation(render_frame_host_.get(),
                                               request.common_params().url);
 
     no_renderer_swap |=
         !request.may_transfer() && !can_renderer_initiate_transfer;
   } else {
     // Subframe navigations will use the current renderer, unless specifically
     // allowed to swap processes.
     no_renderer_swap |= !CanSubframeSwapProcess(
         request.common_params().url, request.source_site_instance(),
         request.dest_site_instance(), was_server_redirect);
   }
 
+  bool notify_webui_of_rf_creation = false;
   if (no_renderer_swap) {
     // GetFrameHostForNavigation will be called more than once during a
     // navigation (currently twice, on request and when it's about to commit in
     // the renderer). In the follow up calls an existing pending WebUI should
     // not be recreated if the URL didn't change. So instead of calling
     // CleanUpNavigation just discard the speculative RenderFrameHost if one
     // exists.
     if (speculative_render_frame_host_)
       DiscardUnusedFrame(UnsetSpeculativeRenderFrameHost());
 
-    UpdatePendingWebUIOnCurrentFrameHost(request.common_params().url,
-                                         request.bindings());
+    // Short-term solution: avoid creating a WebUI for subframes because
+    // non-PlzNavigate code path doesn't do it and some WebUI pages don't
+    // support it. See http://crrev.com/2810583006/.

[Charlie Reis, 2017/04/19 19:34:04]

Thanks, that helps.  Let's change the last line of each of these comments to
reference the bug with a TODO instead of the CL.  Something like:

TODO(crbug.com/713313): Make WebUI objects always be per-frame instead.

[arthursonzogni, 2017/04/20 09:46:22]

Done.

+    if (frame_tree_node_->IsMainFrame()) {
+      UpdatePendingWebUIOnCurrentFrameHost(request.common_params().url,
+                                           request.bindings());
+    }
 
     navigation_rfh = render_frame_host_.get();
 
     DCHECK(!speculative_render_frame_host_);
   } else {
     // If the current RenderFrameHost cannot be used a speculative one is
     // created with the SiteInstance for the current URL. If a speculative
     // RenderFrameHost already exists we try as much as possible to reuse it and
     // its associated WebUI.
 
     // Check if an existing speculative RenderFrameHost can be reused.
     if (!speculative_render_frame_host_ ||
         speculative_render_frame_host_->GetSiteInstance() !=
             dest_site_instance.get()) {
       // If a previous speculative RenderFrameHost didn't exist or if its
       // SiteInstance differs from the one for the current URL, a new one needs
       // to be created.
       CleanUpNavigation();
       bool success = CreateSpeculativeRenderFrameHost(current_site_instance,
                                                       dest_site_instance.get());
       DCHECK(success);
     }
     DCHECK(speculative_render_frame_host_);
 
-    bool changed_web_ui = speculative_render_frame_host_->UpdatePendingWebUI(
-        request.common_params().url, request.bindings());
-    speculative_render_frame_host_->CommitPendingWebUI();
-    DCHECK_EQ(GetNavigatingWebUI(), speculative_render_frame_host_->web_ui());
-    notify_webui_of_rf_creation =
-        changed_web_ui && speculative_render_frame_host_->web_ui();
-
+    // Short-term solution: avoid creating a WebUI for subframes because
+    // non-PlzNavigate code path doesn't do it and some WebUI pages don't
+    // support it. See http://crrev.com/2810583006/.
+    if (frame_tree_node_->IsMainFrame()) {
+      bool changed_web_ui = speculative_render_frame_host_->UpdatePendingWebUI(
+          request.common_params().url, request.bindings());
+      speculative_render_frame_host_->CommitPendingWebUI();
+      DCHECK_EQ(GetNavigatingWebUI(), speculative_render_frame_host_->web_ui());
+      notify_webui_of_rf_creation =
+          changed_web_ui && speculative_render_frame_host_->web_ui();
+    }
     navigation_rfh = speculative_render_frame_host_.get();
 
     // Check if our current RFH is live.
     if (!render_frame_host_->IsRenderFrameLive()) {
       // The current RFH is not live.  There's no reason to sit around with a
       // sad tab or a newly created RFH while we wait for the navigation to
       // complete. Just switch to the speculative RFH now and go back to normal.
       // (Note that we don't care about on{before}unload handlers if the current
       // RFH isn't live.)
       //
@@ skipping 40 matching lines @@
       // removed once the process manager moves away from NotificationService.
       // See https://crbug.com/462682.
       delegate_->NotifyMainFrameSwappedFromRenderManager(
           nullptr, render_frame_host_->render_view_host());
     }
   }
 
   // If a WebUI was created in a speculative RenderFrameHost or a new
   // RenderFrame was created then the WebUI never interacted with the
   // RenderFrame or its RenderView. Notify using RenderFrameCreated.
-  if (notify_webui_of_rf_creation && GetNavigatingWebUI())
+  if (notify_webui_of_rf_creation && GetNavigatingWebUI() &&
+      // Short-term solution: avoid creating a WebUI for subframes because
+      // non-PlzNavigate code path doesn't do it and some WebUI pages don't
+      // support it. See http://crrev.com/2810583006/.

[Charlie Reis, 2017/04/19 19:34:04]

nit: Let's move this comment above line 863, since it's unusual to indent it
within the middle of the conditional.

[arthursonzogni, 2017/04/20 09:46:22]

Done.

+      frame_tree_node_->IsMainFrame()) {
     GetNavigatingWebUI()->RenderFrameCreated(navigation_rfh);
+  }
 
   return navigation_rfh;
 }
 
 // PlzNavigate
 void RenderFrameHostManager::CleanUpNavigation() {
   CHECK(IsBrowserSideNavigationEnabled());
   if (speculative_render_frame_host_) {
     bool was_loading = speculative_render_frame_host_->is_loading();
     DiscardUnusedFrame(UnsetSpeculativeRenderFrameHost());
@@ skipping 1941 matching lines @@
           delegate_->IsHidden()) {
     if (delegate_->IsHidden()) {
       render_frame_host_->GetView()->Hide();
     } else {
       render_frame_host_->GetView()->Show();
     }
   }
 }
 
 }  // namespace content