Fix redirect handling on ThreadableLoader

 - Decrement |cors_redirect_limit_| even when IsAllowedRedirect(...)
   returns true.
 - Redirect count logic is not applied when |out_of_blink_cors_| is
   set.
 - Take the return value of a call of client_->WillFollowRedirect
   introduced at [1] into account.

1: https://chromium-review.googlesource.com/c/chromium/src/+/1150695

Bug: 736308, 353768
Change-Id: I5c251881902a66429d7c601e62786bd20890eb95
Reviewed-on: https://chromium-review.googlesource.com/1156120
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580054}

third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect-loop.js

@@ -40,11 +40,8 @@ var TEST_TARGETS = [
             19,
             OTHER_BASE_URL + '&ACAOrigin=*'))],
    [methodIsGET, authCheckNone]],
-  // FIXME: due to the current implementation of Chromium,
-  // Count=21 is resolved, Count=22 is rejected.
-  // https://crbug.com/353768
   [REDIRECT_LOOP_URL + encodeURIComponent(OTHER_BASE_URL + '&ACAOrigin=*') +
-   '&Count=22&mode=cors&credentials=same-origin&method=GET',
+   '&Count=21&mode=cors&credentials=same-origin&method=GET',
    [fetchRejected]],
 
   // Redirect loop: other origin -> same origin

third_party/blink/renderer/core/loader/threadable_loader.cc

@@ -263,7 +263,7 @@ ThreadableLoader::ThreadableLoader(
       timeout_timer_(execution_context_->GetTaskRunner(TaskType::kNetworking),
                      this,
                      &ThreadableLoader::DidTimeout),
-      cors_redirect_limit_(0),
+      redirect_limit_(kMaxCORSRedirects),
       redirect_mode_(network::mojom::FetchRedirectMode::kFollow),
       override_referrer_(false) {
   DCHECK(client);
@@ -283,9 +283,6 @@ void ThreadableLoader::Start(const ResourceRequest& request) {
              network::mojom::CORSPreflightPolicy::kConsiderPreflight ||
          cors_enabled);
 
-  if (cors_enabled)
-    cors_redirect_limit_ = kMaxCORSRedirects;
-
   initial_request_url_ = request.Url();
   last_request_url_ = initial_request_url_;
   request_context_ = request.GetRequestContext();
@@ -659,7 +656,6 @@ bool ThreadableLoader::RedirectReceived(
         DCHECK(actual_request_.IsNull());
         NotifyFinished(resource);
       }
-
       return false;
     }
 
@@ -667,25 +663,24 @@ bool ThreadableLoader::RedirectReceived(
       ThreadableLoaderClient* client = client_;
       Clear();
       client->DidFailRedirectCheck();
-
       return false;
     }
 
-    // Allow same origin requests to continue after allowing clients to audit
-    // the redirect.
-    if (out_of_blink_cors_ ||
-        IsAllowedRedirect(new_request.GetFetchRequestMode(), new_url)) {
+    if (out_of_blink_cors_)
       return client_->WillFollowRedirect(new_url, redirect_response);
-    }
 
-    if (cors_redirect_limit_ <= 0) {
+    if (redirect_limit_ <= 0) {
       ThreadableLoaderClient* client = client_;
       Clear();
       client->DidFailRedirectCheck();
       return false;
     }
+    --redirect_limit_;
 
-    --cors_redirect_limit_;
+    // Allow same origin requests to continue after allowing clients to audit
+    // the redirect.
+    if (IsAllowedRedirect(new_request.GetFetchRequestMode(), new_url))
+      return client_->WillFollowRedirect(new_url, redirect_response);
 
     probe::didReceiveCORSRedirectResponse(
         execution_context_, resource->Identifier(),
@@ -715,7 +710,8 @@ bool ThreadableLoader::RedirectReceived(
       }
     }
 
-    client_->WillFollowRedirect(new_url, redirect_response);
+    if (!client_->WillFollowRedirect(new_url, redirect_response))
+      return false;
 
     // FIXME: consider combining this with CORS redirect handling performed by
     // CrossOriginAccessControl::handleRedirect().

third_party/blink/renderer/core/loader/threadable_loader.h

@@ -260,11 +260,8 @@ class CORE_EXPORT ThreadableLoader final
   TaskRunnerTimer<ThreadableLoader> timeout_timer_;
   TimeTicks request_started_;  // Time an asynchronous fetch request is started
 
-  // Max number of times that this ThreadableLoader can follow
-  // cross-origin redirects. This is used to limit the number of redirects. But
-  // this value is not the max number of total redirects allowed, because
-  // same-origin redirects are not counted here.
-  int cors_redirect_limit_;
+  // Max number of times that this ThreadableLoader can follow.
+  int redirect_limit_;
 
   network::mojom::FetchRedirectMode redirect_mode_;