Refactor should redirect check to callback

The upcoming robots rules based subresource redirect check is
asynchronous. So this CL refactors the should redirect image check to a
callback. It should be noted that the callback can happen synchronously
too, i.e., while the should redirect call is still executing. This CL
handles both the cases.

Bug: 1144836
Change-Id: I7afc42239e93834ab41128e9ebe611c1fef828ca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2533287
Commit-Queue: rajendrant <rajendrant@chromium.org>
Reviewed-by: Michael Crouse <mcrouse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828619}

chrome/renderer/subresource_redirect/public_image_hints_url_loader_throttle.cc

@@ -47,7 +47,9 @@ PublicImageHintsURLLoaderThrottle::GetSubresourceRedirectHintsAgent() {
       GetRenderFrame());
 }
 
-bool PublicImageHintsURLLoaderThrottle::ShouldRedirectImage(const GURL& url) {
+base::Optional<bool> PublicImageHintsURLLoaderThrottle::ShouldRedirectImage(
+    const GURL& url,
+    SubresourceRedirectURLLoaderThrottle::RedirectDecisionCallback callback) {
   if (redirect_result_ !=
       SubresourceRedirectHintsAgent::RedirectResult::kRedirectable) {
     return false;

chrome/renderer/subresource_redirect/public_image_hints_url_loader_throttle.h

@@ -13,8 +13,7 @@
 
 namespace subresource_redirect {
 
-// This class handles internal redirects for public subresouces on HTTPS sites
-// to compressed versions of subresources.
+// This class allows image compression based on public image hints received.
 class PublicImageHintsURLLoaderThrottle
     : public SubresourceRedirectURLLoaderThrottle {
  public:
@@ -23,8 +22,16 @@ class PublicImageHintsURLLoaderThrottle
 
   ~PublicImageHintsURLLoaderThrottle() override;
 
+  PublicImageHintsURLLoaderThrottle(const PublicImageHintsURLLoaderThrottle&) =
+      delete;
+  PublicImageHintsURLLoaderThrottle& operator=(
+      const PublicImageHintsURLLoaderThrottle&) = delete;
+
   // SubresourceRedirectURLLoaderThrottle:
-  bool ShouldRedirectImage(const GURL& url) override;
+  base::Optional<bool> ShouldRedirectImage(
+      const GURL& url,
+      SubresourceRedirectURLLoaderThrottle::RedirectDecisionCallback callback)
+      override;
   void OnRedirectedLoadCompleteWithError() override;
   void RecordMetricsOnLoadFinished(const GURL& url,
                                    int64_t content_length) override;

chrome/renderer/subresource_redirect/public_image_hints_url_loader_throttle_browsertest.cc

@@ -183,7 +183,7 @@ TEST_F(SubresourceRedirectPublicImageHintsURLLoaderThrottleTest,
     bool defer = true;
     throttle->WillStartRequest(&request, &defer);
 
-    EXPECT_EQ(defer, test_case.redirected_subresource_url.is_empty());
+    EXPECT_FALSE(defer);
     if (!test_case.redirected_subresource_url.is_empty()) {
       EXPECT_EQ(request.url, test_case.redirected_subresource_url);
     } else {

chrome/renderer/subresource_redirect/subresource_redirect_url_loader_throttle.cc

@@ -77,22 +77,49 @@ void SubresourceRedirectURLLoaderThrottle::WillStartRequest(
   if (IsCompressionServerOrigin(request->url))
     return;
 
-  if (!ShouldRedirectImage(request->url))
+  if (!ShouldCompressionServerRedirectSubresource())
     return;
 
-  if (!ShouldCompressionServerRedirectSubresource())
+  auto redirect_decision = ShouldRedirectImage(
+      request->url,
+      base::BindOnce(
+          &SubresourceRedirectURLLoaderThrottle::NotifyRedirectDeciderDecision,
+          weak_ptr_factory_.GetWeakPtr()));
+  if (!redirect_decision) {
+    // Decision cannot be made yet. Defer the subresource and change the URL to
+    // compression server URL. The NotifyRedirectDeciderDecision callback will
+    // continue with compression or disable compression by resetting to original
+    // URL.
+    redirect_state_ = RedirectState::kDeciderDecisionPending;
+    *defer = true;
+    request->url = GetSubresourceURLForURL(request->url);
     return;
+  }
 
-  request->url = GetSubresourceURLForURL(request->url);
-  did_redirect_compressed_origin_ = true;
+  // The decider decision has been made.
   *defer = false;
+  if (*redirect_decision) {
+    redirect_state_ = RedirectState::kRedirectAttempted;
+    request->url = GetSubresourceURLForURL(request->url);
+    StartRedirectTimeoutTimer();
+  } else {
+    redirect_state_ = RedirectState::kDeciderDisallowed;
+  }
+}
 
-  DCHECK(!redirect_timeout_timer_);
-  redirect_timeout_timer_ = std::make_unique<base::OneShotTimer>();
-  redirect_timeout_timer_->Start(
-      FROM_HERE, GetCompressionRedirectTimeout(),
-      base::BindOnce(&SubresourceRedirectURLLoaderThrottle::OnRedirectTimeout,
-                     base::Unretained(this)));
+void SubresourceRedirectURLLoaderThrottle::NotifyRedirectDeciderDecision(
+    bool is_allowed) {
+  DCHECK_EQ(RedirectState::kDeciderDecisionPending, redirect_state_);
+
+  if (is_allowed) {
+    redirect_state_ = RedirectState::kRedirectAttempted;
+    delegate_->Resume();
+    StartRedirectTimeoutTimer();
+  } else {
+    // Restart the fetch to the original URL.
+    redirect_state_ = RedirectState::kDeciderDisallowed;
+    delegate_->RestartWithURLResetAndFlags(net::LOAD_NORMAL);
+  }
 }
 
 void SubresourceRedirectURLLoaderThrottle::WillRedirectRequest(
@@ -102,7 +129,13 @@ void SubresourceRedirectURLLoaderThrottle::WillRedirectRequest(
     std::vector<std::string>* to_be_removed_request_headers,
     net::HttpRequestHeaders* modified_request_headers,
     net::HttpRequestHeaders* modified_cors_exempt_request_headers) {
-  if (did_redirect_compressed_origin_ && redirect_timeout_timer_) {
+  // Check if the redirect is in some terminal state.
+  DCHECK((redirect_state_ == RedirectState::kNone) ||
+         (redirect_state_ == RedirectState::kRedirectAttempted) ||
+         (redirect_state_ == RedirectState::kDeciderDisallowed) ||
+         redirect_state_ == RedirectState::kRedirectFailed);
+  if (redirect_state_ == RedirectState::kRedirectAttempted &&
+      redirect_timeout_timer_) {
     redirect_timeout_timer_->Start(
         FROM_HERE, GetCompressionRedirectTimeout(),
         base::BindOnce(&SubresourceRedirectURLLoaderThrottle::OnRedirectTimeout,
@@ -118,7 +151,12 @@ void SubresourceRedirectURLLoaderThrottle::BeforeWillProcessResponse(
     const GURL& response_url,
     const network::mojom::URLResponseHead& response_head,
     bool* defer) {
-  if (!did_redirect_compressed_origin_)
+  // Check if the redirect is in some terminal state.
+  DCHECK((redirect_state_ == RedirectState::kNone) ||
+         (redirect_state_ == RedirectState::kRedirectAttempted) ||
+         (redirect_state_ == RedirectState::kDeciderDisallowed) ||
+         redirect_state_ == RedirectState::kRedirectFailed);
+  if (redirect_state_ != RedirectState::kRedirectAttempted)
     return;
   DCHECK(ShouldCompressionServerRedirectSubresource());
   // If response was not from the compression server, don't restart it.
@@ -162,7 +200,7 @@ void SubresourceRedirectURLLoaderThrottle::BeforeWillProcessResponse(
 
   // Non 2XX responses from the compression server need to have unaltered
   // requests sent to the original resource.
-  did_redirect_compressed_origin_ = false;
+  redirect_state_ = RedirectState::kRedirectFailed;
   delegate_->RestartWithURLResetAndFlags(net::LOAD_NORMAL);
 }
 
@@ -170,7 +208,13 @@ void SubresourceRedirectURLLoaderThrottle::WillProcessResponse(
     const GURL& response_url,
     network::mojom::URLResponseHead* response_head,
     bool* defer) {
-  // If response was not from the compression server, don't record any metrics.
+  // Check if the redirect is in some terminal state.
+  DCHECK((redirect_state_ == RedirectState::kNone) ||
+         (redirect_state_ == RedirectState::kRedirectAttempted) ||
+         (redirect_state_ == RedirectState::kDeciderDisallowed) ||
+         redirect_state_ == RedirectState::kRedirectFailed);
+  // If response was not from the compression server, don't record any
+  // metrics.
   if (!response_url.is_valid())
     return;
   if (response_head->was_fetched_via_cache)
@@ -181,7 +225,7 @@ void SubresourceRedirectURLLoaderThrottle::WillProcessResponse(
 
   RecordMetricsOnLoadFinished(response_url, content_length);
 
-  if (!did_redirect_compressed_origin_)
+  if (redirect_state_ != RedirectState::kRedirectAttempted)
     return;
   DCHECK(ShouldCompressionServerRedirectSubresource());
 
@@ -212,23 +256,32 @@ void SubresourceRedirectURLLoaderThrottle::WillProcessResponse(
 void SubresourceRedirectURLLoaderThrottle::WillOnCompleteWithError(
     const network::URLLoaderCompletionStatus& status,
     bool* defer) {
-  if (!did_redirect_compressed_origin_)
+  if (redirect_state_ != RedirectState::kRedirectAttempted)
     return;
   DCHECK(ShouldCompressionServerRedirectSubresource());
   OnRedirectedLoadCompleteWithError();
 
   // If the server fails, restart the request to the original resource, and
   // record it.
-  did_redirect_compressed_origin_ = false;
+  redirect_state_ = RedirectState::kRedirectFailed;
   redirect_timeout_timer_.reset();
   delegate_->RestartWithURLResetAndFlags(net::LOAD_NORMAL);
   UMA_HISTOGRAM_BOOLEAN(
       "SubresourceRedirect.CompressionAttempt.ServerResponded", false);
 }
 
+void SubresourceRedirectURLLoaderThrottle::StartRedirectTimeoutTimer() {
+  DCHECK(!redirect_timeout_timer_);
+  redirect_timeout_timer_ = std::make_unique<base::OneShotTimer>();
+  redirect_timeout_timer_->Start(
+      FROM_HERE, GetCompressionRedirectTimeout(),
+      base::BindOnce(&SubresourceRedirectURLLoaderThrottle::OnRedirectTimeout,
+                     base::Unretained(this)));
+}
+
 void SubresourceRedirectURLLoaderThrottle::OnRedirectTimeout() {
-  DCHECK(did_redirect_compressed_origin_);
-  did_redirect_compressed_origin_ = false;
+  DCHECK_EQ(RedirectState::kRedirectAttempted, redirect_state_);
+  redirect_state_ = RedirectState::kRedirectFailed;
   delegate_->RestartWithURLResetAndFlagsNow(net::LOAD_NORMAL);
   if (auto* subresource_redirect_hints_agent =
           SubresourceRedirectHintsAgent::Get(GetRenderFrame())) {

chrome/renderer/subresource_redirect/subresource_redirect_url_loader_throttle.h

@@ -6,6 +6,7 @@
 #define CHROME_RENDERER_SUBRESOURCE_REDIRECT_SUBRESOURCE_REDIRECT_URL_LOADER_THROTTLE_H_
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/timer/timer.h"
 #include "content/public/renderer/render_frame.h"
 #include "third_party/blink/public/common/loader/url_loader_throttle.h"
@@ -22,12 +23,19 @@ namespace subresource_redirect {
 // should implement the decider logic if an URL should be compressed.
 class SubresourceRedirectURLLoaderThrottle : public blink::URLLoaderThrottle {
  public:
+  using RedirectDecisionCallback = base::OnceCallback<void(bool)>;
+
   static std::unique_ptr<SubresourceRedirectURLLoaderThrottle>
   MaybeCreateThrottle(const blink::WebURLRequest& request, int render_frame_id);
 
   explicit SubresourceRedirectURLLoaderThrottle(int render_frame_id);
   ~SubresourceRedirectURLLoaderThrottle() override;
 
+  SubresourceRedirectURLLoaderThrottle(
+      const SubresourceRedirectURLLoaderThrottle&) = delete;
+  SubresourceRedirectURLLoaderThrottle& operator=(
+      const SubresourceRedirectURLLoaderThrottle&) = delete;
+
   // blink::URLLoaderThrottle:
   void WillStartRequest(network::ResourceRequest* request,
                         bool* defer) override;
@@ -50,12 +58,17 @@ class SubresourceRedirectURLLoaderThrottle : public blink::URLLoaderThrottle {
   // Overridden to do nothing as the default implementation is NOT_REACHED()
   void DetachFromCurrentSequence() override;
 
-  // Return whether the image url should be redirected.
-  virtual bool ShouldRedirectImage(const GURL& url) = 0;
+  // Determine whether the image url should be redirected. When the
+  // determination can be made immediately, the decision should be returned.
+  // Otherwise base::nullopt should be returned and the callback should be
+  // invoked with the decision asynchronously.
+  virtual base::Optional<bool> ShouldRedirectImage(
+      const GURL& url,
+      RedirectDecisionCallback callback) = 0;
 
   // Indicates the subresource redirect failed, and the image will be fetched
-  // directly from the  origin instead. The failures can be due to non-2xx http
-  // responses or other net errors
+  // directly from the  origin instead. The failures can be due to non-2xx
+  // http responses or other net errors
   virtual void OnRedirectedLoadCompleteWithError() = 0;
 
   // Notifies the image load finished.
@@ -67,25 +80,48 @@ class SubresourceRedirectURLLoaderThrottle : public blink::URLLoaderThrottle {
   }
 
  private:
+  // Different states the subresource redirection can be in.
+  enum class RedirectState {
+    kNone,
+
+    // The redirect decision is pending from the underlying decider.
+    kDeciderDecisionPending,
+
+    // Redirect was disallowed by the underlying decider e.g., robots rules
+    // decider.
+    kDeciderDisallowed,
+
+    // The decider allowed redirect, and was attempted.
+    kRedirectAttempted,
+
+    // Failed due to http response codes, net errors, and the subresource was
+    // fetched from original origin.
+    kRedirectFailed
+  };
+
   friend class TestPublicImageHintsURLLoaderThrottle;
 
+  // Callback to notify the decision of decider subclasses.
+  void NotifyRedirectDeciderDecision(bool is_allowed);
+
+  // Start the timer for redirect fetch timeout.
+  void StartRedirectTimeoutTimer();
+
   // Callback invoked when the redirect fetch times out.
   void OnRedirectTimeout();
 
   // Render frame id to get the hints agent of the render frame.
   const int render_frame_id_;
 
-  // Whether this resource was actually redirected to compressed server origin.
-  // This will be true when the redirect was attempted. Will be false when
-  // redirect failed due to neterrors, or redirect was not attempted (but
-  // coverage metrics recorded), or redirect was not needed when the initial URL
-  // itself is compressed origin.
-  bool did_redirect_compressed_origin_ = false;
+  // The current state of redirect.
+  RedirectState redirect_state_ = RedirectState::kNone;
 
   // Timer to detect whether the response from compression server has timed out.
   std::unique_ptr<base::OneShotTimer> redirect_timeout_timer_;
 
-  DISALLOW_COPY_AND_ASSIGN(SubresourceRedirectURLLoaderThrottle);
+  // Used to get a weak pointer to |this|.
+  base::WeakPtrFactory<SubresourceRedirectURLLoaderThrottle> weak_ptr_factory_{
+      this};
 };
 
 }  // namespace subresource_redirect