Simplify prefetch + cross-origin prefetch logic

This CL centralizes the cross-origin-prefetch-specific logic to
PrefetchURLLoaderService, whereas previously it was split between
PrefetchURLLoaderService and PrefetchURLLoader. This does not come with
any observable change, however simplifies the implementation in a way
that will make the preload-header-on-prefetch code cleaner.

This CL also eliminates the
PrefetchURLLoaderService::CreateLoaderAndStart method that was called
from the network::mojom::URLLoaderFactory override, leaving only the
URLLoaderFactory override method left. This is much cleaner, as both
were not necessary now that the network service has been enabled by
default.

Bug: 939317
Change-Id: I06b57222ea02a4ac9594eec1f89700a1011655b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1775646
Commit-Queue: Dominic Farolino <dom@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#691583}

content/browser/loader/prefetch_url_loader.cc

@@ -59,27 +59,6 @@ PrefetchURLLoader::PrefetchURLLoader(
   DCHECK(network_loader_factory_);
   RecordPrefetchRedirectHistogram(PrefetchRedirect::kPrefetchMade);
 
-  if (base::FeatureList::IsEnabled(
-          net::features::kSplitCacheByNetworkIsolationKey) &&
-      resource_request_.load_flags & net::LOAD_RESTRICTED_PREFETCH) {
-    // If |resource_request| has been marked as a restricted prefetch by the
-    // renderer, we must verify that the request is valid, so we can set its
-    // NetworkIsolationKey for the response to be cached correctly. An invalid
-    // request could indicate a compromised renderer inappropriately modifying
-    // the request, so we immediately complete it with an error.
-    if (!IsValidCrossOriginPrefetch(resource_request)) {
-      SendOnComplete(
-          network::URLLoaderCompletionStatus(net::ERR_INVALID_ARGUMENT));
-      return;
-    }
-
-    url::Origin destination_origin = url::Origin::Create(resource_request_.url);
-    resource_request_.trusted_params =
-        network::ResourceRequest::TrustedParams();
-    resource_request_.trusted_params->network_isolation_key =
-        net::NetworkIsolationKey(destination_origin, destination_origin);
-  }
-
   if (IsSignedExchangeHandlingEnabled()) {
     // Set the SignedExchange accept header.
     // (https://wicg.github.io/webpackage/draft-yasskin-http-origin-signed-responses.html#internet-media-type-applicationsigned-exchange).
@@ -104,45 +83,6 @@ PrefetchURLLoader::PrefetchURLLoader(
       resource_request_, std::move(network_client), traffic_annotation);
 }
 
-// This method is used to determine whether it is safe to set the
-// NetworkIsolationKey of a cross-origin prefetch request coming from the
-// renderer, so that it can be cached correctly.
-bool PrefetchURLLoader::IsValidCrossOriginPrefetch(
-    const network::ResourceRequest& resource_request) {
-  // The request is expected to be cross-origin. Same-origin prefetches do not
-  // need a special NetworkIsolationKey, and therefore must not be marked for
-  // restricted use.
-  url::Origin destination_origin = url::Origin::Create(resource_request.url);
-  // TODO(domfarolino): We want to check that the request's initiator is
-  // equivalent to the frame's last committed origin. If they are not equal, we
-  // must cancel the request.
-  if (!resource_request.request_initiator ||
-      resource_request.request_initiator->IsSameOriginWith(
-          destination_origin)) {
-    return false;
-  }
-
-  // If the PrefetchRedirectError feature is enabled, the request's redirect
-  // mode must be |kError|.
-  if (base::FeatureList::IsEnabled(blink::features::kPrefetchRedirectError) &&
-      resource_request.redirect_mode != network::mojom::RedirectMode::kError) {
-    return false;
-  }
-
-  // This prefetch request must not be able to reuse restricted prefetches from
-  // the prefetch cache. This is because it is possible that another origin
-  // prefetched the same resource, which should only be reused for top-level
-  // navigations.
-  if (resource_request.load_flags & net::LOAD_CAN_USE_RESTRICTED_PREFETCH)
-    return false;
-
-  // The request must not already have its |trusted_params| initialized.
-  if (resource_request.trusted_params)
-    return false;
-
-  return true;
-}
-
 PrefetchURLLoader::~PrefetchURLLoader() = default;
 
 void PrefetchURLLoader::RecordPrefetchRedirectHistogram(

content/browser/loader/prefetch_url_loader.h

@@ -91,8 +91,6 @@ class CONTENT_EXPORT PrefetchURLLoader : public network::mojom::URLLoader,
   };
 
   void RecordPrefetchRedirectHistogram(PrefetchRedirect event);
-  bool IsValidCrossOriginPrefetch(
-      const network::ResourceRequest& resource_request);
 
   // network::mojom::URLLoader overrides:
   void FollowRedirect(const std::vector<std::string>& removed_headers,

content/browser/loader/prefetch_url_loader_service.cc

@@ -23,6 +23,7 @@
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
 #include "storage/browser/blob/blob_storage_context.h"
+#include "third_party/blink/public/common/features.h"
 #include "third_party/blink/public/common/loader/url_loader_throttle.h"
 #include "third_party/blink/public/mojom/renderer_preferences.mojom.h"
 
@@ -97,14 +98,51 @@ void PrefetchURLLoaderService::CreateLoaderAndStart(
     int32_t routing_id,
     int32_t request_id,
     uint32_t options,
-    const network::ResourceRequest& resource_request,
+    const network::ResourceRequest& resource_request_in,
     network::mojom::URLLoaderClientPtr client,
-    const net::MutableNetworkTrafficAnnotationTag& traffic_annotation,
-    scoped_refptr<network::SharedURLLoaderFactory> network_loader_factory,
-    base::RepeatingCallback<int(void)> frame_tree_node_id_getter) {
+    const net::MutableNetworkTrafficAnnotationTag& traffic_annotation) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  // Make a copy of |resource_request_in|, because we may need to modify the
+  // request.
+  network::ResourceRequest resource_request = resource_request_in;
+
   DCHECK_EQ(static_cast<int>(ResourceType::kPrefetch),
             resource_request.resource_type);
+  const auto& current_context = *loader_factory_receivers_.current_context();
+
+  if (!current_context.render_frame_host) {
+    client->OnComplete(network::URLLoaderCompletionStatus(net::ERR_ABORTED));
+    return;
+  }
+
+  scoped_refptr<network::SharedURLLoaderFactory> network_loader_factory_to_use =
+      current_context.factory;
+
+  if (resource_request.load_flags & net::LOAD_RESTRICTED_PREFETCH) {
+    // The renderer has marked this prefetch as restricted, meaning it is a
+    // cross-origin prefetch intended for top-leve navigation reuse. We must
+    // verify that the request meets the necessary security requirements, and
+    // populate |resource_request|'s NetworkIsolationKey appropriately.
+    EnsureCrossOriginFactory();
+    DCHECK(current_context.cross_origin_factory);
+
+    // An invalid request could indicate a compromised renderer inappropriately
+    // modifying the request, so we immediately complete it with an error.
+    if (!IsValidCrossOriginPrefetch(resource_request)) {
+      client->OnComplete(
+          network::URLLoaderCompletionStatus(net::ERR_INVALID_ARGUMENT));
+      return;
+    }
+
+    // Use the trusted cross-origin prefetch loader factory, and set the
+    // request's NetworkIsolationKey suitable for the cross-origin prefetch.
+    network_loader_factory_to_use = current_context.cross_origin_factory;
+    url::Origin destination_origin = url::Origin::Create(resource_request.url);
+    resource_request.trusted_params = network::ResourceRequest::TrustedParams();
+    resource_request.trusted_params->network_isolation_key =
+        net::NetworkIsolationKey(destination_origin, destination_origin);
+  }
 
   if (prefetch_load_callback_for_testing_)
     prefetch_load_callback_for_testing_.Run();
@@ -116,6 +154,9 @@ void PrefetchURLLoaderService::CreateLoaderAndStart(
             ->prefetched_signed_exchange_cache;
   }
 
+  auto frame_tree_node_id_getter = base::BindRepeating(
+      [](int id) { return id; }, current_context.frame_tree_node_id);
+
   // For now we strongly bind the loader to the request, while we can
   // also possibly make the new loader owned by the factory so that
   // they can live longer than the client (i.e. run in detached mode).
@@ -124,7 +165,7 @@ void PrefetchURLLoaderService::CreateLoaderAndStart(
       std::make_unique<PrefetchURLLoader>(
           routing_id, request_id, options, frame_tree_node_id_getter,
           resource_request, std::move(client), traffic_annotation,
-          std::move(network_loader_factory),
+          std::move(network_loader_factory_to_use),
           base::BindRepeating(
               &PrefetchURLLoaderService::CreateURLLoaderThrottles, this,
               resource_request, frame_tree_node_id_getter),
@@ -136,42 +177,43 @@ void PrefetchURLLoaderService::CreateLoaderAndStart(
 
 PrefetchURLLoaderService::~PrefetchURLLoaderService() = default;
 
-void PrefetchURLLoaderService::CreateLoaderAndStart(
-    network::mojom::URLLoaderRequest request,
-    int32_t routing_id,
-    int32_t request_id,
-    uint32_t options,
-    const network::ResourceRequest& resource_request,
-    network::mojom::URLLoaderClientPtr client,
-    const net::MutableNetworkTrafficAnnotationTag& traffic_annotation) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  const auto& current_context = *loader_factory_receivers_.current_context();
+// This method is used to determine whether it is safe to set the
+// NetworkIsolationKey of a cross-origin prefetch request coming from the
+// renderer, so that it can be cached correctly.
+bool PrefetchURLLoaderService::IsValidCrossOriginPrefetch(
+    const network::ResourceRequest& resource_request) {
+  // The request is expected to be cross-origin. Same-origin prefetches do not
+  // need a special NetworkIsolationKey, and therefore must not be marked for
+  // restricted use.
+  url::Origin destination_origin = url::Origin::Create(resource_request.url);
+  // TODO(domfarolino): We want to check that the request's initiator is
+  // equivalent to the frame's last committed origin. If they are not equal, we
+  // must cancel the request.
+  if (!resource_request.request_initiator ||
+      resource_request.request_initiator->IsSameOriginWith(
+          destination_origin)) {
+    return false;
+  }
 
-  if (!current_context.render_frame_host) {
-    client->OnComplete(network::URLLoaderCompletionStatus(net::ERR_ABORTED));
-    return;
+  // If the PrefetchRedirectError feature is enabled, the request's redirect
+  // mode must be |kError|.
+  if (base::FeatureList::IsEnabled(blink::features::kPrefetchRedirectError) &&
+      resource_request.redirect_mode != network::mojom::RedirectMode::kError) {
+    return false;
   }
 
-  int frame_tree_node_id = current_context.frame_tree_node_id;
-  scoped_refptr<network::SharedURLLoaderFactory> network_loader_factory_to_use =
-      current_context.factory;
+  // This prefetch request must not be able to reuse restricted prefetches from
+  // the prefetch cache. This is because it is possible that another origin
+  // prefetched the same resource, which should only be reused for top-level
+  // navigations.
+  if (resource_request.load_flags & net::LOAD_CAN_USE_RESTRICTED_PREFETCH)
+    return false;
 
-  if (resource_request.load_flags & net::LOAD_RESTRICTED_PREFETCH) {
-    // The renderer has marked this prefetch as restricted. We must give
-    // PrefetchURLLoader the cross-origin prefetch loader factory, which will be
-    // used if the request meets necessary security requirements.
-    // PrefetchURLLoader will then populate |resource_request|'s
-    // NetworkIsolationKey appropriately.
-    EnsureCrossOriginFactory();
-    DCHECK(current_context.cross_origin_factory);
-    network_loader_factory_to_use = current_context.cross_origin_factory;
-  }
+  // The request must not already have its |trusted_params| initialized.
+  if (resource_request.trusted_params)
+    return false;
 
-  CreateLoaderAndStart(
-      std::move(request), routing_id, request_id, options, resource_request,
-      std::move(client), traffic_annotation,
-      std::move(network_loader_factory_to_use),
-      base::BindRepeating([](int id) { return id; }, frame_tree_node_id));
+  return true;
 }
 
 void PrefetchURLLoaderService::EnsureCrossOriginFactory() {

content/browser/loader/prefetch_url_loader_service.h

@@ -22,10 +22,6 @@ namespace storage {
 class BlobStorageContext;
 }
 
-namespace network {
-class SharedURLLoaderFactory;
-}
-
 namespace blink {
 class URLLoaderThrottle;
 }
@@ -54,23 +50,6 @@ class CONTENT_EXPORT PrefetchURLLoaderService final
       scoped_refptr<PrefetchedSignedExchangeCache>
           prefetched_signed_exchange_cache);
 
-  // Used only when NetworkService is not enabled (or indirectly via the
-  // other CreateLoaderAndStart when NetworkService is enabled).
-  // This creates a loader and starts fetching using the given
-  // |network_lader_factory|. |frame_tree_node_id| may be given and used to
-  // create necessary throttles when Network Service is enabled when the
-  // created loader internally makes additional requests.
-  void CreateLoaderAndStart(
-      network::mojom::URLLoaderRequest request,
-      int32_t routing_id,
-      int32_t request_id,
-      uint32_t options,
-      const network::ResourceRequest& resource_request,
-      network::mojom::URLLoaderClientPtr client,
-      const net::MutableNetworkTrafficAnnotationTag& traffic_annotation,
-      scoped_refptr<network::SharedURLLoaderFactory> network_loader_factory,
-      base::RepeatingCallback<int(void)> frame_tree_node_id_getter);
-
   // Register a callback that is fired right before a prefetch load is started
   // by this service.
   void RegisterPrefetchLoaderCallbackForTest(
@@ -98,7 +77,7 @@ class CONTENT_EXPORT PrefetchURLLoaderService final
                             int32_t routing_id,
                             int32_t request_id,
                             uint32_t options,
-                            const network::ResourceRequest& resource_request,
+                            const network::ResourceRequest& resource_request_in,
                             network::mojom::URLLoaderClientPtr client,
                             const net::MutableNetworkTrafficAnnotationTag&
                                 traffic_annotation) override;
@@ -108,6 +87,8 @@ class CONTENT_EXPORT PrefetchURLLoaderService final
   // by setting it to a special URLLoaderFactory created by the current
   // context's RenderFrameHost.
   void EnsureCrossOriginFactory();
+  bool IsValidCrossOriginPrefetch(
+      const network::ResourceRequest& resource_request);
 
   // blink::mojom::RendererPreferenceWatcher.
   void NotifyUpdate(blink::mojom::RendererPreferencesPtr new_prefs) override;