PlzNavigate: add support for BLOCK_REQUEST during redirects

content/browser/frame_host/navigation_handle_impl.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/navigation_handle_impl.h"
 
 #include <iterator>
 
 #include "base/logging.h"
 #include "content/browser/appcache/appcache_navigation_handle.h"
@@ skipping 299 matching lines @@
   if (state_ != DEFERRING_START && state_ != DEFERRING_REDIRECT &&
       state_ != DEFERRING_RESPONSE) {
     return;
   }
 
   NavigationThrottle::ThrottleCheckResult result = NavigationThrottle::DEFER;
   if (state_ == DEFERRING_START) {
     result = CheckWillStartRequest();
   } else if (state_ == DEFERRING_REDIRECT) {
     result = CheckWillRedirectRequest();
+    if (!IsBrowserSideNavigationEnabled() &&
+        result != NavigationThrottle::PROCEED &&
+        result != NavigationThrottle::DEFER) {
+      // The renderer will not be informed of the redirect, as the navigation
+      // will have been cancelled before sending the
+      // ResourceMsg_ReceivedRedirect IPC. Reset the URL of the NavigationHandle
+      // to the URL it had prior to the redirect, to match the commit of an
+      // error page.
+      redirect_chain_.pop_back();
+      DCHECK(!redirect_chain_.empty());
+      url_ = redirect_chain_.back();

[Charlie Reis, 2017/03/09 17:39:03]

Does this mean that if we're redirecting from A to B and B fails, we'll say that
A failed?  (Looks like yes, according to the test.  I might be ok with that.)

[arthursonzogni, 2017/03/10 11:55:28]

Yes unfortunately.

+    }
   } else {
     result = CheckWillProcessResponse();
 
     // If the navigation is about to proceed after having been deferred while
     // processing the response, then it's ready to commit. Determine which
     // RenderFrameHost should render the response, based on its site (after any
     // redirects).
     // Note: if MaybeTransferAndProceed returns false, this means that this
     // NavigationHandle was deleted, so return immediately.
     if (result == NavigationThrottle::PROCEED && !MaybeTransferAndProceed())
@@ skipping 226 matching lines @@
   response_headers_ = response_headers;
   connection_info_ = connection_info;
   was_redirected_ = true;
   redirect_chain_.push_back(new_url);
   if (new_method != "POST")
     resource_request_body_ = nullptr;
 
   state_ = WILL_REDIRECT_REQUEST;
   complete_callback_ = callback;
 
+  // Notify each throttle of the request.
+  NavigationThrottle::ThrottleCheckResult result = NavigationThrottle::DEFER;
   if (IsSelfReferentialURL()) {
     state_ = CANCELING;
-    RunCompleteCallback(NavigationThrottle::CANCEL);
-    return;
+    result = NavigationThrottle::CANCEL;
+  } else {
+    result = CheckWillRedirectRequest();
   }
 
-  // Notify each throttle of the request.
-  NavigationThrottle::ThrottleCheckResult result = CheckWillRedirectRequest();
+  if (!IsBrowserSideNavigationEnabled() &&
+      result != NavigationThrottle::PROCEED &&
+      result != NavigationThrottle::DEFER) {
+    // The renderer will not be informed of the redirect, as the navigation
+    // will have been cancelled before sending the
+    // ResourceMsg_ReceivedRedirect IPC. Reset the URL of the NavigationHandle
+    // to the URL it had prior to the redirect, to match the commit of an
+    // error page.
+    redirect_chain_.pop_back();
+    DCHECK(!redirect_chain_.empty());
+    url_ = redirect_chain_.back();
+  }
 
   // If the navigation is not deferred, run the callback.
   if (result != NavigationThrottle::DEFER)
     RunCompleteCallback(result);
 }
 
 void NavigationHandleImpl::WillProcessResponse(
     RenderFrameHostImpl* render_frame_host,
     scoped_refptr<net::HttpResponseHeaders> response_headers,
     net::HttpResponseInfo::ConnectionInfo connection_info,
@@ skipping 92 matching lines @@
   DCHECK(state_ == WILL_SEND_REQUEST || state_ == DEFERRING_START);
   DCHECK(state_ != WILL_SEND_REQUEST || next_index_ == 0);
   DCHECK(state_ != DEFERRING_START || next_index_ != 0);
   for (size_t i = next_index_; i < throttles_.size(); ++i) {
     NavigationThrottle::ThrottleCheckResult result =
         throttles_[i]->WillStartRequest();
     switch (result) {
       case NavigationThrottle::PROCEED:
         continue;
 
+      case NavigationThrottle::BLOCK_REQUEST:
       case NavigationThrottle::CANCEL:
       case NavigationThrottle::CANCEL_AND_IGNORE:
-      case NavigationThrottle::BLOCK_REQUEST:
         state_ = CANCELING;
         return result;
 
       case NavigationThrottle::DEFER:
         state_ = DEFERRING_START;
         next_index_ = i + 1;
         return result;
 
       case NavigationThrottle::BLOCK_RESPONSE:
         NOTREACHED();
     }
   }
   next_index_ = 0;
   state_ = WILL_SEND_REQUEST;
   return NavigationThrottle::PROCEED;
 }
 
 NavigationThrottle::ThrottleCheckResult
 NavigationHandleImpl::CheckWillRedirectRequest() {
   DCHECK(state_ == WILL_REDIRECT_REQUEST || state_ == DEFERRING_REDIRECT);
   DCHECK(state_ != WILL_REDIRECT_REQUEST || next_index_ == 0);
   DCHECK(state_ != DEFERRING_REDIRECT || next_index_ != 0);
   for (size_t i = next_index_; i < throttles_.size(); ++i) {
     NavigationThrottle::ThrottleCheckResult result =
         throttles_[i]->WillRedirectRequest();
     switch (result) {
       case NavigationThrottle::PROCEED:
         continue;
 
+      case NavigationThrottle::BLOCK_REQUEST:
       case NavigationThrottle::CANCEL:
       case NavigationThrottle::CANCEL_AND_IGNORE:
         state_ = CANCELING;
         return result;
 
       case NavigationThrottle::DEFER:
         state_ = DEFERRING_REDIRECT;
         next_index_ = i + 1;
         return result;
 
-      case NavigationThrottle::BLOCK_REQUEST:
       case NavigationThrottle::BLOCK_RESPONSE:
         NOTREACHED();

[Charlie Reis, 2017/03/09 17:39:03]

Unrelated: NOTREACHED is compiled out of release builds.  Is it safe to act like
PROCEED if some bug makes us get here with BLOCK_RESPONSE, or should we handle
this differently?

[arthursonzogni, 2017/03/10 11:55:28]

It is impossible to have NavigationThrottle::BLOCK_RESPONSE in
WillRedirectRequest() because it must only be returned in WillProcessResponse().
The NOTREACHED() is here to prevent developers to misuse it. 

So, I don't know what it means to have BLOCK_RESPONSE here, I can't say if it is
safe or not.
We probably can have separate enums in order to statically prevent this case or
as you said using something that isn't stripped out in release mode. I think the
comments in the enum and the NOTREACHED() are probably enough to prevent
developers from using it. Let us know if you think we should do something.

[Charlie Reis, 2017/03/13 20:48:02]

My concern was that a poorly written NavigationThrottle might return
BLOCK_RESPONSE here, and because there's no early return or change to state_,
we'll act like it said PROCEED.  Developers would only realize the problem if
they happened to write a test for that case.

That said, it sounds like their code would be buggy in that case regardless, and
the NOTREACHED is a reasonable attempt to help them catch bugs.  It's only a
concerning bug if a NavigationThrottle depends on BLOCK_RESPONSE for some
security decision, in which case the developers should have noticed that it
doesn't work here.  I guess I'm ok with leaving it as is.

     }
   }
   next_index_ = 0;
   state_ = WILL_REDIRECT_REQUEST;
 
   // Notify the delegate that a redirect was encountered and will be followed.
   if (GetDelegate())
     GetDelegate()->DidRedirectNavigation(this);
 
   return NavigationThrottle::PROCEED;
@@ skipping 193 matching lines @@
     if (node->current_url().EqualsIgnoringRef(url_)) {
       if (found_self_reference)
         return true;
       found_self_reference = true;
     }
   }
   return false;
 }
 
 }  // namespace content