diff --git a/components/performance_manager/v8_memory/v8_context_tracker_internal.cc b/components/performance_manager/v8_memory/v8_context_tracker_internal.cc index 6f83a29bf6a301..bc8a0f2b27ecc9 100644 --- a/components/performance_manager/v8_memory/v8_context_tracker_internal.cc +++ b/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); } diff --git a/components/performance_manager/v8_memory/v8_context_tracker_internal_unittest.cc b/components/performance_manager/v8_memory/v8_context_tracker_internal_unittest.cc index d170901e726197..5066364e0a5563 100644 --- a/components/performance_manager/v8_memory/v8_context_tracker_internal_unittest.cc +++ b/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(mock_graph_.process.get())); - std::unique_ptr ec_data = - std::make_unique( - process_data, mock_graph_.frame->frame_token(), nullptr); - std::unique_ptr rf_data; - EXPECT_DCHECK_DEATH( - rf_data = std::make_unique( - process_data, blink::RemoteFrameToken(), ec_data.get())); -} - TEST_F(V8ContextTrackerInternalDeathTest, CrossProcessV8ContextDataExplodes) { auto* process_data = ProcessData::GetOrCreate( static_cast(mock_graph_.process.get())); diff --git a/components/performance_manager/v8_memory/v8_context_tracker_unittest.cc b/components/performance_manager/v8_memory/v8_context_tracker_unittest.cc index a0e5ca6bcc831c..7b681439eb0fa0 100644 --- a/components/performance_manager/v8_memory/v8_context_tracker_unittest.cc +++ b/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 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. diff --git a/content/browser/android/content_feature_list.cc b/content/browser/android/content_feature_list.cc index 7336ce13ff8b4e..9358847db4dea0 100644 --- a/content/browser/android/content_feature_list.cc +++ b/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, diff --git a/content/browser/renderer_host/render_process_host_browsertest.cc b/content/browser/renderer_host/render_process_host_browsertest.cc index dd954e9980896d..9dd103069b5b09 100644 --- a/content/browser/renderer_host/render_process_host_browsertest.cc +++ b/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( 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( shell()->web_contents()->GetMainFrame()); diff --git a/content/browser/site_instance_impl.cc b/content/browser/site_instance_impl.cc index 1b0740a7853d28..4dc300404357e7 100644 --- a/content/browser/site_instance_impl.cc +++ b/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( diff --git a/content/browser/site_instance_impl.h b/content/browser/site_instance_impl.h index ccfa849d290ff7..ef8b504b762d28 100644 --- a/content/browser/site_instance_impl.h +++ b/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. diff --git a/content/public/android/java/src/org/chromium/content_public/browser/ContentFeatureList.java b/content/public/android/java/src/org/chromium/content_public/browser/ContentFeatureList.java index f77a63306ac19c..f2c78f87f5135d 100644 --- a/content/public/android/java/src/org/chromium/content_public/browser/ContentFeatureList.java +++ b/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 = diff --git a/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherIntegrationTest.java b/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherIntegrationTest.java index 527626d7dcc354..cd97cdf1241776 100644 --- a/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherIntegrationTest.java +++ b/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()); + } } }); } diff --git a/third_party/blink/web_tests/TestExpectations b/third_party/blink/web_tests/TestExpectations index 38984724b63b01..de9245c1cb7a88 100644 --- a/third_party/blink/web_tests/TestExpectations +++ b/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 ======