Skip to content

Commit

Permalink
Hook up about:srcdoc to use initiator_base_url plumbing.
Browse files Browse the repository at this point in the history 
The changes in this CL are guarded behind the NewBaseUrlInheritanceBehavior feature.

This CL (mostly) re-implements
https://chromium-review.googlesource.com/c/chromium/src/+/3792049
using the initiator_base_url framework landed in
https://chromium-review.googlesource.com/c/chromium/src/+/4026883.
One notable change is that if a srcdoc reloads itself using
location.reload(), this CL uses the base url of the srcdoc
frame instead of the parent's, since the srcdoc frame is itself the
initiator of the reload.

This CL also removes the machinery used to track each frame's
baseurl, introduced in CL 3792049. Instead, the initiator base url
for about:blank and about:srcdoc frames is stored on their
RenderFrameHostImpls in case it's needed later.

Change-Id: Ia774332d08d94f85d8c570ab03ad311d8018456d
Bug: 1356658, 751329
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4032246
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1096262}
  • Loading branch information
W. James MacLean authored and Chromium LUCI CQ committed Jan 24, 2023
1 parent 4d866ae commit 78e2f87
Show file tree
Hide file tree
Showing 20 changed files with 198 additions and 328 deletions.
2 changes: 1 addition & 1 deletion content/browser/portal/portal.cc
View file
Expand Up @@ -287,7 +287,7 @@ void Portal::Navigate(const GURL& url,
portal_frame, out_validated_url, &frame_token,
owner_render_frame_host_->GetProcess()->GetID(),
owner_render_frame_host_->GetLastCommittedOrigin(),
/* initiator_base_url= */ GURL::EmptyGURL(),
owner_render_frame_host_->GetInheritedBaseUrl(),
owner_render_frame_host_->GetSiteInstance(),
mojo::ConvertTo<Referrer>(referrer), ui::PAGE_TRANSITION_LINK,
should_replace_entry, download_policy, "GET", nullptr, "", nullptr,
Expand Down
7 changes: 1 addition & 6 deletions content/browser/renderer_host/navigation_controller_impl.cc
View file
Expand Up @@ -1873,13 +1873,9 @@ void NavigationControllerImpl::CreateInitialEntry() {
params->referrer = blink::mojom::Referrer::New();

// Create and insert the initial NavigationEntry.
// TODO(https://crbug.com/1356658): in the follow-on CL enabling srcdoc base
// urls, change `temporary_initiator_base_url` to the inherited base_url
// stored on RFHI at the same time that GetLastCommittedOrigin() is set.
absl::optional<GURL> temporary_initiator_base_url;
auto new_entry = std::make_unique<NavigationEntryImpl>(
rfh->GetSiteInstance(), params->url, Referrer(*params->referrer),
rfh->GetLastCommittedOrigin(), temporary_initiator_base_url,
rfh->GetLastCommittedOrigin(), rfh->GetInheritedBaseUrl(),
std::u16string() /* title */, ui::PAGE_TRANSITION_TYPED,
false /* renderer_initiated */, nullptr /* blob_url_loader_factory */,
true /* is_initial_entry */);
Expand Down Expand Up @@ -3846,7 +3842,6 @@ NavigationControllerImpl::CreateNavigationRequestFromLoadParams(
/*early_hints_preloaded_resources=*/std::vector<GURL>(),
// This timestamp will be populated when the commit IPC is sent.
/*commit_sent=*/base::TimeTicks(), /*srcdoc_value=*/std::string(),
/*fallback_srcdoc_baseurl_value=*/GURL(),
/*should_load_data_url=*/false,
/*ancestor_or_self_has_cspee=*/node->AncestorOrSelfHasCSPEE(),
/*reduced_accept_language=*/std::string(),
Expand Down
1 change: 0 additions & 1 deletion content/browser/renderer_host/navigation_entry_impl.cc
View file
Expand Up @@ -929,7 +929,6 @@ NavigationEntryImpl::ConstructCommitNavigationParams(
std::vector<GURL>() /* early_hints_preloaded_resources */,
// This timestamp will be populated when the commit IPC is sent.
base::TimeTicks() /* commit_sent */, std::string() /* srcdoc_value */,
GURL() /* fallback_srcdoc_baseurl */,
false /* should_load_data_url */, ancestor_or_self_has_cspee,
std::string() /* reduced_accept_language */,
/*navigation_delivery_type=*/
Expand Down
19 changes: 3 additions & 16 deletions content/browser/renderer_host/navigation_request.cc
View file
Expand Up @@ -1298,7 +1298,6 @@ std::unique_ptr<NavigationRequest> NavigationRequest::CreateRendererInitiated(
/*early_hints_preloaded_resources=*/std::vector<GURL>(),
// This timestamp will be populated when the commit IPC is sent.
/*commit_sent=*/base::TimeTicks(), /*srcdoc_value=*/std::string(),
/*fallback_srcdoc_baseurl=*/GURL(),
/*should_load_data_url=*/false,
/*ancestor_or_self_has_cspee=*/
frame_tree_node->AncestorOrSelfHasCSPEE(),
Expand Down Expand Up @@ -1442,7 +1441,6 @@ NavigationRequest::CreateForSynchronousRendererCommit(
/*early_hints_preloaded_resources=*/std::vector<GURL>(),
// This timestamp will be populated when the commit IPC is sent.
/*commit_sent=*/base::TimeTicks(), /*srcdoc_value=*/std::string(),
/*fallback_srcdoc_baseurl=*/GURL(),
/*should_load_data_url=*/false,
/*ancestor_or_self_has_cspee=*/
frame_tree_node->AncestorOrSelfHasCSPEE(),
Expand Down Expand Up @@ -1903,20 +1901,6 @@ NavigationRequest::NavigationRequest(
}
}

// For navigations that inherit a base URL, snapshot the parent's base URL at
// the start of the navigation. Currently, this is only stored and sent to the
// renderer if kNewBaseUrlInheritanceBehavior or kIsolateSandboxedIframes is
// enabled, since it is a behavior change relevant for isolated sandboxed
// iframes. See https://crbug.com/1356658.
// TODO(wjmaclean): about:blank frames may also need to inherit base URLs,
// possibly from the initiator rather than the parent. See
// https://crbug.com/1356658#c7.
if (GetURL().IsAboutSrcdoc() && frame_tree_node_->parent() &&
blink::features::IsNewBaseUrlInheritanceBehaviorEnabled()) {
commit_params_->fallback_srcdoc_baseurl =
frame_tree_node_->parent()->GetBaseUrl();
}

// Ask the service worker context to speculatively start a service worker for
// the request URL if necessary for optimization purposes. Don't ask to do
// that if this request is for ReloadType::BYPASSING_CACHE that is supposed to
Expand Down Expand Up @@ -5127,6 +5111,9 @@ void NavigationRequest::CommitErrorPage(
commit_params_->origin_to_commit = GetOriginToCommit();
DCHECK(commit_params_->origin_to_commit->opaque());

// Don't pass the base url in a failed navigation.
common_params_->initiator_base_url = absl::nullopt;

if (request_navigation_client_.is_bound()) {
if (render_frame_host_ == frame_tree_node()->current_frame_host()) {
// Reuse the request NavigationClient for commit.
Expand Down
21 changes: 0 additions & 21 deletions content/browser/renderer_host/navigation_request.h
View file
Expand Up @@ -867,27 +867,6 @@ class CONTENT_EXPORT NavigationRequest
// properly determine SiteInstances and process allocation.
UrlInfo GetUrlInfo();

// Return the parent's base url, snapshotted when this NavigationRequest was
// created. Used for sending to srcdoc renderers. See
// https://crbug.com/1356658 for further details. Note: The returned value
// will be empty unless (i) the navigation is to about:srcdoc, and (ii)
// IsolateSandboxedIframes is enabled.
// TODO(wjmaclean): https://crbug.com/1356658 Make this also apply for
// about:blank navigations as well.

// Return the parent's base url, snapshotted when this NavigationRequest was
// created. Used for sending to srcdoc renderers. See
// https://crbug.com/1356658 for further details.
// Note: The returned value will be empty unless:
// 1. The navigation is to about:srcdoc, and
// 2. IsolateSandboxedIframes is enabled.
//
// TODO(https://crbug.com/1356658) Make this also apply for
// about:blank navigations as well.
const GURL& inherited_base_url() const {
return commit_params_->fallback_srcdoc_baseurl;
}

bool is_overriding_user_agent() const {
return commit_params_->is_overriding_user_agent;
}
Expand Down
68 changes: 20 additions & 48 deletions content/browser/renderer_host/render_frame_host_impl.cc
View file
Expand Up @@ -2331,6 +2331,10 @@ const url::Origin& RenderFrameHostImpl::GetLastCommittedOrigin() const {
return last_committed_origin_;
}

const GURL& RenderFrameHostImpl::GetInheritedBaseUrl() const {
return inherited_base_url_;
}

const net::NetworkIsolationKey& RenderFrameHostImpl::GetNetworkIsolationKey() {
DCHECK(!isolation_info_.IsEmpty());
return isolation_info_.network_isolation_key();
Expand Down Expand Up @@ -3139,8 +3143,7 @@ void RenderFrameHostImpl::RenderProcessGone(
// reset.
RenderFrameDeleted();
SetLastCommittedUrl(GURL());
set_base_url_from_renderer(GURL());
set_inherited_base_url(GURL());
SetInheritedBaseUrl(GURL());
renderer_url_info_ = RendererURLInfo();
web_bundle_handle_.reset();

Expand Down Expand Up @@ -3899,6 +3902,12 @@ void RenderFrameHostImpl::SetLastCommittedOrigin(const url::Origin& origin) {
last_committed_origin_ = origin;
}

void RenderFrameHostImpl::SetInheritedBaseUrl(const GURL& inherited_base_url) {
if (blink::features::IsNewBaseUrlInheritanceBehaviorEnabled()) {
inherited_base_url_ = inherited_base_url;
}
}

void RenderFrameHostImpl::SetLastCommittedOriginForTesting(
const url::Origin& origin) {
SetLastCommittedOrigin(origin);
Expand Down Expand Up @@ -8069,43 +8078,6 @@ void RenderFrameHostImpl::DidChangeSrcDoc(
child->SetSrcdocValue(srcdoc_value);
}

void RenderFrameHostImpl::DidChangeBaseURL(const GURL& base_url) {
if (!blink::features::IsNewBaseUrlInheritanceBehaviorEnabled())
return;

// TODO(https://crbug.com/1356658,1366593): consider restricting base URL in
// web renderers to non-chrome:// URLs.
set_base_url_from_renderer(base_url);
}

const GURL& RenderFrameHostImpl::GetBaseUrl() const {
if (!blink::features::IsNewBaseUrlInheritanceBehaviorEnabled()) {
NOTREACHED() << __func__
<< " should only be invoked when the feature"
" NewBaseUrlInheritanceBehavior is enabled.";
return GURL::EmptyGURL();
}

if (!base_url_from_renderer_.is_empty())
return base_url_from_renderer_;

// If `base_url_from_renderer_` is not set, then the document uses either the
// inherited or default (i.e. last committed URL) value for base URL.
if (!snapshotted_base_url_from_parent_.is_empty()) {
// TODO(wjmaclean): update this DCHECK when we start sending base urls for
// about:blank as well.
DCHECK(GetLastCommittedURL().IsAboutSrcdoc());
return snapshotted_base_url_from_parent_;
}

// If no other base URL is specified or inherited, the last committed URL is
// used. Note that srcdoc cases should always inherit.
// TODO(wjmaclean): update this DCHECK when we start sending base urls for
// about:blank as well.
DCHECK(!GetLastCommittedURL().IsAboutSrcdoc());
return GetLastCommittedURL();
}

void RenderFrameHostImpl::ReceivedDelegatedCapability(
blink::mojom::DelegatedCapability delegated_capability) {
if (delegated_capability ==
Expand Down Expand Up @@ -12179,15 +12151,15 @@ void RenderFrameHostImpl::DidCommitNewDocument(
DCHECK(!navigation_request->IsSameDocument());
DCHECK(!navigation_request->IsPageActivation());

// On every cross-document navigation, reset the the base url as sent from the
// renderer.
set_base_url_from_renderer(GURL());
// Remember any snapshot of the inherited base URL, in case subframes need it.
// This is currently only used for srcdoc frames when the
// IsolateSandboxedIframes feature is enabled.
DCHECK(navigation_request->inherited_base_url().is_empty() ||
navigation_request->GetURL().IsAboutSrcdoc());
set_inherited_base_url(navigation_request->inherited_base_url());
const GURL& request_url = navigation_request->common_params().url;
if (request_url.IsAboutBlank() || request_url.IsAboutSrcdoc()) {
const absl::optional<::GURL>& initiator_base_url =
navigation_request->common_params().initiator_base_url;
SetInheritedBaseUrl(initiator_base_url ? initiator_base_url.value()
: GURL::EmptyGURL());
} else {
SetInheritedBaseUrl(GURL::EmptyGURL());
}

// The nonce to use in credentialless iframe is a page scoped attribute. So it
// needs to change every time the top-level document change.
Expand Down
80 changes: 23 additions & 57 deletions content/browser/renderer_host/render_frame_host_impl.h
View file
Expand Up @@ -753,28 +753,6 @@ class CONTENT_EXPORT RenderFrameHostImpl
void RemoveChild(FrameTreeNode* child);
void ResetChildren();

// Returns the base URL value of the associated document. This uses the
// base URL value sent from the renderer (if any), otherwise it defaults to
// the value inherited from this document's parent (for srcdoc iframes), or
// the last committed URL if neither of these apply.
// Note: this can only be called when IsolateSandboxedIframes is enabled.
// TODO(wjmaclean): https://crbug.com/1356658 In future this will be modified
// to also use the inherited value for about:blank frames, though in that case
// it's inherited from the initiator.
const GURL& GetBaseUrl() const;

// Returns the base url received from the renderer, if any.
// Note: this is only valid when IsolatedSandboxedIframes is enabled.
const GURL& base_url_from_renderer_for_testing() const {
return base_url_from_renderer_;
}

// Returns the base url inherited from the parent, if any.
// Note: this is only valid when IsolatedSandboxedIframes is enabled.
const GURL& snapshotted_base_url_from_parent_for_testing() const {
return snapshotted_base_url_from_parent_;
}

// Set the URL of the document represented by this RenderFrameHost. Called
// when the navigation commits. See also `GetLastCommittedURL`.
void SetLastCommittedUrl(const GURL& url);
Expand All @@ -783,6 +761,13 @@ class CONTENT_EXPORT RenderFrameHostImpl
// cases, use GetLastCommittedURL instead.
const GURL& last_successful_url() const { return last_successful_url_; }

// For about:blank and about:srcdoc documents, this tracks the inherited base
// URL, snapshotted from the initiator's FrameLoadRequest. This is an empty
// URL for all other cases. This is currently only set if
// IsNewBaseUrlInheritanceBehaviorEnabled() returns true. See
// https://crbug.com/1356658.
const GURL& GetInheritedBaseUrl() const;

// The current URL of the document in the renderer process. Note that this
// includes URL updates due to document.open() (where it will be updated to
// the document-open-initiator URL) and special cases like error pages
Expand Down Expand Up @@ -2319,7 +2304,6 @@ class CONTENT_EXPORT RenderFrameHostImpl
void FrameSizeChanged(const gfx::Size& frame_size) override;
void DidChangeSrcDoc(const blink::FrameToken& child_frame_token,
const std::string& srcdoc_value) override;
void DidChangeBaseURL(const GURL& base_url) override;
void ReceivedDelegatedCapability(
blink::mojom::DelegatedCapability delegated_capability) override;
void SendFencedFrameReportingBeacon(
Expand Down Expand Up @@ -3356,6 +3340,15 @@ class CONTENT_EXPORT RenderFrameHostImpl
// Update this frame's last committed origin.
void SetLastCommittedOrigin(const url::Origin& origin);

// Stores a snapshot of the inherited base URL from the initiator's
// FrameLoadRequest, if this document inherited one (e.g., about:srcdoc).
// This value is currently only set when the NewBaseUrlInheritanceBehavior
// feature is enabled.
// TODO(1356658): about:blank frames will also need to inherit base URLs,
// from the initiator rather than the parent. See
// https://crbug.com/1356658#c7.
void SetInheritedBaseUrl(const GURL& inherited_base_url);

// Called when a navigation commits successfully to |url_info->url|. This
// will update |last_committed_site_info_| with the SiteInfo corresponding to
// |url_info|. If |url_info| is empty, |last_committed_site_info_| will be
Expand Down Expand Up @@ -3736,26 +3729,6 @@ class CONTENT_EXPORT RenderFrameHostImpl
// used for communicating with the FrameTreeNode.
FrameTreeNode* GetFrameTreeNodeForUnload();

// Stores an override of this document's base URL when it does not match the
// last committed URL or an inherited value (e.g., if a <base> element is
// added). This is tracked for all frames, for the purpose of GetBaseUrl. This
// value is currently only set when the IsolateSandboxedIframes feature is
// enabled.
void set_base_url_from_renderer(const GURL& base_url) {
base_url_from_renderer_ = base_url;
}

// Stores a snapshot of the inherited base URL from the start of the
// NavigationRequest, if this document inherited one (e.g., about:srcdoc).
// This value is currently only set when the IsolateSandboxedIframes feature
// is enabled.
// TODO(wjmaclean): about:blank frames may also need to inherit base URLs,
// possibly from the initiator rather than the parent. See
// https://crbug.com/1356658#c7.
void set_inherited_base_url(const GURL& inherited_base_url) {
snapshotted_base_url_from_parent_ = inherited_base_url;
}

// Close the page ignoring whether it has unload events registered. This is
// called either (1) when the unload events have already run in the renderer
// and the ACK is received, or (2) when a timeout for running those events
Expand Down Expand Up @@ -3879,20 +3852,6 @@ class CONTENT_EXPORT RenderFrameHostImpl
// avoid separately storing the last committed URL and origin here.
scoped_refptr<FrameNavigationEntry> last_committed_frame_entry_;

// This value stores the Document::GetBaseURL() value. It is used to determine
// the base URL of children about::srcdoc documents.
GURL base_url_from_renderer_;
// For documents whose current_url() is about:srcdoc, this is the GetBaseUrl()
// of the parent node (which is guaranteed to be non-null since about:srcdoc
// can't be used in a top-level frame). It is snapshotted at the time the
// srcdoc's NavigationRequest is created, and stored here to prevent the
// srcdoc from being affected by subsequent changes in the parent's value.
// TODO(wjmaclean): https://crbug.com/1356658 update this when we start
// sending base urls to about:blank as well. In the about:blank case we'll use
// the initiator instead of the parent (as not all about:blanks will have
// parents).
GURL snapshotted_base_url_from_parent_;

// Tracks this frame's last committed navigation's URL. Note that this will be
// empty before the first commit in this *RenderFrameHost*, even if the
// FrameTreeNode has committed before with a different RenderFrameHost.
Expand All @@ -3913,6 +3872,13 @@ class CONTENT_EXPORT RenderFrameHostImpl
// Track this frame's last committed origin.
url::Origin last_committed_origin_;

// For about:blank and about:srcdoc documents, this tracks the inherited base
// URL, snapshotted from the initiator's FrameLoadRequest. This is an empty
// URL for all other cases. This is currently only set if
// IsNewBaseUrlInheritanceBehaviorEnabled() returns true. See
// https://crbug.com/1356658.
GURL inherited_base_url_;

// The storage key for the last committed document in this
// RenderFrameHostImpl.
blink::StorageKey storage_key_;
Expand Down
9 changes: 9 additions & 0 deletions content/browser/site_per_process_browsertest.cc
View file
Expand Up @@ -5788,6 +5788,15 @@ IN_PROC_BROWSER_TEST_P(SitePerProcessBrowserTest,
ASSERT_EQ(1U, new_root->child_count());
EXPECT_EQ(main_url, new_root->current_url());
EXPECT_TRUE(new_root->child_at(0)->current_url().IsAboutSrcdoc());
if (blink::features::IsNewBaseUrlInheritanceBehaviorEnabled()) {
// When NewBaseUrlInheritanceBehavior is enabled, not only should the srcdoc
// inherit its base url from its initiator, but it should also be properly
// restored from the session history.
EXPECT_EQ(
main_url,
GURL(
EvalJs(new_root->child_at(0), "document.baseURI").ExtractString()));
}

EXPECT_EQ(new_root->current_frame_host()->GetSiteInstance(),
new_root->child_at(0)->current_frame_host()->GetSiteInstance());
Expand Down

0 comments on commit 78e2f87

Please sign in to comment.