Fix AppCache subresource loader factory not being proxied

The URLLoaderFactory created in
AppCacheHost::MaybePassSubresourceFactory() was not being passed through
ContentBrowserClient::WillCreateURLLoaderFactory(), so wasn't getting
picked up by things like AwProxyingURLLoaderFactory or
WebRequestProxyingURLLoaderFactory.

This change required threading the correct origin through to
AppCacheHost so it can call WillCreateURLLoaderFactory() with the
correct origin.

Bug: 998340
Change-Id: I3d417ca2d07e14909f9ebca13eef367160951b44
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2028164Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#736941}

content/browser/appcache/appcache_browsertest.cc

@@ -16,12 +16,14 @@
 #include "content/public/browser/ssl_status.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/content_client.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test.h"
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
+#include "content/test/test_content_browser_client.h"
 #include "net/cert/cert_status_flags.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
@@ -193,4 +195,128 @@ IN_PROC_BROWSER_TEST_F(AppCacheNetworkServiceBrowserTest,
   EXPECT_EQ(0, resource_request_count);
 }
 
+// Client which intercepts all requests using WillCreateURLLoaderFactory() and
+// logs them in intercepted_request_map().
+class LoaderFactoryInterceptingBrowserClient : public TestContentBrowserClient {
+ public:
+  bool WillCreateURLLoaderFactory(
+      BrowserContext* browser_context,
+      RenderFrameHost* frame,
+      int render_process_id,
+      URLLoaderFactoryType type,
+      const url::Origin& request_initiator,
+      base::Optional<int64_t> navigation_id,
+      mojo::PendingReceiver<network::mojom::URLLoaderFactory>* factory_receiver,
+      mojo::PendingRemote<network::mojom::TrustedURLLoaderHeaderClient>*
+          header_client,
+      bool* bypass_redirect_checks,
+      bool* disable_secure_dns,
+      network::mojom::URLLoaderFactoryOverridePtr* factory_override) override {
+    auto proxied_receiver = std::move(*factory_receiver);
+    mojo::PendingRemote<network::mojom::URLLoaderFactory> target_factory_remote;
+    *factory_receiver = target_factory_remote.InitWithNewPipeAndPassReceiver();
+
+    proxies_.push_back(std::make_unique<PassThroughURLLoaderFactory>(
+        std::move(proxied_receiver), std::move(target_factory_remote),
+        request_initiator, this));
+
+    return true;
+  }
+
+  const std::map<GURL, url::Origin>& intercepted_request_map() const {
+    return intercepted_request_map_;
+  }
+
+ private:
+  class PassThroughURLLoaderFactory : public network::mojom::URLLoaderFactory {
+   public:
+    PassThroughURLLoaderFactory(
+        mojo::PendingReceiver<network::mojom::URLLoaderFactory> receiver,
+        mojo::PendingRemote<network::mojom::URLLoaderFactory> target_factory,
+        const url::Origin& request_initiator,
+        LoaderFactoryInterceptingBrowserClient* client)
+        : target_factory_(std::move(target_factory)),
+          request_initiator_(request_initiator),
+          client_(client) {
+      proxy_receivers_.Add(this, std::move(receiver));
+    }
+
+    void CreateLoaderAndStart(
+        mojo::PendingReceiver<network::mojom::URLLoader> loader_receiver,
+        int32_t routing_id,
+        int32_t request_id,
+        uint32_t options,
+        const network::ResourceRequest& request,
+        mojo::PendingRemote<network::mojom::URLLoaderClient> client,
+        const net::MutableNetworkTrafficAnnotationTag& traffic_annotation)
+        override {
+      client_->intercepted_request_map_[request.url] = request_initiator_;
+      target_factory_->CreateLoaderAndStart(
+          std::move(loader_receiver), routing_id, request_id, options, request,
+          std::move(client), traffic_annotation);
+    }
+
+    void Clone(mojo::PendingReceiver<network::mojom::URLLoaderFactory>
+                   loader_receiver) override {
+      proxy_receivers_.Add(this, std::move(loader_receiver));
+    }
+
+   private:
+    mojo::Remote<network::mojom::URLLoaderFactory> target_factory_;
+    url::Origin request_initiator_;
+    LoaderFactoryInterceptingBrowserClient* client_;
+    mojo::ReceiverSet<network::mojom::URLLoaderFactory> proxy_receivers_;
+  };
+
+  std::map<GURL, url::Origin> intercepted_request_map_;
+  std::vector<std::unique_ptr<PassThroughURLLoaderFactory>> proxies_;
+};
+
+IN_PROC_BROWSER_TEST_F(AppCacheNetworkServiceBrowserTest,
+                       AppCacheRequestsAreProxied) {
+  LoaderFactoryInterceptingBrowserClient browser_client;
+  ContentBrowserClient* original_client =
+      SetBrowserClientForTesting(&browser_client);
+  net::EmbeddedTestServer embedded_test_server;
+  embedded_test_server.ServeFilesFromSourceDirectory(GetTestDataFilePath());
+
+  ASSERT_TRUE(embedded_test_server.Start());
+
+  GURL main_url =
+      embedded_test_server.GetURL("/appcache/simple_page_with_manifest.html");
+
+  base::string16 expected_title = base::ASCIIToUTF16("AppCache updated");
+
+  EXPECT_TRUE(NavigateToURL(shell(), main_url));
+  TitleWatcher title_watcher(shell()->web_contents(), expected_title);
+  EXPECT_EQ(expected_title, title_watcher.WaitAndGetTitle());
+
+  const auto& request_map = browser_client.intercepted_request_map();
+  EXPECT_EQ(request_map.count(main_url), 1u);
+
+  GURL subresource_url = embedded_test_server.GetURL("/appcache/logo.png");
+  EXPECT_EQ(request_map.count(subresource_url), 1u);
+  EXPECT_EQ(request_map.find(subresource_url)->second,
+            url::Origin::Create(embedded_test_server.base_url()));
+
+  GURL subresource_url2 = embedded_test_server.GetURL("/appcache/logo2.png");
+  EXPECT_EQ(request_map.count(subresource_url2), 0u);
+
+  const char kScript[] = R"(
+      new Promise(function (resolve, reject) {
+          var img = document.createElement('img');
+          img.src = '/appcache/logo2.png';
+          img.onload = _ => resolve('IMG LOADED');
+          img.onerror = reject;
+      })
+  )";
+  EXPECT_EQ("IMG LOADED", EvalJs(shell(), kScript));
+
+  EXPECT_EQ(request_map.count(subresource_url2), 1u);
+  EXPECT_EQ(request_map.find(subresource_url2)->second,
+            url::Origin::Create(embedded_test_server.base_url()));
+
+  SetBrowserClientForTesting(original_client);
+}
+
 }  // namespace content

content/browser/appcache/appcache_group_unittest.cc

@@ -6,12 +6,12 @@
 
 #include <string>
 
-#include "base/test/task_environment.h"
 #include "content/browser/appcache/appcache.h"
 #include "content/browser/appcache/appcache_group.h"
 #include "content/browser/appcache/appcache_host.h"
 #include "content/browser/appcache/appcache_update_job.h"
 #include "content/browser/appcache/mock_appcache_service.h"
+#include "content/public/test/browser_task_environment.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/receiver_set.h"
 #include "mojo/public/cpp/bindings/remote.h"
@@ -104,7 +104,7 @@ class TestAppCacheHost : public AppCacheHost {
 
 class AppCacheGroupTest : public testing::Test {
  private:
-  base::test::TaskEnvironment task_environment_;
+  BrowserTaskEnvironment task_environment_;
 };
 
 TEST_F(AppCacheGroupTest, AddRemoveCache) {

content/browser/appcache/appcache_host.cc

@@ -20,6 +20,7 @@
 #include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/common/appcache_interfaces.h"
+#include "content/public/common/content_client.h"
 #include "content/public/common/url_constants.h"
 #include "net/url_request/url_request.h"
 #include "services/network/public/mojom/url_loader_factory.mojom.h"
@@ -641,13 +642,24 @@ void AppCacheHost::MaybePassSubresourceFactory() {
     return;
 
   mojo::PendingRemote<network::mojom::URLLoaderFactory> factory_remote;
-  AppCacheSubresourceURLFactory::CreateURLLoaderFactory(GetWeakPtr(),
-                                                        &factory_remote);
+  auto factory_receiver = factory_remote.InitWithNewPipeAndPassReceiver();
+  auto* rfh = RenderFrameHost::FromID(process_id_, render_frame_id_);
+  if (rfh) {
+    GetContentClient()->browser()->WillCreateURLLoaderFactory(
+        rfh->GetProcess()->GetBrowserContext(), rfh, process_id_,
+        ContentBrowserClient::URLLoaderFactoryType::kDocumentSubResource,
+        origin_for_url_loader_factory_, base::nullopt /* navigation_id */,
+        &factory_receiver, nullptr /* header_client */,
+        nullptr /* bypass_redirect_checks */, nullptr /* disable_secure_dns */,
+        nullptr /* factory_override */);
+  }
 
   // We may not have bound |factory_remote| if the storage partition has shut
   // down.
-  if (factory_remote)
+  if (AppCacheSubresourceURLFactory::CreateURLLoaderFactory(
+          GetWeakPtr(), std::move(factory_receiver))) {
     frontend()->SetSubresourceFactory(std::move(factory_remote));
+  }
 }
 
 void AppCacheHost::SetAppCacheSubresourceFactory(

content/browser/appcache/appcache_host.h

@@ -229,6 +229,10 @@ class CONTENT_EXPORT AppCacheHost : public blink::mojom::AppCacheHost,
     site_for_cookies_initialized_ = true;
   }
 
+  void set_origin_for_url_loader_factory(const url::Origin& origin) {
+    origin_for_url_loader_factory_ = origin;
+  }
+
   // Returns a weak pointer reference to the host.
   base::WeakPtr<AppCacheHost> GetWeakPtr();
 
@@ -400,6 +404,10 @@ class CONTENT_EXPORT AppCacheHost : public blink::mojom::AppCacheHost,
   // Used to inform the QuotaManager of what origins are currently in use.
   url::Origin origin_in_use_;
 
+  // The origin used when calling
+  // ContentBrowserClient::WillCreateURLLoaderFactory().
+  url::Origin origin_for_url_loader_factory_;
+
   // To be used in policy checks.
   net::SiteForCookies site_for_cookies_;
   bool site_for_cookies_initialized_ = false;

content/browser/appcache/appcache_request_handler.cc

@@ -637,8 +637,8 @@ AppCacheRequestHandler::MaybeCreateSubresourceLoaderParams() {
   // The factory is destroyed when the renderer drops the connection.
   mojo::PendingRemote<network::mojom::URLLoaderFactory> factory_remote;
 
-  AppCacheSubresourceURLFactory::CreateURLLoaderFactory(appcache_host_,
-                                                        &factory_remote);
+  AppCacheSubresourceURLFactory::CreateURLLoaderFactory(
+      appcache_host_, factory_remote.InitWithNewPipeAndPassReceiver());
 
   SubresourceLoaderParams params;
   params.pending_appcache_loader_factory = std::move(factory_remote);

content/browser/appcache/appcache_subresource_url_factory.cc

@@ -333,17 +333,18 @@ AppCacheSubresourceURLFactory::AppCacheSubresourceURLFactory(
                           base::Unretained(this)));
 }
 
-AppCacheSubresourceURLFactory::~AppCacheSubresourceURLFactory() {}
+AppCacheSubresourceURLFactory::~AppCacheSubresourceURLFactory() = default;
 
 // static
-void AppCacheSubresourceURLFactory::CreateURLLoaderFactory(
+bool AppCacheSubresourceURLFactory::CreateURLLoaderFactory(
     base::WeakPtr<AppCacheHost> host,
-    mojo::PendingRemote<network::mojom::URLLoaderFactory>* loader_factory) {
+    mojo::PendingReceiver<network::mojom::URLLoaderFactory>
+        loader_factory_receiver) {
   DCHECK(host.get());
   scoped_refptr<network::SharedURLLoaderFactory> network_loader_factory;
   // The partition has shutdown, return without binding |loader_factory|.
   if (!host->service()->partition())
-    return;
+    return false;
   network_loader_factory =
       host->service()
           ->partition()
@@ -353,11 +354,12 @@ void AppCacheSubresourceURLFactory::CreateURLLoaderFactory(
   // Please see OnMojoDisconnect() for details.
   auto* impl = new AppCacheSubresourceURLFactory(
       std::move(network_loader_factory), host);
-  impl->Clone(loader_factory->InitWithNewPipeAndPassReceiver());
+  impl->Clone(std::move(loader_factory_receiver));
 
   // Save the factory in the host to ensure that we don't create it again when
   // the cache is selected, etc.
   host->SetAppCacheSubresourceFactory(impl);
+  return true;
 }
 
 void AppCacheSubresourceURLFactory::CreateLoaderAndStart(

content/browser/appcache/appcache_subresource_url_factory.h

@@ -35,10 +35,11 @@ class CONTENT_EXPORT AppCacheSubresourceURLFactory
   // Factory function to create an instance of the factory.
   // The |host| parameter contains the appcache host instance. This is used
   // to create the AppCacheRequestHandler instances for handling subresource
-  // requests.
-  static void CreateURLLoaderFactory(
+  // requests. Returns true if the factory was successfully created.
+  static bool CreateURLLoaderFactory(
       base::WeakPtr<AppCacheHost> host,
-      mojo::PendingRemote<network::mojom::URLLoaderFactory>* loader_factory);
+      mojo::PendingReceiver<network::mojom::URLLoaderFactory>
+          loader_factory_receiver);
 
   // network::mojom::URLLoaderFactory implementation.
   void CreateLoaderAndStart(

content/browser/appcache/appcache_unittest.cc

@@ -9,10 +9,10 @@
 #include <vector>
 
 #include "base/macros.h"
-#include "base/test/task_environment.h"
 #include "content/browser/appcache/appcache.h"
 #include "content/browser/appcache/appcache_host.h"
 #include "content/browser/appcache/mock_appcache_service.h"
+#include "content/public/test/browser_task_environment.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/mojom/appcache/appcache.mojom.h"
@@ -22,7 +22,7 @@
 namespace content {
 
 class AppCacheTest : public testing::Test {
-  base::test::TaskEnvironment task_environment_;
+  BrowserTaskEnvironment task_environment_;
 };
 
 TEST_F(AppCacheTest, CleanupUnusedCache) {

content/browser/frame_host/navigation_request.h

@@ -458,6 +458,10 @@ class CONTENT_EXPORT NavigationRequest
 
   std::unique_ptr<AppCacheNavigationHandle> TakeAppCacheHandle();
 
+  AppCacheNavigationHandle* appcache_handle() const {
+    return appcache_handle_.get();
+  }
+
   void set_complete_callback_for_testing(
       ThrottleChecksFinishedCallback callback) {
     complete_callback_for_testing_ = std::move(callback);

content/browser/frame_host/render_frame_host_impl.cc

@@ -5413,6 +5413,15 @@ void RenderFrameHostImpl::CommitNavigation(
 
   url::Origin main_world_origin_for_url_loader_factory =
       GetOriginForURLLoaderFactory(navigation_request);
+  if (navigation_request && navigation_request->appcache_handle()) {
+    // AppCache may create a subresource URLLoaderFactory later, so make sure it
+    // has the correct origin to use when calling
+    // ContentBrowserClient::WillCreateURLLoaderFactory().
+    navigation_request->appcache_handle()
+        ->host()
+        ->set_origin_for_url_loader_factory(
+            main_world_origin_for_url_loader_factory);
+  }
   std::unique_ptr<blink::PendingURLLoaderFactoryBundle>
       subresource_loader_factories;
   if ((!is_same_document || is_first_navigation) && !is_srcdoc) {