Allow URLLoaderThrottle to be restarted immediately

URLLoaderThrottle::Delegate restarts URLLoader only when some response
is returned from server. This does not work when the request got
blackholed due to server unreachability.

This CL adds a RestartWithURLResetAndFlagsNow() API that restarts the
URLLoader immediately. This only works when no response had been
received from the server yet.

Bug: 1110113
Change-Id: I7541c00b05e9fe94a28b95486295b026bfafe020
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2337338
Commit-Queue: rajendrant <rajendrant@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Michael Crouse <mcrouse@chromium.org>
Reviewed-by: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795704}

chrome/browser/data_saver/subresource_redirect_browsertest.cc

@@ -517,11 +517,14 @@ IN_PROC_BROWSER_TEST_F(SubresourceRedirectBrowserTest,
 
   RetryForHistogramUntilCountReached(
       histogram_tester(), "SubresourceRedirect.CompressionAttempt.ResponseCode",
-      1);
+      2);
   RetryForHistogramUntilCountReached(histogram_tester(),
                                      "SubresourceRedirect.BypassDuration", 1);
-  // TODO(rajendrant): Verify that the image got actually loaded too after
-  // https://crbug.com/1110113 is fixed.
+  EXPECT_TRUE(RunScriptExtractBool("checkImage()"));
+  EXPECT_EQ(GURL(RunScriptExtractString("imageSrc()")).port(),
+            https_url().port());
+  histogram_tester()->ExpectUniqueSample(
+      "SubresourceRedirect.CompressionFetchTimeout", true, 1);
   histogram_tester()->ExpectUniqueSample(
       "SubresourceRedirect.PageLoad.BypassResult", false, 1);
   histogram_tester()->ExpectTotalCount("SubresourceRedirect.BypassDuration", 1);
@@ -547,6 +550,7 @@ IN_PROC_BROWSER_TEST_F(SubresourceRedirectBrowserTest,
   ui_test_utils::NavigateToURL(browser(), url);
 
   EXPECT_TRUE(RunScriptExtractBool("checkImage()"));
+  EXPECT_EQ(request_url().port(), compression_url().port());
   RetryForHistogramUntilCountReached(
       histogram_tester(), "SubresourceRedirect.CompressionAttempt.ResponseCode",
       2);

chrome/renderer/subresource_redirect/subresource_redirect_url_loader_throttle.cc

@@ -290,8 +290,8 @@ void SubresourceRedirectURLLoaderThrottle::WillOnCompleteWithError(
 
 void SubresourceRedirectURLLoaderThrottle::OnRedirectTimeout() {
   DCHECK(did_redirect_compressed_origin_);
-  // TODO(rajendrant): Add the ability to restart the request once
-  // https://crbug.com/1110113 is fixed.
+  did_redirect_compressed_origin_ = false;
+  delegate_->RestartWithURLResetAndFlagsNow(net::LOAD_NORMAL);
   if (auto* resource_loading_hints_agent = GetResourceLoadingHintsAgent()) {
     resource_loading_hints_agent->NotifyHttpsImageCompressionFetchFailed(
         base::TimeDelta());

third_party/blink/common/loader/throttling_url_loader.cc

@@ -172,6 +172,14 @@ class ThrottlingURLLoader::ForwardingThrottleDelegate
     loader_->RestartWithURLResetAndFlags(additional_load_flags);
   }
 
+  void RestartWithURLResetAndFlagsNow(int additional_load_flags) override {
+    if (!loader_)
+      return;
+
+    ScopedDelegateCall scoped_delegate_call(this);
+    loader_->RestartWithURLResetAndFlagsNow(additional_load_flags);
+  }
+
   void Detach() { loader_ = nullptr; }
 
  private:
@@ -599,6 +607,13 @@ void ThrottlingURLLoader::RestartWithURLResetAndFlags(
   has_pending_restart_ = true;
 }
 
+void ThrottlingURLLoader::RestartWithURLResetAndFlagsNow(
+    int additional_load_flags) {
+  RestartWithURLResetAndFlags(additional_load_flags);
+  if (!did_receive_response_)
+    RestartWithFlagsNow();
+}
+
 void ThrottlingURLLoader::OnReceiveResponse(
     network::mojom::URLResponseHeadPtr response_head) {
   DCHECK_EQ(DEFERRED_NONE, deferred_stage_);
@@ -606,6 +621,7 @@ void ThrottlingURLLoader::OnReceiveResponse(
   DCHECK(deferring_throttles_.empty());
   TRACE_EVENT1("loading", "ThrottlingURLLoader::OnReceiveResponse", "url",
                response_url_.possibly_invalid_spec());
+  did_receive_response_ = true;
 
   // Dispatch BeforeWillProcessResponse().
   if (!throttles_.empty()) {

third_party/blink/common/loader/throttling_url_loader_unittest.cc

@@ -2110,6 +2110,244 @@ TEST_F(ThrottlingURLLoaderTest, MultipleRestartWithURLResetAndFlags) {
   }
 }
 
+// Verify RestartWithURLResetAndFlagsNow() behaves similar to
+// RestartWithURLResetAndFlags() while called during BeforeWillProcessResponse()
+// processing, and verify that it restarts with the original URL.
+TEST_F(ThrottlingURLLoaderTest, RestartWithURLResetAndFlagsNow) {
+  base::RunLoop run_loop1;
+  base::RunLoop run_loop2;
+  base::RunLoop run_loop3;
+
+  // URL for internal redirect.
+  GURL modified_url = GURL("www.example.uk.com");
+  throttle_->set_modify_url_in_will_start(modified_url);
+
+  // Check that the initial loader uses the default load flags (0).
+  factory_.set_on_create_loader_and_start(base::BindRepeating(
+      [](const base::RepeatingClosure& quit_closure,
+         const network::ResourceRequest& url_request) {
+        EXPECT_EQ(0, url_request.load_flags);
+        quit_closure.Run();
+      },
+      run_loop1.QuitClosure()));
+
+  // Set the client to actually follow redirects to allow URL resetting to
+  // occur.
+  client_.set_on_received_redirect_callback(base::BindLambdaForTesting([&]() {
+    net::HttpRequestHeaders modified_headers;
+    loader_->FollowRedirect({} /* removed_headers */,
+                            std::move(modified_headers),
+                            {} /* modified_cors_exempt_headers */);
+  }));
+
+  // Restart the request when processing BeforeWillProcessResponse(), using
+  // different load flags (1), and an URL reset.
+  throttle_->set_before_will_process_response_callback(base::BindRepeating(
+      [](blink::URLLoaderThrottle::Delegate* delegate, bool* defer) {
+        delegate->RestartWithURLResetAndFlagsNow(1);
+      }));
+
+  CreateLoaderAndStart();
+
+  run_loop1.Run();
+
+  EXPECT_EQ(1u, factory_.create_loader_and_start_called());
+  EXPECT_EQ(1u, throttle_->will_start_request_called());
+  EXPECT_EQ(0u, throttle_->will_redirect_request_called());
+  EXPECT_EQ(0u, throttle_->before_will_process_response_called());
+  EXPECT_EQ(0u, throttle_->will_process_response_called());
+  EXPECT_EQ(throttle_->observed_response_url(), modified_url);
+
+  // The next time we intercept CreateLoaderAndStart() should be for the
+  // restarted request (load flags of 1).
+  factory_.set_on_create_loader_and_start(base::BindRepeating(
+      [](const base::RepeatingClosure& quit_closure,
+         const network::ResourceRequest& url_request) {
+        EXPECT_EQ(1, url_request.load_flags);
+        quit_closure.Run();
+      },
+      run_loop2.QuitClosure()));
+
+  factory_.NotifyClientOnReceiveResponse();
+
+  run_loop2.Run();
+
+  // Now that the restarted request has been made, clear
+  // BeforeWillProcessResponse() so it doesn't restart the request yet again.
+  throttle_->set_before_will_process_response_callback(
+      TestURLLoaderThrottle::ThrottleCallback());
+
+  client_.set_on_complete_callback(
+      base::BindLambdaForTesting([&run_loop3](int error) {
+        EXPECT_EQ(net::OK, error);
+        run_loop3.Quit();
+      }));
+
+  // Complete the response.
+  factory_.NotifyClientOnReceiveResponse();
+  factory_.NotifyClientOnComplete(net::OK);
+
+  run_loop3.Run();
+
+  EXPECT_EQ(2u, factory_.create_loader_and_start_called());
+  EXPECT_EQ(1u, throttle_->will_start_request_called());
+  EXPECT_EQ(1u, throttle_->will_redirect_request_called());
+  EXPECT_EQ(2u, throttle_->before_will_process_response_called());
+  EXPECT_EQ(1u, throttle_->will_process_response_called());
+  EXPECT_EQ(throttle_->observed_response_url(), request_url);
+}
+
+// Verify RestartWithURLResetAndFlagsNow() behaves similar to
+// RestartWithURLResetAndFlags() while called during BeforeWillProcessResponse()
+// processing, and verify that it restarts with the original URL.
+TEST_F(ThrottlingURLLoaderTest,
+       RestartWithURLResetAndFlagsNowBeforeProcessResponse) {
+  base::RunLoop run_loop1;
+  base::RunLoop run_loop2;
+  base::RunLoop run_loop3;
+
+  // URL for internal redirect.
+  GURL modified_url = GURL("www.example.uk.com");
+  throttle_->set_modify_url_in_will_start(modified_url);
+
+  // Check that the initial loader uses the default load flags (0).
+  factory_.set_on_create_loader_and_start(base::BindRepeating(
+      [](const base::RepeatingClosure& quit_closure,
+         const network::ResourceRequest& url_request) {
+        EXPECT_EQ(0, url_request.load_flags);
+        quit_closure.Run();
+      },
+      run_loop1.QuitClosure()));
+
+  // Set the client to actually follow redirects to allow URL resetting to
+  // occur.
+  client_.set_on_received_redirect_callback(base::BindLambdaForTesting([&]() {
+    net::HttpRequestHeaders modified_headers;
+    loader_->FollowRedirect({} /* removed_headers */,
+                            std::move(modified_headers),
+                            {} /* modified_cors_exempt_headers */);
+  }));
+
+  CreateLoaderAndStart();
+
+  run_loop1.Run();
+
+  EXPECT_EQ(1u, factory_.create_loader_and_start_called());
+  EXPECT_EQ(1u, throttle_->will_start_request_called());
+  EXPECT_EQ(0u, throttle_->will_redirect_request_called());
+  EXPECT_EQ(0u, throttle_->before_will_process_response_called());
+  EXPECT_EQ(0u, throttle_->will_process_response_called());
+  EXPECT_EQ(throttle_->observed_response_url(), modified_url);
+
+  // Restarting the request should restart the request immediately.
+  throttle_->delegate()->RestartWithURLResetAndFlagsNow(1);
+
+  // The next time we intercept CreateLoaderAndStart() should be for the
+  // restarted request (load flags of 1).
+  factory_.set_on_create_loader_and_start(base::BindRepeating(
+      [](const base::RepeatingClosure& quit_closure,
+         const network::ResourceRequest& url_request) {
+        EXPECT_EQ(1, url_request.load_flags);
+        quit_closure.Run();
+      },
+      run_loop2.QuitClosure()));
+
+  run_loop2.Run();
+
+  EXPECT_EQ(2u, factory_.create_loader_and_start_called());
+  EXPECT_EQ(1u, throttle_->will_redirect_request_called());
+  EXPECT_EQ(0u, throttle_->before_will_process_response_called());
+  EXPECT_EQ(0u, throttle_->will_process_response_called());
+
+  client_.set_on_complete_callback(
+      base::BindLambdaForTesting([&run_loop3](int error) {
+        EXPECT_EQ(net::OK, error);
+        run_loop3.Quit();
+      }));
+
+  // Complete the response.
+  factory_.NotifyClientOnReceiveResponse();
+  factory_.NotifyClientOnComplete(net::OK);
+
+  run_loop3.Run();
+
+  EXPECT_EQ(2u, factory_.create_loader_and_start_called());
+  EXPECT_EQ(1u, throttle_->will_start_request_called());
+  EXPECT_EQ(1u, throttle_->will_redirect_request_called());
+  EXPECT_EQ(1u, throttle_->before_will_process_response_called());
+  EXPECT_EQ(1u, throttle_->will_process_response_called());
+  EXPECT_EQ(throttle_->observed_response_url(), request_url);
+}
+
+// Verify RestartWithURLResetAndFlagsNow() does not restart request if
+// BeforeWillProcessResponse() has already been called.
+TEST_F(ThrottlingURLLoaderTest,
+       RestartWithURLResetAndFlagsNowAfterProcessResponse) {
+  base::RunLoop run_loop1;
+  base::RunLoop run_loop2;
+  base::RunLoop run_loop3;
+  base::RunLoop run_loop4;
+
+  // URL for internal redirect.
+  GURL modified_url = GURL("www.example.uk.com");
+  throttle_->set_modify_url_in_will_start(modified_url);
+
+  // Check that the initial loader uses the default load flags (0).
+  factory_.set_on_create_loader_and_start(base::BindRepeating(
+      [](const base::RepeatingClosure& quit_closure,
+         const network::ResourceRequest& url_request) {
+        EXPECT_EQ(0, url_request.load_flags);
+        quit_closure.Run();
+      },
+      run_loop1.QuitClosure()));
+
+  // Set the client to actually follow redirects to allow URL resetting to
+  // occur.
+  client_.set_on_received_redirect_callback(base::BindLambdaForTesting([&]() {
+    net::HttpRequestHeaders modified_headers;
+    loader_->FollowRedirect({} /* removed_headers */,
+                            std::move(modified_headers),
+                            {} /* modified_cors_exempt_headers */);
+  }));
+
+  CreateLoaderAndStart();
+  run_loop1.Run();
+
+  throttle_->set_before_will_process_response_callback(
+      base::BindLambdaForTesting(
+          [&run_loop3](blink::URLLoaderThrottle::Delegate* delegate,
+                       bool* defer) { run_loop3.Quit(); }));
+
+  factory_.NotifyClientOnReceiveResponse();
+  run_loop3.Run();
+
+  EXPECT_EQ(1u, factory_.create_loader_and_start_called());
+  EXPECT_EQ(1u, throttle_->will_start_request_called());
+  EXPECT_EQ(0u, throttle_->will_redirect_request_called());
+  EXPECT_EQ(1u, throttle_->before_will_process_response_called());
+  EXPECT_EQ(1u, throttle_->will_process_response_called());
+
+  // Restarting the request should not have any effect.
+  throttle_->delegate()->RestartWithURLResetAndFlagsNow(1);
+
+  client_.set_on_complete_callback(
+      base::BindLambdaForTesting([&run_loop4](int error) {
+        EXPECT_EQ(net::OK, error);
+        run_loop4.Quit();
+      }));
+
+  // Complete the response.
+  factory_.NotifyClientOnComplete(net::OK);
+  run_loop4.Run();
+
+  EXPECT_EQ(1u, factory_.create_loader_and_start_called());
+  EXPECT_EQ(1u, throttle_->will_start_request_called());
+  EXPECT_EQ(0u, throttle_->will_redirect_request_called());
+  EXPECT_EQ(1u, throttle_->before_will_process_response_called());
+  EXPECT_EQ(1u, throttle_->will_process_response_called());
+  EXPECT_EQ(throttle_->observed_response_url(), request_url);
+}
+
 // Call RestartWithURLResetAndFlags() from multiple throttles after having
 // deferred BeforeWillProcessResponse() in each. Ensures that the request is
 // started exactly once, using the combination of all additional load flags,

third_party/blink/common/loader/url_loader_throttle.cc

@@ -37,6 +37,11 @@ void URLLoaderThrottle::Delegate::RestartWithURLResetAndFlags(
   NOTIMPLEMENTED();
 }
 
+void URLLoaderThrottle::Delegate::RestartWithURLResetAndFlagsNow(
+    int additional_load_flags) {
+  NOTIMPLEMENTED();
+}
+
 URLLoaderThrottle::Delegate::~Delegate() {}
 
 URLLoaderThrottle::~URLLoaderThrottle() {}

third_party/blink/public/common/loader/throttling_url_loader.h

@@ -142,6 +142,9 @@ class BLINK_COMMON_EXPORT ThrottlingURLLoader
   // Restart the request using |original_url_|.
   void RestartWithURLResetAndFlags(int additional_load_flags);
 
+  // Restart the request immediately if the response has not started yet.
+  void RestartWithURLResetAndFlagsNow(int additional_load_flags);
+
   // network::mojom::URLLoaderClient implementation:
   void OnReceiveResponse(
       network::mojom::URLResponseHeadPtr response_head) override;
@@ -189,6 +192,7 @@ class BLINK_COMMON_EXPORT ThrottlingURLLoader
   };
   DeferredStage deferred_stage_ = DEFERRED_NONE;
   bool loader_completed_ = false;
+  bool did_receive_response_ = false;
 
   struct ThrottleEntry {
     ThrottleEntry(ThrottlingURLLoader* loader,

third_party/blink/public/common/loader/url_loader_throttle.h

@@ -121,6 +121,12 @@ class BLINK_COMMON_EXPORT URLLoaderThrottle {
     // using a combined value of all of the |additional_load_flags|.
     virtual void RestartWithURLResetAndFlags(int additional_load_flags);
 
+    // Restarts the URL loader immediately using |additional_load_flags| and the
+    // unmodified URL if it was changed in WillStartRequest().
+    //
+    // Restarting is only valid before BeforeWillProcessResponse() is called.
+    virtual void RestartWithURLResetAndFlagsNow(int additional_load_flags);
+
    protected:
     virtual ~Delegate();
   };