Commit 5271432a authored by Chong Zhang's avatar Chong Zhang Committed by Commit Bot

Introduce a SharedURLLoaderFactory wrapper for URLLoaderFactoryGetter

This CL changed the return type of
|URLLoaderFactoryGetter::GetNetworkFactory()| to
|scoped_refptr<SharedURLLoaderFactory>| in an effort to reduce the
usage of raw pointers.

The returned |SharedURLLoaderFactory| is basically a wrapper around
|URLLoaderFactoryGetter|, which holds a reference to
|URLLoaderFactoryGetter| and handles reconnection after crash.

It's safe to use the wrapper after the storage partition has gone, in
which case the url requests will be silently dropped.

Bug: 796425
Change-Id: Ie89b7f8933bc6e1c274cf42bc6bd2265c50c37d4
Reviewed-on: https://chromium-review.googlesource.com/935284
Commit-Queue: Chong Zhang <chongz@chromium.org>
Reviewed-by: default avatarMatt Menke <mmenke@chromium.org>
Reviewed-by: default avatarKinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540346}
parent 5b49db69
......@@ -21,7 +21,6 @@ void AppCacheUpdateJob::UpdateURLLoaderRequest::Start() {
network::mojom::URLLoaderClientPtr client;
client_binding_.Bind(mojo::MakeRequest(&client));
DCHECK(loader_factory_getter_->GetNetworkFactory());
loader_factory_getter_->GetNetworkFactory()->CreateLoaderAndStart(
mojo::MakeRequest(&url_loader_), -1, -1,
network::mojom::kURLLoadOptionNone, request_, std::move(client),
......
......@@ -47,6 +47,7 @@
#include "content/browser/storage_partition_impl.h"
#include "content/browser/web_contents/web_contents_impl.h"
#include "content/common/throttling_url_loader.h"
#include "content/common/wrapper_shared_url_loader_factory.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/content_browser_client.h"
#include "content/public/browser/download_item_utils.h"
......@@ -203,23 +204,26 @@ DownloadManagerImpl::UniqueUrlDownloadHandlerPtr BeginResourceDownload(
return nullptr;
}
network::mojom::URLLoaderFactory* url_loader_factory =
url_loader_factory_getter->GetNetworkFactory();
scoped_refptr<SharedURLLoaderFactory> shared_url_loader_factory;
if (params->url().SchemeIs(url::kBlobScheme)) {
network::mojom::URLLoaderFactoryPtr url_loader_factory_ptr;
network::mojom::URLLoaderFactoryPtrInfo url_loader_factory_ptr_info;
storage::BlobURLLoaderFactory::Create(
std::move(blob_data_handle), params->url(),
mojo::MakeRequest(&url_loader_factory_ptr));
url_loader_factory = url_loader_factory_ptr.get();
mojo::MakeRequest(&url_loader_factory_ptr_info));
shared_url_loader_factory =
base::MakeRefCounted<WrapperSharedURLLoaderFactory>(
std::move(url_loader_factory_ptr_info));
} else {
shared_url_loader_factory = url_loader_factory_getter->GetNetworkFactory();
}
// TODO(qinmin): Check the storage permission before creating the URLLoader.
// This is already done for context menu download, but it is missing for
// download service and download resumption.
return DownloadManagerImpl::UniqueUrlDownloadHandlerPtr(
ResourceDownloader::BeginDownload(download_manager, std::move(params),
std::move(request), url_loader_factory,
site_url, tab_url, tab_referrer_url,
download_id, false)
ResourceDownloader::BeginDownload(
download_manager, std::move(params), std::move(request),
std::move(shared_url_loader_factory), site_url, tab_url,
tab_referrer_url, download_id, false)
.release());
}
......
......@@ -11,6 +11,7 @@
#include "content/browser/download/resource_downloader.h"
#include "content/public/browser/web_contents.h"
#include "content/public/common/content_features.h"
#include "content/public/common/shared_url_loader_factory.h"
#include "services/network/public/cpp/features.h"
namespace content {
......
......@@ -11,6 +11,7 @@
#include "content/browser/download/download_utils.h"
#include "content/public/browser/web_contents.h"
#include "content/public/common/shared_url_loader_factory.h"
namespace network {
struct ResourceResponseHead;
......@@ -63,7 +64,7 @@ std::unique_ptr<ResourceDownloader> ResourceDownloader::BeginDownload(
base::WeakPtr<UrlDownloadHandler::Delegate> delegate,
std::unique_ptr<download::DownloadUrlParameters> params,
std::unique_ptr<network::ResourceRequest> request,
network::mojom::URLLoaderFactory* url_loader_factory,
scoped_refptr<SharedURLLoaderFactory> shared_url_loader_factory,
const GURL& site_url,
const GURL& tab_url,
const GURL& tab_referrer_url,
......@@ -74,7 +75,8 @@ std::unique_ptr<ResourceDownloader> ResourceDownloader::BeginDownload(
params->render_frame_host_routing_id(), site_url, tab_url,
tab_referrer_url, download_id);
downloader->Start(url_loader_factory, std::move(params), is_parallel_request);
downloader->Start(std::move(shared_url_loader_factory), std::move(params),
is_parallel_request);
return downloader;
}
......@@ -121,7 +123,7 @@ ResourceDownloader::ResourceDownloader(
ResourceDownloader::~ResourceDownloader() = default;
void ResourceDownloader::Start(
network::mojom::URLLoaderFactory* url_loader_factory,
scoped_refptr<SharedURLLoaderFactory> shared_url_loader_factory,
std::unique_ptr<download::DownloadUrlParameters> download_url_parameters,
bool is_parallel_request) {
callback_ = download_url_parameters->callback();
......@@ -145,7 +147,7 @@ void ResourceDownloader::Start(
// Set up the URLLoader
network::mojom::URLLoaderRequest url_loader_request =
mojo::MakeRequest(&url_loader_);
url_loader_factory->CreateLoaderAndStart(
shared_url_loader_factory->CreateLoaderAndStart(
std::move(url_loader_request),
0, // routing_id
0, // request_id
......
......@@ -11,10 +11,11 @@
#include "mojo/public/cpp/bindings/binding.h"
#include "services/network/public/cpp/resource_request.h"
#include "services/network/public/mojom/url_loader.mojom.h"
#include "services/network/public/mojom/url_loader_factory.mojom.h"
namespace content {
class SharedURLLoaderFactory;
// Class for handing the download of a url.
class ResourceDownloader : public UrlDownloadHandler,
public DownloadResponseHandler::Delegate {
......@@ -24,7 +25,7 @@ class ResourceDownloader : public UrlDownloadHandler,
base::WeakPtr<UrlDownloadHandler::Delegate> delegate,
std::unique_ptr<download::DownloadUrlParameters> download_url_parameters,
std::unique_ptr<network::ResourceRequest> request,
network::mojom::URLLoaderFactory* url_loader_factory,
scoped_refptr<SharedURLLoaderFactory> shared_url_loader_factory,
const GURL& site_url,
const GURL& tab_url,
const GURL& tab_referrer_url,
......@@ -64,7 +65,7 @@ class ResourceDownloader : public UrlDownloadHandler,
private:
// Helper method to start the network request.
void Start(
network::mojom::URLLoaderFactory* url_loader_factory,
scoped_refptr<SharedURLLoaderFactory> shared_url_loader_factory,
std::unique_ptr<download::DownloadUrlParameters> download_url_parameters,
bool is_parallel_request);
......
......@@ -477,8 +477,7 @@ class NavigationURLLoaderNetworkService::URLLoaderRequestController
handlers_.push_back(std::make_unique<WebPackageRequestHandler>(
url::Origin::Create(request_info->common_params.url),
GetURLLoaderOptions(request_info->is_main_frame),
base::MakeRefCounted<WeakWrapperSharedURLLoaderFactory>(
default_url_loader_factory_getter_->GetNetworkFactory()),
default_url_loader_factory_getter_->GetNetworkFactory(),
base::BindRepeating(
&URLLoaderRequestController::CreateURLLoaderThrottles,
base::Unretained(this))));
......@@ -585,8 +584,6 @@ class NavigationURLLoaderNetworkService::URLLoaderRequestController
factory = base::MakeRefCounted<WeakWrapperSharedURLLoaderFactory>(
non_network_factory.get());
} else {
auto* raw_factory =
default_url_loader_factory_getter_->GetNetworkFactory();
default_loader_used_ = true;
// NOTE: We only support embedders proxying network-service-bound requests
......@@ -598,12 +595,14 @@ class NavigationURLLoaderNetworkService::URLLoaderRequestController
if (proxied_factory_request_.is_pending() &&
!resource_request_->url.SchemeIs(url::kDataScheme)) {
DCHECK(proxied_factory_info_.is_valid());
raw_factory->Clone(std::move(proxied_factory_request_));
// TODO(chongz): |CloneNetworkFactory| doesn't support reconnection and
// we should find a way to avoid using it.
default_url_loader_factory_getter_->CloneNetworkFactory(
std::move(proxied_factory_request_));
factory = base::MakeRefCounted<WrapperSharedURLLoaderFactory>(
std::move(proxied_factory_info_));
} else {
factory = base::MakeRefCounted<WeakWrapperSharedURLLoaderFactory>(
raw_factory);
factory = default_url_loader_factory_getter_->GetNetworkFactory();
}
}
url_chain_.push_back(resource_request_->url);
......
......@@ -98,22 +98,10 @@ void PrefetchURLLoaderService::CreateLoaderAndStart(
const net::MutableNetworkTrafficAnnotationTag& traffic_annotation) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
DCHECK(base::FeatureList::IsEnabled(network::features::kNetworkService));
if (!network_loader_factory_ || network_loader_factory_.encountered_error()) {
loader_factory_getter_->GetNetworkFactory()->Clone(
mojo::MakeRequest(&network_loader_factory_));
}
int frame_tree_node_id = loader_factory_bindings_.dispatch_context();
CreateLoaderAndStart(
std::move(request), routing_id, request_id, options, resource_request,
std::move(client), traffic_annotation,
// NOTE: This should be fine in most cases, where the loader
// factory may become invalid if Network Service process is killed
// and restarted. The load can just fail in the case here, but if
// we want to be extra sure this should create a SharedURLLoaderFactory
// that internally holds a ref to the URLLoaderFactoryGetter so that
// it can re-obtain the factory if necessary.
base::MakeRefCounted<WeakWrapperSharedURLLoaderFactory>(
network_loader_factory_.get()),
CreateLoaderAndStart(std::move(request), routing_id, request_id, options,
resource_request, std::move(client), traffic_annotation,
loader_factory_getter_->GetNetworkFactory(),
frame_tree_node_id);
}
......
......@@ -101,7 +101,6 @@ class CONTENT_EXPORT PrefetchURLLoaderService final
ResourceContext* resource_context_ = nullptr;
scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
network::mojom::URLLoaderFactoryPtr network_loader_factory_;
mojo::BindingSet<network::mojom::URLLoaderFactory,
int /* frame_tree_node_id */>
loader_factory_bindings_;
......
......@@ -39,7 +39,7 @@ network::mojom::NetworkContextPtr CreateNetworkContext() {
}
int LoadBasicRequestOnIOThread(
URLLoaderFactoryGetter* url_loader_factory_getter,
network::mojom::URLLoaderFactory* url_loader_factory,
const GURL& url) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
auto request = std::make_unique<network::ResourceRequest>();
......@@ -53,21 +53,18 @@ int LoadBasicRequestOnIOThread(
network::SimpleURLLoader::Create(std::move(request),
TRAFFIC_ANNOTATION_FOR_TESTS);
// |URLLoaderFactoryGetter::GetNetworkFactory()| can only be accessed on IO
// thread.
BrowserThread::PostTask(
BrowserThread::IO, FROM_HERE,
base::BindOnce(
[](network::SimpleURLLoader* loader,
URLLoaderFactoryGetter* factory_getter,
network::mojom::URLLoaderFactory* factory,
network::SimpleURLLoader::BodyAsStringCallback
body_as_string_callback) {
loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
factory_getter->GetNetworkFactory(),
std::move(body_as_string_callback));
factory, std::move(body_as_string_callback));
},
base::Unretained(simple_loader.get()),
base::Unretained(url_loader_factory_getter),
base::Unretained(url_loader_factory),
simple_loader_helper.GetCallback()));
simple_loader_helper.WaitForCallback();
......@@ -91,6 +88,34 @@ int LoadBasicRequestOnUIThread(
return simple_loader->NetError();
}
scoped_refptr<SharedURLLoaderFactory> GetSharedFactoryOnIOThread(
URLLoaderFactoryGetter* url_loader_factory_getter) {
scoped_refptr<SharedURLLoaderFactory> shared_factory;
base::RunLoop run_loop;
BrowserThread::PostTaskAndReply(
BrowserThread::IO, FROM_HERE,
base::BindOnce(
[](URLLoaderFactoryGetter* getter,
scoped_refptr<SharedURLLoaderFactory>* shared_factory_ptr) {
*shared_factory_ptr = getter->GetNetworkFactory();
},
base::Unretained(url_loader_factory_getter),
base::Unretained(&shared_factory)),
run_loop.QuitClosure());
run_loop.Run();
return shared_factory;
}
void ReleaseOnIOThread(scoped_refptr<SharedURLLoaderFactory> shared_factory) {
BrowserThread::PostTask(
BrowserThread::IO, FROM_HERE,
base::BindOnce(
[](scoped_refptr<SharedURLLoaderFactory> factory) {
factory = nullptr;
},
std::move(shared_factory)));
}
} // namespace
// This test source has been excluded from Android as Android doesn't have
......@@ -301,8 +326,13 @@ IN_PROC_BROWSER_TEST_F(NetworkServiceRestartBrowserTest,
BrowserContext::GetDefaultStoragePartition(browser_context()));
scoped_refptr<URLLoaderFactoryGetter> url_loader_factory_getter =
partition->url_loader_factory_getter();
EXPECT_EQ(net::OK, LoadBasicRequestOnIOThread(url_loader_factory_getter.get(),
GetTestURL()));
scoped_refptr<SharedURLLoaderFactory> shared_factory =
GetSharedFactoryOnIOThread(url_loader_factory_getter.get());
EXPECT_EQ(net::OK,
LoadBasicRequestOnIOThread(shared_factory.get(), GetTestURL()));
ReleaseOnIOThread(std::move(shared_factory));
// Crash the NetworkService process. Existing interfaces should receive error
// notifications at some point.
SimulateNetworkServiceCrash();
......@@ -312,8 +342,61 @@ IN_PROC_BROWSER_TEST_F(NetworkServiceRestartBrowserTest,
// |url_loader_factory_getter| should be able to get a valid new pointer after
// crash.
EXPECT_EQ(net::OK, LoadBasicRequestOnIOThread(url_loader_factory_getter.get(),
GetTestURL()));
shared_factory = GetSharedFactoryOnIOThread(url_loader_factory_getter.get());
EXPECT_EQ(net::OK,
LoadBasicRequestOnIOThread(shared_factory.get(), GetTestURL()));
ReleaseOnIOThread(std::move(shared_factory));
}
// Make sure the factory returned from
// |URLLoaderFactoryGetter::GetNetworkFactory()| continues to work after
// crashes.
IN_PROC_BROWSER_TEST_F(NetworkServiceRestartBrowserTest,
BrowserIOSharedURLLoaderFactory) {
StoragePartitionImpl* partition = static_cast<StoragePartitionImpl*>(
BrowserContext::GetDefaultStoragePartition(browser_context()));
scoped_refptr<SharedURLLoaderFactory> shared_factory =
GetSharedFactoryOnIOThread(partition->url_loader_factory_getter().get());
EXPECT_EQ(net::OK,
LoadBasicRequestOnIOThread(shared_factory.get(), GetTestURL()));
// Crash the NetworkService process. Existing interfaces should receive error
// notifications at some point.
SimulateNetworkServiceCrash();
// Flush the interface to make sure the error notification was received.
partition->FlushNetworkInterfaceForTesting();
partition->url_loader_factory_getter()
->FlushNetworkInterfaceOnIOThreadForTesting();
// |shared_factory| should continue to work.
EXPECT_EQ(net::OK,
LoadBasicRequestOnIOThread(shared_factory.get(), GetTestURL()));
ReleaseOnIOThread(std::move(shared_factory));
}
// Make sure the factory returned from
// |URLLoaderFactoryGetter::GetNetworkFactory()| doesn't crash if
// it's called after the StoragePartition is deleted.
IN_PROC_BROWSER_TEST_F(NetworkServiceRestartBrowserTest,
BrowserIOSharedFactoryAfterStoragePartitionGone) {
base::ScopedAllowBlockingForTesting allow_blocking;
std::unique_ptr<ShellBrowserContext> browser_context =
std::make_unique<ShellBrowserContext>(true, nullptr);
auto* partition = static_cast<StoragePartitionImpl*>(
BrowserContext::GetDefaultStoragePartition(browser_context.get()));
scoped_refptr<content::SharedURLLoaderFactory> shared_factory(
GetSharedFactoryOnIOThread(partition->url_loader_factory_getter().get()));
EXPECT_EQ(net::OK,
LoadBasicRequestOnIOThread(shared_factory.get(), GetTestURL()));
browser_context.reset();
EXPECT_EQ(net::ERR_FAILED,
LoadBasicRequestOnIOThread(shared_factory.get(), GetTestURL()));
ReleaseOnIOThread(std::move(shared_factory));
}
// Make sure basic navigation works after crash.
......
......@@ -7,6 +7,7 @@
#include "base/bind.h"
#include "base/lazy_instance.h"
#include "content/browser/storage_partition_impl.h"
#include "content/public/common/shared_url_loader_factory.h"
#include "services/network/public/mojom/network_service.mojom.h"
namespace content {
......@@ -16,6 +17,46 @@ base::LazyInstance<URLLoaderFactoryGetter::GetNetworkFactoryCallback>::Leaky
g_get_network_factory_callback = LAZY_INSTANCE_INITIALIZER;
}
class URLLoaderFactoryGetter::URLLoaderFactoryForIOThread
: public SharedURLLoaderFactory {
public:
explicit URLLoaderFactoryForIOThread(
scoped_refptr<URLLoaderFactoryGetter> factory_getter)
: factory_getter_(std::move(factory_getter)) {}
// mojom::URLLoaderFactory implementation:
void CreateLoaderAndStart(network::mojom::URLLoaderRequest request,
int32_t routing_id,
int32_t request_id,
uint32_t options,
const network::ResourceRequest& url_request,
network::mojom::URLLoaderClientPtr client,
const net::MutableNetworkTrafficAnnotationTag&
traffic_annotation) override {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
if (!factory_getter_)
return;
factory_getter_->GetURLLoaderFactory()->CreateLoaderAndStart(
std::move(request), routing_id, request_id, options, url_request,
std::move(client), traffic_annotation);
}
// SharedURLLoaderFactory implementation:
std::unique_ptr<SharedURLLoaderFactoryInfo> Clone() override {
NOTREACHED() << "This isn't supported. If you need a SharedURLLoaderFactory"
" on the UI thread, get it from StoragePartition.";
return nullptr;
}
private:
friend class base::RefCounted<URLLoaderFactoryForIOThread>;
~URLLoaderFactoryForIOThread() override = default;
scoped_refptr<URLLoaderFactoryGetter> factory_getter_;
DISALLOW_COPY_AND_ASSIGN(URLLoaderFactoryForIOThread);
};
URLLoaderFactoryGetter::URLLoaderFactoryGetter() {}
void URLLoaderFactoryGetter::Initialize(StoragePartitionImpl* partition) {
......@@ -41,11 +82,19 @@ void URLLoaderFactoryGetter::OnStoragePartitionDestroyed() {
partition_ = nullptr;
}
network::mojom::URLLoaderFactory* URLLoaderFactoryGetter::GetNetworkFactory() {
scoped_refptr<SharedURLLoaderFactory>
URLLoaderFactoryGetter::GetNetworkFactory() {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
if (g_get_network_factory_callback.Get() && !test_factory_)
g_get_network_factory_callback.Get().Run(this);
return base::MakeRefCounted<URLLoaderFactoryForIOThread>(
base::WrapRefCounted(this));
}
network::mojom::URLLoaderFactory*
URLLoaderFactoryGetter::GetURLLoaderFactory() {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
if (test_factory_)
return test_factory_;
......@@ -59,6 +108,15 @@ network::mojom::URLLoaderFactory* URLLoaderFactoryGetter::GetNetworkFactory() {
return network_factory_.get();
}
void URLLoaderFactoryGetter::CloneNetworkFactory(
network::mojom::URLLoaderFactoryRequest network_factory_request) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
if (g_get_network_factory_callback.Get() && !test_factory_)
g_get_network_factory_callback.Get().Run(this);
GetURLLoaderFactory()->Clone(std::move(network_factory_request));
}
network::mojom::URLLoaderFactory* URLLoaderFactoryGetter::GetBlobFactory() {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
return blob_factory_.get();
......
......@@ -14,6 +14,7 @@
namespace content {
class SharedURLLoaderFactory;
class StoragePartitionImpl;
// Holds on to URLLoaderFactory for a given StoragePartition and allows code
......@@ -34,9 +35,16 @@ class URLLoaderFactoryGetter
// be called when the partition is going away.
void OnStoragePartitionDestroyed();
// Called on the IO thread to get the URLLoaderFactory to the network service.
// The pointer shouldn't be cached.
CONTENT_EXPORT network::mojom::URLLoaderFactory* GetNetworkFactory();
// Called on the IO thread to get a shared wrapper to this
// URLLoaderFactoryGetter, which can be used to access the URLLoaderFactory
// to the network service and supports auto-reconnect after crash.
CONTENT_EXPORT scoped_refptr<SharedURLLoaderFactory> GetNetworkFactory();
// Called on the IO thread. Will clone the internal factory to the network
// service which doesn't support auto-reconnect after crash.
// TODO(chongz): Remove this method and use |GetNetworkFactory()| instead.
CONTENT_EXPORT void CloneNetworkFactory(
network::mojom::URLLoaderFactoryRequest network_factory_request);
// Called on the IO thread to get the URLLoaderFactory to the blob service.
// The pointer shouldn't be cached.
......@@ -52,10 +60,10 @@ class URLLoaderFactoryGetter
return &network_factory_;
}
// When this global function is set, if GetNetworkFactory is called and
// |test_factory_| is null, then the callback will be run.
// This method must be called either on the IO thread or before threads start.
// This callback is run on the IO thread.
// When this global function is set, if GetNetworkFactory or
// CloneNetworkFactory is called and |test_factory_| is null, then the
// callback will be run. This method must be called either on the IO thread or
// before threads start. This callback is run on the IO thread.
using GetNetworkFactoryCallback = base::RepeatingCallback<void(
URLLoaderFactoryGetter* url_loader_factory_getter)>;
CONTENT_EXPORT static void SetGetNetworkFactoryCallbackForTesting(
......@@ -65,6 +73,8 @@ class URLLoaderFactoryGetter
CONTENT_EXPORT void FlushNetworkInterfaceOnIOThreadForTesting();
private:
class URLLoaderFactoryForIOThread;
friend class base::DeleteHelper<URLLoaderFactoryGetter>;
friend struct BrowserThread::DeleteOnThread<BrowserThread::IO>;
......@@ -77,6 +87,10 @@ class URLLoaderFactoryGetter
void HandleNetworkFactoryRequestOnUIThread(
network::mojom::URLLoaderFactoryRequest network_factory_request);
// Called on the IO thread to get the URLLoaderFactory to the network service.
// The pointer shouldn't be cached.
network::mojom::URLLoaderFactory* GetURLLoaderFactory();
// Call |network_factory_.FlushForTesting()|. For test use only.
void FlushNetworkInterfaceForTesting();
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment