Skip to content

Commit

Permalink
[M103] MPArch: Adjust initial SiteInstance for Fenced Frames
Browse files Browse the repository at this point in the history
This CL makes MPArch fenced frames reuse the embedder's process for
the initial empty site instance, and only makes it use a different
process when it navigates cross-site (w.r.t the embedder).

(cherry picked from commit 0fd1546)

Change-Id: Id743dae8c7a695828adcbb962d7c90bedb72b262
Bug: 1326594
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3645368
Reviewed-by: Sophie Chang <sophiechang@chromium.org>
Reviewed-by: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Theodore Olsauskas-Warren <sauski@google.com>
Commit-Queue: Adithya Srinivasan <adithyas@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1009693}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3692391
Owners-Override: Krishna Govind <govind@chromium.org>
Reviewed-by: Krishna Govind <govind@chromium.org>
Commit-Queue: Krishna Govind <govind@chromium.org>
Cr-Commit-Position: refs/branch-heads/5060@{#651}
Cr-Branched-From: b83393d-refs/heads/main@{#1002911}
  • Loading branch information
a4sriniv authored and Chromium LUCI CQ committed Jun 7, 2022
1 parent 446f9d4 commit 591cbce
Show file tree
Hide file tree
Showing 14 changed files with 215 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -888,6 +888,7 @@ TEST_F(LoadingPredictorTabHelperTestCollectorFencedFramesTest,
content::NavigationSimulator::CreateForFencedFrame(fenced_frame_url,
fenced_frame_root);
navigation_simulator->Commit();
fenced_frame_root = navigation_simulator->GetFinalRenderFrameHost();

EXPECT_EQ(0u, test_collector_->count_resource_loads_completed());

Expand Down
6 changes: 4 additions & 2 deletions chrome/browser/site_isolation/site_details_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -875,6 +875,7 @@ class FencedFrameSiteDetailsBrowserTest : public InProcessBrowserTest {
const FencedFrameSiteDetailsBrowserTest&) = delete;

void SetUpOnMainThread() override {
host_resolver()->AddRule("*", "127.0.0.1");
ASSERT_TRUE(embedded_test_server()->Start());
}

Expand All @@ -892,12 +893,13 @@ class FencedFrameSiteDetailsBrowserTest : public InProcessBrowserTest {

IN_PROC_BROWSER_TEST_F(FencedFrameSiteDetailsBrowserTest,
MemoryDetailsForFencedFrame) {
auto initial_url = embedded_test_server()->GetURL("/empty.html");
content::IsolateAllSitesForTesting(base::CommandLine::ForCurrentProcess());
auto initial_url = embedded_test_server()->GetURL("a.com", "/empty.html");
ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), initial_url));

// Load a fenced frame.
GURL fenced_frame_url =
embedded_test_server()->GetURL("/fenced_frames/iframe.html");
embedded_test_server()->GetURL("b.com", "/fenced_frames/iframe.html");
content::RenderFrameHost* fenced_frame_host =
fenced_frame_test_helper().CreateFencedFrame(
web_contents()->GetMainFrame(), fenced_frame_url);
Expand Down
1 change: 1 addition & 0 deletions chrome/browser/ui/hats/hats_helper_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -108,5 +108,6 @@ TEST_F(HatsHelperFencedFrameTest,
content::NavigationSimulator::CreateForFencedFrame(fenced_frame_url,
fenced_frame_rfh);
navigation_simulator->Commit();
fenced_frame_rfh = navigation_simulator->GetFinalRenderFrameHost();
EXPECT_TRUE(fenced_frame_rfh->IsFencedFrameRoot());
}
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,7 @@ TEST_F(SafeBrowsingTriggeredPopupBlockerFencedFrameTest,
content::NavigationSimulator::CreateForFencedFrame(fenced_frame_url,
fenced_frame_root);
navigation_simulator->Commit();
fenced_frame_root = navigation_simulator->GetFinalRenderFrameHost();

// The popup blocker is not triggered for a fenced frame.
EXPECT_FALSE(popup_blocker()->ShouldApplyAbusivePopupBlocker(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,7 @@ TEST_F(ContentPasswordManagerDriverFencedFramesTest,
content::NavigationSimulator::CreateForFencedFrame(fenced_frame_url,
fenced_frame_root);
navigation_simulator->Commit();
fenced_frame_root = navigation_simulator->GetFinalRenderFrameHost();

autofill::FormData initial_form;
autofill::FormData form_in_fenced_frame =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ TEST_F(SecurityInterstitialTabHelperFencedFrameTest,
content::NavigationSimulator::CreateForFencedFrame(fenced_frame_url,
fenced_frame_rfh);
navigation_simulator->Commit();
fenced_frame_rfh = navigation_simulator->GetFinalRenderFrameHost();
EXPECT_TRUE(fenced_frame_rfh->IsFencedFrameRoot());
EXPECT_FALSE(blocking_page_destroyed);
EXPECT_TRUE(helper->IsDisplayingInterstitial());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1749,6 +1749,7 @@ TEST_P(ContentSubresourceFilterThrottleManagerFencedFrameTest,
throttle_manager);

navigation_simulator()->Commit();
fenced_frame_root = navigation_simulator()->GetFinalRenderFrameHost();

// Committing the fenced frame navigation should not change the Page's
// throttle manager.
Expand Down
14 changes: 4 additions & 10 deletions content/browser/fenced_frame/fenced_frame.cc
Original file line number Diff line number Diff line change
Expand Up @@ -54,16 +54,10 @@ FencedFrame::FencedFrame(
/*page_delegate=*/web_contents_,
FrameTree::Type::kFencedFrame)),
mode_(mode) {
scoped_refptr<SiteInstance> site_instance;
if (owner_render_frame_host->GetSiteInstance()->IsGuest()) {
site_instance = SiteInstance::CreateForGuest(
owner_render_frame_host->GetBrowserContext(),
owner_render_frame_host->GetSiteInstance()
->GetStoragePartitionConfig());
} else {
site_instance =
SiteInstance::Create(owner_render_frame_host->GetBrowserContext());
}
scoped_refptr<SiteInstance> site_instance =
SiteInstanceImpl::CreateForFencedFrame(
owner_render_frame_host->GetSiteInstance());

// Note that even though this is happening in response to an event in the
// renderer (i.e., the creation of a <fencedframe> element), we are still
// putting `renderer_initiated_creation` as false. This is because that
Expand Down
162 changes: 162 additions & 0 deletions content/browser/fenced_frame/fenced_frame_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "content/public/browser/frame_type.h"
#include "content/public/browser/navigation_handle.h"
#include "content/public/common/content_features.h"
#include "content/public/common/content_switches.h"
#include "content/public/test/browser_test.h"
#include "content/public/test/browser_test_utils.h"
#include "content/public/test/content_browser_test.h"
Expand Down Expand Up @@ -945,6 +946,167 @@ IN_PROC_BROWSER_TEST_F(FencedFrameBrowserTest, UserInteractionForFencedFrame) {
fenced_frame_rfh->GetRenderWidgetHost()->ForwardMouseEvent(mouse_event);
}

IN_PROC_BROWSER_TEST_F(FencedFrameBrowserTest,
ProcessAllocationWithFullSiteIsolation) {
ASSERT_TRUE(https_server()->Start());
IsolateAllSitesForTesting(base::CommandLine::ForCurrentProcess());
ASSERT_TRUE(AreAllSitesIsolatedForTesting());

const GURL main_url = https_server()->GetURL("a.test", "/title1.html");
const GURL same_site_fenced_frame_url =
https_server()->GetURL("a.test", "/fenced_frames/title1.html");
const GURL cross_site_fenced_frame_url =
https_server()->GetURL("b.test", "/fenced_frames/title1.html");

EXPECT_TRUE(NavigateToURL(shell(), main_url));

// Empty fenced frame document should have a different site instance, but
// should be in the same process as embedder.
RenderFrameHost* fenced_frame_rfh =
fenced_frame_test_helper().CreateFencedFrame(primary_main_frame_host(),
GURL());
EXPECT_NE(fenced_frame_rfh->GetSiteInstance(),
primary_main_frame_host()->GetSiteInstance());
EXPECT_FALSE(fenced_frame_rfh->GetSiteInstance()->IsRelatedSiteInstance(
primary_main_frame_host()->GetSiteInstance()));
EXPECT_EQ(fenced_frame_rfh->GetProcess(),
primary_main_frame_host()->GetProcess());

// Same-site fenced frame document should be in the same process as embedder.
fenced_frame_rfh = fenced_frame_test_helper().NavigateFrameInFencedFrameTree(
fenced_frame_rfh, same_site_fenced_frame_url);
EXPECT_NE(fenced_frame_rfh->GetSiteInstance(),
primary_main_frame_host()->GetSiteInstance());
EXPECT_FALSE(fenced_frame_rfh->GetSiteInstance()->IsRelatedSiteInstance(
primary_main_frame_host()->GetSiteInstance()));
EXPECT_EQ(fenced_frame_rfh->GetProcess(),
primary_main_frame_host()->GetProcess());

// Cross-site fenced frame document should be in a different process from its
// embedder.
fenced_frame_rfh = fenced_frame_test_helper().NavigateFrameInFencedFrameTree(
fenced_frame_rfh, cross_site_fenced_frame_url);
EXPECT_NE(fenced_frame_rfh->GetProcess(),
primary_main_frame_host()->GetProcess());
}

IN_PROC_BROWSER_TEST_F(FencedFrameBrowserTest,
CrossSiteFencedFramesShareProcess) {
ASSERT_TRUE(https_server()->Start());
IsolateAllSitesForTesting(base::CommandLine::ForCurrentProcess());
ASSERT_TRUE(AreAllSitesIsolatedForTesting());

const GURL main_url = https_server()->GetURL("a.test", "/title1.html");
const GURL same_site_fenced_frame_url =
https_server()->GetURL("a.test", "/fenced_frames/title1.html");
const GURL cross_site_fenced_frame_url =
https_server()->GetURL("b.test", "/fenced_frames/title1.html");

EXPECT_TRUE(NavigateToURL(shell(), main_url));

// Two fenced frames that are same-site with each other, and cross-site with
// the embedder should be in the same process. This happens due to the
// subframe process reuse policy which also applies to fenced frames (the
// second fenced frame will try to reuse an existing process that is locked to
// the same site).
RenderFrameHost* ff_rfh_1 = fenced_frame_test_helper().CreateFencedFrame(
primary_main_frame_host(), cross_site_fenced_frame_url);
RenderFrameHost* ff_rfh_2 = fenced_frame_test_helper().CreateFencedFrame(
primary_main_frame_host(), cross_site_fenced_frame_url);
EXPECT_NE(ff_rfh_1->GetSiteInstance(), ff_rfh_2->GetSiteInstance());
EXPECT_NE(ff_rfh_1->GetProcess(), primary_main_frame_host()->GetProcess());
EXPECT_EQ(ff_rfh_1->GetProcess(), ff_rfh_2->GetProcess());

// The cross-site fenced frame should be moved to the same process as embedder
// when navigated to same-site (similar to before).
ff_rfh_2 = fenced_frame_test_helper().NavigateFrameInFencedFrameTree(
ff_rfh_2, same_site_fenced_frame_url);
EXPECT_EQ(ff_rfh_2->GetProcess(), primary_main_frame_host()->GetProcess());
}

class FencedFrameWithSiteIsolationDisabledBrowserTest
: public FencedFrameBrowserTest {
public:
FencedFrameWithSiteIsolationDisabledBrowserTest() = default;
~FencedFrameWithSiteIsolationDisabledBrowserTest() override = default;

void SetUpCommandLine(base::CommandLine* command_line) override {
FencedFrameBrowserTest::SetUpCommandLine(command_line);
command_line->AppendSwitch(switches::kDisableSiteIsolation);
}
};

IN_PROC_BROWSER_TEST_F(FencedFrameWithSiteIsolationDisabledBrowserTest,
ProcessAllocationWithSiteIsolationDisabled) {
ASSERT_TRUE(https_server()->Start());
if (AreAllSitesIsolatedForTesting()) {
LOG(ERROR) << "Site isolation should be disabled for this test.";
return;
}

const GURL main_url = https_server()->GetURL("a.test", "/title1.html");
const GURL same_site_fenced_frame_url =
https_server()->GetURL("a.test", "/fenced_frames/title1.html");
const GURL cross_site_fenced_frame_url =
https_server()->GetURL("b.test", "/fenced_frames/title1.html");

EXPECT_TRUE(NavigateToURL(shell(), main_url));

// Empty fenced frame document should be in the same process as embedder.
RenderFrameHost* fenced_frame_rfh =
fenced_frame_test_helper().CreateFencedFrame(primary_main_frame_host(),
GURL());
EXPECT_NE(fenced_frame_rfh->GetSiteInstance(),
primary_main_frame_host()->GetSiteInstance());
EXPECT_EQ(fenced_frame_rfh->GetProcess(),
primary_main_frame_host()->GetProcess());

// Same-site fenced frame document should be in the same process as embedder.
fenced_frame_rfh = fenced_frame_test_helper().NavigateFrameInFencedFrameTree(
fenced_frame_rfh, same_site_fenced_frame_url);
EXPECT_EQ(fenced_frame_rfh->GetProcess(),
primary_main_frame_host()->GetProcess());

// Cross-site fenced frame document should be in the same process as the
// embedder (with site isolation disabled).
fenced_frame_rfh = fenced_frame_test_helper().NavigateFrameInFencedFrameTree(
fenced_frame_rfh, cross_site_fenced_frame_url);
EXPECT_EQ(fenced_frame_rfh->GetProcess(),
primary_main_frame_host()->GetProcess());
}

IN_PROC_BROWSER_TEST_F(FencedFrameWithSiteIsolationDisabledBrowserTest,
ProcessAllocationWithDynamicIsolatedOrigin) {
ASSERT_TRUE(https_server()->Start());
if (AreAllSitesIsolatedForTesting()) {
LOG(ERROR) << "Site isolation should be disabled for this test.";
return;
}
const GURL main_url = https_server()->GetURL("a.test", "/title1.html");
const GURL isolated_cross_site_fenced_frame_url =
https_server()->GetURL("isolated.b.test", "/fenced_frames/title1.html");
const GURL cross_site_fenced_frame_url =
https_server()->GetURL("c.test", "/fenced_frames/title1.html");

EXPECT_TRUE(NavigateToURL(shell(), main_url));

// Start isolating "isolated.b.test".
auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
policy->AddFutureIsolatedOrigins(
{url::Origin::Create(isolated_cross_site_fenced_frame_url)},
ChildProcessSecurityPolicy::IsolatedOriginSource::TEST);

RenderFrameHost* ff_rfh_1 = fenced_frame_test_helper().CreateFencedFrame(
primary_main_frame_host(), cross_site_fenced_frame_url);
RenderFrameHost* ff_rfh_2 = fenced_frame_test_helper().CreateFencedFrame(
primary_main_frame_host(), isolated_cross_site_fenced_frame_url);

// The c.test fenced frame should share a process with the embedder, but
// the isolated.b.test fenced frame should be in a different process.
EXPECT_EQ(ff_rfh_1->GetProcess(), primary_main_frame_host()->GetProcess());
EXPECT_NE(ff_rfh_2->GetProcess(), ff_rfh_1->GetProcess());
}

namespace {

enum class FrameTypeWithOrigin {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4558,6 +4558,8 @@ TEST_F(NavigationControllerFencedFrameTest, NoURLRewriteForFencedFrames) {
std::unique_ptr<NavigationSimulator> navigation_simulator =
NavigationSimulator::CreateForFencedFrame(kUrl2, fenced_frame_root);
navigation_simulator->Commit();
fenced_frame_root = static_cast<RenderFrameHostImpl*>(
navigation_simulator->GetFinalRenderFrameHost());

// Simulate the fenced frame receiving a request from a RenderFrameProxyHost
// to navigate to `kTestRewriteURL`.
Expand Down
9 changes: 5 additions & 4 deletions content/browser/renderer_host/render_frame_host_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2798,14 +2798,15 @@ void RenderFrameHostImpl::InitializePolicyContainerHost(
}

// The initial empty document inherits its policy container from its creator.
// The creator is either its parent for iframes or its opener for new windows.
// The creator is either its parent for subframes and embedded frames, or its
// opener for new windows.
//
// Note 1: For normal document created from a navigation, the policy container
// is computed from the NavigationRequest and assigned in
// DidCommitNewDocument().

if (parent_) {
SetPolicyContainerHost(parent_->policy_container_host()->Clone());
if (GetParentOrOuterDocument()) {
SetPolicyContainerHost(
GetParentOrOuterDocument()->policy_container_host()->Clone());
} else if (frame_tree_node_->opener()) {
SetPolicyContainerHost(frame_tree_node_->opener()
->current_frame_host()
Expand Down
28 changes: 28 additions & 0 deletions content/browser/site_instance_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,34 @@ scoped_refptr<SiteInstanceImpl> SiteInstanceImpl::CreateForGuest(
return site_instance;
}

// static
scoped_refptr<SiteInstanceImpl> SiteInstanceImpl::CreateForFencedFrame(
SiteInstanceImpl* embedder_site_instance) {
DCHECK(embedder_site_instance);
BrowserContext* browser_context = embedder_site_instance->GetBrowserContext();

if (embedder_site_instance->IsGuest()) {
return CreateForGuest(browser_context,
embedder_site_instance->GetStoragePartitionConfig());
}

// Give the new fenced frame SiteInstance the same site url as its embedder's
// SiteInstance to allow it to reuse its embedder's process. We avoid doing
// this in the default SiteInstance case as the url will be invalid; process
// reuse will still happen below though, as the embedder's SiteInstance's
// process will not be locked to any site.
scoped_refptr<SiteInstanceImpl> site_instance =
base::WrapRefCounted(new SiteInstanceImpl(new BrowsingInstance(
browser_context, embedder_site_instance->GetWebExposedIsolationInfo(),
embedder_site_instance->IsGuest())));
if (!embedder_site_instance->IsDefaultSiteInstance()) {
site_instance->SetSite(embedder_site_instance->GetSiteInfo());
}
site_instance->ReuseCurrentProcessIfPossible(
embedder_site_instance->GetProcess());
return site_instance;
}

// static
scoped_refptr<SiteInstanceImpl>
SiteInstanceImpl::CreateReusableInstanceForTesting(
Expand Down
2 changes: 2 additions & 0 deletions content/browser/site_instance_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance {
static scoped_refptr<SiteInstanceImpl> CreateForGuest(
BrowserContext* browser_context,
const StoragePartitionConfig& partition_config);
static scoped_refptr<SiteInstanceImpl> CreateForFencedFrame(
SiteInstanceImpl* embedder_site_instance);

// Similar to above, but creates an appropriate SiteInstance in a new
// BrowsingInstance for a particular `url_info`. This is a more generic
Expand Down
2 changes: 2 additions & 0 deletions content/public/test/fenced_frame_test_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ RenderFrameHost* FencedFrameTestHelper::CreateFencedFrame(
// frame if the removal-and-stop-loading scenario is a useful one to test.
EXPECT_EQ(previous_fenced_frame_count + 1,
fenced_frame_parent_rfh->GetFencedFrames().size());
if (url.is_empty())
return fenced_frame->GetInnerRoot();
return NavigateFrameInFencedFrameTree(fenced_frame->GetInnerRoot(), url,
expected_error_code);
}
Expand Down

0 comments on commit 591cbce

Please sign in to comment.