Convert from storing lowest BrowsingInstanceID to keeping all in a set.

At present ChildProcessSecurityPolicyImpl::SecurityState stores only a
single BrowsingInstanceID (the lowest) even though its associated
process may contain multiple BrowsingInstances. This may lead
CanAccessDataForOrigin to consider the wrong BrowsingInstance when
performing its checks for opt-in isolated origins.

This CL converts SecurityState to track an ordered set of all the
BrowsingInstanceIDs so that they can all be checked.
CanAccessDataForOrigin is modified to return 'true' if *any*
BrowsingInstance in the set would allow the access, otherwise it returns
'false', and logs the failure reasons for each of the BrowsingInstances.

This CL also includes delayed cleanup of BrowsingInstance state from
ChildProcessSecurityPolicy when BrowsingInstances are deleted. This
avoids memory leaks but may pose a small risk of renderer kills. We will
monitor the CanAccessDataForOrigin crash keys to see if such cases
occur in practice.

This is a reland of
https://chromium-review.googlesource.com/c/chromium/src/+/2446370.
The issues that led to https://crbug.com/1141877 are fixed in
https://chromium-review.googlesource.com/c/chromium/src/+/2523732
and include a test that becomes meaningful once this CL lands.

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

content/browser/browsing_instance.cc

@@ -214,7 +214,8 @@ BrowsingInstance::~BrowsingInstance() {
   // Remove any origin isolation opt-ins related to this instance.
   ChildProcessSecurityPolicyImpl* policy =
       ChildProcessSecurityPolicyImpl::GetInstance();
-  policy->RemoveOptInIsolatedOriginsForBrowsingInstance(isolation_context_);
+  policy->RemoveOptInIsolatedOriginsForBrowsingInstance(
+      isolation_context_.browsing_instance_id());
 }
 
 SiteInfo BrowsingInstance::ComputeSiteInfoForURL(

content/browser/child_process_security_policy_impl.h

@@ -364,9 +364,9 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl
                                  url::Origin* result);
 
   // Removes any origin isolation opt-in entries associated with the
-  // |isolation_context| of the BrowsingInstance.
+  // |browsing_instance_id| of the BrowsingInstance.
   void RemoveOptInIsolatedOriginsForBrowsingInstance(
-      const IsolationContext& isolation_context);
+      const BrowsingInstanceId& browsing_instance_id);
 
   // Registers |origin|'s isolation status with respect to the BrowsingInstance
   // associated with |isolation_context|. If it has already been registered,
@@ -606,6 +606,12 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl
                                     const url::Origin& origin,
                                     bool is_global_walk_or_frame_removal);
 
+  // Allows tests to modify the delay in cleaning up BrowsingInstanceIds. If the
+  // delay is set to zero, cleanup happens immediately.
+  void SetBrowsingInstanceCleanupDelayForTesting(int64_t delay_in_seconds) {
+    browsing_instance_cleanup_delay_in_seconds_ = delay_in_seconds;
+  }
+
  private:
   friend class ChildProcessSecurityPolicyInProcessBrowserTest;
   friend class ChildProcessSecurityPolicyTest;
@@ -784,6 +790,10 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl
   void RemoveProcessReferenceLocked(int child_id)
       EXCLUSIVE_LOCKS_REQUIRED(lock_);
 
+  // Internal helper for RemoveOptInIsolatedOriginsForBrowsingInstance().
+  void RemoveOptInIsolatedOriginsForBrowsingInstanceInternal(
+      const BrowsingInstanceId browsing_instance_id);
+
   // Creates the value to place in the "killed_process_origin_lock" crash key
   // based on the contents of |security_state|.
   static std::string GetKilledProcessOriginLock(
@@ -897,6 +907,11 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl
       origin_isolation_non_isolated_by_browsing_instance_
           GUARDED_BY(origins_isolation_opt_in_lock_);
 
+  // When we are notified a BrowsingInstance has destructed, delay cleanup by
+  // this amount to allow outstanding IO thread requests to complete. May be set
+  // to different values in tests.
+  int64_t browsing_instance_cleanup_delay_in_seconds_ = 10;
+
   DISALLOW_COPY_AND_ASSIGN(ChildProcessSecurityPolicyImpl);
 };
 

content/browser/renderer_host/navigation_entry_impl_unittest.cc

@@ -80,15 +80,17 @@ class NavigationEntryTest : public testing::Test {
 
   void TearDown() override {}
 
+ private:
+  BrowserTaskEnvironment task_environment_;
+  TestBrowserContext browser_context_;
+
  protected:
+  // Destructors for SiteInstances must run before |task_environment_| shuts
+  // down.
   std::unique_ptr<NavigationEntryImpl> entry1_;
   std::unique_ptr<NavigationEntryImpl> entry2_;
   // SiteInstances are deleted when their NavigationEntries are gone.
   scoped_refptr<SiteInstanceImpl> instance_;
-
- private:
-  BrowserTaskEnvironment task_environment_;
-  TestBrowserContext browser_context_;
 };
 
 // Test unique ID accessors

content/browser/renderer_host/render_frame_host_manager_browsertest.cc

@@ -9122,13 +9122,12 @@ IN_PROC_BROWSER_TEST_P(RenderFrameHostManagerNoSiteIsolationTest,
   // Both should use the same process.
   EXPECT_EQ(old_instance->GetProcess(), new_instance->GetProcess());
 
+  // Make sure the BrowsingInstanceId is cleaned up immediately.
+  ChildProcessSecurityPolicyImpl::GetInstance()
+      ->SetBrowsingInstanceCleanupDelayForTesting(0);
+
   // 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.

content/test/content_browser_test_utils_internal.cc

@@ -295,18 +295,33 @@ std::string FrameTreeVisualizer::GetName(SiteInstance* site_instance) {
 Shell* OpenPopup(const ToRenderFrameHost& opener,
                  const GURL& url,
                  const std::string& name) {
+  return OpenPopup(opener, url, name, "", true);
+}
+
+Shell* OpenPopup(const ToRenderFrameHost& opener,
+                 const GURL& url,
+                 const std::string& name,
+                 const std::string& features,
+                 bool expect_return_from_window_open) {
   TestNavigationObserver observer(url);
   observer.StartWatchingNewWebContents();
 
   ShellAddedObserver new_shell_observer;
   bool did_create_popup = false;
-  bool did_execute_script = ExecuteScriptAndExtractBool(
-      opener,
+  std::string popup_script =
       "window.domAutomationController.send("
-      "    !!window.open('" + url.spec() + "', '" + name + "'));",
-      &did_create_popup);
-  if (!did_execute_script || !did_create_popup)
+      "    !!window.open('" +
+      url.spec() + "', '" + name + "', '" + features + "'));";
+  bool did_execute_script =
+      ExecuteScriptAndExtractBool(opener, popup_script, &did_create_popup);
+
+  // Don't check the value of |did_create_popup| since there are valid reasons
+  // for it to be false, e.g. |features| specifies 'noopener', or 'noreferrer'
+  // or others.
+  if (!did_execute_script ||
+      !(did_create_popup || !expect_return_from_window_open)) {
     return nullptr;
+  }
 
   observer.Wait();
 

content/test/content_browser_test_utils_internal.h

@@ -104,10 +104,19 @@ class FrameTreeVisualizer {
 };
 
 // Uses window.open to open a popup from the frame |opener| with the specified
-// |url| and |name|.   Waits for the navigation to |url| to finish and then
-// returns the new popup's Shell.  Note that since this navigation to |url| is
-// renderer-initiated, it won't cause a process swap unless used in
-// --site-per-process mode.
+// |url|, |name| and window |features|. |expect_return_from_window_open| is used
+// to indicate if the caller expects window.open() to return a non-null value.
+// Waits for the navigation to |url| to finish and then returns the new popup's
+// Shell.  Note that since this navigation to |url| is renderer-initiated, it
+// won't cause a process swap unless used in --site-per-process mode.
+Shell* OpenPopup(const ToRenderFrameHost& opener,
+                 const GURL& url,
+                 const std::string& name,
+                 const std::string& features,
+                 bool expect_return_from_window_open);
+
+// Same as above, but with an empty |features| and
+// |expect_return_from_window_open| assumed to be true..
 Shell* OpenPopup(const ToRenderFrameHost& opener,
                  const GURL& url,
                  const std::string& name);