Fix test failing with kProcessSharingWithStrictSiteInstances enabled.

This change fixes a few tests that were failing when the "default
process" process model is enabled.

- Removed DCHECK from RemoteFrameData and removed the tests that
  verified the DCHECK fired. The default process model allows remote
  frames to be in the same process as a local frame so this check
  is overly strict.
- Fixed a few tests that needed origins isolated so that the default
  process mode would not place multiple sites in a single process
  when the test explicitly needed multiple processes.
- Made the ProcessSharingWithStrictSiteInstances feature flag available
  to some android tests so they could change their expectations when
  the flag was set. The expectations needed to be modified to reflect
  that the default process mode puts multiple sites in a single process.
- Changed SiteInstanceImpl::DoesSiteInfoForURLMatch() to use an exact
  SiteInfo comparison because COOPIsolationTest.SameOrigin would fail
  in default process mode with the "same principal" match.
- Added MayReuseAndIsSuitable() checks to
  SiteInstanceImpl::GetDefaultProcessIfUsable() and
  MaybeSetBrowsingInstanceDefaultProcess() because tests were failing
  because the default process was being used for unsuitable
  SiteInstances which were causing tests to crash.

Bug: 1158650
Change-Id: I7c5bea2739f2695ca2da2a02c42ee4cce822baa3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2792001
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Aaron Colwell <acolwell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#869538}

components/performance_manager/v8_memory/v8_context_tracker_internal.cc

@@ -99,8 +99,6 @@ RemoteFrameData::RemoteFrameData(ProcessData* process_data,
       execution_context_data_(execution_context_data) {
   DCHECK(process_data);
   DCHECK(execution_context_data);
-  // This and the ExecutionContext *must* be cross-process.
-  DCHECK_NE(process_data, execution_context_data->process_data());
   execution_context_data->SetRemoteFrameData(PassKey(), this);
 }
 

components/performance_manager/v8_memory/v8_context_tracker_internal_unittest.cc

@@ -103,18 +103,6 @@ TEST_F(V8ContextTrackerInternalDeathTest,
   data_store()->Pass(std::move(ec_data));
 }
 
-TEST_F(V8ContextTrackerInternalDeathTest, SameProcessRemoteFrameDataExplodes) {
-  auto* process_data = ProcessData::GetOrCreate(
-      static_cast<ProcessNodeImpl*>(mock_graph_.process.get()));
-  std::unique_ptr<ExecutionContextData> ec_data =
-      std::make_unique<ExecutionContextData>(
-          process_data, mock_graph_.frame->frame_token(), nullptr);
-  std::unique_ptr<RemoteFrameData> rf_data;
-  EXPECT_DCHECK_DEATH(
-      rf_data = std::make_unique<RemoteFrameData>(
-          process_data, blink::RemoteFrameToken(), ec_data.get()));
-}
-
 TEST_F(V8ContextTrackerInternalDeathTest, CrossProcessV8ContextDataExplodes) {
   auto* process_data = ProcessData::GetOrCreate(
       static_cast<ProcessNodeImpl*>(mock_graph_.process.get()));

components/performance_manager/v8_memory/v8_context_tracker_unittest.cc

@@ -158,30 +158,6 @@ TEST_F(V8ContextTrackerDeathTest, IframeAttributionDataForMainFrameExplodes) {
       GetFakeIframeAttributionDataPtr()));
 }
 
-TEST_F(V8ContextTrackerDeathTest, IframeAttributionDataForInProcessChildFrame) {
-  // Create a child of mock_graph.frame that is in the same process.
-  TestNodeWrapper<FrameNodeImpl> child2_frame(graph()->CreateFrameNodeAutoId(
-      mock_graph.process.get(), mock_graph.page.get(), mock_graph.frame.get(),
-      3));
-
-  // Trying to provide IFrameAttribution data via a RemoteFrameAttached
-  // notification should explode because |child2_frame| is in the same process
-  // as its parent.
-  EXPECT_DCHECK_DEATH(tracker->OnRemoteIframeAttachedForTesting(
-      child2_frame.get(), mock_graph.frame.get(), blink::RemoteFrameToken(),
-      GetFakeIframeAttributionDataPtr()));
-
-  // This should succeed because iframe data is provided.
-  tracker->OnV8ContextCreated(
-      ProcessNodeImpl::CreatePassKeyForTesting(), mock_graph.process.get(),
-      mojom::V8ContextDescription(
-          /* token */ kChildFrameMainWorld,
-          /* world_type */ mojom::V8ContextWorldType::kMain,
-          /* world_name */ base::nullopt,
-          /* execution_context_token */ child2_frame->frame_token()),
-      GetFakeIframeAttributionDataPtr());
-}
-
 TEST_F(V8ContextTrackerDeathTest,
        NoIframeAttributionDataForInProcessChildFrameExplodes) {
   // Create a child of mock_graph.frame that is in the same process.

content/browser/android/content_feature_list.cc

@@ -24,6 +24,7 @@ const base::Feature* kFeaturesExposedToJava[] = {
     &features::kBackgroundMediaRendererHasModerateBinding,
     &features::kBindingManagementWaiveCpu,
     &features::kExperimentalAccessibilityLabels,
+    &features::kProcessSharingWithStrictSiteInstances,
     &features::kWebAuth,
     &features::kWebBluetoothNewPermissionsBackend,
     &features::kWebNfc,

content/browser/renderer_host/render_process_host_browsertest.cc

@@ -1149,8 +1149,14 @@ IN_PROC_BROWSER_TEST_F(RenderProcessHostTest,
       base::BindRepeating(HandleHungBeacon, base::RepeatingClosure()));
   ASSERT_TRUE(embedded_test_server()->Start());
 
-  EXPECT_TRUE(NavigateToURL(
-      shell(), embedded_test_server()->GetURL("/send-beacon.html")));
+  const auto kTestUrl = embedded_test_server()->GetURL("/send-beacon.html");
+  if (IsIsolatedOriginRequiredToGuaranteeDedicatedProcess()) {
+    // Isolate host so that the first and second navigation are guaranteed to
+    // be in different processes.
+    IsolateOriginsForTesting(embedded_test_server(), shell()->web_contents(),
+                             {kTestUrl.host()});
+  }
+  EXPECT_TRUE(NavigateToURL(shell(), kTestUrl));
 
   RenderFrameHostImpl* rfh = static_cast<RenderFrameHostImpl*>(
       shell()->web_contents()->GetMainFrame());
@@ -1184,8 +1190,15 @@ IN_PROC_BROWSER_TEST_F(RenderProcessHostTest,
       base::BindRepeating(HandleHungBeacon, base::RepeatingClosure()));
   ASSERT_TRUE(embedded_test_server()->Start());
 
-  EXPECT_TRUE(NavigateToURL(
-      shell(), embedded_test_server()->GetURL("/fetch-keepalive.html")));
+  const auto kTestUrl = embedded_test_server()->GetURL("/fetch-keepalive.html");
+  if (IsIsolatedOriginRequiredToGuaranteeDedicatedProcess()) {
+    // Isolate host so that the first and second navigation are guaranteed to
+    // be in different processes.
+    IsolateOriginsForTesting(embedded_test_server(), shell()->web_contents(),
+                             {kTestUrl.host()});
+  }
+
+  EXPECT_TRUE(NavigateToURL(shell(), kTestUrl));
 
   RenderFrameHostImpl* rfh = static_cast<RenderFrameHostImpl*>(
       shell()->web_contents()->GetMainFrame());

content/browser/site_instance_impl.cc

@@ -235,6 +235,25 @@ bool SiteInfo::IsSamePrincipalWith(const SiteInfo& other) const {
   return MakeSecurityPrincipalKey(*this) == MakeSecurityPrincipalKey(other);
 }
 
+bool SiteInfo::IsExactMatch(const SiteInfo& other) const {
+  bool is_match = site_url_ == other.site_url_ &&
+                  process_lock_url_ == other.process_lock_url_ &&
+                  is_origin_keyed_ == other.is_origin_keyed_ &&
+                  coop_coep_cross_origin_isolated_info_ ==
+                      other.coop_coep_cross_origin_isolated_info_ &&
+                  is_guest_ == other.is_guest_ &&
+                  does_site_request_dedicated_process_for_coop_ ==
+                      other.does_site_request_dedicated_process_for_coop_;
+
+  if (is_match) {
+    // If all the fields match, then the "same principal" subset must also
+    // match. This is used to ensure these 2 methods stay in sync and all fields
+    // used by IsSamePrincipalWith() are used by this function.
+    DCHECK(IsSamePrincipalWith(other));
+  }
+  return is_match;
+}
+
 bool SiteInfo::operator==(const SiteInfo& other) const {
   return IsSamePrincipalWith(other);
 }
@@ -776,11 +795,15 @@ const IsolationContext& SiteInstanceImpl::GetIsolationContext() {
 
 RenderProcessHost* SiteInstanceImpl::GetDefaultProcessIfUsable() {
   if (!base::FeatureList::IsEnabled(
-          features::kProcessSharingWithStrictSiteInstances)) {
+          features::kProcessSharingWithStrictSiteInstances) ||
+      !browsing_instance_->default_process()) {
     return nullptr;
   }
-  if (RequiresDedicatedProcess())
+  if (RequiresDedicatedProcess() ||
+      !RenderProcessHostImpl::MayReuseAndIsSuitable(
+          browsing_instance_->default_process(), this)) {
     return nullptr;
+  }
   return browsing_instance_->default_process();
 }
 
@@ -809,7 +832,12 @@ void SiteInstanceImpl::MaybeSetBrowsingInstanceDefaultProcess() {
   if (!process_ || !has_site_ || RequiresDedicatedProcess())
     return;
   if (browsing_instance_->default_process()) {
-    DCHECK_EQ(process_, browsing_instance_->default_process());
+    if (RenderProcessHostImpl::MayReuseAndIsSuitable(
+            browsing_instance_->default_process(), this)) {
+      // Make sure the default process was actually used if it is appropriate
+      // for this SiteInstance.
+      DCHECK_EQ(process_, browsing_instance_->default_process());
+    }
     return;
   }
   browsing_instance_->SetDefaultProcess(process_);
@@ -1580,7 +1608,7 @@ bool SiteInstanceImpl::DoesSiteInfoForURLMatch(const UrlInfo& url_info) {
         GetCoopCoepCrossOriginIsolatedInfo());
   }
 
-  return site_info_ == site_info;
+  return site_info_.IsExactMatch(site_info);
 }
 
 void SiteInstanceImpl::PreventOptInOriginIsolation(

content/browser/site_instance_impl.h

@@ -288,6 +288,10 @@ class CONTENT_EXPORT SiteInfo {
   // implementation).
   bool IsSamePrincipalWith(const SiteInfo& other) const;
 
+  // Returns true if all fields in `other` match the corresponding fields in
+  // this object.
+  bool IsExactMatch(const SiteInfo& other) const;
+
   // Note: equality operators are defined in terms of IsSamePrincipalWith().
   bool operator==(const SiteInfo& other) const;
   bool operator!=(const SiteInfo& other) const;
@@ -736,7 +740,8 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance,
   const IsolationContext& GetIsolationContext();
 
   // If this SiteInstance doesn't require a dedicated process, this will return
-  // the BrowsingInstance's default process.
+  // the BrowsingInstance's default process if it is suitable for this
+  // SiteInstance.
   RenderProcessHost* GetDefaultProcessIfUsable();
 
   // Returns true if this object was constructed as a default site instance.

content/public/android/java/src/org/chromium/content_public/browser/ContentFeatureList.java

@@ -34,6 +34,9 @@ public static boolean isEnabled(String featureName) {
     public static final String EXPERIMENTAL_ACCESSIBILITY_LABELS =
             "ExperimentalAccessibilityLabels";
 
+    public static final String PROCESS_SHARING_WITH_STRICT_SITE_INSTANCES =
+            "ProcessSharingWithStrictSiteInstances";
+
     public static final String WEB_AUTH = "WebAuthentication";
 
     public static final String WEB_BLUETOOTH_NEW_PERMISSIONS_BACKEND =

content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherIntegrationTest.java

@@ -21,6 +21,7 @@
 import org.chromium.base.test.BaseJUnit4ClassRunner;
 import org.chromium.base.test.util.CriteriaHelper;
 import org.chromium.base.test.util.UrlUtils;
+import org.chromium.content_public.browser.ContentFeatureList;
 import org.chromium.content_public.browser.LoadUrlParams;
 import org.chromium.content_public.browser.NavigationController;
 import org.chromium.content_public.browser.test.util.TestCallbackHelperContainer;
@@ -136,9 +137,17 @@ public void run() {
         ChildProcessLauncherTestUtils.runOnLauncherThreadBlocking(new Runnable() {
             @Override
             public void run() {
-                Assert.assertEquals(2, connections.size());
-                // connections.get(0).didDropBothInitialAndImportantBindings();
-                connections.get(1).throwIfDroppedBothModerateAndStrongBinding();
+                if (ContentFeatureList.isEnabled(
+                            ContentFeatureList.PROCESS_SHARING_WITH_STRICT_SITE_INSTANCES)) {
+                    // If this feature is turned on all the URLs will use the same process.
+                    // Verify that the process has not lost its importance now that the
+                    // data: URL is also in the same process as the file: URLs.
+                    Assert.assertEquals(1, connections.size());
+                    connections.get(0).throwIfDroppedBothModerateAndStrongBinding();
+                } else {
+                    Assert.assertEquals(2, connections.size());
+                    connections.get(1).throwIfDroppedBothModerateAndStrongBinding();
+                }
             }
         });
     }
@@ -169,8 +178,21 @@ public void testIntentionalKillToFreeServiceSlot() throws Throwable {
         ChildProcessLauncherTestUtils.runOnLauncherThreadBlocking(new Runnable() {
             @Override
             public void run() {
-                Assert.assertEquals(2, connections.size());
-                Assert.assertTrue(connections.get(0).isKilledByUs());
+                if (ContentFeatureList.isEnabled(
+                            ContentFeatureList.PROCESS_SHARING_WITH_STRICT_SITE_INSTANCES)) {
+                    // If this feature is turned on all the URLs will use the same process
+                    // and this test will not observe any kills.
+                    Assert.assertEquals(1, connections.size());
+                    Assert.assertFalse(connections.get(0).isKilledByUs());
+                } else {
+                    // The file: URLs and data: URL are expected to be in different processes and
+                    // the data: URL is expected to kill the process used for the file: URLs.
+                    // Note: The default SiteInstance process model also follows this path because
+                    // file: URLs are not allowed in the default SiteInstance process while data:
+                    // URLs are.
+                    Assert.assertEquals(2, connections.size());
+                    Assert.assertTrue(connections.get(0).isKilledByUs());
+                }
             }
         });
     }

third_party/blink/web_tests/TestExpectations

@@ -115,8 +115,6 @@ crbug.com/901056 virtual/synchronous_html_parser/external/wpt/html/semantics/emb
 # TODO(lukasza, alexmos): Burn down this list.
 crbug.com/949003 http/tests/printing/cross-site-frame-scrolled.html [ Pass Failure ]
 crbug.com/949003 http/tests/printing/cross-site-frame.html [ Pass Failure ]
-crbug.com/949003 virtual/not-site-per-process/http/tests/printing/cross-site-frame-scrolled.html [ Pass ]
-crbug.com/949003 virtual/not-site-per-process/http/tests/printing/cross-site-frame.html [ Pass ]
 # ====== Site Isolation failures until here ======
 
 # ====== Oilpan-only failures from here ======