Fix allow-any-site processes to track additional IsolationContexts.

Previously, when LockProcessIfNeeded() was called for SiteInstances
without a site, it only ensured that the resulting process has an
allow-any-site lock.  However, when LockProcessIfNeeded() is called on
the same process more than once, such as when it gets reused by
different BrowsingInstances with blank SiteInstances, this is
insufficient.  We also need to call IncludeIsolationContext()
to inform ChildProcessSecurityPolicy of new BrowsingInstances.

This is required for relanding https://crrev.com/c/2446370, where
ChildProcessSecurityPolicy will start keeping track of a list of
BrowsingInstances for each process, and not doing this would lead to
mismatched process locks and crashes.

Bug: 1135539
Change-Id: Ie7b5320cb5a2101d9685169cf6eba254178165fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2523732
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: James MacLean <wjmaclean@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#825510}

content/browser/renderer_host/render_frame_host_manager_browsertest.cc

@@ -9078,6 +9078,64 @@ IN_PROC_BROWSER_TEST_P(RenderFrameHostManagerTest,
   }
 }
 
+// Helper class to run tests without site isolation.
+class RenderFrameHostManagerNoSiteIsolationTest
+    : public RenderFrameHostManagerTest {
+ public:
+  RenderFrameHostManagerNoSiteIsolationTest() = default;
+  ~RenderFrameHostManagerNoSiteIsolationTest() override = default;
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitch(switches::kDisableSiteIsolation);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(RenderFrameHostManagerNoSiteIsolationTest);
+};
+
+// Ensure that when a process that allows any site gets reused by new
+// BrowsingInstances, ChildProcessSecurityPolicy gets notified about those new
+// BrowsingInstances.  Failure to do so will lead to a crash at commit time due
+// to mismatched process locks.  See https://crbug.com/1141877.
+IN_PROC_BROWSER_TEST_P(RenderFrameHostManagerNoSiteIsolationTest,
+                       IncludeIsolationContextInProcessThatAllowsAnySite) {
+  StartEmbeddedServer();
+  // Ensure we have one renderer process that's reused for everything.
+  RenderProcessHost::SetMaxRendererProcessCount(1);
+
+  // The test starts out with an initial window with a blank SiteInstance.
+  // Create a new window in a new BrowsingInstance and another blank
+  // SiteInstance.
+  Shell* shell2 =
+      Shell::CreateNewWindow(shell()->web_contents()->GetBrowserContext(),
+                             GURL(), nullptr, gfx::Size());
+  SiteInstanceImpl* old_instance = static_cast<SiteInstanceImpl*>(
+      shell()->web_contents()->GetMainFrame()->GetSiteInstance());
+  SiteInstanceImpl* new_instance = static_cast<SiteInstanceImpl*>(
+      shell2->web_contents()->GetMainFrame()->GetSiteInstance());
+  EXPECT_FALSE(old_instance->IsRelatedSiteInstance(new_instance));
+
+  // At this point, neither SiteInstance should have a site assigned.
+  EXPECT_FALSE(old_instance->HasSite());
+  EXPECT_FALSE(new_instance->HasSite());
+
+  // Both should use the same process.
+  EXPECT_EQ(old_instance->GetProcess(), new_instance->GetProcess());
+
+  // Close the test's initial window.  This should destroy the initial
+  // BrowsingInstance and remove it from ChildProcessSecurityPolicy.
+  //
+  // TODO(wjmaclean): Update this to handle timeouts once
+  // ChildProcessSecurityPolicy is updated to keep track of multiple
+  // BrowsingInstances per process, and BrowsingInstance removal uses a
+  // timeout.
+  shell()->Close();
+
+  // Navigate to a web URL in the second window.  This shouldn't crash.
+  GURL url(embedded_test_server()->GetURL("a.com", "/title1.html"));
+  ASSERT_TRUE(NavigateToURL(shell2->web_contents(), url));
+}
+
 INSTANTIATE_TEST_SUITE_P(All,
                          RenderFrameHostManagerTest,
                          testing::ValuesIn(RenderDocumentFeatureLevelValues()));
@@ -9107,4 +9165,8 @@ INSTANTIATE_TEST_SUITE_P(All,
 INSTANTIATE_TEST_SUITE_P(All,
                          RenderFrameHostManagerDefaultProcessTest,
                          testing::ValuesIn(RenderDocumentFeatureLevelValues()));
+INSTANTIATE_TEST_SUITE_P(All,
+                         RenderFrameHostManagerNoSiteIsolationTest,
+                         testing::ValuesIn(RenderDocumentFeatureLevelValues()));
+
 }  // namespace content

content/browser/site_instance_impl.cc

@@ -1434,15 +1434,21 @@ void SiteInstanceImpl::LockProcessIfNeeded() {
     CHECK(!process_lock.is_locked_to_site())
         << "A process that's already locked to " << process_lock.ToString()
         << " cannot be updated to a more permissive lock";
+    // Update the process lock state to signal that the process has been
+    // associated with a SiteInstance that is not locked to a site yet.  Note
+    // that even if the process lock is already set to a lock that allows any
+    // site, we still need to notify ChildProcessSecurityPolicy about the
+    // current SiteInstance's IsolationContext, so that the corresponding
+    // BrowsingInstance can be associated with |process_|.  See
+    // https://crbug.com/1135539.
     if (process_lock.is_invalid()) {
-      // Update the process lock state to signal that the process has been
-      // associated with a SiteInstance that is not locked to a site yet.
       auto new_process_lock =
           ProcessLock::CreateAllowAnySite(GetCoopCoepCrossOriginIsolatedInfo());
       process_->SetProcessLock(GetIsolationContext(), new_process_lock);
     } else {
       CHECK(process_lock.allows_any_site())
           << "Unexpected process lock " << process_lock.ToString();
+      policy->IncludeIsolationContext(process_->GetID(), GetIsolationContext());
     }
     return;
   }