Stop trying to reuse processes for web iframes on extension pages.

There's evidence that some web iframes embedded on extension
background pages may be doing a lot of work with the assumption that
since this work isn't on a foreground tab, it doesn't need to be
optimized for responsiveness.  However, OOPIFs currently use an
aggressive process reuse policy, which also affects this use case,
making it possible for a web iframe on an extension to get into
another tab's process and adversely affect its performance.  This CL
avoids using that reuse policy for web iframes on extensions.

Caveats:

- When over the process limit, web iframes on extensions will still
  attempt to reuse an existing process.

- OOPIFs from normal tabs may still join processes for web iframes on
  extensions.  This and the previous point imply that there won't be a
  guarantee of perfect performance isolation for web iframes on
  extensions; the goal is to be more resilient to problems in the
  common case.

- We will stop potentially useful process reuse between same-site
  iframes on multiple instances of extensions, even for instances of
  the same extension.  We expect this to be rare in practice though.

Bug: 899418, 899838
Change-Id: I35f6ecc1945292f9fab1c21f65d1ac4b7970dbe3
Reviewed-on: https://chromium-review.googlesource.com/c/1306410Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605028}

chrome/browser/chrome_content_browser_client.cc

@@ -1702,6 +1702,16 @@ bool ChromeContentBrowserClient::ShouldTryToUseExistingProcessHost(
 #endif
 }
 
+bool ChromeContentBrowserClient::ShouldSubframesTryToReuseExistingProcess(
+    content::RenderFrameHost* main_frame) {
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+  return ChromeContentBrowserClientExtensionsPart::
+      ShouldSubframesTryToReuseExistingProcess(main_frame);
+#else
+  return true;
+#endif
+}
+
 void ChromeContentBrowserClient::SiteInstanceGotProcess(
     SiteInstance* site_instance) {
   CHECK(site_instance->HasProcess());

chrome/browser/chrome_content_browser_client.h

@@ -168,6 +168,8 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
   bool ShouldTryToUseExistingProcessHost(
       content::BrowserContext* browser_context,
       const GURL& url) override;
+  bool ShouldSubframesTryToReuseExistingProcess(
+      content::RenderFrameHost* main_frame) override;
   void SiteInstanceGotProcess(content::SiteInstance* site_instance) override;
   void SiteInstanceDeleting(content::SiteInstance* site_instance) override;
   bool ShouldSwapBrowsingInstancesForNavigation(

chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc

@@ -38,6 +38,7 @@
 #include "content/public/browser/browser_url_handler.h"
 #include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/page_navigator.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/resource_dispatcher_host.h"
@@ -604,6 +605,32 @@ ChromeContentBrowserClientExtensionsPart::ShouldTryToUseExistingProcessHost(
           (max_process_count * chrome::kMaxShareOfExtensionProcesses));
 }
 
+// static
+bool ChromeContentBrowserClientExtensionsPart::
+    ShouldSubframesTryToReuseExistingProcess(
+        content::RenderFrameHost* main_frame) {
+  DCHECK(!main_frame->GetParent());
+
+  // Most out-of-process iframes aggressively look for a random same-site
+  // process to reuse if possible, to keep the process count low. Skip this for
+  // web iframes inside extensions (not including hosted apps), since the
+  // workload here tends to be different and we want to avoid slowing down
+  // normal web pages with misbehaving extension-related content.
+  //
+  // Note that this does not prevent process sharing with tabs when over the
+  // process limit, and OOPIFs from tabs (which will aggressively look for
+  // existing processes) may still join the process of an extension's web
+  // iframe.  This mainly reduces the likelihood of problems with main frames
+  // and makes it more likely that the subframe process will be shown near the
+  // extension in Chrome's task manager for blame purposes. See
+  // https://crbug.com/899418.
+  const Extension* extension =
+      ExtensionRegistry::Get(main_frame->GetSiteInstance()->GetBrowserContext())
+          ->enabled_extensions()
+          .GetExtensionOrAppByURL(main_frame->GetSiteInstance()->GetSiteURL());
+  return !extension || !extension->is_extension();
+}
+
 // static
 bool ChromeContentBrowserClientExtensionsPart::
     ShouldSwapBrowsingInstancesForNavigation(SiteInstance* site_instance,

chrome/browser/extensions/chrome_content_browser_client_extensions_part.h

@@ -17,6 +17,7 @@
 
 namespace content {
 struct Referrer;
+class RenderFrameHost;
 class RenderProcessHost;
 class ResourceContext;
 class VpnServiceProxy;
@@ -61,6 +62,8 @@ class ChromeContentBrowserClientExtensionsPart
                              const GURL& site_url);
   static bool ShouldTryToUseExistingProcessHost(Profile* profile,
                                                 const GURL& url);
+  static bool ShouldSubframesTryToReuseExistingProcess(
+      content::RenderFrameHost* main_frame);
   static bool ShouldSwapBrowsingInstancesForNavigation(
       content::SiteInstance* site_instance,
       const GURL& current_url,

chrome/browser/extensions/process_manager_browsertest.cc

@@ -1647,6 +1647,61 @@ IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest,
   }
 }
 
+// Verify that web iframes on extension frames do not attempt to aggressively
+// reuse existing processes for the same site.  This helps prevent a
+// misbehaving web iframe on an extension from slowing down other processes.
+// See https://crbug.com/899418.
+IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest,
+                       WebSubframeOnExtensionDoesNotReuseExistingProcess) {
+  // This test matters only *with* --site-per-process.  It depends on process
+  // reuse logic that subframes use to look for existing processes, but that
+  // logic is only turned on for sites that require a dedicated process.
+  if (!content::AreAllSitesIsolatedForTesting())
+    return;
+
+  // Create a simple extension with a background page that has an empty iframe.
+  const Extension* extension = CreateExtension("Extension", true);
+  embedded_test_server()->ServeFilesFromDirectory(extension->path());
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  // Navigate main tab to a web page on foo.com.
+  GURL foo_url(embedded_test_server()->GetURL("foo.com", "/title1.html"));
+  NavigateToURL(foo_url);
+  content::WebContents* tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_EQ(foo_url, tab->GetLastCommittedURL());
+
+  // So far, there should be two extension frames: one for the background page,
+  // one for the empty subframe on it.
+  ProcessManager* pm = ProcessManager::Get(profile());
+  EXPECT_EQ(2u, pm->GetAllFrames().size());
+  EXPECT_EQ(2u, pm->GetRenderFrameHostsForExtension(extension->id()).size());
+
+  // Navigate the subframe on the extension background page to foo.com, and
+  // wait for the old subframe to go away.
+  ExtensionHost* background_host =
+      pm->GetBackgroundHostForExtension(extension->id());
+  content::RenderFrameHost* background_rfh =
+      background_host->host_contents()->GetMainFrame();
+  content::RenderFrameHost* extension_subframe =
+      ChildFrameAt(background_rfh, 0);
+  content::RenderFrameDeletedObserver deleted_observer(extension_subframe);
+  EXPECT_TRUE(
+      content::ExecJs(extension_subframe,
+                      content::JsReplace("window.location = $1;", foo_url)));
+  deleted_observer.WaitUntilDeleted();
+
+  // There should now only be one extension frame for the background page.  The
+  // subframe should've swapped processes and should now be a web frame.
+  EXPECT_EQ(1u, pm->GetAllFrames().size());
+  EXPECT_EQ(1u, pm->GetRenderFrameHostsForExtension(extension->id()).size());
+  content::RenderFrameHost* subframe = ChildFrameAt(background_rfh, 0);
+  EXPECT_EQ(foo_url, subframe->GetLastCommittedURL());
+
+  // Verify that the subframe did *not* reuse the existing foo.com process.
+  EXPECT_NE(tab->GetMainFrame()->GetProcess(), subframe->GetProcess());
+}
+
 // Test to verify that loading a resource other than an icon file is
 // disallowed for hosted apps, while icons are allowed.
 // See https://crbug.com/717626.

chrome/browser/site_details_browsertest.cc

@@ -857,17 +857,13 @@ IN_PROC_BROWSER_TEST_F(SiteDetailsBrowserTest, MAYBE_IsolateExtensions) {
                   "SiteIsolation.IsolateExtensionsProcessCountNoLimit"),
               HasOneSample(4));
 
-  // As part of https://crbug.com/512560, subframes that require a dedicated
-  // process started reusing existing processes when possible, so under
-  // --site-per-process, tab1's web iframe will share the process with tab2's
-  // web iframe, since they have the same site. This won't affect
-  // --isolate-extensions, because the web iframe's site won't require a
-  // dedicated process in that mode. Hence, with site-per-process, there should
-  // be three total renderer processes: one for the two web iframes, one for
-  // extension3, and one for extension 1's background page. With only
-  // --isolate-extensions, there should be four total renderer processes, as
-  // each web iframe will go into its own process.
-  EXPECT_THAT(GetRenderProcessCount(), DependingOnPolicy(2, 4, 3));
+  // There should be four total renderer processes: one for each of the two web
+  // iframes, one for extension3, and one for extension 1's background page.
+  // Note that the optimization in https://crbug.com/512560, where subframes
+  // that require a dedicated process reuse existing processes where possible,
+  // does not apply to web iframes in extensions anymore -- see
+  // https://crbug.com/899418.
+  EXPECT_THAT(GetRenderProcessCount(), DependingOnPolicy(2, 4, 4));
   EXPECT_THAT(details->GetOutOfProcessIframeCount(),
               DependingOnPolicy(0, 2, 2));
 }

content/browser/frame_host/render_frame_host_manager.cc

@@ -1075,8 +1075,16 @@ RenderFrameHostManager::GetSiteInstanceForNavigation(
       static_cast<SiteInstanceImpl*>(new_instance.get());
   if (!frame_tree_node_->IsMainFrame() && !new_instance_impl->HasProcess() &&
       new_instance_impl->RequiresDedicatedProcess()) {
-    new_instance_impl->set_process_reuse_policy(
-        SiteInstanceImpl::ProcessReusePolicy::REUSE_PENDING_OR_COMMITTED_SITE);
+    // Also give the embedder a chance to override this decision.  Certain
+    // frames have different enough workloads so that it's better to avoid
+    // placing a subframe into an existing process for better performance
+    // isolation.  See https://crbug.com/899418.
+    if (GetContentClient()->browser()->ShouldSubframesTryToReuseExistingProcess(
+            frame_tree_node_->frame_tree()->GetMainFrame())) {
+      new_instance_impl->set_process_reuse_policy(
+          SiteInstanceImpl::ProcessReusePolicy::
+              REUSE_PENDING_OR_COMMITTED_SITE);
+    }
   }
 
   return new_instance;

content/public/browser/content_browser_client.cc

@@ -194,6 +194,11 @@ bool ContentBrowserClient::ShouldTryToUseExistingProcessHost(
   return false;
 }
 
+bool ContentBrowserClient::ShouldSubframesTryToReuseExistingProcess(
+    RenderFrameHost* main_frame) {
+  return true;
+}
+
 bool ContentBrowserClient::ShouldSwapBrowsingInstancesForNavigation(
     SiteInstance* site_instance,
     const GURL& current_url,

content/public/browser/content_browser_client.h

@@ -419,6 +419,15 @@ class CONTENT_EXPORT ContentBrowserClient {
   virtual bool ShouldTryToUseExistingProcessHost(
       BrowserContext* browser_context, const GURL& url);
 
+  // Returns whether or not subframes of |main_frame| should try to
+  // aggressively reuse existing processes, even when below process limit.
+  // This gets called when navigating a subframe to a URL that requires a
+  // dedicated process and defaults to true, which minimizes the process count.
+  // The embedder can choose to override this if there is a reason to avoid the
+  // reuse.
+  virtual bool ShouldSubframesTryToReuseExistingProcess(
+      RenderFrameHost* main_frame);
+
   // Called when a site instance is first associated with a process.
   virtual void SiteInstanceGotProcess(SiteInstance* site_instance) {}