Skip to content

Commit

Permalink
Remove subresource request blocking logic
Browse files Browse the repository at this point in the history
Subresource request blocking logic was introduced [1] to block
subresource requests from the browser process by calling BlockRequests()
IPC. InterstitialPageImpl used this logic. But it was removed [2] after
Committed Interstitials [3] was introduced.

Currently RenderFrameHostImpl::CreateNewWindow() calls BlockRequests()
to block subresource requests when `waiting_for_init_` is true. And
RenderFrameHostImpl::Init() is calling ResumeBlockedRequests() to resume
them. But it sounds useless because navigation is also blocked until
Init() is called, and no subresource requests should be fetched. So this
CL removes this logic, and all subresource request blocking related
logic.

[1] https://crrev.com/c/1164542
[2] https://crrev.com/c/2146137
[3] https://crbug.com/755632

Bug: 581037
Change-Id: Iecab896362b54fa2b5c3fcb1eac2cd3575e29f7e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3954952
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1187690}
  • Loading branch information
horo-t authored and Chromium LUCI CQ committed Aug 24, 2023
1 parent 13b77f3 commit ee5bb55
Show file tree
Hide file tree
Showing 14 changed files with 0 additions and 277 deletions.
19 changes: 0 additions & 19 deletions content/browser/renderer_host/render_frame_host_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3701,17 +3701,6 @@ void RenderFrameHostImpl::Init() {

GetLocalRenderWidgetHost()->Init();

// TODO(danakj): We only blocked the main frame, so we should only need to
// resume that?
ForEachRenderFrameHostIncludingSpeculative(
[this](RenderFrameHostImpl* render_frame_host) {
// Inner frame trees shouldn't be possible here.
DCHECK_EQ(render_frame_host->frame_tree(), frame_tree());

if (render_frame_host->IsRenderFrameLive())
render_frame_host->frame_->ResumeBlockedRequests();
});

if (pending_navigate_) {
// `pending_navigate_` is set only by BeginNavigation(), and
// BeginNavigation() should only be triggered when the navigation is
Expand Down Expand Up @@ -8356,14 +8345,6 @@ void RenderFrameHostImpl::CreateNewWindow(
std::move(callback).Run(mojom::CreateNewWindowStatus::kSuccess,
std::move(reply));

// When `waiting_for_init_` is true, the browser waits for the renderer to
// request to show the window (which becomes a call to Init() on the new
// window's `new_main_rfh`) before servicing subresource requests. We ensure
// this is the first message received by the remote frame (instead of plumbing
// it with the CreateNewWindow IPC).
if (new_main_rfh->waiting_for_init_)
new_main_rfh->GetMojomFrameInRenderer()->BlockRequests();

// The mojom reply callback with kSuccess causes the renderer to create the
// renderer-side objects.
new_main_rfh->render_view_host()->RenderViewCreated(new_main_rfh);
Expand Down
8 changes: 0 additions & 8 deletions content/common/frame.mojom
Original file line number Diff line number Diff line change
Expand Up @@ -496,14 +496,6 @@ interface Frame {
blink.mojom.RemoteFrameInterfacesFromBrowser new_remote_frame_interfaces,
blink.mojom.RemoteMainFrameInterfaces new_remote_main_frame_interfaces);

// Causes all new subresource requests to be blocked (not being started) until
// ResumeBlockedRequests is called.
BlockRequests();

// Resumes blocked requests.
// It is safe to call this without calling BlockRequests.
ResumeBlockedRequests();

GetInterfaceProvider(
pending_receiver<service_manager.mojom.InterfaceProvider> interfaces);

Expand Down
15 changes: 0 additions & 15 deletions content/renderer/render_frame_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1920,8 +1920,6 @@ void RenderFrameImpl::Initialize(blink::WebFrame* parent) {
GetBrowserInterfaceBroker());
}

frame_request_blocker_ = blink::WebFrameRequestBlocker::Create();

// Bind this class to mojom::Frame and to the message router for legacy IPC.
// These must be called after |frame_| is set since binding requires a
// per-frame task runner.
Expand Down Expand Up @@ -2484,14 +2482,6 @@ void RenderFrameImpl::GetInterfaceProvider(
interface_provider_receivers_.Add(this, std::move(receiver), task_runner);
}

void RenderFrameImpl::BlockRequests() {
frame_request_blocker_->Block();
}

void RenderFrameImpl::ResumeBlockedRequests() {
frame_request_blocker_->Resume();
}

void RenderFrameImpl::AllowBindings(int32_t enabled_bindings_flags) {
enabled_bindings_ |= enabled_bindings_flags;
}
Expand Down Expand Up @@ -3367,8 +3357,6 @@ RenderFrameImpl::CreateWorkerFetchContext() {

web_dedicated_or_shared_worker_fetch_context->set_ancestor_frame_id(
routing_id_);
web_dedicated_or_shared_worker_fetch_context->set_frame_request_blocker(
frame_request_blocker_);
web_dedicated_or_shared_worker_fetch_context->set_site_for_cookies(
frame_->GetDocument().SiteForCookies());
web_dedicated_or_shared_worker_fetch_context->set_top_frame_origin(
Expand Down Expand Up @@ -3409,8 +3397,6 @@ RenderFrameImpl::CreateWorkerFetchContextForPlzDedicatedWorker(

web_dedicated_or_shared_worker_fetch_context->set_ancestor_frame_id(
routing_id_);
web_dedicated_or_shared_worker_fetch_context->set_frame_request_blocker(
frame_request_blocker_);
web_dedicated_or_shared_worker_fetch_context->set_site_for_cookies(
frame_->GetDocument().SiteForCookies());
web_dedicated_or_shared_worker_fetch_context->set_top_frame_origin(
Expand Down Expand Up @@ -4318,7 +4304,6 @@ void RenderFrameImpl::WillSendRequestInternal(
GetContentClient()->renderer()->IsPrefetchOnly(this);
url_request_extra_data->set_is_for_no_state_prefetch(
is_for_no_state_prefetch);
url_request_extra_data->set_frame_request_blocker(frame_request_blocker_);
url_request_extra_data->set_allow_cross_origin_auth_prompt(
GetWebView()->GetRendererPreferences().allow_cross_origin_auth_prompt);
url_request_extra_data->set_top_frame_origin(GetSecurityOriginOfTopFrame());
Expand Down
5 changes: 0 additions & 5 deletions content/renderer/render_frame_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,6 @@ class WeakWrapperResourceLoadInfoNotifier;
class WebComputedAXTree;
class WebContentDecryptionModule;
class WebElement;
class WebFrameRequestBlocker;
class WebLocalFrame;
class WebMediaStreamDeviceObserver;
class WebString;
Expand Down Expand Up @@ -871,8 +870,6 @@ class CONTENT_EXPORT RenderFrameImpl
blink::mojom::RemoteFrameInterfacesFromBrowserPtr remote_frame_interfaces,
blink::mojom::RemoteMainFrameInterfacesPtr remote_main_frame_interfaces)
override;
void BlockRequests() override;
void ResumeBlockedRequests() override;
void GetInterfaceProvider(
mojo::PendingReceiver<service_manager::mojom::InterfaceProvider> receiver)
override;
Expand Down Expand Up @@ -1436,8 +1433,6 @@ class CONTENT_EXPORT RenderFrameImpl
// The storage key which |pending_storage_info_| is associated with.
blink::StorageKey original_storage_key_;

scoped_refptr<blink::WebFrameRequestBlocker> frame_request_blocker_;

// AndroidOverlay routing token from the browser, if we have one yet.
absl::optional<base::UnguessableToken> overlay_routing_token_;

Expand Down
1 change: 0 additions & 1 deletion third_party/blink/public/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,6 @@ source_set("blink_headers") {
"platform/web_font.h",
"platform/web_font_description.h",
"platform/web_font_render_style.h",
"platform/web_frame_request_blocker.h",
"platform/web_gesture_curve.h",
"platform/web_graphics_context_3d_provider.h",
"platform/web_http_body.h",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@

namespace blink {

class WebFrameRequestBlocker;
class WebString;
template <typename T>
class WebVector;
Expand Down Expand Up @@ -94,8 +93,6 @@ class BLINK_PLATFORM_EXPORT WebDedicatedOrSharedWorkerFetchContext
// TODO(nhiroki): Add more comments about security/privacy implications to
// each property, for example, site_for_cookies and top_frame_origin.
virtual void set_ancestor_frame_id(int id) = 0;
virtual void set_frame_request_blocker(
scoped_refptr<WebFrameRequestBlocker> frame_request_blocker) = 0;
virtual void set_site_for_cookies(
const net::SiteForCookies& site_for_cookies) = 0;
virtual void set_top_frame_origin(
Expand Down
40 changes: 0 additions & 40 deletions third_party/blink/public/platform/web_frame_request_blocker.h

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
#include "base/memory/ref_counted.h"
#include "third_party/blink/public/common/loader/url_loader_throttle.h"
#include "third_party/blink/public/platform/web_common.h"
#include "third_party/blink/public/platform/web_frame_request_blocker.h"
#include "third_party/blink/public/platform/web_string.h"
#include "third_party/blink/public/platform/web_vector.h"
#include "ui/base/page_transition_types.h"
Expand Down Expand Up @@ -74,13 +73,6 @@ class BLINK_PLATFORM_EXPORT WebURLRequestExtraData
WebVector<std::unique_ptr<URLLoaderThrottle>> throttles) {
url_loader_throttles_ = std::move(throttles);
}
void set_frame_request_blocker(
scoped_refptr<WebFrameRequestBlocker> frame_request_blocker) {
frame_request_blocker_ = frame_request_blocker;
}
scoped_refptr<WebFrameRequestBlocker> frame_request_blocker() {
return frame_request_blocker_;
}
bool allow_cross_origin_auth_prompt() const {
return allow_cross_origin_auth_prompt_;
}
Expand All @@ -101,7 +93,6 @@ class BLINK_PLATFORM_EXPORT WebURLRequestExtraData
bool originated_from_service_worker_ = false;
WebString custom_user_agent_;
WebVector<std::unique_ptr<URLLoaderThrottle>> url_loader_throttles_;
scoped_refptr<WebFrameRequestBlocker> frame_request_blocker_;
bool allow_cross_origin_auth_prompt_ = false;

// The origin of the top most frame. Only applicable for frames.
Expand Down
2 changes: 0 additions & 2 deletions third_party/blink/renderer/platform/loader/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,6 @@ blink_platform_sources("loader") {
"fetch/url_loader/worker_main_script_loader.h",
"fetch/url_loader/worker_main_script_loader_client.h",
"fetch/worker_resource_timing_notifier.h",
"frame_request_blocker.cc",
"frame_request_blocker.h",
"internet_disconnected_url_loader.cc",
"internet_disconnected_url_loader.h",
"link_header.cc",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
#include "third_party/blink/public/platform/url_loader_throttle_provider.h"
#include "third_party/blink/public/platform/weak_wrapper_resource_load_info_notifier.h"
#include "third_party/blink/public/platform/web_code_cache_loader.h"
#include "third_party/blink/public/platform/web_frame_request_blocker.h"
#include "third_party/blink/public/platform/web_security_origin.h"
#include "third_party/blink/public/platform/web_url_request_extra_data.h"
#include "third_party/blink/public/platform/websocket_handshake_throttle_provider.h"
Expand Down Expand Up @@ -298,11 +297,6 @@ void DedicatedOrSharedWorkerFetchContextImpl::set_ancestor_frame_id(int id) {
ancestor_frame_id_ = id;
}

void DedicatedOrSharedWorkerFetchContextImpl::set_frame_request_blocker(
scoped_refptr<WebFrameRequestBlocker> frame_request_blocker) {
frame_request_blocker_ = frame_request_blocker;
}

void DedicatedOrSharedWorkerFetchContextImpl::set_site_for_cookies(
const net::SiteForCookies& site_for_cookies) {
site_for_cookies_ = site_for_cookies;
Expand Down Expand Up @@ -391,7 +385,6 @@ void DedicatedOrSharedWorkerFetchContextImpl::WillSendRequest(
}

auto url_request_extra_data = base::MakeRefCounted<WebURLRequestExtraData>();
url_request_extra_data->set_frame_request_blocker(frame_request_blocker_);
if (throttle_provider_) {
url_request_extra_data->set_url_loader_throttles(
throttle_provider_->CreateThrottles(ancestor_frame_id_, request));
Expand Down Expand Up @@ -548,7 +541,6 @@ DedicatedOrSharedWorkerFetchContextImpl::CloneForNestedWorkerInternal(
std::move(pending_resource_load_info_notifier)));
new_context->is_on_sub_frame_ = is_on_sub_frame_;
new_context->ancestor_frame_id_ = ancestor_frame_id_;
new_context->frame_request_blocker_ = frame_request_blocker_;
new_context->site_for_cookies_ = site_for_cookies_;
new_context->top_frame_origin_ = top_frame_origin_;
child_preference_watchers_.Add(std::move(preference_watcher));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ namespace blink {
class ResourceLoadInfoNotifierWrapper;
class URLLoaderThrottleProvider;
class WeakWrapperResourceLoadInfoNotifier;
class WebFrameRequestBlocker;
class WebServiceWorkerProviderContext;
class WebSocketHandshakeThrottleProvider;

Expand Down Expand Up @@ -105,8 +104,6 @@ class BLINK_PLATFORM_EXPORT DedicatedOrSharedWorkerFetchContextImpl final
// TODO(nhiroki): Add more comments about security/privacy implications to
// each property, for example, site_for_cookies and top_frame_origin.
void set_ancestor_frame_id(int id) override;
void set_frame_request_blocker(
scoped_refptr<WebFrameRequestBlocker> frame_request_blocker) override;
void set_site_for_cookies(
const net::SiteForCookies& site_for_cookies) override;
void set_top_frame_origin(const WebSecurityOrigin& top_frame_origin) override;
Expand Down Expand Up @@ -262,10 +259,6 @@ class BLINK_PLATFORM_EXPORT DedicatedOrSharedWorkerFetchContextImpl final
// workers, this is the shadow page.
bool is_on_sub_frame_ = false;
int ancestor_frame_id_ = MSG_ROUTING_NONE;
// Set to non-null if the ancestor frame has an associated RequestBlocker,
// which blocks requests from this worker too when the ancestor frame is
// blocked.
scoped_refptr<WebFrameRequestBlocker> frame_request_blocker_;
net::SiteForCookies site_for_cookies_;
absl::optional<url::Origin> top_frame_origin_;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -271,9 +271,6 @@ void URLLoader::Context::Start(
// TODO(horo): Check credentials flag is unset when credentials mode is omit.
// Check credentials flag is set when credentials mode is include.

const network::mojom::RequestDestination request_destination =
request->destination;

scoped_refptr<WebURLRequestExtraData> empty_url_request_extra_data;
WebURLRequestExtraData* url_request_extra_data;
if (passed_url_request_extra_data) {
Expand All @@ -287,15 +284,6 @@ void URLLoader::Context::Start(

auto throttles =
url_request_extra_data->TakeURLLoaderThrottles().ReleaseVector();
// The frame request blocker is only for a frame's subresources.
if (url_request_extra_data->frame_request_blocker() &&
!IsRequestDestinationFrame(request_destination)) {
auto throttle = url_request_extra_data->frame_request_blocker()
->GetThrottleIfRequestsBlocked();
if (throttle) {
throttles.push_back(std::move(throttle));
}
}

// TODO(falken): URLLoader should be able to get the top frame origin via some
// plumbing such as through ResourceLoader -> FetchContext -> LocalFrame
Expand Down

0 comments on commit ee5bb55

Please sign in to comment.