Loosen the restriction on the number of keepalive requests

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

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

content/browser/renderer_host/render_process_host_browsertest.cc

@@ -52,6 +52,93 @@
 
 namespace content {
 
+namespace {
+
+// Similar to net::test_server::DelayedHttpResponse, but the delay is resolved
+// through Resolver.
+class DelayedHttpResponseWithResolver final
+    : public net::test_server::BasicHttpResponse {
+ public:
+  struct ResponseWithCallbacks final {
+    net::test_server::SendBytesCallback send_callback;
+    net::test_server::SendCompleteCallback done_callback;
+    std::string response_string;
+  };
+
+  class Resolver final : public base::RefCountedThreadSafe<Resolver> {
+   public:
+    void Resolve() {
+      base::AutoLock auto_lock(lock_);
+      DCHECK(!resolved_);
+      resolved_ = true;
+
+      if (!task_runner_) {
+        return;
+      }
+
+      task_runner_->PostTask(
+          FROM_HERE,
+          base::BindOnce(&Resolver::ResolveInServerTaskRunner, this));
+    }
+
+    void Add(const ResponseWithCallbacks& response) {
+      base::AutoLock auto_lock(lock_);
+
+      if (resolved_) {
+        response.send_callback.Run(response.response_string,
+                                   response.done_callback);
+        return;
+      }
+
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner =
+          base::ThreadTaskRunnerHandle::Get();
+      if (task_runner_) {
+        DCHECK_EQ(task_runner_, task_runner);
+      } else {
+        task_runner_ = std::move(task_runner);
+      }
+
+      responses_with_callbacks_.push_back(response);
+    }
+
+   private:
+    void ResolveInServerTaskRunner() {
+      auto responses_with_callbacks = std::move(responses_with_callbacks_);
+      for (const auto& response_with_callbacks : responses_with_callbacks) {
+        response_with_callbacks.send_callback.Run(
+            response_with_callbacks.response_string,
+            response_with_callbacks.done_callback);
+      }
+    }
+
+    friend class base::RefCountedThreadSafe<Resolver>;
+    ~Resolver() = default;
+
+    base::Lock lock_;
+
+    std::vector<ResponseWithCallbacks> responses_with_callbacks_;
+    bool resolved_ GUARDED_BY(lock_) = false;
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner_ GUARDED_BY(lock_);
+  };
+
+  explicit DelayedHttpResponseWithResolver(scoped_refptr<Resolver> resolver)
+      : resolver_(std::move(resolver)) {}
+
+  DelayedHttpResponseWithResolver(const DelayedHttpResponseWithResolver&) =
+      delete;
+  DelayedHttpResponseWithResolver& operator=(
+      const DelayedHttpResponseWithResolver&) = delete;
+
+  void SendResponse(
+      const net::test_server::SendBytesCallback& send,
+      const net::test_server::SendCompleteCallback& done) override {
+    resolver_->Add({send, done, ToResponseString()});
+  }
+
+ private:
+  const scoped_refptr<Resolver> resolver_;
+};
+
 std::unique_ptr<net::test_server::HttpResponse> HandleBeacon(
     const net::test_server::HttpRequest& request) {
   if (request.relative_url != "/beacon")
@@ -70,6 +157,16 @@ std::unique_ptr<net::test_server::HttpResponse> HandleHungBeacon(
   return std::make_unique<net::test_server::HungResponse>();
 }
 
+std::unique_ptr<net::test_server::HttpResponse> HandleHungBeaconWithResolver(
+    scoped_refptr<DelayedHttpResponseWithResolver::Resolver> resolver,
+    const net::test_server::HttpRequest& request) {
+  if (request.relative_url != "/beacon")
+    return nullptr;
+  return std::make_unique<DelayedHttpResponseWithResolver>(std::move(resolver));
+}
+
+}  // namespace
+
 class RenderProcessHostTest : public ContentBrowserTest,
                               public RenderProcessHostObserver {
  public:
@@ -1113,6 +1210,48 @@ IN_PROC_BROWSER_TEST_F(RenderProcessHostTest,
     rph->RemoveObserver(this);
 }
 
+IN_PROC_BROWSER_TEST_F(RenderProcessHostTest, ManyKeepaliveRequests) {
+  auto resolver =
+      base::MakeRefCounted<DelayedHttpResponseWithResolver::Resolver>();
+  embedded_test_server()->RegisterRequestHandler(
+      base::BindRepeating(HandleHungBeaconWithResolver, resolver));
+  const base::string16 title = base::ASCIIToUTF16("Resolved");
+  const base::string16 waiting = base::ASCIIToUTF16("Waiting");
+
+  ASSERT_TRUE(embedded_test_server()->Start());
+  EXPECT_TRUE(NavigateToURL(
+      shell(),
+      embedded_test_server()->GetURL("/fetch-keepalive.html?requests=256")));
+
+  {
+    // Wait for the page to make all the keepalive requests.
+    TitleWatcher watcher(shell()->web_contents(), waiting);
+    EXPECT_EQ(waiting, watcher.WaitAndGetTitle());
+  }
+
+  resolver->Resolve();
+
+  {
+    TitleWatcher watcher(shell()->web_contents(), title);
+    EXPECT_EQ(title, watcher.WaitAndGetTitle());
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(RenderProcessHostTest, TooManyKeepaliveRequests) {
+  embedded_test_server()->RegisterRequestHandler(
+      base::BindRepeating(HandleHungBeacon, base::RepeatingClosure()));
+  ASSERT_TRUE(embedded_test_server()->Start());
+  const base::string16 title = base::ASCIIToUTF16("Rejected");
+
+  TitleWatcher watcher(shell()->web_contents(), title);
+
+  EXPECT_TRUE(NavigateToURL(
+      shell(),
+      embedded_test_server()->GetURL("/fetch-keepalive.html?requests=257")));
+
+  EXPECT_EQ(title, watcher.WaitAndGetTitle());
+}
+
 IN_PROC_BROWSER_TEST_F(RenderProcessHostTest, LowPriorityFramesDisabled) {
   // RenderProcessHostImpl::UpdateProcessPriority has an early check of
   // run_renderer_in_process and exits for RenderProcessHosts without a child

content/test/data/fetch-keepalive.html

 <!doctype html>
+<title>Fetching</title>
 <script>
-p = fetch('/beacon', {keepalive: true});
+const params = new URL(location.href).searchParams;
+const numRequests = params.has('requests') ? Number(params.get('requests')) : 1;
+const promises = [];
+for (let i = 0; i < numRequests; i += 1) {
+    promises.push(fetch('/beacon', {keepalive: true, cache: 'no-store'}));
+}
+document.title = 'Waiting';
+Promise.all(promises).then(() => {
+    document.title = 'Resolved';
+}, () => {
+    document.title = 'Rejected';
+});
 </script>

services/network/url_loader_factory.cc

@@ -26,7 +26,6 @@ namespace network {
 
 constexpr int URLLoaderFactory::kMaxKeepaliveConnections;
 constexpr int URLLoaderFactory::kMaxKeepaliveConnectionsPerProcess;
-constexpr int URLLoaderFactory::kMaxKeepaliveConnectionsPerProcessForFetchAPI;
 
 URLLoaderFactory::URLLoaderFactory(
     NetworkContext* context,
@@ -102,14 +101,6 @@ void URLLoaderFactory::CreateLoaderAndStart(
 
   bool exhausted = false;
   if (url_request.keepalive && keepalive_statistics_recorder) {
-    // This logic comes from
-    // content::ResourceDispatcherHostImpl::BeginRequestInternal.
-    // This is needed because we want to know whether the request is initiated
-    // by fetch() or not. We hope that we can unify these restrictions and
-    // remove the reference to fetch_request_context_type in the future.
-    constexpr uint32_t kInitiatedByFetchAPI = 8;
-    const bool is_initiated_by_fetch_api =
-        url_request.fetch_request_context_type == kInitiatedByFetchAPI;
     const auto& recorder = *keepalive_statistics_recorder;
     if (recorder.num_inflight_requests() >= kMaxKeepaliveConnections)
       exhausted = true;
@@ -117,11 +108,6 @@ void URLLoaderFactory::CreateLoaderAndStart(
         kMaxKeepaliveConnectionsPerProcess) {
       exhausted = true;
     }
-    if (is_initiated_by_fetch_api &&
-        recorder.NumInflightRequestsPerProcess(params_->process_id) >=
-            kMaxKeepaliveConnectionsPerProcessForFetchAPI) {
-      exhausted = true;
-    }
   }
 
   if (!context_->CanCreateLoader(params_->process_id))

services/network/url_loader_factory.h

@@ -61,9 +61,8 @@ class URLLoaderFactory : public mojom::URLLoaderFactory {
                                 traffic_annotation) override;
   void Clone(mojo::PendingReceiver<mojom::URLLoaderFactory> receiver) override;
 
-  static constexpr int kMaxKeepaliveConnections = 256;
-  static constexpr int kMaxKeepaliveConnectionsPerProcess = 20;
-  static constexpr int kMaxKeepaliveConnectionsPerProcessForFetchAPI = 10;
+  static constexpr int kMaxKeepaliveConnections = 2048;
+  static constexpr int kMaxKeepaliveConnectionsPerProcess = 256;
 
  private:
   // The NetworkContext that indirectly owns |this|.

third_party/blink/web_tests/http/tests/fetch/chromium/keepalive-resource-exhaustion.html

-<html>
-<script src="/resources/testharness.js"></script>
-<script src="/resources/testharnessreport.js"></script>
-<script>
-
-promise_test((t) => {
-  const n = 100;
-  const promises = [];
-
-  for (let i = 0; i < n; ++i) {
-    promises.push(fetch('/', {keepalive: true}));
-  }
-
-  return promise_rejects(t, TypeError(), Promise.all(promises));
-}, 'The number of requests with keepalive set should be restricted.');
-</script>