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.

Bug: 1135539
Change-Id: Icb7d25a84601b378c0c495a19266cd013667136a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2446370Reviewed-by: Jesse Doherty <jwd@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Aaron Colwell <acolwell@chromium.org>
Commit-Queue: James MacLean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/master@{#819830}

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,
@@ -784,6 +784,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(

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/test/content_browser_test_utils_internal.cc

@@ -296,18 +296,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

@@ -101,10 +101,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);

tools/metrics/histograms/histograms_xml/others/histograms.xml

@@ -13428,6 +13428,17 @@ should be kept until we remove incident reporting. -->
   </summary>
 </histogram>
 
+<histogram name="SiteIsolation.BrowsingInstance.MaxCountPerProcess"
+    units="units" expires_after="2021-10-15">
+  <owner>wjmaclean@chromium.org</owner>
+  <owner>creis@chromium.org</owner>
+  <summary>
+    The maximum number of BrowsingInstances seen in a RenderProcessHost over its
+    lifetime. Recorded once when
+    ChildProcessImpl::SecurityState::~SecurityState() is invoked.
+  </summary>
+</histogram>
+
 <histogram name="SiteIsolation.BrowsingInstanceCount" units="units"
     expires_after="2021-09-30">
   <owner>alexmos@chromium.org</owner>