PlzNavigate: add support for BLOCK_REQUEST during redirects

content/browser/frame_host/navigation_handle_impl.cc

Index: content/browser/frame_host/navigation_handle_impl.cc
diff --git a/content/browser/frame_host/navigation_handle_impl.cc b/content/browser/frame_host/navigation_handle_impl.cc
index 2e525bbe4956a473d0c41d0d2051e1d5801bdef4..aa09fe9997c00d754f8996a6f004ee86f0683815 100644
--- a/content/browser/frame_host/navigation_handle_impl.cc
+++ b/content/browser/frame_host/navigation_handle_impl.cc
@@ -317,6 +317,18 @@ void NavigationHandleImpl::Resume() {
     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();
 
@@ -563,14 +575,27 @@ void NavigationHandleImpl::WillRedirectRequest(
   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)
@@ -683,9 +708,9 @@ NavigationHandleImpl::CheckWillStartRequest() {
       case NavigationThrottle::PROCEED:
         continue;
 
+      case NavigationThrottle::BLOCK_REQUEST:
       case NavigationThrottle::CANCEL:
       case NavigationThrottle::CANCEL_AND_IGNORE:
-      case NavigationThrottle::BLOCK_REQUEST:
         state_ = CANCELING;
         return result;
 
@@ -715,6 +740,7 @@ NavigationHandleImpl::CheckWillRedirectRequest() {
       case NavigationThrottle::PROCEED:
         continue;
 
+      case NavigationThrottle::BLOCK_REQUEST:
       case NavigationThrottle::CANCEL:
       case NavigationThrottle::CANCEL_AND_IGNORE:
         state_ = CANCELING;
@@ -725,7 +751,6 @@ NavigationHandleImpl::CheckWillRedirectRequest() {
         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.

     }