Fix and re-enable separate URLLoaderFactories for extensions (AppCache).

Overview
========

This CL re-enables separate URLLoaderFactories for extensions.  This is
possible because of AppCache-related fixes described below.  Re-enabling
is done by effectively reverting r599434.

AppCache is important, because it represents the scenarios when
RenderFrameHostImpl::CommitNavigation uses a non-network-bound default
URLLoaderFactory (e.g. in these scenarios the default factory is *not*
created via CreateNetworkServiceDefaultFactoryAndObserve).
A regression test for such scenarios is added in
CrossOriginReadBlockingExtensionTest.ProgrammaticContentScriptVsAppCache

For an overview of separate URLLoaderFactories for extensions, see
https://docs.google.com/document/d/1fuBUmLZj1H319jMkK8waUbf5WdQS0N95J_mWXVzbP7A


Avoiding cloberring of the default factory
==========================================

This CL ensures that the initial injection of a content script doesn't
result in cloberring the default URLLoaderFactory, by making sure
RenderFrameHostImpl::MarkInitiatorsAsRequiringSeparateURLLoaderFactory
is capable of pushing down only the new, initiator-specific factories
(without touching the default factory and/or other, untouched
initiator-specific factories).  This also requires ensuring that
RenderFrameHostImpl::CreateInitiatorSpecificURLLoaderFactories takes
as input the origins that need new factories (instead of using all the
origins that the frame needs initiator-specific factories for).


Handling NetworkService crashes
===============================

By having RenderFrameHostImpl::CreateInitiatorSpecificURLLoaderFactories
call CreateNetworkServiceDefaultFactoryAndObserve instead of
CreateNetworkServiceDefaultFactoryInternal, this CL ensures that a
NetworkService crash is detected when initiator-specific (network-bound)
factories are used (and that the crash is detected even if the default
factory is not network-bound).  This makes sure that content scripts in
the test continue to be able to issue network-bound requests after the
crash.

Additionally, introducing
|recreate_default_url_loader_factory_after_network_service_crash_| field
ensures that if the default factory is not network-bound, then it will
not get cloberred when handling a network service crash.  This helps
ensure that the final AppCache request made by the test is successful.


Bug: 846346
Change-Id: Ie53710d17526c140ef8ee65a41de5d3312bfae5b
Reviewed-on: https://chromium-review.googlesource.com/c/1318082
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606352}

chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc

@@ -8,6 +8,7 @@
 
 #include <memory>
 #include <set>
+#include <string>
 #include <vector>
 
 #include "base/command_line.h"
@@ -885,12 +886,8 @@ ChromeContentBrowserClientExtensionsPart::
         content::RenderProcessHost* process,
         network::mojom::NetworkContext* network_context,
         const url::Origin& request_initiator) {
-  // TODO(lukasza): https://crbug.com/894766: Re-enable after a real fix for
-  // this bug.  For now, let's just avoid using separate URLLoaderFactories
-  // for extensions.
-  // return URLLoaderFactoryManager::CreateFactory(process, network_context,
-  //                                              request_initiator);
-  return network::mojom::URLLoaderFactoryPtrInfo();
+  return URLLoaderFactoryManager::CreateFactory(process, network_context,
+                                                request_initiator);
 }
 
 // static

content/browser/frame_host/render_frame_host_impl.cc

@@ -1075,15 +1075,32 @@ bool RenderFrameHostImpl::CreateNetworkServiceDefaultFactory(
 }
 
 void RenderFrameHostImpl::MarkInitiatorsAsRequiringSeparateURLLoaderFactory(
-    std::vector<url::Origin> request_initiators,
+    base::flat_set<url::Origin> request_initiators,
     bool push_to_renderer_now) {
   size_t old_size = initiators_requiring_separate_url_loader_factory_.size();
   initiators_requiring_separate_url_loader_factory_.insert(
       request_initiators.begin(), request_initiators.end());
   size_t new_size = initiators_requiring_separate_url_loader_factory_.size();
   bool insertion_took_place = (old_size != new_size);
-  if (push_to_renderer_now && insertion_took_place)
-    UpdateSubresourceLoaderFactories();
+
+  // Push the updated set of factories to the renderer, but only if
+  // 1) the caller requested an immediate push (e.g. for content scripts
+  //    injected programmatically chrome.tabs.executeCode, but not for content
+  //    scripts declared in the manifest - the difference is that the latter
+  //    happen at a commit and the factories can just be send in the commit
+  //    IPC).
+  // 2) an insertion actually took place / the factories have been modified
+  // 3) a commit has taken place before (i.e. the frame has received a factory
+  //    bundle before).
+  if (push_to_renderer_now && insertion_took_place &&
+      has_committed_any_navigation_) {
+    std::unique_ptr<URLLoaderFactoryBundleInfo> subresource_loader_factories =
+        std::make_unique<URLLoaderFactoryBundleInfo>();
+    subresource_loader_factories->initiator_specific_factory_infos() =
+        CreateInitiatorSpecificURLLoaderFactories(request_initiators);
+    GetNavigationControl()->UpdateSubresourceLoaderFactories(
+        std::move(subresource_loader_factories));
+  }
 }
 
 bool RenderFrameHostImpl::IsSandboxed(blink::WebSandboxFlags flags) const {
@@ -1091,12 +1108,12 @@ bool RenderFrameHostImpl::IsSandboxed(blink::WebSandboxFlags flags) const {
 }
 
 URLLoaderFactoryBundleInfo::OriginMap
-RenderFrameHostImpl::CreateInitiatorSpecificURLLoaderFactories() {
+RenderFrameHostImpl::CreateInitiatorSpecificURLLoaderFactories(
+    const base::flat_set<url::Origin>& initiator_origins) {
   URLLoaderFactoryBundleInfo::OriginMap result;
-  for (const url::Origin& initiator :
-       initiators_requiring_separate_url_loader_factory_) {
+  for (const url::Origin& initiator : initiator_origins) {
     network::mojom::URLLoaderFactoryPtrInfo factory_info;
-    CreateNetworkServiceDefaultFactoryInternal(
+    CreateNetworkServiceDefaultFactoryAndObserve(
         initiator, mojo::MakeRequest(&factory_info));
     result[initiator] = std::move(factory_info);
   }
@@ -4385,6 +4402,7 @@ void RenderFrameHostImpl::CommitNavigation(
   std::unique_ptr<URLLoaderFactoryBundleInfo> subresource_loader_factories;
   if (base::FeatureList::IsEnabled(network::features::kNetworkService) &&
       (!is_same_document || is_first_navigation)) {
+    recreate_default_url_loader_factory_after_network_service_crash_ = false;
     subresource_loader_factories =
         std::make_unique<URLLoaderFactoryBundleInfo>();
     BrowserContext* browser_context = GetSiteInstance()->GetBrowserContext();
@@ -4425,6 +4443,7 @@ void RenderFrameHostImpl::CommitNavigation(
     if (!default_factory_info) {
       // Otherwise default to a Network Service-backed loader from the
       // appropriate NetworkContext.
+      recreate_default_url_loader_factory_after_network_service_crash_ = true;
       bool bypass_redirect_checks =
           CreateNetworkServiceDefaultFactoryAndObserve(
               GetOriginForURLLoaderFactory(common_params.url,
@@ -4501,7 +4520,8 @@ void RenderFrameHostImpl::CommitNavigation(
     }
 
     subresource_loader_factories->initiator_specific_factory_infos() =
-        CreateInitiatorSpecificURLLoaderFactories();
+        CreateInitiatorSpecificURLLoaderFactories(
+            initiators_requiring_separate_url_loader_factory_);
   }
 
   // It is imperative that cross-document navigations always provide a set of
@@ -5163,14 +5183,19 @@ void RenderFrameHostImpl::UpdateSubresourceLoaderFactories() {
   DCHECK(network_service_connection_error_handler_holder_.is_bound());
 
   network::mojom::URLLoaderFactoryPtrInfo default_factory_info;
-  bool bypass_redirect_checks = CreateNetworkServiceDefaultFactoryAndObserve(
-      last_committed_origin_, mojo::MakeRequest(&default_factory_info));
+  bool bypass_redirect_checks = false;
+  if (recreate_default_url_loader_factory_after_network_service_crash_) {
+    bypass_redirect_checks = CreateNetworkServiceDefaultFactoryAndObserve(
+        last_committed_origin_, mojo::MakeRequest(&default_factory_info));
+  }
 
   std::unique_ptr<URLLoaderFactoryBundleInfo> subresource_loader_factories =
       std::make_unique<URLLoaderFactoryBundleInfo>(
           std::move(default_factory_info),
           URLLoaderFactoryBundleInfo::SchemeMap(),
-          CreateInitiatorSpecificURLLoaderFactories(), bypass_redirect_checks);
+          CreateInitiatorSpecificURLLoaderFactories(
+              initiators_requiring_separate_url_loader_factory_),
+          bypass_redirect_checks);
   SaveSubresourceFactories(std::move(subresource_loader_factories));
   GetNavigationControl()->UpdateSubresourceLoaderFactories(
       CloneSubresourceFactories());

content/browser/frame_host/render_frame_host_impl.h

@@ -254,9 +254,10 @@ class CONTENT_EXPORT RenderFrameHostImpl
   bool CreateNetworkServiceDefaultFactory(
       network::mojom::URLLoaderFactoryRequest default_factory_request) override;
   void MarkInitiatorsAsRequiringSeparateURLLoaderFactory(
-      std::vector<url::Origin> request_initiators,
+      base::flat_set<url::Origin> request_initiators,
       bool push_to_renderer_now) override;
   bool IsSandboxed(blink::WebSandboxFlags flags) const override;
+  void FlushNetworkAndNavigationInterfacesForTesting() override;
 
   // IPC::Sender
   bool Send(IPC::Message* msg) override;
@@ -768,11 +769,6 @@ class CONTENT_EXPORT RenderFrameHostImpl
     return active_sandbox_flags_;
   }
 
-  // Calls |FlushForTesting()| on Network Service and FrameNavigationControl
-  // related interfaces to make sure all in-flight mojo messages have been
-  // received by the other end. For test use only.
-  void FlushNetworkAndNavigationInterfacesForTesting();
-
   // Notifies the render frame about a user activation from the browser side.
   void NotifyUserActivation();
 
@@ -1322,10 +1318,11 @@ class CONTENT_EXPORT RenderFrameHostImpl
   std::unique_ptr<base::trace_event::TracedValue> CommitAsTracedValue(
       FrameHostMsg_DidCommitProvisionalLoad_Params* validated_params) const;
 
-  // Creates initiator-specific URLLoaderFactory objects for intiator origins
-  // registered via MarkInitiatorAsRequiringSeparateURLLoaderFactory method.
+  // Creates initiator-specific URLLoaderFactory objects for
+  // |initiator_origins|.
   URLLoaderFactoryBundleInfo::OriginMap
-  CreateInitiatorSpecificURLLoaderFactories();
+  CreateInitiatorSpecificURLLoaderFactories(
+      const base::flat_set<url::Origin>& initiator_origins);
 
   // Based on the termination |status|, may generate a crash report to be routed
   // to the Reporting API.
@@ -1766,6 +1763,12 @@ class CONTENT_EXPORT RenderFrameHostImpl
   network::mojom::URLLoaderFactoryPtr
       network_service_connection_error_handler_holder_;
 
+  // Whether UpdateSubresourceLoaderFactories should recreate the default
+  // URLLoaderFactory when handling a NetworkService crash.  In case the frame
+  // is covered by AppCache, only initiator-specific factories need to be
+  // refreshed, but the main, AppCache-specific factory shouldn't be refreshed.
+  bool recreate_default_url_loader_factory_after_network_service_crash_ = false;
+
   // Set of request-initiator-origins that require a separate URLLoaderFactory
   // (e.g. for handling requests initiated by extension content scripts that
   // require relaxed CORS/CORB rules).

content/common/url_loader_factory_bundle.mojom

@@ -7,13 +7,18 @@ module content.mojom;
 import "services/network/public/mojom/url_loader_factory.mojom";
 import "url/mojom/origin.mojom";
 
-// Serializes a collection of URLLoaderFactory interfaces.
+// A collection of URLLoaderFactory interfaces.
+//
+// All URLLoaderFactories below are optional.  This supports a scenario where
+// URLLoaderFactoryBundle contains only the factories that need to be updated
+// (and allows leaving out factories that should not be updated/cloberred).
+// See also content::URLLoaderFactoryBundle::Update.
 struct URLLoaderFactoryBundle {
   // The default factory to be used when no others apply.
   //
   // TODO(jam): https://crbug.com/887109: Remove |default_factory| and put it
   // inside |scheme_specific_factories| instead.
-  network.mojom.URLLoaderFactory default_factory;
+  network.mojom.URLLoaderFactory? default_factory;
 
   // A mapping from URL scheme to factory interface.
   map<string, network.mojom.URLLoaderFactory> scheme_specific_factories;

content/public/browser/render_frame_host.h

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/callback_forward.h"
+#include "base/containers/flat_set.h"
 #include "base/feature_list.h"
 #include "build/build_config.h"
 #include "content/common/content_export.h"
@@ -332,7 +333,7 @@ class CONTENT_EXPORT RenderFrameHost : public IPC::Listener,
   // origin will be created via
   // ContentBrowserClient::CreateURLLoaderFactoryForNetworkRequests method.
   virtual void MarkInitiatorsAsRequiringSeparateURLLoaderFactory(
-      std::vector<url::Origin> request_initiators,
+      base::flat_set<url::Origin> request_initiators,
       bool push_to_renderer_now) = 0;
 
   // Returns true if the given sandbox flag |flags| is in effect on this frame.
@@ -341,6 +342,11 @@ class CONTENT_EXPORT RenderFrameHost : public IPC::Listener,
   // embedding frame.
   virtual bool IsSandboxed(blink::WebSandboxFlags flags) const = 0;
 
+  // Calls |FlushForTesting()| on Network Service and FrameNavigationControl
+  // related interfaces to make sure all in-flight mojo messages have been
+  // received by the other end. For test use only.
+  virtual void FlushNetworkAndNavigationInterfacesForTesting() = 0;
+
  private:
   // This interface should only be implemented inside content.
   friend class RenderFrameHostImpl;

content/public/browser/site_isolation_policy.cc

@@ -60,13 +60,6 @@ void SiteIsolationPolicy::PopulateURLLoaderFactoryParamsPtrForCORB(
   params->is_corb_enabled = true;
   params->corb_detachable_resource_type = RESOURCE_TYPE_PREFETCH;
   params->corb_excluded_resource_type = RESOURCE_TYPE_PLUGIN_RESOURCE;
-
-  const char* initiator_scheme_exception =
-      GetContentClient()
-          ->browser()
-          ->GetInitiatorSchemeBypassingDocumentBlocking();
-  if (initiator_scheme_exception)
-    params->corb_excluded_initiator_scheme = initiator_scheme_exception;
 }
 
 // static

content/test/data/appcache/simple_page.manifest

 CACHE MANIFEST
 /appcache/logo.png
+/appcache/logo2.png
+/appcache/logo3.png
 
 NETWORK:
-*
\ No newline at end of file
+*

extensions/browser/url_loader_factory_manager.cc

@@ -5,6 +5,7 @@
 #include "extensions/browser/url_loader_factory_manager.h"
 
 #include <algorithm>
+#include <memory>
 #include <utility>
 #include <vector>
 
@@ -92,11 +93,8 @@ void MarkInitiatorsAsRequiringSeparateURLLoaderFactory(
     bool push_to_renderer_now) {
   DCHECK(!request_initiators.empty());
   if (base::FeatureList::IsEnabled(network::features::kNetworkService)) {
-    // TODO(lukasza): https://crbug.com/894766: Re-enable after a real fix for
-    // this bug.  For now, let's just avoid using separate URLLoaderFactories
-    // for extensions.
-    // frame->MarkInitiatorsAsRequiringSeparateURLLoaderFactory(
-    //    std::move(request_initiators), push_to_renderer_now);
+    frame->MarkInitiatorsAsRequiringSeparateURLLoaderFactory(
+        std::move(request_initiators), push_to_renderer_now);
   } else {
     // TODO(lukasza): In non-NetworkService implementation of CORB, make an
     // exception only for specific extensions (e.g. based on process id,
@@ -276,10 +274,6 @@ network::mojom::URLLoaderFactoryPtrInfo URLLoaderFactoryManager::CreateFactory(
     content::RenderProcessHost* process,
     network::mojom::NetworkContext* network_context,
     const url::Origin& initiator_origin) {
-  // TODO(lukasza): https://crbug.com/894766: Re-enable after a real fix for
-  // this bug.  For now, we should never reach CreateFactory method.
-  NOTREACHED();
-
   content::BrowserContext* browser_context = process->GetBrowserContext();
   const ExtensionRegistry* registry = ExtensionRegistry::Get(browser_context);
   DCHECK(registry);  // CreateFactory shouldn't happen during shutdown.

extensions/shell/browser/shell_content_browser_client.cc

@@ -318,12 +318,8 @@ ShellContentBrowserClient::CreateURLLoaderFactoryForNetworkRequests(
     content::RenderProcessHost* process,
     network::mojom::NetworkContext* network_context,
     const url::Origin& request_initiator) {
-  // TODO(lukasza): https://crbug.com/894766: Re-enable after a real fix for
-  // this bug.  For now, let's just avoid using separate URLLoaderFactories
-  // for extensions.
-  // return URLLoaderFactoryManager::CreateFactory(process, network_context,
-  //                                              request_initiator);
-  return network::mojom::URLLoaderFactoryPtrInfo();
+  return URLLoaderFactoryManager::CreateFactory(process, network_context,
+                                                request_initiator);
 }
 
 ShellBrowserMainParts* ShellContentBrowserClient::CreateShellBrowserMainParts(

services/network/public/mojom/network_context.mojom

@@ -346,11 +346,6 @@ struct URLLoaderFactoryParams {
   // (even as an opaque int) in //services/network.  See also the TODO comment
   // for network::ResourceRequest::resource_type.
   int32 corb_excluded_resource_type = -1;
-  // TODO(lukasza): https://crbug.com/846346: Replace the field below with a
-  // granular list of origins that content scripts can XHR into (based on
-  // extension manifest V3 / assumming that content scripts have a
-  // URLLoaderFactory separate from the rest of the renderer).
-  string corb_excluded_initiator_scheme;
 
   // True if web related security (e.g., CORS) should be disabled. This is
   // mainly used by people testing their sites, via a command line switch.

services/network/url_loader.cc

@@ -721,10 +721,7 @@ void URLLoader::OnResponseStarted(net::URLRequest* url_request, int net_error) {
                  base::Unretained(this)));
 
   // Figure out if we need to sniff (for MIME type detection or for CORB).
-  if (factory_params_->is_corb_enabled && !is_nocors_corb_excluded_request_ &&
-      (factory_params_->corb_excluded_initiator_scheme.empty() ||
-       factory_params_->corb_excluded_initiator_scheme !=
-           url_request->initiator().value_or(url::Origin()).scheme())) {
+  if (factory_params_->is_corb_enabled && !is_nocors_corb_excluded_request_) {
     CrossOriginReadBlocking::LogAction(
         CrossOriginReadBlocking::Action::kResponseStarted);