Avoid StoragePartition considerations for spare RenderProcessHost.

This is a refactoring that
1) Avoids using the spare RenderProcessHost for Top Document Isolation
   (TDI) default subframe process.
and
2) Because of (1) simplifies SpareRenderProcessHostManager so that it
   can avoid considering StoragePartition when managing the spare
   process.  Note that after (1) the only caller of
   RenderProcessHostImpl::CreateOrUseSpareRenderProcessHost
   (RenderProcessHostImpl::GetProcessHostForSiteInstance) would always
   pass |nullptr| as the partition argument.

Bug: 808114
Change-Id: Ic87b9142b9e27ca1e95233a4e6e42cc1e8e89f97
Reviewed-on: https://chromium-review.googlesource.com/991058Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549718}

components/data_reduction_proxy/content/browser/content_lofi_ui_service_unittest.cc

@@ -31,7 +31,7 @@ class ContentLoFiUIServiceTest : public content::RenderViewHostTestHarness {
  public:
   ContentLoFiUIServiceTest()
       : content::RenderViewHostTestHarness(
-            content::TestBrowserThreadBundle::REAL_IO_THREAD),
+            content::TestBrowserThreadBundle::DEFAULT),
         callback_called_(false) {}
 
   void RunTestOnIOThread(base::RunLoop* ui_run_loop) {

content/browser/renderer_host/render_process_host_browsertest.cc

@@ -357,6 +357,104 @@ IN_PROC_BROWSER_TEST_F(RenderProcessHostTest,
   // down.
 }
 
+// Class that simulates the fact that some //content embedders (e.g. by
+// overriding ChromeContentBrowserClient::GetStoragePartitionConfigForSite) can
+// cause BrowserContext::GetDefaultStoragePartition(browser_context) to differ
+// from BrowserContext::GetStoragePartition(browser_context, site_instance) even
+// if |site_instance| is not for guests.
+class CustomStoragePartitionForSomeSites : public TestContentBrowserClient {
+ public:
+  explicit CustomStoragePartitionForSomeSites(const GURL& site_to_isolate)
+      : site_to_isolate_(site_to_isolate) {}
+
+  void GetStoragePartitionConfigForSite(BrowserContext* browser_context,
+                                        const GURL& site,
+                                        bool can_be_default,
+                                        std::string* partition_domain,
+                                        std::string* partition_name,
+                                        bool* in_memory) override {
+    // Default to the browser-wide storage partition and override based on
+    // |site| below.
+    partition_domain->clear();
+    partition_name->clear();
+    *in_memory = false;
+
+    // Override for |site_to_isolate_|.
+    if (site == site_to_isolate_) {
+      *partition_domain = "blah_isolated_storage";
+      *partition_name = "blah_isolated_storage";
+      *in_memory = false;
+    }
+  }
+
+  std::string GetStoragePartitionIdForSite(BrowserContext* browser_context,
+                                           const GURL& site) override {
+    if (site == site_to_isolate_)
+      return "custom";
+    return "";
+  }
+
+  bool IsValidStoragePartitionId(BrowserContext* browser_context,
+                                 const std::string& partition_id) override {
+    return partition_id == "" || partition_id == "custom";
+  }
+
+ private:
+  GURL site_to_isolate_;
+
+  DISALLOW_COPY_AND_ASSIGN(CustomStoragePartitionForSomeSites);
+};
+
+// This test verifies that SpareRenderProcessHostManager correctly accounts
+// for StoragePartition differences when handing out the spare process.
+IN_PROC_BROWSER_TEST_F(RenderProcessHostTest,
+                       SpareProcessVsCustomStoragePartition) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  // Provide custom storage partition for test sites.
+  GURL test_url = embedded_test_server()->GetURL("/simple_page.html");
+  BrowserContext* browser_context =
+      ShellContentBrowserClient::Get()->browser_context();
+  GURL test_site = SiteInstance::GetSiteForURL(browser_context, test_url);
+  CustomStoragePartitionForSomeSites modified_client(test_site);
+  ContentBrowserClient* old_client =
+      SetBrowserClientForTesting(&modified_client);
+  StoragePartition* default_storage =
+      BrowserContext::GetDefaultStoragePartition(browser_context);
+  StoragePartition* custom_storage =
+      BrowserContext::GetStoragePartitionForSite(browser_context, test_site);
+  EXPECT_NE(default_storage, custom_storage);
+
+  // Open a test window - it should be associated with the default storage
+  // partition.
+  Shell* window = CreateBrowser();
+  RenderProcessHost* old_process =
+      window->web_contents()->GetMainFrame()->GetProcess();
+  EXPECT_EQ(default_storage, old_process->GetStoragePartition());
+
+  // Warm up the spare process - it should be associated with the default
+  // storage partition.
+  RenderProcessHost::WarmupSpareRenderProcessHost(browser_context);
+  RenderProcessHost* spare_renderer =
+      RenderProcessHostImpl::GetSpareRenderProcessHostForTesting();
+  ASSERT_TRUE(spare_renderer);
+  EXPECT_EQ(default_storage, spare_renderer->GetStoragePartition());
+
+  // Navigate to a URL that requires a custom storage partition.
+  EXPECT_TRUE(NavigateToURL(window, test_url));
+  RenderProcessHost* new_process =
+      window->web_contents()->GetMainFrame()->GetProcess();
+  // Requirement to use a custom storage partition should force a process swap.
+  EXPECT_NE(new_process, old_process);
+  // The new process should be associated with the custom storage partition.
+  EXPECT_EQ(custom_storage, new_process->GetStoragePartition());
+  // And consequently, the spare shouldn't have been used.
+  EXPECT_NE(spare_renderer, new_process);
+
+  // Restore the original ContentBrowserClient.
+  SetBrowserClientForTesting(old_client);
+}
+
 class ShellCloser : public RenderProcessHostObserver {
  public:
   ShellCloser(Shell* shell, std::string* logging_string)

content/browser/renderer_host/render_process_host_impl.cc

@@ -541,11 +541,10 @@ class SessionStorageHolder : public base::SupportsUserData::Data {
 // SpareRenderProcessHostManager::MaybeTakeSpareRenderProcessHost when creating
 // a new RPH. In this implementation, the spare renderer is bound to a
 // BrowserContext and its default StoragePartition. If
-// MaybeTakeSpareRenderProcessHost is called with a BrowserContext and
-// StoragePartition that does not match, the spare renderer is discarded. In
-// particular, only the default StoragePartition will be able to use a spare
-// renderer. The spare renderer will also not be used as a guest renderer
-// (is_for_guests_ == true).
+// MaybeTakeSpareRenderProcessHost is called with a BrowserContext that does not
+// match, the spare renderer is discarded. Only the default StoragePartition
+// will be able to use a spare renderer. The spare renderer will also not be
+// used as a guest renderer (is_for_guests_ == true).
 //
 // It is safe to call WarmupSpareRenderProcessHost multiple times, although if
 // called in a context where the spare renderer is not likely to be used
@@ -555,14 +554,12 @@ class SpareRenderProcessHostManager : public RenderProcessHostObserver {
   SpareRenderProcessHostManager() {}
 
   void WarmupSpareRenderProcessHost(BrowserContext* browser_context) {
-    StoragePartitionImpl* current_partition =
-        static_cast<StoragePartitionImpl*>(
-            BrowserContext::GetStoragePartition(browser_context, nullptr));
-
     if (spare_render_process_host_ &&
-        matching_browser_context_ == browser_context &&
-        matching_storage_partition_ == current_partition)
+        spare_render_process_host_->GetBrowserContext() == browser_context) {
+      DCHECK_EQ(BrowserContext::GetDefaultStoragePartition(browser_context),
+                spare_render_process_host_->GetStoragePartition());
       return;  // Nothing to warm up.
+    }
 
     CleanupSpareRenderProcessHost();
 
@@ -574,21 +571,15 @@ class SpareRenderProcessHostManager : public RenderProcessHostObserver {
             RenderProcessHostImpl::GetMaxRendererProcessCount())
       return;
 
-    matching_browser_context_ = browser_context;
-    matching_storage_partition_ = current_partition;
-
     spare_render_process_host_ = RenderProcessHostImpl::CreateRenderProcessHost(
-        browser_context, current_partition, nullptr,
-        false /* is_for_guests_only */);
+        browser_context, nullptr /* storage_partition_impl */,
+        nullptr /* site_instance */, false /* is_for_guests_only */);
     spare_render_process_host_->AddObserver(this);
     spare_render_process_host_->Init();
   }
 
-  // If |partition| is null, this gets the default partition from the browser
-  // context.
   RenderProcessHost* MaybeTakeSpareRenderProcessHost(
       BrowserContext* browser_context,
-      StoragePartition* partition,
       SiteInstance* site_instance,
       bool is_for_guests_only) {
     // Give embedder a chance to disable using a spare RenderProcessHost for
@@ -601,21 +592,14 @@ class SpareRenderProcessHostManager : public RenderProcessHostObserver {
             browser_context, site_instance->GetSiteURL()))
       return nullptr;
 
-    if (spare_render_process_host_ &&
-        browser_context == matching_browser_context_ && !is_for_guests_only &&
-        !partition) {
-      // If the spare renderer matches for everything but possibly the storage
-      // partition, and the passed-in partition is null, get the default storage
-      // partition. If this is the case, the default storage partition will
-      // already have been created and there is no possibility of breaking tests
-      // by GetDefaultStoragePartition prematurely creating one.
-      partition =
-          BrowserContext::GetStoragePartition(browser_context, site_instance);
-    }
-
+    // Get the StoragePartition for |site_instance|.  Note that this might be
+    // different than the default StoragePartition for |browser_context|.
+    StoragePartition* site_storage =
+        BrowserContext::GetStoragePartition(browser_context, site_instance);
     if (!spare_render_process_host_ ||
-        browser_context != matching_browser_context_ ||
-        partition != matching_storage_partition_ || is_for_guests_only) {
+        browser_context != spare_render_process_host_->GetBrowserContext() ||
+        site_storage != spare_render_process_host_->GetStoragePartition() ||
+        is_for_guests_only) {
       // As a new RenderProcessHost will almost certainly be created, we cleanup
       // the non-matching one so as not to waste resources.
       CleanupSpareRenderProcessHost();
@@ -692,11 +676,6 @@ class SpareRenderProcessHostManager : public RenderProcessHostObserver {
   // all its instances; see g_all_hosts, above.
   RenderProcessHost* spare_render_process_host_ = nullptr;
 
-  // Used only to check if a creation request matches the spare, and not
-  // accessed.
-  const BrowserContext* matching_browser_context_ = nullptr;
-  const StoragePartition* matching_storage_partition_ = nullptr;
-
   DISALLOW_COPY_AND_ASSIGN(SpareRenderProcessHostManager);
 };
 
@@ -736,7 +715,7 @@ class DefaultSubframeProcessHostHolder : public base::SupportsUserData::Data,
     if (host_)
       return host_;
 
-    host_ = RenderProcessHostImpl::CreateOrUseSpareRenderProcessHost(
+    host_ = RenderProcessHostImpl::CreateRenderProcessHost(
         browser_context_, partition, site_instance,
         false /* is for guests only */);
     host_->SetIsNeverSuitableForReuse();
@@ -1351,18 +1330,15 @@ const unsigned int RenderProcessHostImpl::kMaxFrameDepthForPriority =
 // static
 RenderProcessHost* RenderProcessHostImpl::CreateOrUseSpareRenderProcessHost(
     BrowserContext* browser_context,
-    StoragePartitionImpl* storage_partition_impl,
     SiteInstance* site_instance,
     bool is_for_guests_only) {
   RenderProcessHost* render_process_host =
       g_spare_render_process_host_manager.Get().MaybeTakeSpareRenderProcessHost(
-          browser_context, storage_partition_impl, site_instance,
-          is_for_guests_only);
+          browser_context, site_instance, is_for_guests_only);
 
   if (!render_process_host) {
-    render_process_host =
-        CreateRenderProcessHost(browser_context, storage_partition_impl,
-                                site_instance, is_for_guests_only);
+    render_process_host = CreateRenderProcessHost(
+        browser_context, nullptr, site_instance, is_for_guests_only);
   }
 
   DCHECK(render_process_host);
@@ -3690,7 +3666,7 @@ RenderProcessHost* RenderProcessHostImpl::GetProcessHostForSiteInstance(
     // creating one here with GetStoragePartition() can run into cross-thread
     // issues as TestBrowserContext initialization is done on the main thread.
     render_process_host = CreateOrUseSpareRenderProcessHost(
-        browser_context, nullptr, site_instance, is_for_guests_only);
+        browser_context, site_instance, is_for_guests_only);
   }
 
   if (is_unmatched_service_worker) {

content/browser/renderer_host/render_process_host_impl.h

@@ -129,13 +129,10 @@ class CONTENT_EXPORT RenderProcessHostImpl
   static const unsigned int kMaxFrameDepthForPriority;
 
   // Use the spare RenderProcessHost if it exists, or create a new one. This
-  // should be the usual way to get a new RenderProcessHost.
-  // If |storage_partition_impl| is null, the default partition from the
-  // browser_context is used, using |site_instance| (for which a null value is
-  // legal).
+  // should be the usual way to get a new RenderProcessHost.  Default storage
+  // partition from the browser_context is always used.
   static RenderProcessHost* CreateOrUseSpareRenderProcessHost(
       BrowserContext* browser_context,
-      StoragePartitionImpl* storage_partition_impl,
       SiteInstance* site_instance,
       bool is_for_guests_only);
 

content/shell/browser/shell_browser_context.cc

@@ -185,7 +185,13 @@ ShellBrowserContext::CreateRequestContextForStoragePartition(
     bool in_memory,
     ProtocolHandlerMap* protocol_handlers,
     URLRequestInterceptorScopedVector request_interceptors) {
-  return nullptr;
+  scoped_refptr<ShellURLRequestContextGetter>& context_getter =
+      isolated_url_request_getters_[partition_path];
+  if (!context_getter) {
+    context_getter = CreateURLRequestContextGetter(
+        protocol_handlers, std::move(request_interceptors));
+  }
+  return context_getter.get();
 }
 
 net::URLRequestContextGetter*

content/shell/browser/shell_browser_context.h

@@ -128,6 +128,8 @@ class ShellBrowserContext : public BrowserContext {
   base::FilePath path_;
   BrowserPluginGuestManager* guest_manager_;
   scoped_refptr<ShellURLRequestContextGetter> url_request_getter_;
+  std::map<base::FilePath, scoped_refptr<ShellURLRequestContextGetter>>
+      isolated_url_request_getters_;
 
   DISALLOW_COPY_AND_ASSIGN(ShellBrowserContext);
 };