Throttle transfer size updates and relax when it was reported

Currently transfer size updates are reported only when report_raw_headers is set.
This CL relaxes that requirement and checks if either report_raw_headers is set
(to work with devtools) or the renderer has permission to read the resource.

This CL also throttles the transfer size updates to be reported only once per
500 ms per resource.

Bug: 836029
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I7e34a0b0c5fad7e424772c075041c0609dc1b365
Reviewed-on: https://chromium-review.googlesource.com/1050814
Commit-Queue: rajendrant <rajendrant@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572064}

content/browser/loader/cross_site_document_resource_handler.cc

@@ -435,6 +435,7 @@ void CrossSiteDocumentResourceHandler::OnReadCompleted(
 
     // Block the response and throw away the data.  Report zero bytes read.
     blocked_read_completed_ = true;
+    info->set_blocked_response_from_reaching_renderer(true);
     if (analyzer_->ShouldReportBlockedResponse())
       info->set_should_report_corb_blocking(true);
     network::CrossOriginReadBlocking::SanitizeBlockedResponse(

content/browser/loader/mojo_async_resource_handler.cc

@@ -38,6 +38,21 @@ constexpr size_t kMinAllocationSize = 2 * net::kMaxBytesToSniff;
 
 constexpr size_t kMaxChunkSize = 32 * 1024;
 
+// Time between sending the transfer size updates to renderer. This threshold is
+// chosen as a compromise between sending too frequent updates and the limit its
+// consumers (DevTools and page load metrics) expect.
+constexpr base::TimeDelta kTransferSizeReportInterval =
+    base::TimeDelta::FromMilliseconds(500);
+
+bool ShouldReportTransferSize(
+    const ResourceRequestInfoImpl* resource_request_info) {
+  // Transfer size is reported only when report_raw_headers is set or the
+  // renderer is allowed to receive the resource response metadata (e.g. by
+  // Cross-Origin Read Blocking).
+  return resource_request_info->ShouldReportRawHeaders() ||
+         !resource_request_info->blocked_response_from_reaching_renderer();
+}
+
 }  // namespace
 
 // This class is for sharing the ownership of a ScopedDataPipeProducerHandle
@@ -303,11 +318,15 @@ void MojoAsyncResourceHandler::OnReadCompleted(
     return;
   }
 
-  const ResourceRequestInfoImpl* info = GetRequestInfo();
-  if (info->ShouldReportRawHeaders()) {
+  if (ShouldReportTransferSize(GetRequestInfo()) &&
+      (time_transfer_size_next_report_.is_null() ||
+       time_transfer_size_next_report_ <= base::TimeTicks::Now())) {
     auto transfer_size_diff = CalculateRecentlyReceivedBytes();
-    if (transfer_size_diff > 0)
+    if (transfer_size_diff > 0) {
       url_loader_client_->OnTransferSizeUpdated(transfer_size_diff);
+      time_transfer_size_next_report_ =
+          base::TimeTicks::Now() + kTransferSizeReportInterval;
+    }
   }
 
   if (response_body_consumer_handle_.is_valid()) {
@@ -487,6 +506,12 @@ void MojoAsyncResourceHandler::OnResponseCompleted(
     loader_status.ssl_info = request()->ssl_info();
   }
 
+  if (ShouldReportTransferSize(GetRequestInfo())) {
+    auto transfer_size_diff = CalculateRecentlyReceivedBytes();
+    if (transfer_size_diff > 0)
+      url_loader_client_->OnTransferSizeUpdated(transfer_size_diff);
+  }
+
   url_loader_client_->OnComplete(loader_status);
   controller->Resume();
 }

content/browser/loader/mojo_async_resource_handler.h

@@ -149,6 +149,9 @@ class CONTENT_EXPORT MojoAsyncResourceHandler
   bool did_defer_on_writing_ = false;
   bool did_defer_on_redirect_ = false;
   bool did_defer_on_response_started_ = false;
+
+  // The time transfer size should be reported next.
+  base::TimeTicks time_transfer_size_next_report_;
   int64_t reported_total_received_bytes_ = 0;
   int64_t total_written_bytes_ = 0;
 

content/browser/loader/mojo_async_resource_handler_unittest.cc

@@ -49,7 +49,9 @@
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_job_factory_impl.h"
 #include "net/url_request/url_request_status.h"
+#include "net/url_request/url_request_test_job.h"
 #include "net/url_request/url_request_test_util.h"
 #include "services/network/public/cpp/resource_response.h"
 #include "services/network/public/cpp/url_loader_completion_status.h"
@@ -304,9 +306,9 @@ class MojoAsyncResourceHandlerTestBase {
 
     // Create and initialize |request_|.  None of this matters, for these tests,
     // just need something non-NULL.
-    net::URLRequestContext* request_context =
+    request_context_ =
         browser_context_->GetResourceContext()->GetRequestContext();
-    request_ = request_context->CreateRequest(
+    request_ = request_context_->CreateRequest(
         GURL("http://foo/"), net::DEFAULT_PRIORITY, &url_request_delegate_,
         TRAFFIC_ANNOTATION_FOR_TESTS);
     request_->set_upload(std::move(upload_stream));
@@ -403,6 +405,7 @@ class MojoAsyncResourceHandlerTestBase {
   network::TestURLLoaderClient url_loader_client_;
   std::unique_ptr<TestBrowserContext> browser_context_;
   net::TestDelegate url_request_delegate_;
+  net::URLRequestContext* request_context_;
   std::unique_ptr<net::URLRequest> request_;
   std::unique_ptr<MojoAsyncResourceHandlerWithStubOperations> handler_;
   std::unique_ptr<MockResourceLoader> mock_loader_;
@@ -1459,6 +1462,66 @@ TEST_F(MojoAsyncResourceHandlerSendSSLInfoForCertificateError,
   EXPECT_FALSE(url_loader_client_.completion_status().ssl_info);
 };
 
+TEST_F(MojoAsyncResourceHandlerTest,
+       TransferSizeUpdateCalledForNonBlockedResponse) {
+  net::URLRequestJobFactoryImpl test_job_factory_;
+  auto test_job = std::make_unique<net::URLRequestTestJob>(
+      request_.get(), request_context_->network_delegate(), "response headers",
+      "response body", true);
+  auto test_job_interceptor = std::make_unique<net::TestJobInterceptor>();
+  net::TestJobInterceptor* raw_test_job_interceptor =
+      test_job_interceptor.get();
+  EXPECT_TRUE(test_job_factory_.SetProtocolHandler(
+      url::kHttpScheme, std::move(test_job_interceptor)));
+
+  request_context_->set_job_factory(&test_job_factory_);
+  raw_test_job_interceptor->set_main_intercept_job(std::move(test_job));
+  request_->Start();
+
+  ASSERT_TRUE(CallOnWillStartAndOnResponseStarted());
+  url_loader_client_.RunUntilComplete();
+
+  net::URLRequestStatus status(net::URLRequestStatus::SUCCESS, net::OK);
+  ASSERT_EQ(MockResourceLoader::Status::IDLE,
+            mock_loader_->OnResponseCompleted(status));
+  url_loader_client_.RunUntilComplete();
+  EXPECT_LT(0, url_loader_client_.body_transfer_size());
+  EXPECT_EQ(request_->GetTotalReceivedBytes(),
+            url_loader_client_.body_transfer_size());
+}
+
+TEST_F(MojoAsyncResourceHandlerTest,
+       TransferSizeUpdateNotCalledForBlockedResponse) {
+  net::URLRequestJobFactoryImpl test_job_factory_;
+  auto test_job = std::make_unique<net::URLRequestTestJob>(
+      request_.get(), request_context_->network_delegate(), "response headers",
+      "response body", true);
+  auto test_job_interceptor = std::make_unique<net::TestJobInterceptor>();
+  net::TestJobInterceptor* raw_test_job_interceptor =
+      test_job_interceptor.get();
+  EXPECT_TRUE(test_job_factory_.SetProtocolHandler(
+      url::kHttpScheme, std::move(test_job_interceptor)));
+
+  request_context_->set_job_factory(&test_job_factory_);
+  raw_test_job_interceptor->set_main_intercept_job(std::move(test_job));
+  request_->Start();
+
+  // Block the response to reach renderer.
+  ResourceRequestInfoImpl::ForRequest(request_.get())
+      ->set_blocked_response_from_reaching_renderer(true);
+
+  ASSERT_TRUE(CallOnWillStartAndOnResponseStarted());
+
+  net::URLRequestStatus status(net::URLRequestStatus::SUCCESS, net::OK);
+  ASSERT_EQ(MockResourceLoader::Status::IDLE,
+            mock_loader_->OnResponseCompleted(status));
+  url_loader_client_.RunUntilComplete();
+  EXPECT_TRUE(ResourceRequestInfoImpl::ForRequest(request_.get())
+                  ->blocked_response_from_reaching_renderer());
+  EXPECT_EQ(0, url_loader_client_.body_transfer_size());
+  EXPECT_LT(0, request_->GetTotalReceivedBytes());
+}
+
 INSTANTIATE_TEST_CASE_P(MojoAsyncResourceHandlerWithAllocationSizeTest,
                         MojoAsyncResourceHandlerWithAllocationSizeTest,
                         ::testing::Values(8, 32 * 2014));

content/browser/loader/resource_request_info_impl.cc

@@ -189,6 +189,7 @@ ResourceRequestInfoImpl::ResourceRequestInfoImpl(
       previews_state_(previews_state),
       body_(body),
       initiated_in_secure_context_(initiated_in_secure_context),
+      blocked_response_from_reaching_renderer_(false),
       should_report_corb_blocking_(false),
       first_auth_attempt_(true) {}
 

content/browser/loader/resource_request_info_impl.h

@@ -192,6 +192,12 @@ class ResourceRequestInfoImpl : public ResourceRequestInfo,
 
   void SetBlobHandles(BlobHandles blob_handles);
 
+  bool blocked_response_from_reaching_renderer() const {
+    return blocked_response_from_reaching_renderer_;
+  }
+  void set_blocked_response_from_reaching_renderer(bool value) {
+    blocked_response_from_reaching_renderer_ = value;
+  }
   bool should_report_corb_blocking() const {
     return should_report_corb_blocking_;
   }
@@ -249,6 +255,12 @@ class ResourceRequestInfoImpl : public ResourceRequestInfo,
   scoped_refptr<network::ResourceRequestBody> body_;
   bool initiated_in_secure_context_;
   std::unique_ptr<NavigationUIData> navigation_ui_data_;
+
+  // Whether response details (response headers, timing information, metadata)
+  // have been blocked from reaching the renderer process (e.g. by Cross-Origin
+  // Read Blocking).
+  bool blocked_response_from_reaching_renderer_;
+
   bool should_report_corb_blocking_;
   bool first_auth_attempt_;
 

content/browser/loader/url_loader_factory_impl_unittest.cc

@@ -203,8 +203,8 @@ TEST_P(URLLoaderFactoryImplTest, GetResponse) {
             client.completion_status().encoded_data_length);
   EXPECT_EQ(static_cast<int64_t>(expected.size()),
             client.completion_status().encoded_body_length);
-  // OnTransferSizeUpdated is not dispatched as report_raw_headers is not set.
-  EXPECT_EQ(0, client.body_transfer_size());
+  EXPECT_EQ(static_cast<int64_t>(expected.size()), client.body_transfer_size());
+  EXPECT_GT(client.body_transfer_size(), 0);
   EXPECT_GT(client.response_head().encoded_data_length, 0);
   EXPECT_GT(client.completion_status().encoded_data_length, 0);
 }

services/network/public/mojom/url_loader.mojom

@@ -101,14 +101,17 @@ interface URLLoaderClient {
   OnReceiveCachedMetadata(array<uint8> data);
 
   // Called when the transfer size is updated. This is only called if
-  // |report_raw_headers| is set in the request.
-  // The transfer size is the length of the response (including both headers
-  // and the body) over the network. |transfer_size_diff| is the difference from
-  // the value previously reported one (including the one in OnReceiveResponse
-  // and OnReceiveRedirect). It must be positive.
+  // |report_raw_headers| is set or the renderer has permission to read the
+  // request. The transfer size is the length of the response (including both
+  // headers and the body) over the network. |transfer_size_diff| is the
+  // difference from the value previously reported one (including the one in
+  // OnReceiveResponse and OnReceiveRedirect). It must be positive.
   // TODO(yhirano): Dispatch this notification even when |download_to_file| is
   // set.
   // TODO(yhirano): Consider using an unsigned type.
+  // TODO(rajendrant): Consider reporting the transfer size directly to browser
+  // process from net service, and not pass it via renderer process. This can be
+  // done after the upcoming network servicification work.
   OnTransferSizeUpdated(int32 transfer_size_diff);
 
   // Called when the loader starts loading response body. This is called after

services/network/test/test_url_loader_client.cc

@@ -53,7 +53,6 @@ void TestURLLoaderClient::OnReceiveCachedMetadata(
 }
 
 void TestURLLoaderClient::OnTransferSizeUpdated(int32_t transfer_size_diff) {
-  EXPECT_TRUE(has_received_response_);
   EXPECT_FALSE(has_received_completion_);
   EXPECT_GT(transfer_size_diff, 0);
   body_transfer_size_ += transfer_size_diff;