[fetch keepalive] Introduce limit on the size of URL and headers

This is (3) in
https://docs.google.com/document/d/1sMG4xAT-myWtFaNa0kuLjRqsyxSy12ahgRIbffy1Bxk/.

Bug: 1018050
Change-Id: Ib265bd86153842b4f90c836fb0b06d200e94a787
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1880361
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#709882}

content/browser/loader/navigation_url_loader_impl_unittest.cc

@@ -96,7 +96,8 @@ class TestNavigationLoaderInterceptor : public NavigationLoaderInterceptor {
         std::move(request), 0 /* options */, resource_request,
         std::move(client), TRAFFIC_ANNOTATION_FOR_TESTS, &params,
         0, /* request_id */
-        resource_scheduler_client_, nullptr /* keepalive_statistics_recorder */,
+        0 /* keepalive_request_size */, resource_scheduler_client_,
+        nullptr /* keepalive_statistics_recorder */,
         nullptr /* network_usage_accumulator */, nullptr /* header_client */,
         nullptr /* origin_policy_manager */);
   }

services/network/keepalive_statistics_recorder.cc

@@ -51,10 +51,12 @@ void KeepaliveStatisticsRecorder::Unregister(int process_id) {
   --it->second.num_registrations;
 }
 
-void KeepaliveStatisticsRecorder::OnLoadStarted(int process_id) {
+void KeepaliveStatisticsRecorder::OnLoadStarted(int process_id,
+                                                int request_size) {
   auto it = per_process_records_.find(process_id);
   if (it != per_process_records_.end()) {
     ++it->second.num_inflight_requests;
+    it->second.total_request_size += request_size;
     if (it->second.peak_inflight_requests < it->second.num_inflight_requests) {
       it->second.peak_inflight_requests = it->second.num_inflight_requests;
       if (!base::FeatureList::IsEnabled(features::kDisableKeepaliveFetch)) {
@@ -75,10 +77,14 @@ void KeepaliveStatisticsRecorder::OnLoadStarted(int process_id) {
   }
 }
 
-void KeepaliveStatisticsRecorder::OnLoadFinished(int process_id) {
+void KeepaliveStatisticsRecorder::OnLoadFinished(int process_id,
+                                                 int request_size) {
   auto it = per_process_records_.find(process_id);
-  if (it != per_process_records_.end())
+  if (it != per_process_records_.end()) {
     --it->second.num_inflight_requests;
+    DCHECK_GE(it->second.total_request_size, request_size);
+    it->second.total_request_size -= request_size;
+  }
   --num_inflight_requests_;
 }
 
@@ -90,6 +96,14 @@ int KeepaliveStatisticsRecorder::NumInflightRequestsPerProcess(
   return it->second.num_inflight_requests;
 }
 
+int KeepaliveStatisticsRecorder::GetTotalRequestSizePerProcess(
+    int process_id) const {
+  auto it = per_process_records_.find(process_id);
+  if (it == per_process_records_.end())
+    return 0;
+  return it->second.total_request_size;
+}
+
 bool KeepaliveStatisticsRecorder::HasRecordForProcess(int process_id) const {
   auto it = per_process_records_.find(process_id);
   if (it != per_process_records_.end()) {

services/network/keepalive_statistics_recorder.h

@@ -23,6 +23,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) KeepaliveStatisticsRecorder
     int num_registrations = 1;
     int num_inflight_requests = 0;
     int peak_inflight_requests = 0;
+    int total_request_size = 0;
   };
 
   KeepaliveStatisticsRecorder();
@@ -36,9 +37,9 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) KeepaliveStatisticsRecorder
   void Unregister(int process_id);
 
   // Called when a request with keepalive set starts.
-  void OnLoadStarted(int process_id);
+  void OnLoadStarted(int process_id, int request_size);
   // Called when a request with keepalive set finishes.
-  void OnLoadFinished(int process_id);
+  void OnLoadFinished(int process_id, int request_size);
 
   const std::unordered_map<int, PerProcessStats>& per_process_records() const {
     return per_process_records_;
@@ -46,6 +47,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) KeepaliveStatisticsRecorder
   // Returns true iff. number of Register calls > Unregister calls.
   bool HasRecordForProcess(int process_id) const;
   int NumInflightRequestsPerProcess(int process_id) const;
+  int GetTotalRequestSizePerProcess(int process_id) const;
   int num_inflight_requests() const { return num_inflight_requests_; }
   int peak_inflight_requests() const { return peak_inflight_requests_; }
 

services/network/keepalive_statistics_recorder_unittest.cc

@@ -80,7 +80,7 @@ TEST(KeepaliveStatisticsRecorderTest, IssueOneRequest) {
   constexpr int process = 4;
 
   r.Register(process);
-  r.OnLoadStarted(process);
+  r.OnLoadStarted(process, 12);
   {
     const auto& map = r.per_process_records();
     EXPECT_EQ(1u, map.size());
@@ -89,12 +89,13 @@ TEST(KeepaliveStatisticsRecorderTest, IssueOneRequest) {
     EXPECT_EQ(1, it->second.num_registrations);
     EXPECT_EQ(1, it->second.num_inflight_requests);
     EXPECT_EQ(1, it->second.peak_inflight_requests);
+    EXPECT_EQ(12, it->second.total_request_size);
 
     EXPECT_EQ(1, r.num_inflight_requests());
     EXPECT_EQ(1, r.peak_inflight_requests());
   }
 
-  r.OnLoadFinished(process);
+  r.OnLoadFinished(process, 12);
   {
     const auto& map = r.per_process_records();
     EXPECT_EQ(1u, map.size());
@@ -103,6 +104,7 @@ TEST(KeepaliveStatisticsRecorderTest, IssueOneRequest) {
     EXPECT_EQ(1, it->second.num_registrations);
     EXPECT_EQ(0, it->second.num_inflight_requests);
     EXPECT_EQ(1, it->second.peak_inflight_requests);
+    EXPECT_EQ(0, it->second.total_request_size);
 
     EXPECT_EQ(0, r.num_inflight_requests());
     EXPECT_EQ(1, r.peak_inflight_requests());
@@ -121,23 +123,23 @@ TEST(KeepaliveStatisticsRecorderTest, IssueRequests) {
   r.Register(process2);
   r.Register(process2);
 
-  r.OnLoadStarted(process1);
-  r.OnLoadStarted(process1);
-  r.OnLoadStarted(process2);
-  r.OnLoadStarted(process2);
-  r.OnLoadStarted(process2);
-  r.OnLoadStarted(process2);
-  r.OnLoadStarted(no_process);
-  r.OnLoadFinished(process2);
-  r.OnLoadFinished(process2);
-  r.OnLoadFinished(process2);
-  r.OnLoadStarted(process2);
-  r.OnLoadStarted(no_process);
-  r.OnLoadStarted(no_process);
-  r.OnLoadStarted(no_process);
-  r.OnLoadStarted(no_process);
-  r.OnLoadStarted(no_process);
-  r.OnLoadFinished(no_process);
+  r.OnLoadStarted(process1, 13);
+  r.OnLoadStarted(process1, 5);
+  r.OnLoadStarted(process2, 8);
+  r.OnLoadStarted(process2, 4);
+  r.OnLoadStarted(process2, 82);
+  r.OnLoadStarted(process2, 3);
+  r.OnLoadStarted(no_process, 1);
+  r.OnLoadFinished(process2, 4);
+  r.OnLoadFinished(process2, 8);
+  r.OnLoadFinished(process2, 82);
+  r.OnLoadStarted(process2, 13);
+  r.OnLoadStarted(no_process, 4);
+  r.OnLoadStarted(no_process, 5);
+  r.OnLoadStarted(no_process, 6);
+  r.OnLoadStarted(no_process, 7);
+  r.OnLoadStarted(no_process, 8);
+  r.OnLoadFinished(no_process, 6);
 
   const auto& map = r.per_process_records();
   EXPECT_EQ(2u, map.size());
@@ -147,11 +149,13 @@ TEST(KeepaliveStatisticsRecorderTest, IssueRequests) {
   EXPECT_EQ(3, it1->second.num_registrations);
   EXPECT_EQ(2, it1->second.num_inflight_requests);
   EXPECT_EQ(2, it1->second.peak_inflight_requests);
+  EXPECT_EQ(18, it1->second.total_request_size);
 
   ASSERT_NE(it2, map.end());
   EXPECT_EQ(2, it2->second.num_registrations);
   EXPECT_EQ(2, it2->second.num_inflight_requests);
   EXPECT_EQ(4, it2->second.peak_inflight_requests);
+  EXPECT_EQ(16, it2->second.total_request_size);
 
   EXPECT_EQ(9, r.num_inflight_requests());
   EXPECT_EQ(10, r.peak_inflight_requests());
@@ -162,12 +166,12 @@ TEST(KeepaliveStatisticsRecorderTest, ProcessReuse) {
   constexpr int process = 2;
 
   r.Register(process);
-  r.OnLoadStarted(process);
-  r.OnLoadStarted(process);
-  r.OnLoadStarted(process);
-  r.OnLoadFinished(process);
-  r.OnLoadFinished(process);
-  r.OnLoadFinished(process);
+  r.OnLoadStarted(process, 1);
+  r.OnLoadStarted(process, 2);
+  r.OnLoadStarted(process, 3);
+  r.OnLoadFinished(process, 2);
+  r.OnLoadFinished(process, 3);
+  r.OnLoadFinished(process, 1);
   r.Unregister(process);
 
   r.Register(process);
@@ -178,6 +182,7 @@ TEST(KeepaliveStatisticsRecorderTest, ProcessReuse) {
   EXPECT_EQ(1, it->second.num_registrations);
   EXPECT_EQ(0, it->second.num_inflight_requests);
   EXPECT_EQ(0, it->second.peak_inflight_requests);
+  EXPECT_EQ(0, it->second.total_request_size);
 }
 
 }  // namespace

services/network/url_loader.cc

@@ -330,6 +330,7 @@ URLLoader::URLLoader(
     const net::NetworkTrafficAnnotationTag& traffic_annotation,
     const mojom::URLLoaderFactoryParams* factory_params,
     uint32_t request_id,
+    int keepalive_request_size,
     scoped_refptr<ResourceSchedulerClient> resource_scheduler_client,
     base::WeakPtr<KeepaliveStatisticsRecorder> keepalive_statistics_recorder,
     base::WeakPtr<NetworkUsageAccumulator> network_usage_accumulator,
@@ -346,6 +347,7 @@ URLLoader::URLLoader(
       factory_params_(factory_params),
       render_frame_id_(request.render_frame_id),
       request_id_(request_id),
+      keepalive_request_size_(keepalive_request_size),
       keepalive_(request.keepalive),
       do_not_prompt_for_login_(request.do_not_prompt_for_login),
       binding_(this, std::move(url_loader_request)),
@@ -473,21 +475,9 @@ URLLoader::URLLoader(
         base::Bind(&URLLoader::SetRawResponseHeaders, base::Unretained(this)));
   }
 
-  if (keepalive_ && keepalive_statistics_recorder_)
-    keepalive_statistics_recorder_->OnLoadStarted(factory_params_->process_id);
-
-  if (keepalive_) {
-    const size_t url_size = request.url.spec().size();
-    size_t headers_size = 0;
-    for (const auto& pair : merged_headers.GetHeaderVector()) {
-      headers_size += (pair.key.size() + pair.value.size());
-    }
-
-    UMA_HISTOGRAM_COUNTS_10000("Net.KeepaliveRequest.UrlSize", url_size);
-    UMA_HISTOGRAM_COUNTS_10000("Net.KeepaliveRequest.HeadersSize",
-                               headers_size);
-    UMA_HISTOGRAM_COUNTS_10000("Net.KeepaliveRequest.UrlPlusHeadersSize",
-                               url_size + headers_size);
+  if (keepalive_ && keepalive_statistics_recorder_) {
+    keepalive_statistics_recorder_->OnLoadStarted(factory_params_->process_id,
+                                                  keepalive_request_size_);
   }
 
   // Resolve elements from request_body and prepare upload data.
@@ -676,8 +666,10 @@ void URLLoader::ScheduleStart() {
 
 URLLoader::~URLLoader() {
   RecordBodyReadFromNetBeforePausedIfNeeded();
-  if (keepalive_ && keepalive_statistics_recorder_)
-    keepalive_statistics_recorder_->OnLoadFinished(factory_params_->process_id);
+  if (keepalive_ && keepalive_statistics_recorder_) {
+    keepalive_statistics_recorder_->OnLoadFinished(factory_params_->process_id,
+                                                   keepalive_request_size_);
+  }
 }
 
 // static

services/network/url_loader.h

@@ -80,6 +80,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) URLLoader
       const net::NetworkTrafficAnnotationTag& traffic_annotation,
       const mojom::URLLoaderFactoryParams* factory_params,
       uint32_t request_id,
+      int keepalive_request_size,
       scoped_refptr<ResourceSchedulerClient> resource_scheduler_client,
       base::WeakPtr<KeepaliveStatisticsRecorder> keepalive_statistics_recorder,
       base::WeakPtr<NetworkUsageAccumulator> network_usage_accumulator,
@@ -263,6 +264,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) URLLoader
 
   int render_frame_id_;
   uint32_t request_id_;
+  const int keepalive_request_size_;
   const bool keepalive_;
   const bool do_not_prompt_for_login_;
   std::unique_ptr<net::URLRequest> url_request_;

services/network/url_loader_factory.cc

@@ -26,6 +26,7 @@ namespace network {
 
 constexpr int URLLoaderFactory::kMaxKeepaliveConnections;
 constexpr int URLLoaderFactory::kMaxKeepaliveConnectionsPerProcess;
+constexpr int URLLoaderFactory::kMaxTotalKeepaliveRequestSize;
 
 URLLoaderFactory::URLLoaderFactory(
     NetworkContext* context,
@@ -99,13 +100,36 @@ void URLLoaderFactory::CreateLoaderAndStart(
         context_->network_service()->network_usage_accumulator()->AsWeakPtr();
   }
 
+  int keepalive_request_size = 0;
   bool exhausted = false;
   if (url_request.keepalive && keepalive_statistics_recorder) {
+    const size_t url_size = url_request.url.spec().size();
+    size_t headers_size = 0;
+
+    net::HttpRequestHeaders merged_headers = url_request.headers;
+    merged_headers.MergeFrom(url_request.cors_exempt_headers);
+
+    for (const auto& pair : merged_headers.GetHeaderVector()) {
+      headers_size += (pair.key.size() + pair.value.size());
+    }
+
+    UMA_HISTOGRAM_COUNTS_10000("Net.KeepaliveRequest.UrlSize", url_size);
+    UMA_HISTOGRAM_COUNTS_10000("Net.KeepaliveRequest.HeadersSize",
+                               headers_size);
+    UMA_HISTOGRAM_COUNTS_10000("Net.KeepaliveRequest.UrlPlusHeadersSize",
+                               url_size + headers_size);
+
+    keepalive_request_size = url_size + headers_size;
+
     const auto& recorder = *keepalive_statistics_recorder;
-    if (recorder.num_inflight_requests() >= kMaxKeepaliveConnections)
+    if (recorder.num_inflight_requests() >= kMaxKeepaliveConnections) {
+      exhausted = true;
+    } else if (recorder.NumInflightRequestsPerProcess(params_->process_id) >=
+               kMaxKeepaliveConnectionsPerProcess) {
       exhausted = true;
-    if (recorder.NumInflightRequestsPerProcess(params_->process_id) >=
-        kMaxKeepaliveConnectionsPerProcess) {
+    } else if (recorder.GetTotalRequestSizePerProcess(params_->process_id) +
+                   keepalive_request_size >
+               kMaxTotalKeepaliveRequestSize) {
       exhausted = true;
     }
   }
@@ -129,8 +153,8 @@ void URLLoaderFactory::CreateLoaderAndStart(
                      base::Unretained(cors_url_loader_factory_)),
       std::move(request), options, url_request, std::move(client),
       static_cast<net::NetworkTrafficAnnotationTag>(traffic_annotation),
-      params_.get(), request_id, resource_scheduler_client_,
-      std::move(keepalive_statistics_recorder),
+      params_.get(), request_id, keepalive_request_size,
+      resource_scheduler_client_, std::move(keepalive_statistics_recorder),
       std::move(network_usage_accumulator),
       header_client_.is_bound() ? header_client_.get() : nullptr,
       context_->origin_policy_manager());

services/network/url_loader_factory.h

@@ -63,6 +63,7 @@ class URLLoaderFactory : public mojom::URLLoaderFactory {
 
   static constexpr int kMaxKeepaliveConnections = 2048;
   static constexpr int kMaxKeepaliveConnectionsPerProcess = 256;
+  static constexpr int kMaxTotalKeepaliveRequestSize = 256 * 1024;
 
  private:
   // The NetworkContext that indirectly owns |this|.