Skip to content

Commit

Permalink
request_initiator_origin_lock: "url_loader_factory_debug_tag" crash key.
Browse files Browse the repository at this point in the history
This CL reintroduces the "url_loader_factory_debug_tag" crash key that
hopefully will help understand the root cause behind
`request_initiator_origin_lock` mismatches observed in the wild.

The crash key has been previously introduced in r803843 and removed
in r817762.

Bug: 1151008, 1151438
Change-Id: I38c0355c04ca6109c76440bf9e095af79f6bc803
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2575368
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Clark DuVall <cduvall@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Auto-Submit: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#834318}
  • Loading branch information
anforowicz authored and Chromium LUCI CQ committed Dec 7, 2020
1 parent fb7bb96 commit cfc799f
Show file tree
Hide file tree
Showing 16 changed files with 65 additions and 24 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,8 @@ class DevtoolsNetworkResourceLoaderTest : public ContentBrowserTest {
frame, frame->GetLastCommittedOrigin(),
mojo::Clone(frame->last_committed_client_security_state()),
/**coep_reporter=*/mojo::NullRemote(), frame->GetProcess(),
network::mojom::TrustTokenRedemptionPolicy::kForbid);
network::mojom::TrustTokenRedemptionPolicy::kForbid,
"DevtoolsNetworkResourceLoaderTest");
// Let DevTools fetch resources without CORS and CORB. Source maps are valid
// JSON and would otherwise require a CORS fetch + correct response headers.
// See BUG(chromium:1076435) for more context.
Expand Down
3 changes: 2 additions & 1 deletion content/browser/devtools/protocol/network_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2628,7 +2628,8 @@ void NetworkHandler::LoadNetworkResource(
frame, frame->GetLastCommittedOrigin(),
mojo::Clone(frame->last_committed_client_security_state()),
/**coep_reporter=*/mojo::NullRemote(), frame->GetProcess(),
network::mojom::TrustTokenRedemptionPolicy::kForbid);
network::mojom::TrustTokenRedemptionPolicy::kForbid,
"NetworkHandler::LoadNetworkResource");

auto factory =
CreateNetworkFactoryForDevTools(frame->GetProcess(), std::move(params));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,8 @@ ServiceWorkerDevToolsAgentHost::CreateNetworkFactoryParamsForDevTools() {
net::IsolationInfo::Create(net::IsolationInfo::RequestType::kOther,
origin, origin,
net::SiteForCookies::FromOrigin(origin)),
/*coep_reporter=*/mojo::NullRemote());
/*coep_reporter=*/mojo::NullRemote(),
/*debug_tag=*/"SWDTAH::CreateNetworkFactoryParamsForDevTools");
return {url::Origin::Create(GetURL()), net::SiteForCookies::FromUrl(GetURL()),
std::move(factory)};
}
Expand Down
18 changes: 12 additions & 6 deletions content/browser/renderer_host/render_frame_host_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1599,7 +1599,8 @@ bool RenderFrameHostImpl::CreateNetworkServiceDefaultFactory(
NavigationRequest* navigation_request = nullptr;

return CreateNetworkServiceDefaultFactoryAndObserve(
CreateURLLoaderFactoryParamsForMainWorld(navigation_request),
CreateURLLoaderFactoryParamsForMainWorld(
navigation_request, "RFHI::CreateNetworkServiceDefaultFactory"),
ukm::SourceIdObj::FromInt64(GetPageUkmSourceId()),
std::move(default_factory_receiver));
}
Expand Down Expand Up @@ -3988,7 +3989,8 @@ void RenderFrameHostImpl::UpdateSubresourceLoaderFactories() {
if (recreate_default_url_loader_factory_after_network_service_crash_) {
bypass_redirect_checks = CreateNetworkServiceDefaultFactoryAndObserve(
CreateURLLoaderFactoryParamsForMainWorld(
latest_nav_request_still_committing),
latest_nav_request_still_committing,
"RFHI::UpdateSubresourceLoaderFactories"),
ukm::SourceIdObj::FromInt64(
latest_nav_request_still_committing
? latest_nav_request_still_committing->GetNextPageUkmSourceId()
Expand Down Expand Up @@ -6351,7 +6353,8 @@ void RenderFrameHostImpl::CommitNavigation(
// appropriate NetworkContext.
bool bypass_redirect_checks =
CreateNetworkServiceDefaultFactoryAndObserve(
CreateURLLoaderFactoryParamsForMainWorld(navigation_request),
CreateURLLoaderFactoryParamsForMainWorld(
navigation_request, "RFHI::CommitNavigation"),
next_page_ukm_source_id,
pending_default_factory.InitWithNewPipeAndPassReceiver());
subresource_loader_factories->set_bypass_redirect_checks(
Expand Down Expand Up @@ -6633,7 +6636,8 @@ void RenderFrameHostImpl::FailedNavigation(
subresource_loader_factories;
mojo::PendingRemote<network::mojom::URLLoaderFactory> default_factory_remote;
bool bypass_redirect_checks = CreateNetworkServiceDefaultFactoryAndObserve(
CreateURLLoaderFactoryParamsForMainWorld(navigation_request),
CreateURLLoaderFactoryParamsForMainWorld(navigation_request,
"RFHI::FailedNavigation"),
ukm::kInvalidSourceIdObj,
default_factory_remote.InitWithNewPipeAndPassReceiver());
subresource_loader_factories =
Expand Down Expand Up @@ -7293,7 +7297,8 @@ void RenderFrameHostImpl::

network::mojom::URLLoaderFactoryParamsPtr
RenderFrameHostImpl::CreateURLLoaderFactoryParamsForMainWorld(
NavigationRequest* navigation_request) {
NavigationRequest* navigation_request,
base::StringPiece debug_tag) {
url::Origin main_world_origin;
network::mojom::ClientSecurityStatePtr client_security_state;
mojo::PendingRemote<network::mojom::CrossOriginEmbedderPolicyReporter>
Expand All @@ -7307,7 +7312,7 @@ RenderFrameHostImpl::CreateURLLoaderFactoryParamsForMainWorld(
return URLLoaderFactoryParamsHelper::CreateForFrame(
this, main_world_origin, std::move(client_security_state),
std::move(coep_reporter_remote), GetProcess(),
trust_token_redemption_policy);
trust_token_redemption_policy, debug_tag);
}

bool RenderFrameHostImpl::CreateNetworkServiceDefaultFactoryAndObserve(
Expand All @@ -7329,6 +7334,7 @@ bool RenderFrameHostImpl::CreateNetworkServiceDefaultFactoryAndObserve(
network::mojom::URLLoaderFactoryParamsPtr monitoring_factory_params =
network::mojom::URLLoaderFactoryParams::New();
monitoring_factory_params->process_id = GetProcess()->GetID();
monitoring_factory_params->debug_tag = "RFHI - monitoring_factory_params";

// This factory should never be used to issue actual requests (i.e. it
// should only be used to monitor for Network Service crashes). Below is an
Expand Down
3 changes: 2 additions & 1 deletion content/browser/renderer_host/render_frame_host_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -2185,7 +2185,8 @@ class CONTENT_EXPORT RenderFrameHostImpl
// committed navigation.
network::mojom::URLLoaderFactoryParamsPtr
CreateURLLoaderFactoryParamsForMainWorld(
NavigationRequest* navigation_request);
NavigationRequest* navigation_request,
base::StringPiece debug_tag);

// Creates a Network Service-backed factory from appropriate |NetworkContext|
// and sets a connection error handler to trigger
Expand Down
3 changes: 2 additions & 1 deletion content/browser/service_worker/embedded_worker_instance.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1140,7 +1140,8 @@ EmbeddedWorkerInstance::CreateFactoryBundleOnUI(
net::IsolationInfo::Create(net::IsolationInfo::RequestType::kOther,
origin, origin,
net::SiteForCookies::FromOrigin(origin)),
std::move(coep_reporter));
std::move(coep_reporter),
"EmbeddedWorkerInstance::CreateFactoryBundlesOnUI");
bool bypass_redirect_checks = false;

DCHECK(factory_type ==
Expand Down
25 changes: 17 additions & 8 deletions content/browser/url_loader_factory_params_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ network::mojom::URLLoaderFactoryParamsPtr CreateParams(
bool allow_universal_access_from_file_urls,
bool is_for_isolated_world,
mojo::PendingRemote<network::mojom::CookieAccessObserver> cookie_observer,
network::mojom::TrustTokenRedemptionPolicy trust_token_redemption_policy) {
network::mojom::TrustTokenRedemptionPolicy trust_token_redemption_policy,
base::StringPiece debug_tag) {
DCHECK(process);

// "chrome-guest://..." is never used as a main or isolated world origin.
Expand Down Expand Up @@ -91,6 +92,8 @@ network::mojom::URLLoaderFactoryParamsPtr CreateParams(

params->cookie_observer = std::move(cookie_observer);

params->debug_tag = debug_tag.as_string();

return params;
}

Expand All @@ -105,7 +108,8 @@ URLLoaderFactoryParamsHelper::CreateForFrame(
mojo::PendingRemote<network::mojom::CrossOriginEmbedderPolicyReporter>
coep_reporter,
RenderProcessHost* process,
network::mojom::TrustTokenRedemptionPolicy trust_token_redemption_policy) {
network::mojom::TrustTokenRedemptionPolicy trust_token_redemption_policy,
base::StringPiece debug_tag) {
return CreateParams(
process,
frame_origin, // origin
Expand All @@ -117,7 +121,8 @@ URLLoaderFactoryParamsHelper::CreateForFrame(
->GetOrCreateWebPreferences()
.allow_universal_access_from_file_urls,
false, // is_for_isolated_world
frame->CreateCookieAccessObserver(), trust_token_redemption_policy);
frame->CreateCookieAccessObserver(), trust_token_redemption_policy,
debug_tag);
}

// static
Expand All @@ -140,7 +145,8 @@ URLLoaderFactoryParamsHelper::CreateForIsolatedWorld(
->GetOrCreateWebPreferences()
.allow_universal_access_from_file_urls,
true, // is_for_isolated_world
frame->CreateCookieAccessObserver(), trust_token_redemption_policy);
frame->CreateCookieAccessObserver(), trust_token_redemption_policy,
"ParamHelper::CreateForIsolatedWorld");
}

network::mojom::URLLoaderFactoryParamsPtr
Expand All @@ -164,7 +170,8 @@ URLLoaderFactoryParamsHelper::CreateForPrefetch(
.allow_universal_access_from_file_urls,
false, // is_for_isolated_world
frame->CreateCookieAccessObserver(),
network::mojom::TrustTokenRedemptionPolicy::kForbid);
network::mojom::TrustTokenRedemptionPolicy::kForbid,
"ParamHelper::CreateForPrefetch");
}

// static
Expand All @@ -174,7 +181,8 @@ URLLoaderFactoryParamsHelper::CreateForWorker(
const url::Origin& request_initiator,
const net::IsolationInfo& isolation_info,
mojo::PendingRemote<network::mojom::CrossOriginEmbedderPolicyReporter>
coep_reporter) {
coep_reporter,
base::StringPiece debug_tag) {
return CreateParams(
process,
request_initiator, // origin
Expand All @@ -192,7 +200,7 @@ URLLoaderFactoryParamsHelper::CreateForWorker(
// false in non-Document contexts, no worker should ever
// execute a trust token redemption or signing operation,
// as these operations require the Feature Policy feature.
network::mojom::TrustTokenRedemptionPolicy::kForbid);
network::mojom::TrustTokenRedemptionPolicy::kForbid, debug_tag);
}

// static
Expand Down Expand Up @@ -224,7 +232,8 @@ URLLoaderFactoryParamsHelper::CreateForRendererProcess(
mojo::NullRemote(), // coep_reporter
false, // allow_universal_access_from_file_urls
false, // is_for_isolated_world
mojo::NullRemote(), network::mojom::TrustTokenRedemptionPolicy::kForbid);
mojo::NullRemote(), network::mojom::TrustTokenRedemptionPolicy::kForbid,
"ParamHelper::CreateForRendererProcess");
}

} // namespace content
6 changes: 4 additions & 2 deletions content/browser/url_loader_factory_params_helper.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ class URLLoaderFactoryParamsHelper {
mojo::PendingRemote<network::mojom::CrossOriginEmbedderPolicyReporter>
coep_reporter,
RenderProcessHost* process,
network::mojom::TrustTokenRedemptionPolicy trust_token_redemption_policy);
network::mojom::TrustTokenRedemptionPolicy trust_token_redemption_policy,
base::StringPiece debug_tag);

// Creates URLLoaderFactoryParams to be used by |isolated_world_origin| hosted
// within the |frame|.
Expand All @@ -73,7 +74,8 @@ class URLLoaderFactoryParamsHelper {
const url::Origin& request_initiator,
const net::IsolationInfo& isolation_info,
mojo::PendingRemote<network::mojom::CrossOriginEmbedderPolicyReporter>
coep_reporter);
coep_reporter,
base::StringPiece debug_tag);

// TODO(kinuko, lukasza): https://crbug.com/1114822: Remove, once all
// URLLoaderFactories vended to a renderer process are associated with a
Expand Down
4 changes: 3 additions & 1 deletion content/browser/worker_host/dedicated_worker_host.cc
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,8 @@ DedicatedWorkerHost::CreateNetworkFactoryForSubresources(
ancestor_render_frame_host->IsFeatureEnabled(
blink::mojom::FeaturePolicyFeature::kTrustTokenRedemption)
? network::mojom::TrustTokenRedemptionPolicy::kPotentiallyPermit
: network::mojom::TrustTokenRedemptionPolicy::kForbid);
: network::mojom::TrustTokenRedemptionPolicy::kForbid,
"DedicatedWorkerHost::CreateNetworkFactoryForSubresources");
GetContentClient()->browser()->WillCreateURLLoaderFactory(
worker_process_host_->GetBrowserContext(),
/*frame=*/nullptr, worker_process_host_->GetID(),
Expand Down Expand Up @@ -486,6 +487,7 @@ void DedicatedWorkerHost::ObserveNetworkServiceCrash(
StoragePartitionImpl* storage_partition_impl) {
auto params = network::mojom::URLLoaderFactoryParams::New();
params->process_id = worker_process_host_->GetID();
params->debug_tag = "DedicatedWorkerHost::ObserveNetworkServiceCrash";
network_service_connection_error_handler_holder_.reset();
storage_partition_impl->GetNetworkContext()->CreateURLLoaderFactory(
network_service_connection_error_handler_holder_
Expand Down
3 changes: 2 additions & 1 deletion content/browser/worker_host/shared_worker_host.cc
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,8 @@ SharedWorkerHost::CreateNetworkFactoryParamsForSubresources() {
net::IsolationInfo::Create(net::IsolationInfo::RequestType::kOther,
origin, origin,
net::SiteForCookies::FromOrigin(origin)),
/*coep_reporter=*/mojo::NullRemote());
/*coep_reporter=*/mojo::NullRemote(),
/*debug_tag=*/"SharedWorkerHost::CreateNetworkFactoryForSubresource");
return factory_params;
}

Expand Down
3 changes: 2 additions & 1 deletion content/browser/worker_host/worker_script_fetch_initiator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,8 @@ void WorkerScriptFetchInitiator::CreateScriptLoader(
network::mojom::URLLoaderFactoryParamsPtr factory_params =
URLLoaderFactoryParamsHelper::CreateForWorker(
factory_process, request_initiator, trusted_isolation_info,
/*coep_reporter=*/mojo::NullRemote());
/*coep_reporter=*/mojo::NullRemote(),
/*debug_tag=*/"WorkerScriptFetchInitiator::CreateScriptLoader");

mojo::PendingReceiver<network::mojom::URLLoaderFactory>
default_factory_receiver =
Expand Down
3 changes: 3 additions & 0 deletions services/network/cors/cors_url_loader_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ CorsURLLoaderFactory::CorsURLLoaderFactory(
ignore_isolated_world_origin_(params->ignore_isolated_world_origin),
trust_token_redemption_policy_(params->trust_token_redemption_policy),
isolation_info_(params->isolation_info),
debug_tag_(params->debug_tag),
origin_access_list_(origin_access_list) {
DCHECK(context_);
DCHECK(origin_access_list_);
Expand Down Expand Up @@ -432,6 +433,8 @@ bool CorsURLLoaderFactory::IsValidRequest(const ResourceRequest& request,
url::debug::ScopedOriginCrashKey initiator_lock_crash_key(
debug::GetRequestInitiatorOriginLockCrashKey(),
base::OptionalOrNullptr(request_initiator_origin_lock_));
base::debug::ScopedCrashKeyString debug_tag_crash_key(
debug::GetFactoryDebugTagCrashKey(), debug_tag_);
mojo::ReportBadMessage(
"CorsURLLoaderFactory: lock VS initiator mismatch");
return false;
Expand Down
1 change: 1 addition & 0 deletions services/network/cors/cors_url_loader_factory.h
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CorsURLLoaderFactory final
const bool ignore_isolated_world_origin_;
const mojom::TrustTokenRedemptionPolicy trust_token_redemption_policy_;
net::IsolationInfo isolation_info_;
const std::string debug_tag_;

// Relative order of |network_loader_factory_| and |loaders_| matters -
// URLLoaderFactory needs to live longer than URLLoaders created using the
Expand Down
6 changes: 6 additions & 0 deletions services/network/crash_keys.cc
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ base::debug::CrashKeyString* GetRequestInitiatorOriginLockCrashKey() {
return crash_key;
}

base::debug::CrashKeyString* GetFactoryDebugTagCrashKey() {
static auto* crash_key = base::debug::AllocateCrashKeyString(
"url_loader_factory_debug_tag", base::debug::CrashKeySize::Size64);
return crash_key;
}

ScopedRequestCrashKeys::ScopedRequestCrashKeys(
const network::ResourceRequest& request)
: url_(GetRequestUrlCrashKey(), request.url.possibly_invalid_spec()),
Expand Down
1 change: 1 addition & 0 deletions services/network/crash_keys.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ struct ResourceRequest;
namespace debug {

base::debug::CrashKeyString* GetRequestInitiatorOriginLockCrashKey();
base::debug::CrashKeyString* GetFactoryDebugTagCrashKey();

class ScopedRequestCrashKeys {
public:
Expand Down
4 changes: 4 additions & 0 deletions services/network/public/mojom/network_context.mojom
Original file line number Diff line number Diff line change
Expand Up @@ -697,6 +697,10 @@ struct URLLoaderFactoryParams {
// trusted source, it would be good to set this depending on the headers'
// values, too.
TrustTokenRedemptionPolicy trust_token_redemption_policy = kPotentiallyPermit;

// TODO(lukasza): https://crbug.com/1151008: Consider removing this
// diagnostic aid once the bug is understood.
string debug_tag = "";
};

// The |credentials| output parameter is given to URLRequest::SetAuth()
Expand Down

0 comments on commit cfc799f

Please sign in to comment.