0

Navigation: Separate loading start from ctor of NavigationURLLoader

NavigationURLLoaderImpl and CachedNavigationURLLoader start loading from
their constructor. This is troublesome when we make loading by
CachedNavigationURLLoader synchronous for prerender activation.
See https://crrev.com/c/2992411/45..48/content/browser/renderer_host/navigation_request.cc#b3534

To be more specific, if it's synchronous, NavigationRequest that is the
caller of CachedNavigationURLLoader's constructor can be destroyed
before exiting from the constructor. After the constructor,
NavigationRequest attempts to set NavigationRequest::loader_ to the
returned loader but crashes as NavigationRequest has already been gone.

To avoid this, this CL moves loading start from the constructor to a
separate function Start(). This shouldn't change existing behavior.

Bug: 1195751
Change-Id: I8eead5efe0c82d7a778f20d96ccc2147b25b6b44
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3011311
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#899757}
This commit is contained in:
Hiroki Nakagawa
2021-07-08 23:29:11 +00:00
committed by Chromium LUCI CQ
parent 8468c9639d
commit 6d377e4a66
10 changed files with 62 additions and 31 deletions

@ -24,17 +24,7 @@ CachedNavigationURLLoader::CachedNavigationURLLoader(
network::mojom::URLResponseHeadPtr cached_response_head)
: request_info_(std::move(request_info)),
delegate_(delegate),
cached_response_head_(std::move(cached_response_head)) {
// Respond with a fake response. We use PostTask here to mimic the flow of
// a normal navigation.
//
// Normal navigations never call OnResponseStarted on the same message loop
// iteration that the NavigationURLLoader is created, because they have to
// make a network request.
GetUIThreadTaskRunner({})->PostTask(
FROM_HERE, base::BindOnce(&CachedNavigationURLLoader::OnResponseStarted,
weak_factory_.GetWeakPtr()));
}
cached_response_head_(std::move(cached_response_head)) {}
void CachedNavigationURLLoader::OnResponseStarted() {
GlobalRequestID global_id = GlobalRequestID::MakeBrowserInitiated();
@ -58,6 +48,18 @@ std::unique_ptr<NavigationURLLoader> CachedNavigationURLLoader::Create(
std::move(request_info), delegate, std::move(cached_response_head));
}
void CachedNavigationURLLoader::Start() {
// Respond with a fake response. We use PostTask here to mimic the flow of
// a normal navigation.
//
// Normal navigations never call OnResponseStarted on the same message loop
// iteration that the NavigationURLLoader is created, because they have to
// make a network request.
GetUIThreadTaskRunner({})->PostTask(
FROM_HERE, base::BindOnce(&CachedNavigationURLLoader::OnResponseStarted,
weak_factory_.GetWeakPtr()));
}
void CachedNavigationURLLoader::FollowRedirect(
const std::vector<std::string>& removed_headers,
const net::HttpRequestHeaders& modified_headers,

@ -28,6 +28,7 @@ class CachedNavigationURLLoader : public NavigationURLLoader {
network::mojom::URLResponseHeadPtr cached_response_head);
// NavigationURLLoader implementation.
void Start() override;
void FollowRedirect(
const std::vector<std::string>& removed_headers,
const net::HttpRequestHeaders& modified_headers,

@ -80,6 +80,9 @@ class CONTENT_EXPORT NavigationURLLoader {
virtual ~NavigationURLLoader() {}
// Called right after the loader is constructed.
virtual void Start() = 0;
// Called in response to OnRequestRedirected to continue processing the
// request. |new_previews_state| will be updated for newly created URLLoaders,
// but the existing default URLLoader will not see |new_previews_state| unless

@ -326,7 +326,7 @@ NavigationURLLoaderImpl::~NavigationURLLoaderImpl() {
}
}
void NavigationURLLoaderImpl::Start(
void NavigationURLLoaderImpl::StartImpl(
scoped_refptr<network::SharedURLLoaderFactory> network_loader_factory,
AppCacheNavigationHandle* appcache_handle,
scoped_refptr<PrefetchedSignedExchangeCache>
@ -1284,11 +1284,17 @@ NavigationURLLoaderImpl::NavigationURLLoaderImpl(
std::move(factory_remote));
}
Start(network_factory, appcache_handle,
std::move(prefetched_signed_exchange_cache),
std::move(signed_exchange_prefetch_metric_recorder),
std::move(factory_for_webui), std::move(accept_langs),
needs_loader_factory_interceptor);
start_closure_ =
base::BindOnce(&NavigationURLLoaderImpl::StartImpl,
base::Unretained(this), network_factory, appcache_handle,
std::move(prefetched_signed_exchange_cache),
std::move(signed_exchange_prefetch_metric_recorder),
std::move(factory_for_webui), std::move(accept_langs),
needs_loader_factory_interceptor);
}
void NavigationURLLoaderImpl::Start() {
std::move(start_closure_).Run();
}
void NavigationURLLoaderImpl::FollowRedirect(

@ -68,20 +68,6 @@ class CONTENT_EXPORT NavigationURLLoaderImpl
// TODO(kinuko): Some method parameters can probably be just kept as
// member variables rather than being passed around.
// Starts the loader by finalizing loader factories initialization and
// calling Restart().
// This is called only once (while Restart can be called multiple times).
// Sets |started_| true.
void Start(
scoped_refptr<network::SharedURLLoaderFactory> network_loader_factory,
AppCacheNavigationHandle* appcache_handle,
scoped_refptr<PrefetchedSignedExchangeCache>
prefetched_signed_exchange_cache,
scoped_refptr<SignedExchangePrefetchMetricRecorder>
signed_exchange_prefetch_metric_recorder,
mojo::PendingRemote<network::mojom::URLLoaderFactory> factory_for_webui,
std::string accept_langs,
bool needs_loader_factory_interceptor);
void CreateInterceptors(AppCacheNavigationHandle* appcache_handle,
scoped_refptr<PrefetchedSignedExchangeCache>
prefetched_signed_exchange_cache,
@ -160,6 +146,7 @@ class CONTENT_EXPORT NavigationURLLoaderImpl
base::OnceClosure continuation);
// NavigationURLLoader implementation:
void Start() override;
void FollowRedirect(
const std::vector<std::string>& removed_headers,
const net::HttpRequestHeaders& modified_headers,
@ -195,6 +182,21 @@ class CONTENT_EXPORT NavigationURLLoaderImpl
StoragePartitionImpl* partition);
private:
// Starts the loader by finalizing loader factories initialization and
// calling Restart().
// This is called only once (while Restart can be called multiple times).
// Sets |started_| true.
void StartImpl(
scoped_refptr<network::SharedURLLoaderFactory> network_loader_factory,
AppCacheNavigationHandle* appcache_handle,
scoped_refptr<PrefetchedSignedExchangeCache>
prefetched_signed_exchange_cache,
scoped_refptr<SignedExchangePrefetchMetricRecorder>
signed_exchange_prefetch_metric_recorder,
mojo::PendingRemote<network::mojom::URLLoaderFactory> factory_for_webui,
std::string accept_langs,
bool needs_loader_factory_interceptor);
void BindNonNetworkURLLoaderFactoryReceiver(
const GURL& url,
mojo::PendingReceiver<network::mojom::URLLoaderFactory> factory_receiver);
@ -311,6 +313,10 @@ class CONTENT_EXPORT NavigationURLLoaderImpl
std::unique_ptr<NavigationEarlyHintsManager> early_hints_manager_;
// Set on the constructor and runs in Start(). This is used for transferring
// parameters prepared in the constructor to Start().
base::OnceClosure start_closure_;
base::WeakPtrFactory<NavigationURLLoaderImpl> weak_factory_{this};
DISALLOW_COPY_AND_ASSIGN(NavigationURLLoaderImpl);

@ -254,6 +254,7 @@ class NavigationURLLoaderImplTest : public testing::Test {
base::StringPrintf("%s: %s", net::HttpRequestHeaders::kOrigin,
redirect_url.GetOrigin().spec().c_str()),
request_method, &delegate);
loader->Start();
delegate.WaitForRequestRedirected();
loader->FollowRedirect({}, {}, {}, blink::PreviewsTypes::PREVIEWS_OFF);
@ -293,6 +294,7 @@ class NavigationURLLoaderImplTest : public testing::Test {
url.GetOrigin().spec().c_str()),
"GET", &delegate, blink::NavigationDownloadPolicy(),
true /*is_main_frame*/, upgrade_if_insecure);
loader->Start();
delegate.WaitForRequestRedirected();
loader->FollowRedirect({}, {}, {}, blink::PreviewsTypes::PREVIEWS_OFF);
if (expect_request_fail) {
@ -325,6 +327,7 @@ TEST_F(NavigationURLLoaderImplTest, IsolationInfoOfMainFrameNavigation) {
url.GetOrigin().spec().c_str()),
"GET", &delegate, blink::NavigationDownloadPolicy(),
true /*is_main_frame*/, false /*upgrade_if_insecure*/);
loader->Start();
delegate.WaitForResponseStarted();
ASSERT_TRUE(most_recent_resource_request_);
@ -434,6 +437,7 @@ TEST_F(NavigationURLLoaderImplTest, RedirectModifiedHeaders) {
TestNavigationURLLoaderDelegate delegate;
std::unique_ptr<NavigationURLLoader> loader = CreateTestLoader(
redirect_url, "Header1: Value1\r\nHeader2: Value2", "GET", &delegate);
loader->Start();
delegate.WaitForRequestRedirected();
ASSERT_TRUE(most_recent_resource_request_);

@ -117,6 +117,7 @@ TEST_F(NavigationURLLoaderTest, RequestFailedNoCertError) {
TestNavigationURLLoaderDelegate delegate;
std::unique_ptr<NavigationURLLoader> loader =
MakeTestLoader(GURL("bogus:bogus"), &delegate);
loader->Start();
// Wait for the request to fail as expected.
delegate.WaitForRequestFailed();
@ -136,6 +137,7 @@ TEST_F(NavigationURLLoaderTest, RequestFailedCertError) {
TestNavigationURLLoaderDelegate delegate;
std::unique_ptr<NavigationURLLoader> loader =
MakeTestLoader(https_server.GetURL("/"), &delegate);
loader->Start();
// Wait for the request to fail as expected.
delegate.WaitForRequestFailed();
@ -169,6 +171,7 @@ TEST_F(NavigationURLLoaderTest, RequestFailedCertErrorFatal) {
TestNavigationURLLoaderDelegate delegate;
std::unique_ptr<NavigationURLLoader> loader = MakeTestLoader(url, &delegate);
loader->Start();
// Wait for the request to fail as expected.
delegate.WaitForRequestFailed();

@ -3485,6 +3485,7 @@ void NavigationRequest::OnStartChecksComplete(
frame_tree_node_->frame_tree_node_id()),
NetworkServiceDevToolsObserver::MakeSelfOwned(frame_tree_node_),
std::move(cached_response_head), std::move(interceptor));
loader_->Start();
DCHECK(!render_frame_host_);
}

@ -31,6 +31,10 @@ TestNavigationURLLoader::TestNavigationURLLoader(
redirect_count_(0),
loader_type_(loader_type) {}
void TestNavigationURLLoader::Start() {
// Do nothing.
}
void TestNavigationURLLoader::FollowRedirect(
const std::vector<std::string>& removed_headers,
const net::HttpRequestHeaders& modified_headers,

@ -31,6 +31,7 @@ class TestNavigationURLLoader
NavigationURLLoader::LoaderType loader_type);
// NavigationURLLoader implementation.
void Start() override;
void FollowRedirect(
const std::vector<std::string>& removed_headers,
const net::HttpRequestHeaders& modified_headers,