Make URLLoaderFactoryGetter available w/o Network Service

This CL makes |StoragePartitionImpl::url_loader_factory_getter_|
available even when Network Service is disable, such that
|StoragePartitionImpl::GetURLLoaderFactoryForBrowserProcessIOThread()|
can be used in all cases.

Note that |URLLoaderFactoryGetter::GetBlobFactory()| is only available
when the network service or servicified service worker is enabled.

This CL also affects some functional unrelated tests, which are tracked
as TODOs and will be addressed in followup CLs.

Bug: 826869
Change-Id: If408dc20f5e4564a5865c93a4c66316d5682d9e2
Reviewed-on: https://chromium-review.googlesource.com/1033728
Commit-Queue: Chong Zhang <chongz@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555984}

content/browser/network_service_restart_browsertest.cc

@@ -188,6 +188,22 @@ void WaitForCondition(base::RepeatingCallback<bool()> condition) {
 
 }  // namespace
 
+// TODO(crbug.com/826869): These tests don't have to have Network Service
+// enabled and should be merged into 'storage_partition_impl_browsertest.cc'.
+class NetworkServiceStoragePartititionBrowsertest : public ContentBrowserTest {
+ public:
+  NetworkServiceStoragePartititionBrowsertest() = default;
+
+  GURL GetTestURL() const {
+    // Use '/echoheader' instead of '/echo' to avoid a disk_cache bug.
+    // See https://crbug.com/792255.
+    return embedded_test_server()->GetURL("/echoheader");
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(NetworkServiceStoragePartititionBrowsertest);
+};
+
 // This test source has been excluded from Android as Android doesn't have
 // out-of-process Network Service.
 class NetworkServiceRestartBrowserTest : public ContentBrowserTest {
@@ -576,11 +592,33 @@ IN_PROC_BROWSER_TEST_F(NetworkServiceRestartBrowserTest, BrowserIOFactory) {
   ReleaseOnIOThread(std::move(shared_url_loader_factory));
 }
 
+// Make sure the factory getter returned from
+// |StoragePartition::GetURLLoaderFactoryForBrowserProcessIOThread()| works.
+IN_PROC_BROWSER_TEST_F(NetworkServiceStoragePartititionBrowsertest,
+                       GetURLLoaderFactoryForBrowserProcessIOThread) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  base::ScopedAllowBlockingForTesting allow_blocking;
+  auto shared_url_loader_factory_info =
+      BrowserContext::GetDefaultStoragePartition(
+          shell()->web_contents()->GetBrowserContext())
+          ->GetURLLoaderFactoryForBrowserProcessIOThread();
+
+  scoped_refptr<network::SharedURLLoaderFactory> shared_url_loader_factory =
+      GetSharedFactoryOnIOThread(std::move(shared_url_loader_factory_info));
+
+  EXPECT_EQ(net::OK, LoadBasicRequestOnIOThread(shared_url_loader_factory.get(),
+                                                GetTestURL()));
+  ReleaseOnIOThread(std::move(shared_url_loader_factory));
+}
+
 // Make sure the factory getter returned from
 // |StoragePartition::GetURLLoaderFactoryForBrowserProcessIOThread()| doesn't
 // crash if it's called after the StoragePartition is deleted.
-IN_PROC_BROWSER_TEST_F(NetworkServiceRestartBrowserTest,
+IN_PROC_BROWSER_TEST_F(NetworkServiceStoragePartititionBrowsertest,
                        BrowserIOFactoryGetterAfterStoragePartitionGone) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
   base::ScopedAllowBlockingForTesting allow_blocking;
   std::unique_ptr<ShellBrowserContext> browser_context =
       std::make_unique<ShellBrowserContext>(true, nullptr);
@@ -603,8 +641,10 @@ IN_PROC_BROWSER_TEST_F(NetworkServiceRestartBrowserTest,
 // Make sure the factory returned from
 // |StoragePartition::GetURLLoaderFactoryForBrowserProcessIOThread()| doesn't
 // crash if it's called after the StoragePartition is deleted.
-IN_PROC_BROWSER_TEST_F(NetworkServiceRestartBrowserTest,
+IN_PROC_BROWSER_TEST_F(NetworkServiceStoragePartititionBrowsertest,
                        BrowserIOFactoryAfterStoragePartitionGone) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
   base::ScopedAllowBlockingForTesting allow_blocking;
   std::unique_ptr<ShellBrowserContext> browser_context =
       std::make_unique<ShellBrowserContext>(true, nullptr);

content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc

@@ -48,24 +48,20 @@ url::Origin SecurityOrigin() {
 
 // TestBrowserContext has a URLRequestContextGetter which uses a NullTaskRunner.
 // This causes it to be destroyed on the wrong thread. This BrowserContext
-// instead uses the IO thread task runner for the URLRequestContextGetter.
-class TestBrowserContextWithRealURLRequestContextGetter
+// instead returns nullptr since it's not required by the test.
+class TestBrowserContextWithoutURLRequestContextGetter
     : public TestBrowserContext {
  public:
-  TestBrowserContextWithRealURLRequestContextGetter() {
-    request_context_ =
-        base::MakeRefCounted<net::TrivialURLRequestContextGetter>(
-            &context_,
-            BrowserThread::GetTaskRunnerForThread(BrowserThread::IO));
+  TestBrowserContextWithoutURLRequestContextGetter() {
     salt_ = TestBrowserContext::GetMediaDeviceIDSalt();
   }
 
-  ~TestBrowserContextWithRealURLRequestContextGetter() override {}
+  ~TestBrowserContextWithoutURLRequestContextGetter() override {}
 
   net::URLRequestContextGetter* CreateRequestContext(
       ProtocolHandlerMap* protocol_handlers,
       URLRequestInterceptorScopedVector request_interceptors) override {
-    return request_context_.get();
+    return nullptr;
   }
 
   std::string GetMediaDeviceIDSalt() override { return salt_; }
@@ -73,8 +69,6 @@ class TestBrowserContextWithRealURLRequestContextGetter
   void set_media_device_id_salt(std::string salt) { salt_ = std::move(salt); }
 
  private:
-  net::TestURLRequestContext context_;
-  scoped_refptr<net::URLRequestContextGetter> request_context_;
   std::string salt_;
 };
 
@@ -96,7 +90,7 @@ class AudioOutputAuthorizationHandlerTest : public RenderViewHostTestHarness {
 
   BrowserContext* CreateBrowserContext() override {
     // Caller takes ownership.
-    return new TestBrowserContextWithRealURLRequestContextGetter();
+    return new TestBrowserContextWithoutURLRequestContextGetter();
   }
 
   void SetUp() override {
@@ -411,7 +405,7 @@ TEST_F(AudioOutputAuthorizationHandlerTest,
   // Reset the salt and expect authorization of the device ID hashed with
   // the old salt to fail.
   auto* context =
-      static_cast<TestBrowserContextWithRealURLRequestContextGetter*>(
+      static_cast<TestBrowserContextWithoutURLRequestContextGetter*>(
           browser_context());
   context->set_media_device_id_salt("new salt");
   EXPECT_CALL(listener, Run(media::OUTPUT_DEVICE_STATUS_ERROR_NOT_FOUND, _, _,

content/browser/storage_partition_impl.cc

@@ -657,11 +657,11 @@ std::unique_ptr<StoragePartitionImpl> StoragePartitionImpl::Create(
         base::BindOnce(&BlobStorageContextGetterForStorage, blob_context);
     partition->blob_url_loader_factory_ =
         BlobURLLoaderFactory::Create(std::move(blob_getter));
-
-    partition->url_loader_factory_getter_ = new URLLoaderFactoryGetter();
-    partition->url_loader_factory_getter_->Initialize(partition.get());
   }
 
+  partition->url_loader_factory_getter_ = new URLLoaderFactoryGetter();
+  partition->url_loader_factory_getter_->Initialize(partition.get());
+
   partition->service_worker_context_->Init(
       path, quota_manager_proxy.get(), context->GetSpecialStoragePolicy(),
       blob_context.get(), partition->url_loader_factory_getter_.get());

content/browser/storage_partition_impl_map.cc

@@ -436,15 +436,17 @@ StoragePartitionImpl* StoragePartitionImplMap::Get(
       browser_context_->CreateMediaRequestContextForStoragePartition(
           partition->GetPath(), in_memory));
 
-  if (ServiceWorkerUtils::IsServicificationEnabled() &&
-      !base::FeatureList::IsEnabled(network::features::kNetworkService)) {
+  if (!base::FeatureList::IsEnabled(network::features::kNetworkService)) {
     // This needs to happen after SetURLRequestContext() since we need this
     // code path only for non-NetworkService case where NetworkContext needs to
     // be initialized using |url_request_context_|, which is initialized by
     // SetURLRequestContext().
     DCHECK(partition->url_loader_factory_getter());
-    DCHECK(partition->url_request_context_);
-    partition->url_loader_factory_getter()->HandleFactoryRequests();
+    // TODO(crbug.com/826869): |url_request_context_| is not configured
+    // correctly in some unittests. We should fix those tests and turn this 'if'
+    // into a DCHECK.
+    if (partition->url_request_context_)
+      partition->url_loader_factory_getter()->HandleFactoryRequests();
   }
 
   PostCreateInitialization(partition, in_memory);

content/browser/url_loader_factory_getter.cc

@@ -8,6 +8,7 @@
 #include "base/feature_list.h"
 #include "base/lazy_instance.h"
 #include "content/browser/storage_partition_impl.h"
+#include "content/common/service_worker/service_worker_utils.h"
 #include "services/network/public/cpp/features.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/network/public/mojom/network_service.mojom.h"
@@ -136,8 +137,16 @@ void URLLoaderFactoryGetter::HandleFactoryRequests() {
   DCHECK(pending_blob_factory_request_.is_pending());
   HandleNetworkFactoryRequestOnUIThread(
       std::move(pending_network_factory_request_));
-  partition_->GetBlobURLLoaderFactory()->HandleRequest(
-      std::move(pending_blob_factory_request_));
+
+  // |partition->blob_url_loader_factory_| is not available without the feature.
+  if (base::FeatureList::IsEnabled(network::features::kNetworkService) ||
+      ServiceWorkerUtils::IsServicificationEnabled()) {
+    DCHECK(partition_->GetBlobURLLoaderFactory());
+    partition_->GetBlobURLLoaderFactory()->HandleRequest(
+        std::move(pending_blob_factory_request_));
+  } else {
+    pending_blob_factory_request_ = nullptr;
+  }
 }
 
 void URLLoaderFactoryGetter::OnStoragePartitionDestroyed() {

content/browser/url_loader_factory_getter.h

@@ -67,7 +67,8 @@ class URLLoaderFactoryGetter
       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.
+  // Must be used only if the network service or servicified service worker is
+  // enabled. The pointer shouldn't be cached.
   CONTENT_EXPORT network::mojom::URLLoaderFactory* GetBlobFactory();
 
   // Overrides the network URLLoaderFactory for subsequent requests. Passing a