Remove usage of ResourceType parameters in URLLoaderFactoryParams.

Since the resource type is trusted from the renderer, we can have the renderer pass this per-request based on data it has. This is a step towards the TODO to remove this from network::ResourceRequest, since network service shouldn't know about this enum. Bug: 960143 Change-Id: I8afa72ff14236e7123196f47e17a574527ac1e3c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1597975 Reviewed-by: Nasko Oskov <nasko@chromium.org> Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org> Commit-Queue: John Abd-El-Malek <jam@chromium.org> Cr-Commit-Position: refs/heads/master@{#657464}
Denger-Network · May 7, 2019 · 54a786e · 54a786e
1 parent f0a92d4
commit 54a786e
Show file tree

Hide file tree

Showing 11 changed files with 39 additions and 25 deletions.
diff --git a/content/public/browser/site_isolation_policy.cc b/content/public/browser/site_isolation_policy.cc
@@ -80,10 +80,6 @@ void SiteIsolationPolicy::PopulateURLLoaderFactoryParamsPtrForCORB(
   }
 
   params->is_corb_enabled = true;
-  params->corb_detachable_resource_type =
-      static_cast<int>(ResourceType::kPrefetch);
-  params->corb_excluded_resource_type =
-      static_cast<int>(ResourceType::kPluginResource);
 }
 
 // static

diff --git a/content/renderer/loader/web_url_loader_impl.cc b/content/renderer/loader/web_url_loader_impl.cc
@@ -835,6 +835,16 @@ void WebURLLoaderImpl::Context::Start(const WebURLRequest& request,
         std::make_unique<WebURLLoaderImpl::RequestPeerImpl>(this, discard_body);
   }
 
+  if (resource_request->resource_type ==
+      static_cast<int>(ResourceType::kPrefetch)) {
+    resource_request->corb_detachable = true;
+  }
+
+  if (resource_request->resource_type ==
+      static_cast<int>(ResourceType::kPluginResource)) {
+    resource_request->corb_excluded = true;
+  }
+
   auto throttles = extra_data->TakeURLLoaderThrottles();
   // The frame request blocker is only for a frame's subresources.
   if (extra_data->frame_request_blocker() &&

diff --git a/services/network/public/cpp/resource_request.cc b/services/network/public/cpp/resource_request.cc
@@ -38,6 +38,8 @@ bool ResourceRequest::EqualsForTesting(const ResourceRequest& request) const {
          originated_from_service_worker ==
              request.originated_from_service_worker &&
          skip_service_worker == request.skip_service_worker &&
+         corb_detachable == request.corb_detachable &&
+         corb_excluded == request.corb_excluded &&
          fetch_request_mode == request.fetch_request_mode &&
          fetch_credentials_mode == request.fetch_credentials_mode &&
          fetch_redirect_mode == request.fetch_redirect_mode &&

diff --git a/services/network/public/cpp/resource_request.h b/services/network/public/cpp/resource_request.h
@@ -62,6 +62,8 @@ struct COMPONENT_EXPORT(NETWORK_CPP_BASE) ResourceRequest {
       mojom::CorsPreflightPolicy::kConsiderPreflight;
   bool originated_from_service_worker = false;
   bool skip_service_worker = false;
+  bool corb_detachable = false;
+  bool corb_excluded = false;
   mojom::FetchRequestMode fetch_request_mode = mojom::FetchRequestMode::kNoCors;
   mojom::FetchCredentialsMode fetch_credentials_mode =
       mojom::FetchCredentialsMode::kInclude;

diff --git a/services/network/public/cpp/url_request_mojom_traits.cc b/services/network/public/cpp/url_request_mojom_traits.cc
@@ -194,6 +194,8 @@ bool StructTraits<
   out->is_external_request = data.is_external_request();
   out->originated_from_service_worker = data.originated_from_service_worker();
   out->skip_service_worker = data.skip_service_worker();
+  out->corb_detachable = data.corb_detachable();
+  out->corb_excluded = data.corb_excluded();
   out->fetch_request_context_type = data.fetch_request_context_type();
   out->keepalive = data.keepalive();
   out->has_user_gesture = data.has_user_gesture();

diff --git a/services/network/public/cpp/url_request_mojom_traits.h b/services/network/public/cpp/url_request_mojom_traits.h
@@ -125,6 +125,12 @@ struct COMPONENT_EXPORT(NETWORK_CPP_BASE)
   static bool skip_service_worker(const network::ResourceRequest& request) {
     return request.skip_service_worker;
   }
+  static bool corb_detachable(const network::ResourceRequest& request) {
+    return request.corb_detachable;
+  }
+  static bool corb_excluded(const network::ResourceRequest& request) {
+    return request.corb_excluded;
+  }
   static network::mojom::FetchRequestMode fetch_request_mode(
       const network::ResourceRequest& request) {
     return request.fetch_request_mode;

diff --git a/services/network/public/mojom/network_context.mojom b/services/network/public/mojom/network_context.mojom
@@ -462,20 +462,6 @@ struct URLLoaderFactoryParams {
 
   // Cross-origin read blocking (CORB) configuration.
   bool is_corb_enabled = true;
-  // TODO(lukasza): The field below in practice is always set to
-  // ResourceType::kPrefetch by the //content layer, but in the long-term we
-  // want to avoid using resource types (even as an opaque int) in
-  // //services/network.  See also the TODO comment for
-  // network::ResourceRequest::resource_type.
-  int32 corb_detachable_resource_type = -1;
-  // TODO(lukasza): https://crbug.com/846339: Remove the field below and instead
-  // make plugins use a separate URLoaderFactory. Note requests of this type are
-  // only excluded if ResourceRequest::fetch_request_mode is kNoCors. The field
-  // below in practice is always set to ResourceType::kPluginResource by the
-  //  content layer, but in the long-term we want to avoid using resource types
-  // (even as an opaque int) in //services/network.  See also the TODO comment
-  // for network::ResourceRequest::resource_type.
-  int32 corb_excluded_resource_type = -1;
 
   // 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.

diff --git a/services/network/public/mojom/url_loader.mojom b/services/network/public/mojom/url_loader.mojom
@@ -198,6 +198,14 @@ struct URLRequest {
   // about this.
   bool skip_service_worker;
 
+  // If true then the request continues even if it's blocked by CORB.
+  bool corb_detachable = false;
+
+  // TODO(lukasza): https://crbug.com/846339: Remove the field below and instead
+  // make plugins use a separate URLoaderFactory. Note requests of this type are
+  // only excluded if fetch_request_mode is kNoCors.
+  bool corb_excluded = false;
+
   // https://fetch.spec.whatwg.org/#concept-request-mode
   // Used mainly by CORS handling (out-of-blink CORS), CORB, Service Worker.
   // CORS handling needs a proper origin (including a unique opaque origin).

diff --git a/services/network/url_loader.cc b/services/network/url_loader.cc
@@ -317,6 +317,7 @@ URLLoader::URLLoader(
       network_service_client_(network_service_client),
       delete_callback_(std::move(delete_callback)),
       options_(options),
+      corb_detachable_(request.corb_detachable),
       resource_type_(request.resource_type),
       is_load_timing_enabled_(request.enable_load_timing),
       factory_params_(factory_params),
@@ -393,7 +394,7 @@ URLLoader::URLLoader(
                             std::make_unique<UnownedPointer>(this));
 
   is_nocors_corb_excluded_request_ =
-      resource_type_ == factory_params_->corb_excluded_resource_type &&
+      request.corb_excluded &&
       request.fetch_request_mode == mojom::FetchRequestMode::kNoCors &&
       CrossOriginReadBlocking::ShouldAllowForPlugin(
           factory_params_->process_id);
@@ -1415,7 +1416,7 @@ URLLoader::BlockResponseForCorbResult URLLoader::BlockResponseForCorb() {
   // Tell the real URLLoaderClient that the response has been completed.
   bool should_report_corb_blocking =
       corb_analyzer_->ShouldReportBlockedResponse();
-  if (resource_type_ == factory_params_->corb_detachable_resource_type) {
+  if (corb_detachable_) {
     // TODO(lukasza): https://crbug.com/827633#c5: Consider passing net::ERR_OK
     // instead.  net::ERR_ABORTED was chosen for consistency with the old CORB
     // implementation that used to go through DetachableResourceHandler.
@@ -1428,7 +1429,7 @@ URLLoader::BlockResponseForCorbResult URLLoader::BlockResponseForCorb() {
   // If the factory is asking to complete requests of this type, then we need to
   // continue processing the response to make sure the network cache is
   // populated.  Otherwise we can cancel the request.
-  if (resource_type_ == factory_params_->corb_detachable_resource_type) {
+  if (corb_detachable_) {
     // Discard any remaining callbacks or data by rerouting the pipes to
     // EmptyURLLoaderClient (deleting |self_ptr| when the URL request
     // completes).

diff --git a/services/network/url_loader.h b/services/network/url_loader.h
@@ -231,6 +231,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) URLLoader
   DeleteCallback delete_callback_;
 
   int32_t options_;
+  bool corb_detachable_;
   int resource_type_;
   bool is_load_timing_enabled_;
 

diff --git a/services/network/url_loader_unittest.cc b/services/network/url_loader_unittest.cc
@@ -2601,12 +2601,12 @@ TEST_F(URLLoaderTest, CorbEffectiveWithCors) {
   request.resource_type = kResourceType;
   request.fetch_request_mode = mojom::FetchRequestMode::kCors;
   request.request_initiator = url::Origin::Create(GURL("http://foo.com/"));
+  request.corb_excluded = true;
 
   base::RunLoop delete_run_loop;
   mojom::URLLoaderPtr loader;
   std::unique_ptr<URLLoader> url_loader;
   mojom::URLLoaderFactoryParams params;
-  params.corb_excluded_resource_type = kResourceType;
   url_loader = std::make_unique<URLLoader>(
       context(), nullptr /* network_service_client */,
       DeleteLoaderCallback(&delete_run_loop, &url_loader),
@@ -2624,7 +2624,7 @@ TEST_F(URLLoaderTest, CorbEffectiveWithCors) {
 
   // Blocked because this is a cross-origin request made with a CORS request
   // header, but without a valid CORS response header.
-  // params.corb_excluded_resource_type does not apply in that case.
+  // request.corb_excluded does not apply in that case.
   ASSERT_EQ(std::string(), body);
 }
 
@@ -2637,12 +2637,12 @@ TEST_F(URLLoaderTest, CorbExcludedWithNoCors) {
   request.resource_type = kResourceType;
   request.fetch_request_mode = mojom::FetchRequestMode::kNoCors;
   request.request_initiator = url::Origin::Create(GURL("http://foo.com/"));
+  request.corb_excluded = true;
 
   base::RunLoop delete_run_loop;
   mojom::URLLoaderPtr loader;
   std::unique_ptr<URLLoader> url_loader;
   mojom::URLLoaderFactoryParams params;
-  params.corb_excluded_resource_type = kResourceType;
   params.process_id = 123;
   CrossOriginReadBlocking::AddExceptionForPlugin(123);
   url_loader = std::make_unique<URLLoader>(
@@ -2677,12 +2677,12 @@ TEST_F(URLLoaderTest, CorbEffectiveWithNoCorsWhenNoActualPlugin) {
   request.resource_type = kResourceType;
   request.fetch_request_mode = mojom::FetchRequestMode::kNoCors;
   request.request_initiator = url::Origin::Create(GURL("http://foo.com/"));
+  request.corb_excluded = true;
 
   base::RunLoop delete_run_loop;
   mojom::URLLoaderPtr loader;
   std::unique_ptr<URLLoader> url_loader;
   mojom::URLLoaderFactoryParams params;
-  params.corb_excluded_resource_type = kResourceType;
   params.process_id = 234;
   // No call to CrossOriginReadBlocking::AddExceptionForPlugin(123) - this is
   // what we primarily want to cover in this test.