Support prefetching in NetworkService version of CORB.

Bug: 846334
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I63ed2ebb60528b004e3f2ffa605c3f492bf15efd
Reviewed-on: https://chromium-review.googlesource.com/1108746
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570430}

components/mirroring/service/session.cc

@@ -4,6 +4,12 @@
 
 #include "components/mirroring/service/session.h"
 
+#include <algorithm>
+#include <memory>
+#include <string>
+#include <utility>
+#include <vector>
+
 #include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/rand_util.h"
@@ -171,10 +177,10 @@ void AddStreamObject(int stream_index,
       (config.rtp_payload_type <= media::cast::RtpPayloadType::AUDIO_LAST);
   stream.SetKey("rtpPayloadType",
                 base::Value(is_audio ? kAudioPayloadType : kVideoPayloadType));
-  stream.SetKey("ssrc", base::Value(int(config.sender_ssrc)));
-  stream.SetKey(
-      "targetDelay",
-      base::Value(int(config.animated_playout_delay.InMilliseconds())));
+  stream.SetKey("ssrc", base::Value(static_cast<int>(config.sender_ssrc)));
+  stream.SetKey("targetDelay",
+                base::Value(static_cast<int>(
+                    config.animated_playout_delay.InMilliseconds())));
   stream.SetKey("aesKey", base::Value(base::HexEncode(config.aes_key.data(),
                                                       config.aes_key.size())));
   stream.SetKey("aesIvMask",
@@ -290,11 +296,13 @@ Session::Session(int32_t session_id,
 
   auto wifi_status_monitor =
       std::make_unique<WifiStatusMonitor>(session_id_, &message_dispatcher_);
+  network::mojom::URLLoaderFactoryParamsPtr params =
+      network::mojom::URLLoaderFactoryParams::New();
+  params->process_id = network::mojom::kBrowserProcessId;
+  params->is_corb_enabled = false;
   network::mojom::URLLoaderFactoryPtr url_loader_factory;
   network_context_->CreateURLLoaderFactory(
-      mojo::MakeRequest(&url_loader_factory),
-      network::mojom::URLLoaderFactoryParams::New(
-          network::mojom::kBrowserProcessId, false, -1, std::string()));
+      mojo::MakeRequest(&url_loader_factory), std::move(params));
 
   // Generate session level tags.
   base::Value session_tags(base::Value::Type::DICTIONARY);

content/browser/loader/cross_site_document_blocking_browsertest.cc

@@ -689,6 +689,7 @@ IN_PROC_BROWSER_TEST_F(CrossSiteDocumentBlockingTest, PrefetchIsNotImpacted) {
   // Respond to the prefetch request in a way that:
   // 1) will enable caching
   // 2) won't finish until after CORB has blocked the response.
+  FetchHistogramsFromChildProcesses();
   base::HistogramTester histograms;
   std::string response_bytes =
       "HTTP/1.1 200 OK\r\n"

content/public/browser/site_isolation_policy.cc

@@ -20,6 +20,7 @@
 #include "content/public/common/content_client.h"
 #include "content/public/common/content_features.h"
 #include "content/public/common/content_switches.h"
+#include "content/public/common/resource_type.h"
 #include "services/network/public/mojom/network_service.mojom.h"
 #include "url/gurl.h"
 
@@ -87,6 +88,7 @@ void SiteIsolationPolicy::PopulateURLLoaderFactoryParamsPtrForCORB(
   if (!params->is_corb_enabled)
     return;
 
+  params->corb_detachable_resource_type = RESOURCE_TYPE_PREFETCH;
   params->corb_excluded_resource_type = RESOURCE_TYPE_PLUGIN_RESOURCE;
 
   const char* initiator_scheme_exception =

services/network/BUILD.gn

@@ -28,6 +28,8 @@ component("network_service") {
     "cross_origin_read_blocking.h",
     "data_pipe_element_reader.cc",
     "data_pipe_element_reader.h",
+    "empty_url_loader_client.cc",
+    "empty_url_loader_client.h",
     "expect_ct_reporter.cc",
     "expect_ct_reporter.h",
     "http_cache_data_remover.cc",

services/network/empty_url_loader_client.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "services/network/empty_url_loader_client.h"
+
+#include <utility>
+
+#include "base/bind.h"
+#include "base/threading/sequenced_task_runner_handle.h"
+
+namespace network {
+
+// static
+void EmptyURLLoaderClient::DrainURLRequest(
+    mojom::URLLoaderClientRequest client_request,
+    mojom::URLLoaderPtr url_loader) {
+  // Raw |new| is okay, because the newly constructed EmptyURLLoaderClient will
+  // delete itself after consuming all the data/callbacks.
+  new EmptyURLLoaderClient(std::move(client_request), std::move(url_loader));
+}
+
+EmptyURLLoaderClient::EmptyURLLoaderClient(
+    mojom::URLLoaderClientRequest request,
+    mojom::URLLoaderPtr url_loader)
+    : binding_(this, std::move(request)), url_loader_(std::move(url_loader)) {
+  binding_.set_connection_error_handler(base::BindOnce(
+      &EmptyURLLoaderClient::DeleteSelf, base::Unretained(this)));
+}
+
+EmptyURLLoaderClient::~EmptyURLLoaderClient() {}
+
+void EmptyURLLoaderClient::DeleteSelf() {
+  delete this;
+}
+
+void EmptyURLLoaderClient::OnReceiveResponse(const ResourceResponseHead& head) {
+}
+
+void EmptyURLLoaderClient::OnReceiveRedirect(
+    const net::RedirectInfo& redirect_info,
+    const ResourceResponseHead& head) {}
+
+void EmptyURLLoaderClient::OnUploadProgress(int64_t current_position,
+                                            int64_t total_size,
+                                            OnUploadProgressCallback callback) {
+  std::move(callback).Run();
+}
+
+void EmptyURLLoaderClient::OnReceiveCachedMetadata(
+    const std::vector<uint8_t>& data) {}
+
+void EmptyURLLoaderClient::OnTransferSizeUpdated(int32_t transfer_size_diff) {}
+
+void EmptyURLLoaderClient::OnStartLoadingResponseBody(
+    mojo::ScopedDataPipeConsumerHandle body) {
+  DCHECK(!response_body_drainer_);
+  response_body_drainer_ =
+      std::make_unique<mojo::DataPipeDrainer>(this, std::move(body));
+}
+
+void EmptyURLLoaderClient::OnComplete(const URLLoaderCompletionStatus& status) {
+  DeleteSelf();
+}
+
+void EmptyURLLoaderClient::OnDataAvailable(const void* data, size_t num_bytes) {
+}
+
+void EmptyURLLoaderClient::OnDataComplete() {}
+
+}  // namespace network

services/network/empty_url_loader_client.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SERVICES_NETWORK_EMPTY_URL_LOADER_CLIENT_H_
+#define SERVICES_NETWORK_EMPTY_URL_LOADER_CLIENT_H_
+
+#include <memory>
+
+#include "base/macros.h"
+#include "mojo/public/cpp/bindings/binding.h"
+#include "mojo/public/cpp/system/data_pipe_drainer.h"
+#include "services/network/public/mojom/url_loader.mojom.h"
+
+namespace network {
+
+// Helper for draining/discarding data and callbacks that go to URLLoaderClient.
+class EmptyURLLoaderClient : public mojom::URLLoaderClient,
+                             public mojo::DataPipeDrainer::Client {
+ public:
+  // Binds |client_request| to a newly constructed EmptyURLLoaderClient which
+  // will drain/discard all callbacks/data.  Takes ownership of |url_loader| and
+  // discards ith (together with EmptyURLLoaderClient) when the URL request has
+  // been completed.
+  static void DrainURLRequest(mojom::URLLoaderClientRequest client_request,
+                              mojom::URLLoaderPtr url_loader);
+
+ private:
+  EmptyURLLoaderClient(mojom::URLLoaderClientRequest client_request,
+                       mojom::URLLoaderPtr url_loader);
+
+  ~EmptyURLLoaderClient() override;
+  void DeleteSelf();
+
+  // mojom::URLLoaderClient overrides:
+  void OnReceiveResponse(const ResourceResponseHead& head) override;
+  void OnReceiveRedirect(const net::RedirectInfo& redirect_info,
+                         const ResourceResponseHead& head) override;
+  void OnUploadProgress(int64_t current_position,
+                        int64_t total_size,
+                        OnUploadProgressCallback callback) override;
+  void OnReceiveCachedMetadata(const std::vector<uint8_t>& data) override;
+  void OnTransferSizeUpdated(int32_t transfer_size_diff) override;
+  void OnStartLoadingResponseBody(
+      mojo::ScopedDataPipeConsumerHandle body) override;
+  void OnComplete(const URLLoaderCompletionStatus& status) override;
+
+  // mojo::DataPipeDrainer::Client overrides:
+  void OnDataAvailable(const void* data, size_t num_bytes) override;
+  void OnDataComplete() override;
+
+  mojo::Binding<mojom::URLLoaderClient> binding_;
+
+  std::unique_ptr<mojo::DataPipeDrainer> response_body_drainer_;
+
+  mojom::URLLoaderPtr url_loader_;
+
+  DISALLOW_COPY_AND_ASSIGN(EmptyURLLoaderClient);
+};
+
+}  // namespace network
+
+#endif  // SERVICES_NETWORK_EMPTY_URL_LOADER_CLIENT_H_

services/network/public/mojom/network_context.mojom

@@ -290,6 +290,12 @@ struct URLLoaderFactoryParams {
 
   // Cross-origin read blocking (CORB) configuration.
   bool is_corb_enabled = true;
+  // TODO(lukasza): The field below in practice is always set to
+  // RESOURCE_TYPE_PREFETCH 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.  The field below in practice
   // is always set to RESOURCE_TYPE_PLUGIN_RESOURCE by the //content layer, but

services/network/url_loader.cc

@@ -27,6 +27,7 @@
 #include "net/url_request/url_request_context_getter.h"
 #include "services/network/chunked_data_pipe_upload_data_stream.h"
 #include "services/network/data_pipe_element_reader.h"
+#include "services/network/empty_url_loader_client.h"
 #include "services/network/loader_util.h"
 #include "services/network/network_usage_accumulator.h"
 #include "services/network/public/cpp/features.h"
@@ -622,7 +623,8 @@ void URLLoader::OnResponseStarted(net::URLRequest* url_request, int net_error) {
     if (corb_analyzer_->ShouldBlock()) {
       DCHECK(!is_more_corb_sniffing_needed_);
       corb_analyzer_->LogBlockedResponse();
-      BlockResponseForCorb();
+      if (BlockResponseForCorb() == kWillCancelRequest)
+        return;
     } else if (corb_analyzer_->ShouldAllow()) {
       DCHECK(!is_more_corb_sniffing_needed_);
       corb_analyzer_->LogAllowedResponse();
@@ -734,7 +736,8 @@ void URLLoader::DidRead(int num_bytes, bool completed_synchronously) {
       if (corb_analyzer_->ShouldBlock()) {
         corb_analyzer_->LogBlockedResponse();
         is_more_corb_sniffing_needed_ = false;
-        BlockResponseForCorb();
+        if (BlockResponseForCorb() == kWillCancelRequest)
+          return;
       } else if (corb_analyzer_->ShouldAllow()) {
         corb_analyzer_->LogAllowedResponse();
         is_more_corb_sniffing_needed_ = false;
@@ -830,20 +833,18 @@ void URLLoader::NotifyCompleted(int error_code) {
     upload_progress_tracker_ = nullptr;
   }
 
-  if (consumer_handle_.is_valid())
-    SendResponseToClient();
+  if (network_usage_accumulator_) {
+    network_usage_accumulator_->OnBytesTransferred(
+        factory_params_->process_id, render_frame_id_,
+        url_request_->GetTotalReceivedBytes(),
+        url_request_->GetTotalSentBytes());
+  }
 
-  URLLoaderCompletionStatus status;
-  if (did_corb_block_response_) {
-    // CORB responses are reported as a success.
-    status.error_code = net::OK;
-    status.completion_time = base::TimeTicks::Now();
-    status.encoded_data_length = 0;
-    status.encoded_body_length = 0;
-    status.decoded_body_length = 0;
-    status.should_report_corb_blocking =
-        corb_analyzer_->ShouldReportBlockedResponse();
-  } else {
+  if (url_loader_client_) {
+    if (consumer_handle_.is_valid())
+      SendResponseToClient();
+
+    URLLoaderCompletionStatus status;
     status.error_code = error_code;
     if (error_code == net::ERR_QUIC_PROTOCOL_ERROR) {
       net::NetErrorDetails details;
@@ -861,16 +862,10 @@ void URLLoader::NotifyCompleted(int error_code) {
         !net::IsCertStatusMinorError(url_request_->ssl_info().cert_status)) {
       status.ssl_info = url_request_->ssl_info();
     }
-  }
 
-  if (network_usage_accumulator_) {
-    network_usage_accumulator_->OnBytesTransferred(
-        factory_params_->process_id, render_frame_id_,
-        url_request_->GetTotalReceivedBytes(),
-        url_request_->GetTotalSentBytes());
+    url_loader_client_->OnComplete(status);
   }
 
-  url_loader_client_->OnComplete(status);
   DeleteSelf();
 }
 
@@ -992,23 +987,71 @@ void URLLoader::RecordBodyReadFromNetBeforePausedIfNeeded() {
   }
 }
 
-void URLLoader::BlockResponseForCorb() {
-  // TODO(lukasza): https://crbug.com/846334: Need to make sure that the cache
-  // is still populated in the prefetch case (e.g. the implementation of CORB in
-  // CrossSiteDocumentResourceHandler would detach rather than cancel).
-  url_request_->Cancel();
-
-  // Remember that blocking happened (so that we can report success, rather than
-  // cancellation status to the URLLoaderClient).
-  did_corb_block_response_ = true;
+URLLoader::BlockResponseForCorbResult URLLoader::BlockResponseForCorb() {
+  // The response headers and body shouldn't yet be sent to the URLLoaderClient.
+  DCHECK(response_);
+  DCHECK(consumer_handle_.is_valid());
 
-  // Block the headers.
+  // Send stripped headers to the real URLLoaderClient.
   CrossOriginReadBlocking::SanitizeBlockedResponse(response_);
+  url_loader_client_->OnReceiveResponse(response_->head);
 
-  // Block the response body.
-  mojo::DataPipe data_pipe(kBlockedBodyAllocationSize);
-  data_pipe.producer_handle.reset();  // No bytes in the blocked body.
-  consumer_handle_ = std::move(data_pipe.consumer_handle);
+  // Send empty body to the real URLLoaderClient.
+  mojo::DataPipe empty_data_pipe(kBlockedBodyAllocationSize);
+  empty_data_pipe.producer_handle.reset();
+  url_loader_client_->OnStartLoadingResponseBody(
+      std::move(empty_data_pipe.consumer_handle));
+
+  // Tell the real URLLoaderClient that the response has been completed.
+  URLLoaderCompletionStatus status;
+  if (resource_type_ == factory_params_->corb_detachable_resource_type) {
+    // 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.
+    status.error_code = net::ERR_ABORTED;
+  } else {
+    // CORB responses are reported as a success.
+    status.error_code = net::OK;
+  }
+  status.completion_time = base::TimeTicks::Now();
+  status.encoded_data_length = 0;
+  status.encoded_body_length = 0;
+  status.decoded_body_length = 0;
+  status.should_report_corb_blocking =
+      corb_analyzer_->ShouldReportBlockedResponse();
+  url_loader_client_->OnComplete(status);
+
+  // Reset the connection to the URLLoaderClient.  This helps ensure that we
+  // won't accidentally leak any data to the renderer from this point on.
+  url_loader_client_.reset();
+
+  // 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) {
+    // Discard any remaining callbacks or data by rerouting the pipes to
+    // EmptyURLLoaderClient (deleting |self_ptr| when the URL request
+    // completes).
+    mojom::URLLoaderPtr self_ptr;
+    binding_.Close();
+    binding_.Bind(mojo::MakeRequest(&self_ptr));
+    binding_.set_connection_error_handler(
+        base::BindOnce(&URLLoader::OnConnectionError, base::Unretained(this)));
+    EmptyURLLoaderClient::DrainURLRequest(
+        mojo::MakeRequest(&url_loader_client_), std::move(self_ptr));
+
+    // Ask the caller to continue processing the request.
+    return kContinueRequest;
+  }
+  // Delete self and cancel the request - the caller doesn't need to continue.
+  //
+  // DeleteSelf is posted asynchronously, to make sure that the callers (e.g.
+  // URLLoader::OnResponseStarted and/or URLLoader::DidRead instance methods)
+  // can still safely dereference |this|.
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&URLLoader::DeleteSelf, weak_ptr_factory_.GetWeakPtr()));
+  return kWillCancelRequest;
 }
 
 }  // namespace network

services/network/url_loader.h

@@ -147,7 +147,17 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) URLLoader
   bool HasDataPipe() const;
   void RecordBodyReadFromNetBeforePausedIfNeeded();
   void ResumeStart();
-  void BlockResponseForCorb();
+
+  enum BlockResponseForCorbResult {
+    // Returned when caller of BlockResponseForCorb doesn't need to continue,
+    // because the request will be cancelled soon.
+    kWillCancelRequest,
+
+    // Returned when the caller of BlockResponseForCorb should continue
+    // processing the request (e.g. by calling ReadMore as necessary).
+    kContinueRequest,
+  };
+  BlockResponseForCorbResult BlockResponseForCorb();
 
   net::URLRequestContext* url_request_context_;
   mojom::NetworkServiceClient* network_service_client_;
@@ -186,7 +196,6 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) URLLoader
 
   // Sniffing state.
   std::unique_ptr<CrossOriginReadBlocking::ResponseAnalyzer> corb_analyzer_;
-  bool did_corb_block_response_ = false;
   bool is_more_corb_sniffing_needed_ = false;
   bool is_more_mime_sniffing_needed_ = false;
 

testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter

@@ -34,9 +34,6 @@
 # Cross-origin request to file:// URL should be blocked. crbug.com/759230
 -BrowserSideNavigationBrowserDisableWebSecurityTest.ValidateBaseUrlForDataUrl
 
-# https://crbug.com/846334: CORB: Need to make sure that prefetch/caching works
--CrossSiteDocumentBlockingTest.PrefetchIsNotImpacted
-
 # https://crbug.com/846352: CORB/NetworkService: Remove
 # CrossSiteDocumentResourceHandler while preserving test coverage - the tests
 # below can be probably removed altogether once NetworkService ships and the