0

COOP: restrict-properties 7/*: tokens update after navigation.

This patch is concerned with updating the blink::Page browsing context
group when a cross browsing context group navigation takes place. This
can take two forms:

- When a local frame navigates, the BrowsingContextGroupInfo is passed
directly to the CommitNavigationParams, for an immediate update.

- When a remote frame navigates in another process, the renderer gets
updated by a message from the PageBroadcast interface. This incurs an
inevitable small delay.

DETAILS

For the local frame update, the passed BrowsingContextGroupInfo is optional and is only set for top-level navigations to a different
browsing context group. It takes the following route:
- RenderFrameImpl::CommitNavigation
- WebLocalFrameImpl::CommitNavigation
- FrameLoader::CommitNavigation
- DocumentLoader::DocumentLoader
- DocumentLoader::CommitNavigation
- Page::UpdateBrowsingContextGroup

When the browser receives the information that a page has navigated to
another browsing context group, we use the PageBroadcast interface to
tell each renderer process holding a proxy of the navigated frame, that
the blink::Page is now in another browsing context group. This goes
through:
- Navigator::DidNavigate
- RenderFrameHostManager::ExecutePageBroadcastMethod
- WebViewImpl::UpdatePageBrowsingContextGroup
- Page::UpdateBrowsingContextGroup

Testing is done in the next patch with the
CoopRestrictPropertiesAccessBrowserTest suite.

Bug: 1221127
Change-Id: Ie6300caee1c3b3e92add97a002416c8e5eddf316
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4514696
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1152564}
This commit is contained in:
Arthur Hemery
2023-06-02 15:50:00 +00:00
committed by Chromium LUCI CQ
parent d81e47fbca
commit d51484b18b
19 changed files with 205 additions and 5 deletions

@ -3981,7 +3981,8 @@ NavigationControllerImpl::CreateNavigationRequestFromLoadParams(
base::flat_map<::blink::mojom::RuntimeFeatureState, bool>(),
/*fenced_frame_properties=*/absl::nullopt,
/*not_restored_reasons=*/nullptr,
/*load_with_storage_access=*/false);
/*load_with_storage_access=*/false,
/*browsing_context_group_info=*/absl::nullopt);
#if BUILDFLAG(IS_ANDROID)
if (ValidateDataURLAsString(params.data_url_as_string)) {
commit_params->data_url_as_string = params.data_url_as_string->data();

@ -59,6 +59,7 @@
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/public/common/frame/frame_policy.h"
#include "third_party/blink/public/common/page/browsing_context_group_info.h"
#include "third_party/blink/public/common/page_state/page_state.h"
#include "third_party/blink/public/common/tokens/tokens.h"
#include "third_party/blink/public/mojom/frame/frame_owner_properties.mojom.h"
@ -144,6 +145,12 @@ class MockPageBroadcast : public blink::mojom::PageBroadcast {
blink::mojom::RemoteMainFrameInterfacesPtr remote_main_frame_interfaces),
(override));
MOCK_METHOD(
void,
UpdatePageBrowsingContextGroup,
(const blink::BrowsingContextGroupInfo& browsing_context_group_info),
(override));
mojo::PendingAssociatedRemote<blink::mojom::PageBroadcast> GetRemote() {
return receiver_.BindNewEndpointAndPassDedicatedRemote();
}
@ -4661,6 +4668,53 @@ TEST_F(NavigationControllerTest, NavigationApiDisposedEntries) {
EXPECT_EQ(main_frame_disposed_keys[0], "3");
}
// Once instantiated, will insert `mock_page_broadcast` as the PageBroadcast on
// a newly created RenderViewHost. This is important for listening for the
// update to a RenderViewHost which was created for a proxy, as it swaps to a
// local frame in a different browsing context group. Note that this this will
// only work once, as MockPageBroadcast does not support multiple bindings.
class PageBroadcastMockInserter : public WebContentsObserver {
public:
explicit PageBroadcastMockInserter(
content::WebContents* web_contents,
testing::NiceMock<MockPageBroadcast>* mock_page_broadcast)
: WebContentsObserver(web_contents),
mock_page_broadcast_(mock_page_broadcast) {}
void RenderViewHostChanged(RenderViewHost* old_host,
RenderViewHost* new_host) override {
static_cast<TestRenderViewHost*>(new_host)->BindPageBroadcast(
mock_page_broadcast_->GetRemote());
}
private:
raw_ptr<testing::NiceMock<MockPageBroadcast>> mock_page_broadcast_;
};
// Test that navigations across browsing context groups trigger a page broadcast
// with up to date browsing context group information.
TEST_F(NavigationControllerTest, BrowsingContextGroupUpdate) {
const GURL url1("http://a/");
const GURL url2("chrome://ukm");
// Start on a first page.
NavigateAndCommit(url1);
SiteInstanceImpl* initial_instance = main_test_rfh()->GetSiteInstance();
// Setup the page broadcast expectations. We expect no call to be made, as the
// RenderViewHost for B will get its update through the local frame commit.
testing::NiceMock<MockPageBroadcast> mock_page_broadcast;
EXPECT_CALL(mock_page_broadcast, UpdatePageBrowsingContextGroup(testing::_))
.Times(0);
PageBroadcastMockInserter mock_inserter(contents(), &mock_page_broadcast);
// Navigate to a cross browsing context group page. The update function should
// not be called.
NavigateAndCommit(url2);
SiteInstanceImpl* final_instance = main_test_rfh()->GetSiteInstance();
EXPECT_FALSE(initial_instance->IsRelatedSiteInstance(final_instance));
}
class NavigationControllerFencedFrameTest : public NavigationControllerTest {
public:
NavigationControllerFencedFrameTest() {

@ -964,7 +964,8 @@ NavigationEntryImpl::ConstructCommitNavigationParams(
base::flat_map<::blink::mojom::RuntimeFeatureState, bool>(),
/*fenced_frame_properties=*/absl::nullopt,
/*not_restored_reasons=*/nullptr,
/*load_with_storage_access=*/false);
/*load_with_storage_access=*/false,
/*browsing_context_group_info=*/absl::nullopt);
#if BUILDFLAG(IS_ANDROID)
// `data_url_as_string` is saved in NavigationEntry but should only be used by
// main frames, because loadData* navigations can only happen on the main

@ -1318,7 +1318,8 @@ std::unique_ptr<NavigationRequest> NavigationRequest::CreateRendererInitiated(
base::flat_map<::blink::mojom::RuntimeFeatureState, bool>(),
/*fenced_frame_properties=*/absl::nullopt,
/*not_restored_reasons=*/nullptr,
/*load_with_storage_access=*/load_with_storage_access);
/*load_with_storage_access=*/load_with_storage_access,
/*browsing_context_group_info=*/absl::nullopt);
commit_params->navigation_timing->system_entropy_at_navigation_start =
SystemEntropyUtils::ComputeSystemEntropyForFrameTreeNode(
@ -1460,7 +1461,8 @@ NavigationRequest::CreateForSynchronousRendererCommit(
base::flat_map<::blink::mojom::RuntimeFeatureState, bool>(),
/*fenced_frame_properties=*/absl::nullopt,
/*not_restored_reasons=*/nullptr,
/*load_with_storage_access=*/false);
/*load_with_storage_access=*/false,
/*browsing_context_group_info=*/absl::nullopt);
blink::mojom::BeginNavigationParamsPtr begin_params =
blink::mojom::BeginNavigationParams::New();
std::unique_ptr<NavigationRequest> navigation_request(new NavigationRequest(

@ -615,6 +615,31 @@ void Navigator::DidNavigate(
site_instance->group());
}
// If this was the navigation of a top-level frame to another browsing context
// group, update the browsing context group in all the renderers that have a
// representation of this page. Do not update the page in the main frame's own
// process, as it was already updated during commit.
// TODO(https://crbug.com/1446696): See if that can be consolidated with other
// similar IPCs.
if (render_frame_host->is_main_frame() &&
navigation_request->browsing_context_group_swap().ShouldSwap()) {
SiteInstanceImpl* final_site_instance =
render_frame_host->GetSiteInstance();
blink::BrowsingContextGroupInfo browsing_context_group_info(
final_site_instance->browsing_instance_token(),
final_site_instance->coop_related_group_token());
frame_tree.root()->render_manager()->ExecutePageBroadcastMethod(
base::BindRepeating(
[](const blink::BrowsingContextGroupInfo& info,
RenderViewHostImpl* rvh) {
if (auto& broadcast = rvh->GetAssociatedPageBroadcast()) {
broadcast->UpdatePageBrowsingContextGroup(info);
}
},
browsing_context_group_info),
final_site_instance->group());
}
// Store some information for recording WebPlatform security metrics. These
// metrics depends on information present in the NavigationRequest. However
// they must be recorded after the NavigationRequest has been destroyed and

@ -13107,6 +13107,20 @@ void RenderFrameHostImpl::SendCommitNavigation(
// otherwise).
MaybeSendFencedFrameReportingBeacon(*navigation_request);
// If this commit is for a main frame in another browsing context group, warn
// the renderer that it should update the browsing context group information
// of the page if this frame successfully commits. Note that the
// BrowsingContextGroupInfo in the params should only be populated at commit
// time, and only in the case of a swap.
CHECK(!commit_params->browsing_context_group_info.has_value());
if (is_main_frame() &&
navigation_request->browsing_context_group_swap().ShouldSwap()) {
commit_params->browsing_context_group_info =
blink::BrowsingContextGroupInfo(
GetSiteInstance()->browsing_instance_token(),
GetSiteInstance()->coop_related_group_token());
}
commit_params->commit_sent = base::TimeTicks::Now();
navigation_client->CommitNavigation(
std::move(common_params), std::move(commit_params),
@ -13146,6 +13160,20 @@ void RenderFrameHostImpl::SendCommitFailedNavigation(
DCHECK_NE(GURL(), common_params->url);
DCHECK_NE(net::OK, error_code);
IncreaseCommitNavigationCounter();
// If this commit is for a main frame in another browsing context group, warn
// the renderer that it should update the browsing context group information
// of the page. Note that the BrowsingContextGroupInfo in the params should
// only be populated at commit time, and only in the case of a swap.
CHECK(!commit_params->browsing_context_group_info.has_value());
if (is_main_frame() &&
navigation_request->browsing_context_group_swap().ShouldSwap()) {
commit_params->browsing_context_group_info =
blink::BrowsingContextGroupInfo(
GetSiteInstance()->browsing_instance_token(),
GetSiteInstance()->coop_related_group_token());
}
navigation_client->CommitFailedNavigation(
std::move(common_params), std::move(commit_params),
has_stale_copy_in_cache, error_code, extended_error_code,

@ -1072,6 +1072,9 @@ void FillMiscNavigationParams(
navigation_params->ancestor_or_self_has_cspee =
commit_params.ancestor_or_self_has_cspee;
navigation_params->browsing_context_group_info =
commit_params.browsing_context_group_info;
}
std::string GetUniqueNameOfWebFrame(WebFrame* web_frame) {

@ -57,4 +57,7 @@ void TestPageBroadcast::CreateRemoteMainFrame(
blink::mojom::RemoteFrameInterfacesFromBrowserPtr remote_frame_interfaces,
blink::mojom::RemoteMainFrameInterfacesPtr remote_main_frame_interfaces) {}
void TestPageBroadcast::UpdatePageBrowsingContextGroup(
const blink::BrowsingContextGroupInfo& browsing_context_group_info) {}
} // namespace content

@ -43,6 +43,8 @@ class TestPageBroadcast : public blink::mojom::PageBroadcast {
blink::mojom::RemoteFrameInterfacesFromBrowserPtr remote_frame_interfaces,
blink::mojom::RemoteMainFrameInterfacesPtr remote_main_frame_interfaces)
override;
void UpdatePageBrowsingContextGroup(const blink::BrowsingContextGroupInfo&
browsing_context_group_info) override;
mojo::AssociatedReceiver<blink::mojom::PageBroadcast> receiver_;
};

@ -30,6 +30,7 @@ import "third_party/blink/public/mojom/navigation/navigation_policy.mojom";
import "third_party/blink/public/mojom/navigation/prefetched_signed_exchange_info.mojom";
import "third_party/blink/public/mojom/navigation/system_entropy.mojom";
import "third_party/blink/public/mojom/navigation/was_activated_option.mojom";
import "third_party/blink/public/mojom/page/browsing_context_group_info.mojom";
import "third_party/blink/public/mojom/page/page.mojom";
import "third_party/blink/public/mojom/permissions_policy/permissions_policy.mojom";
import "third_party/blink/public/mojom/runtime_feature_state/runtime_feature_state.mojom";
@ -594,4 +595,11 @@ struct CommitNavigationParams {
// Indicates whether the target document of this navigation should load with
// the `has_storage_access` bit set.
bool load_with_storage_access = false;
// Indicates which browsing context group this frame belongs to. This starts
// as nullopt and is only set when we commit a main frame in another browsing
// context group. Same browsing context group navigations never set this
// because no update is required. Subframes navigations never set this,
// because they cannot change browsing context group.
blink.mojom.BrowsingContextGroupInfo? browsing_context_group_info;
};

@ -10,6 +10,7 @@ import "third_party/blink/public/mojom/frame/frame_replication_state.mojom";
import "third_party/blink/public/mojom/frame/remote_frame.mojom";
import "third_party/blink/public/mojom/frame/view_transition_state.mojom";
import "third_party/blink/public/mojom/navigation/was_activated_option.mojom";
import "third_party/blink/public/mojom/page/browsing_context_group_info.mojom";
import "third_party/blink/public/mojom/page/page_visibility_state.mojom";
import "third_party/blink/public/mojom/tokens/tokens.mojom";
import "third_party/blink/public/mojom/webpreferences/web_preferences.mojom";
@ -149,4 +150,9 @@ interface PageBroadcast {
mojo_base.mojom.UnguessableToken devtools_frame_token,
RemoteFrameInterfacesFromBrowser remote_frame_interfaces,
RemoteMainFrameInterfaces remote_main_frame_interfaces);
// Update the page's browsing context group, with the information contained
// in `browsing_context_group_info`.
UpdatePageBrowsingContextGroup(
blink.mojom.BrowsingContextGroupInfo browsing_context_group_info);
};

@ -22,6 +22,7 @@
#include "third_party/blink/public/common/frame/frame_policy.h"
#include "third_party/blink/public/common/frame/view_transition_state.h"
#include "third_party/blink/public/common/navigation/impression.h"
#include "third_party/blink/public/common/page/browsing_context_group_info.h"
#include "third_party/blink/public/common/storage_key/storage_key.h"
#include "third_party/blink/public/common/tokens/tokens.h"
#include "third_party/blink/public/mojom/blob/blob_url_store.mojom-shared.h"
@ -538,6 +539,14 @@ struct BLINK_EXPORT WebNavigationParams {
// Whether the document should be loaded with the has_storage_access bit set.
bool load_with_storage_access = false;
// Indicates which browsing context group this frame belongs to. This starts
// as nullopt and is only set when we commit a main frame in another browsing
// context group. Same browsing context group navigations never set this
// because no update is required. Subframes navigations never set this,
// because they cannot change browsing context group.
absl::optional<BrowsingContextGroupInfo> browsing_context_group_info =
absl::nullopt;
};
} // namespace blink

@ -3944,4 +3944,12 @@ scheduler::WebAgentGroupScheduler& WebViewImpl::GetWebAgentGroupScheduler() {
return web_agent_group_scheduler_;
}
void WebViewImpl::UpdatePageBrowsingContextGroup(
const BrowsingContextGroupInfo& browsing_context_group_info) {
Page* page = GetPage();
CHECK(page);
page->UpdateBrowsingContextGroup(browsing_context_group_info);
}
} // namespace blink

@ -318,6 +318,8 @@ class CORE_EXPORT WebViewImpl final : public WebView,
mojom::blink::RemoteFrameInterfacesFromBrowserPtr remote_frame_interfaces,
mojom::blink::RemoteMainFrameInterfacesPtr remote_main_frame_interfaces)
override;
void UpdatePageBrowsingContextGroup(
const BrowsingContextGroupInfo& browsing_context_group_info) override;
void DispatchPersistedPageshow(base::TimeTicks navigation_start);
void DispatchPagehide(mojom::blink::PagehideDispatch pagehide_dispatch);

@ -52,6 +52,7 @@
#include "third_party/blink/public/common/client_hints/client_hints.h"
#include "third_party/blink/public/common/features.h"
#include "third_party/blink/public/common/metrics/accept_language_and_content_language_usage.h"
#include "third_party/blink/public/common/page/browsing_context_group_info.h"
#include "third_party/blink/public/common/permissions_policy/permissions_policy.h"
#include "third_party/blink/public/common/scheme_registry.h"
#include "third_party/blink/public/mojom/commit_result/commit_result.mojom-blink.h"
@ -320,6 +321,7 @@ struct SameSizeAsDocumentLoader
fenced_frame_properties;
bool has_storage_access;
mojom::blink::ParentResourceTimingAccess parent_resource_timing_access;
const absl::optional<BrowsingContextGroupInfo> browsing_context_group_info;
};
// Asserts size of DocumentLoader, so that whenever a new attribute is added to
@ -519,7 +521,8 @@ DocumentLoader::DocumentLoader(
reduced_accept_language_(params_->reduced_accept_language),
navigation_delivery_type_(params_->navigation_delivery_type),
view_transition_state_(std::move(params_->view_transition_state)),
load_with_storage_access_(params_->load_with_storage_access) {
load_with_storage_access_(params_->load_with_storage_access),
browsing_context_group_info_(params_->browsing_context_group_info) {
DCHECK(frame_);
DCHECK(params_);
@ -2608,6 +2611,16 @@ void DocumentLoader::CommitNavigation() {
if (commit_reason_ == CommitReason::kXSLT)
DocumentXSLT::SetHasTransformSource(*document);
// If we've received browsing context group information, update the Page's
// browsing context group. This can only ever happen for a top-level frame,
// because subframes can never change browsing context group, and the
// value is omitted by the browser process at commit time.
if (browsing_context_group_info_.has_value()) {
CHECK(frame_->IsMainFrame());
frame_->GetPage()->UpdateBrowsingContextGroup(
browsing_context_group_info_.value());
}
DidInstallNewDocument(document);
// This must be called before the document is opened, otherwise HTML parser

@ -819,6 +819,10 @@ class CORE_EXPORT DocumentLoader : public GarbageCollected<DocumentLoader>,
// Only container-initiated navigations (e.g. iframe change src) report
// their resource timing to the parent.
mojom::blink::ParentResourceTimingAccess parent_resource_timing_access_;
// Indicates which browsing context group this frame belongs to. It is only
// set for a main frame committing in another browsing context group.
const absl::optional<BrowsingContextGroupInfo> browsing_context_group_info_;
};
DECLARE_WEAK_IDENTIFIER_MAP(DocumentLoader);

@ -1156,6 +1156,11 @@ const base::UnguessableToken& Page::CoopRelatedGroupToken() {
return browsing_context_group_info_.coop_related_group_token;
}
void Page::UpdateBrowsingContextGroup(
const blink::BrowsingContextGroupInfo& browsing_context_group_info) {
browsing_context_group_info_ = browsing_context_group_info;
}
template class CORE_TEMPLATE_EXPORT Supplement<Page>;
const char InternalSettingsPageSupplementBase::kSupplementName[] =

@ -432,6 +432,10 @@ class CORE_EXPORT Page final : public GarbageCollected<Page>,
// in.
const base::UnguessableToken& CoopRelatedGroupToken();
// Update this Page's browsing context group after a navigation has taken
// place.
void UpdateBrowsingContextGroup(const blink::BrowsingContextGroupInfo&);
private:
friend class ScopedPagePauser;

@ -37,4 +37,26 @@ TEST(PageTest, CreateNonOrdinaryBrowsingContextGroup) {
EXPECT_NE(page->BrowsingContextGroupToken(), page->CoopRelatedGroupToken());
}
TEST(PageTest, BrowsingContextGroupUpdate) {
EmptyChromeClient client;
auto* scheduler = scheduler::CreateDummyAgentGroupScheduler();
auto initial_bcg_info = BrowsingContextGroupInfo::CreateUnique();
Page* page = Page::CreateOrdinary(client, /*opener=*/nullptr, *scheduler,
initial_bcg_info);
EXPECT_EQ(page->BrowsingContextGroupToken(),
initial_bcg_info.browsing_context_group_token);
EXPECT_EQ(page->CoopRelatedGroupToken(),
initial_bcg_info.coop_related_group_token);
auto updated_bcg_info = BrowsingContextGroupInfo::CreateUnique();
page->UpdateBrowsingContextGroup(updated_bcg_info);
EXPECT_EQ(page->BrowsingContextGroupToken(),
updated_bcg_info.browsing_context_group_token);
EXPECT_EQ(page->CoopRelatedGroupToken(),
updated_bcg_info.coop_related_group_token);
}
} // namespace blink