Navigation: Remove NavigationRequest::RunCompleteCallback.

Make the callback function boolean, that return true to skip the
On*ChecksComplete functions.

And use |state| instead of |complete_callback| to determine the right
function to call.

Bug: 916537
Change-Id: I4ae0af25e687868d121a238c31632b89b0c0df2e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1919210Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Commit-Queue: Mohamed Abdelhalim <zetamoo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#719119}

content/browser/frame_host/navigation_request.cc

@@ -1230,12 +1230,7 @@ void NavigationRequest::BeginNavigation() {
       GetContentClient()->browser()->DetermineAllowedPreviews(
           common_params_->previews_state, this, common_params_->url);
 
-  // It's safe to use base::Unretained because this NavigationRequest owns
-  // the NavigationHandle where the callback will be stored.
-  // TODO(clamy): pass the method to the NavigationHandle instead of a
-  // boolean.
-  WillStartRequest(base::BindOnce(&NavigationRequest::OnStartChecksComplete,
-                                  base::Unretained(this)));
+  WillStartRequest();
 }
 
 void NavigationRequest::SetWaitingForRendererResponse() {
@@ -1428,10 +1423,7 @@ void NavigationRequest::OnRequestRedirected(
     // Update the navigation handle to point to the new url to ensure
     // AwWebContents sees the new URL and thus passes that URL to onPageFinished
     // (rather than passing the old URL).
-    UpdateStateFollowingRedirect(
-        GURL(redirect_info.new_referrer),
-        base::BindOnce(&NavigationRequest::OnRedirectChecksComplete,
-                       base::Unretained(this)));
+    UpdateStateFollowingRedirect(GURL(redirect_info.new_referrer));
     frame_tree_node_->ResetNavigationRequest(false);
     return;
   }
@@ -1574,12 +1566,7 @@ void NavigationRequest::OnRequestRedirected(
   RenderProcessHost* expected_process =
       site_instance->HasProcess() ? site_instance->GetProcess() : nullptr;
 
-  // It's safe to use base::Unretained because this NavigationRequest owns the
-  // NavigationHandle where the callback will be stored.
-  WillRedirectRequest(
-      common_params_->referrer->url, expected_process,
-      base::BindOnce(&NavigationRequest::OnRedirectChecksComplete,
-                     base::Unretained(this)));
+  WillRedirectRequest(common_params_->referrer->url, expected_process);
 }
 
 void NavigationRequest::OnResponseStarted(
@@ -1863,9 +1850,7 @@ void NavigationRequest::OnResponseStarted(
   }
 
   // Check if the navigation should be allowed to proceed.
-  WillProcessResponse(
-      base::BindOnce(&NavigationRequest::OnWillProcessResponseChecksComplete,
-                     base::Unretained(this)));
+  WillProcessResponse();
 }
 
 void NavigationRequest::OnRequestFailed(
@@ -1974,8 +1959,7 @@ void NavigationRequest::OnRequestFailedInternal(
     CommitErrorPage(error_page_content);
   } else {
     // Check if the navigation should be allowed to proceed.
-    WillFailRequest(base::BindOnce(&NavigationRequest::OnFailureChecksComplete,
-                                   base::Unretained(this)));
+    WillFailRequest();
   }
 }
 
@@ -2949,9 +2933,14 @@ void NavigationRequest::OnWillStartRequestProcessed(
   if (result.action() != NavigationThrottle::PROCEED)
     state_ = CANCELING;
 
-  // TODO(zetamoo): Remove CompleteCallback, and call NavigationRequest methods
-  // directly.
-  RunCompleteCallback(result);
+  if (complete_callback_for_testing_ &&
+      std::move(complete_callback_for_testing_).Run(result)) {
+    return;
+  }
+  OnStartChecksComplete(result);
+
+  // DO NOT ADD CODE AFTER THIS, as the NavigationRequest might have been
+  // deleted by the previous calls.
 }
 
 void NavigationRequest::OnWillRedirectRequestProcessed(
@@ -2967,7 +2956,15 @@ void NavigationRequest::OnWillRedirectRequestProcessed(
   } else {
     state_ = CANCELING;
   }
-  RunCompleteCallback(result);
+
+  if (complete_callback_for_testing_ &&
+      std::move(complete_callback_for_testing_).Run(result)) {
+    return;
+  }
+  OnRedirectChecksComplete(result);
+
+  // DO NOT ADD CODE AFTER THIS, as the NavigationRequest might have been
+  // deleted by the previous calls.
 }
 
 void NavigationRequest::OnWillFailRequestProcessed(
@@ -2982,7 +2979,15 @@ void NavigationRequest::OnWillFailRequestProcessed(
   } else {
     state_ = CANCELING;
   }
-  RunCompleteCallback(result);
+
+  if (complete_callback_for_testing_ &&
+      std::move(complete_callback_for_testing_).Run(result)) {
+    return;
+  }
+  OnFailureChecksComplete(result);
+
+  // DO NOT ADD CODE AFTER THIS, as the NavigationRequest might have been
+  // deleted by the previous calls.
 }
 
 void NavigationRequest::OnWillProcessResponseProcessed(
@@ -3001,7 +3006,15 @@ void NavigationRequest::OnWillProcessResponseProcessed(
   } else {
     state_ = CANCELING;
   }
-  RunCompleteCallback(result);
+
+  if (complete_callback_for_testing_ &&
+      std::move(complete_callback_for_testing_).Run(result)) {
+    return;
+  }
+  OnWillProcessResponseChecksComplete(result);
+
+  // DO NOT ADD CODE AFTER THIS, as the NavigationRequest might have been
+  // deleted by the previous calls.
 }
 
 NavigatorDelegate* NavigationRequest::GetDelegate() const {
@@ -3059,21 +3072,49 @@ void NavigationRequest::CancelDeferredNavigationInternal(
 
   TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationRequest", this,
                                "CancelDeferredNavigation");
+  NavigationState old_state = state_;
   state_ = CANCELING;
-  RunCompleteCallback(result);
+  if (complete_callback_for_testing_ &&
+      std::move(complete_callback_for_testing_).Run(result)) {
+    return;
+  }
+
+  switch (old_state) {
+    case WILL_START_REQUEST:
+      OnStartChecksComplete(result);
+      return;
+    case WILL_REDIRECT_REQUEST:
+      OnRedirectChecksComplete(result);
+      return;
+    case WILL_FAIL_REQUEST:
+      OnFailureChecksComplete(result);
+      return;
+    case WILL_PROCESS_RESPONSE:
+      OnWillProcessResponseChecksComplete(result);
+      return;
+    default:
+      NOTREACHED();
+  }
+  // DO NOT ADD CODE AFTER THIS, as the NavigationRequest might have been
+  // deleted by the previous calls.
 }
 
-void NavigationRequest::WillStartRequest(
-    ThrottleChecksFinishedCallback callback) {
+void NavigationRequest::WillStartRequest() {
   TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationRequest", this,
                                "WillStartRequest");
   DCHECK_EQ(state_, WILL_START_REQUEST);
 
-  complete_callback_ = std::move(callback);
-
   if (IsSelfReferentialURL()) {
     state_ = CANCELING;
-    RunCompleteCallback(NavigationThrottle::CANCEL);
+    if (complete_callback_for_testing_ &&
+        std::move(complete_callback_for_testing_)
+            .Run(NavigationThrottle::CANCEL)) {
+      return;
+    }
+    OnWillProcessResponseChecksComplete(NavigationThrottle::CANCEL);
+
+    // DO NOT ADD CODE AFTER THIS, as the NavigationRequest might have been
+    // deleted by the previous calls.
     return;
   }
 
@@ -3096,17 +3137,24 @@ void NavigationRequest::WillStartRequest(
 
 void NavigationRequest::WillRedirectRequest(
     const GURL& new_referrer_url,
-    RenderProcessHost* post_redirect_process,
-    ThrottleChecksFinishedCallback callback) {
+    RenderProcessHost* post_redirect_process) {
   TRACE_EVENT_ASYNC_STEP_INTO1("navigation", "NavigationRequest", this,
                                "WillRedirectRequest", "url",
                                common_params_->url.possibly_invalid_spec());
-  UpdateStateFollowingRedirect(new_referrer_url, std::move(callback));
+  UpdateStateFollowingRedirect(new_referrer_url);
   UpdateSiteURL(post_redirect_process);
 
   if (IsSelfReferentialURL()) {
     state_ = CANCELING;
-    RunCompleteCallback(NavigationThrottle::CANCEL);
+    if (complete_callback_for_testing_ &&
+        std::move(complete_callback_for_testing_)
+            .Run(NavigationThrottle::CANCEL)) {
+      return;
+    }
+    OnWillProcessResponseChecksComplete(NavigationThrottle::CANCEL);
+
+    // DO NOT ADD CODE AFTER THIS, as the NavigationRequest might have been
+    // deleted by the previous calls.
     return;
   }
 
@@ -3117,12 +3165,10 @@ void NavigationRequest::WillRedirectRequest(
   // by the previous call.
 }
 
-void NavigationRequest::WillFailRequest(
-    ThrottleChecksFinishedCallback callback) {
+void NavigationRequest::WillFailRequest() {
   TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationRequest", this,
                                "WillFailRequest");
 
-  complete_callback_ = std::move(callback);
   state_ = WILL_FAIL_REQUEST;
   processing_navigation_throttle_ = true;
 
@@ -3133,13 +3179,11 @@ void NavigationRequest::WillFailRequest(
   // by the previous call.
 }
 
-void NavigationRequest::WillProcessResponse(
-    ThrottleChecksFinishedCallback callback) {
+void NavigationRequest::WillProcessResponse() {
   TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationRequest", this,
                                "WillProcessResponse");
   DCHECK_EQ(state_, WILL_PROCESS_RESPONSE);
 
-  complete_callback_ = std::move(callback);
   processing_navigation_throttle_ = true;
 
   // Notify each throttle of the response.
@@ -3245,8 +3289,7 @@ GURL NavigationRequest::GetSiteForCommonParamsURL() const {
 
 // TODO(zetamoo): Try to merge this function inside its callers.
 void NavigationRequest::UpdateStateFollowingRedirect(
-    const GURL& new_referrer_url,
-    ThrottleChecksFinishedCallback callback) {
+    const GURL& new_referrer_url) {
   // The navigation should not redirect to a "renderer debug" url. It should be
   // blocked in NavigationRequest::OnRequestRedirected or in
   // ResourceLoader::OnReceivedRedirect.
@@ -3272,8 +3315,6 @@ void NavigationRequest::UpdateStateFollowingRedirect(
 #if defined(OS_ANDROID)
   navigation_handle_proxy_->DidRedirect();
 #endif
-
-  complete_callback_ = std::move(callback);
 }
 
 void NavigationRequest::SetNavigationClient(
@@ -3350,22 +3391,6 @@ bool NavigationRequest::IsWaitingToCommit() {
   return state_ == READY_TO_COMMIT;
 }
 
-void NavigationRequest::RunCompleteCallback(
-    NavigationThrottle::ThrottleCheckResult result) {
-  DCHECK(result.action() != NavigationThrottle::DEFER);
-
-  ThrottleChecksFinishedCallback callback = std::move(complete_callback_);
-
-  if (!complete_callback_for_testing_.is_null())
-    std::move(complete_callback_for_testing_).Run(result);
-
-  if (!callback.is_null())
-    std::move(callback).Run(result);
-
-  // No code after running the callback, as it might have resulted in our
-  // destruction.
-}
-
 void NavigationRequest::RenderProcessBlockedStateChanged(bool blocked) {
   AddNetworkServiceDebugEvent(std::string("B") + (blocked ? "1" : "0"));
   if (blocked)

content/browser/frame_host/navigation_request.h

@@ -394,7 +394,7 @@ class CONTENT_EXPORT NavigationRequest
   // Simulates renderer aborting navigation.
   void RendererAbortedNavigationForTesting();
 
-  typedef base::OnceCallback<void(NavigationThrottle::ThrottleCheckResult)>
+  typedef base::OnceCallback<bool(NavigationThrottle::ThrottleCheckResult)>
       ThrottleChecksFinishedCallback;
 
   NavigationThrottle* GetDeferringThrottleForTesting() const {
@@ -697,14 +697,10 @@ class CONTENT_EXPORT NavigationRequest
 
   // TODO(zetamoo): Remove the Will* methods and fold them into their callers.
 
-  // Called when the URLRequest will start in the network stack. |callback| will
-  // be called when all throttle checks have completed. This will allow the
-  // caller to cancel the navigation or let it proceed.
-  void WillStartRequest(ThrottleChecksFinishedCallback callback);
+  // Called when the URLRequest will start in the network stack.
+  void WillStartRequest();
 
   // Called when the URLRequest will be redirected in the network stack.
-  // |callback| will be called when all throttles check have completed. This
-  // will allow the caller to cancel the navigation or let it proceed.
   // This will also inform the delegate that the request was redirected.
   //
   // |post_redirect_process| is the renderer process we expect to use to commit
@@ -712,14 +708,10 @@ class CONTENT_EXPORT NavigationRequest
   // no live process that can be used. In that case, a suitable renderer process
   // will be created at commit time.
   void WillRedirectRequest(const GURL& new_referrer_url,
-                           RenderProcessHost* post_redirect_process,
-                           ThrottleChecksFinishedCallback callback);
+                           RenderProcessHost* post_redirect_process);
 
-  // Called when the URLRequest will fail. |callback| will be called when all
-  // throttles check have completed. This will allow the caller to explicitly
-  // cancel the navigation (with a custom error code and/or custom error page
-  // HTML) or let the failure proceed as normal.
-  void WillFailRequest(ThrottleChecksFinishedCallback callback);
+  // Called when the URLRequest will fail.
+  void WillFailRequest();
 
   // Called when the URLRequest has delivered response headers and metadata.
   // |callback| will be called when all throttle checks have completed,
@@ -727,7 +719,7 @@ class CONTENT_EXPORT NavigationRequest
   // NavigationHandle will not call |callback| with a result of DEFER.
   // If the result is PROCEED, then 'ReadyToCommitNavigation' will be called
   // just before calling |callback|.
-  void WillProcessResponse(ThrottleChecksFinishedCallback callback);
+  void WillProcessResponse();
 
   // Checks for attempts to navigate to a page that is already referenced more
   // than once in the frame's ancestors.  This is a helper function used by
@@ -748,8 +740,7 @@ class CONTENT_EXPORT NavigationRequest
 
   // Updates the state of the navigation handle after encountering a server
   // redirect.
-  void UpdateStateFollowingRedirect(const GURL& new_referrer_url,
-                                    ThrottleChecksFinishedCallback callback);
+  void UpdateStateFollowingRedirect(const GURL& new_referrer_url);
 
   // NeedsUrlLoader() returns true if the navigation needs to use the
   // NavigationURLLoader for loading the document.
@@ -787,11 +778,6 @@ class CONTENT_EXPORT NavigationRequest
   // navigation or an error page.
   bool IsWaitingToCommit();
 
-  // Helper function to run and reset the |complete_callback_|. This marks the
-  // end of a round of NavigationThrottleChecks.
-  // TODO(zetamoo): This can be removed once the navigation states are merged.
-  void RunCompleteCallback(NavigationThrottle::ThrottleCheckResult result);
-
   // Called if READY_TO_COMMIT -> COMMIT state transition takes an unusually
   // long time.
   void OnCommitTimeout();
@@ -1026,12 +1012,9 @@ class CONTENT_EXPORT NavigationRequest
   // with this html as content and |net_error| as the network error.
   std::string post_commit_error_page_html_;
 
-  // This callback will be run when all throttle checks have been performed.
-  // TODO(zetamoo): This can be removed once the navigation states are merged.
-  ThrottleChecksFinishedCallback complete_callback_;
-
   // This test-only callback will be run when all throttle checks have been
-  // performed.
+  // performed. If the callback returns true, On*ChecksComplete functions are
+  // skipped, and only the test callback is being performed.
   // TODO(clamy): Revisit the unit test architecture.
   ThrottleChecksFinishedCallback complete_callback_for_testing_;
 

content/browser/frame_host/navigation_request_unittest.cc

@@ -90,11 +90,13 @@ class NavigationRequestTest : public RenderViewHostImplTestHarness {
     was_callback_called_ = false;
     callback_result_ = NavigationThrottle::DEFER;
 
-    // It's safe to use base::Unretained since the NavigationHandle is owned by
+    // It's safe to use base::Unretained since the NavigationRequest is owned by
     // the NavigationRequestTest.
-    request_->WillStartRequest(
+    request_->set_complete_callback_for_testing(
         base::BindOnce(&NavigationRequestTest::UpdateThrottleCheckResult,
                        base::Unretained(this)));
+
+    request_->WillStartRequest();
   }
 
   // Helper function to call WillRedirectRequest on |handle|. If this function
@@ -106,12 +108,13 @@ class NavigationRequestTest : public RenderViewHostImplTestHarness {
     was_callback_called_ = false;
     callback_result_ = NavigationThrottle::DEFER;
 
-    // It's safe to use base::Unretained since the NavigationHandle is owned by
+    // It's safe to use base::Unretained since the NavigationRequest is owned by
     // the NavigationRequestTest.
-    request_->WillRedirectRequest(
-        GURL(), nullptr,
+    request_->set_complete_callback_for_testing(
         base::BindOnce(&NavigationRequestTest::UpdateThrottleCheckResult,
                        base::Unretained(this)));
+
+    request_->WillRedirectRequest(GURL(), nullptr);
   }
 
   // Helper function to call WillFailRequest on |handle|. If this function
@@ -124,11 +127,13 @@ class NavigationRequestTest : public RenderViewHostImplTestHarness {
     callback_result_ = NavigationThrottle::DEFER;
     request_->set_net_error(net_error_code);
 
-    // It's safe to use base::Unretained since the NavigationHandle is owned by
+    // It's safe to use base::Unretained since the NavigationRequest is owned by
     // the NavigationRequestTest.
-    request_->WillFailRequest(
+    request_->set_complete_callback_for_testing(
         base::BindOnce(&NavigationRequestTest::UpdateThrottleCheckResult,
                        base::Unretained(this)));
+
+    request_->WillFailRequest();
   }
 
   // Whether the callback was called.
@@ -199,10 +204,11 @@ class NavigationRequestTest : public RenderViewHostImplTestHarness {
   // The callback provided to NavigationRequest::WillStartRequest,
   // NavigationRequest::WillRedirectRequest, and
   // NavigationRequest::WillFailRequest during the tests.
-  void UpdateThrottleCheckResult(
+  bool UpdateThrottleCheckResult(
       NavigationThrottle::ThrottleCheckResult result) {
     callback_result_ = result;
     was_callback_called_ = true;
+    return true;
   }
 
   std::unique_ptr<NavigationRequest> request_;

content/browser/frame_host/render_frame_host_impl_browsertest.cc

@@ -1209,12 +1209,12 @@ class NavigationHandleGrabber : public WebContentsObserver {
                        base::Unretained(this), navigation_handle));
   }
 
-  void SendingNavigationCommitted(
+  bool SendingNavigationCommitted(
       NavigationHandle* navigation_handle,
       NavigationThrottle::ThrottleCheckResult result) {
-    if (navigation_handle->GetURL().path() != "/title2.html")
-      return;
+    if (navigation_handle->GetURL().path() == "/title2.html")
       ExecuteScriptAsync(web_contents(), "document.open();");
+    return false;
   }
 
   void DidFinishNavigation(NavigationHandle* navigation_handle) override {

content/test/navigation_simulator_impl.cc

@@ -1173,7 +1173,7 @@ void NavigationSimulatorImpl::Wait() {
   run_loop.Run();
 }
 
-void NavigationSimulatorImpl::OnThrottleChecksComplete(
+bool NavigationSimulatorImpl::OnThrottleChecksComplete(
     NavigationThrottle::ThrottleCheckResult result) {
   CHECK(!last_throttle_check_result_);
   last_throttle_check_result_ = result;
@@ -1181,13 +1181,14 @@ void NavigationSimulatorImpl::OnThrottleChecksComplete(
     std::move(wait_closure_).Run();
   if (throttle_checks_complete_closure_)
     std::move(throttle_checks_complete_closure_).Run();
+  return false;
 }
 
 void NavigationSimulatorImpl::PrepareCompleteCallbackOnRequest() {
   last_throttle_check_result_.reset();
   request_->set_complete_callback_for_testing(
       base::BindOnce(&NavigationSimulatorImpl::OnThrottleChecksComplete,
-                     weak_factory_.GetWeakPtr()));
+                     base::Unretained(this)));
 }
 
 RenderFrameHost* NavigationSimulatorImpl::GetFinalRenderFrameHost() {

content/test/navigation_simulator_impl.h

@@ -214,7 +214,7 @@ class NavigationSimulatorImpl : public NavigationSimulator,
   // Sets |last_throttle_check_result_| and calls both the
   // |wait_closure_| and the |throttle_checks_complete_closure_|, if they are
   // set.
-  void OnThrottleChecksComplete(NavigationThrottle::ThrottleCheckResult result);
+  bool OnThrottleChecksComplete(NavigationThrottle::ThrottleCheckResult result);
 
   // Helper method to set the OnThrottleChecksComplete callback on the
   // NavigationRequest.