Navigation: Move states from NavigationHandleState to NavigationState 1.

This moves WILL_START_REQUEST and CANCELING to
NavigationRequest::NavigationState. And removes INITIAL and uses
navigation_handle_id instead.

Bug: 916537
Change-Id: Ibe078a9d2947adb5baead6c5d499d79a7fff201f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1816507Reviewed-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@{#711249}

content/browser/frame_host/navigation_request.cc

@@ -1037,7 +1037,7 @@ NavigationRequest::NavigationRequest(
 NavigationRequest::~NavigationRequest() {
   TRACE_EVENT_ASYNC_END0("navigation", "NavigationRequest", this);
   ResetExpectedProcess();
-  if (state_ == STARTED) {
+  if (state_ >= WILL_START_NAVIGATION && state_ < RESPONSE_STARTED) {
     devtools_instrumentation::OnNavigationRequestFailed(
         *this, network::URLLoaderCompletionStatus(net::ERR_ABORTED));
   }
@@ -1071,7 +1071,7 @@ void NavigationRequest::BeginNavigation() {
   DCHECK(!loader_);
   DCHECK(!render_frame_host_);
 
-  state_ = STARTED;
+  state_ = WILL_START_NAVIGATION;
 
 #if defined(OS_ANDROID)
   base::WeakPtr<NavigationRequest> this_ptr(weak_factory_.GetWeakPtr());
@@ -1275,8 +1275,8 @@ void NavigationRequest::StartNavigation(bool is_for_commit) {
         common_params_->url, *common_params_->referrer);
   }
 
-  DCHECK_EQ(handle_state_, NOT_CREATED);
-  handle_state_ = INITIAL;
+  DCHECK(!IsNavigationStarted());
+  state_ = WILL_START_REQUEST;
   navigation_handle_id_ = CreateUniqueHandleID();
 
   request_headers_ = std::move(headers);
@@ -1317,11 +1317,12 @@ void NavigationRequest::ResetForCrossDocumentRestart() {
     TraceNavigationEnd();
   }
 
-  // Reset the states of the NavigationRequest.
+  // Reset the states of the NavigationRequest, and the navigation_handle_id.
   StopCommitTimeout();
   state_ = NOT_STARTED;
   handle_state_ = NOT_CREATED;
   processing_navigation_throttle_ = false;
+  navigation_handle_id_ = 0;
 
 #if defined(OS_ANDROID)
   if (navigation_handle_proxy_)
@@ -1583,7 +1584,7 @@ void NavigationRequest::OnResponseStarted(
     RecordDownloadUseCountersPostPolicyCheck();
   request_id_ = request_id;
 
-  DCHECK_EQ(state_, STARTED);
+  DCHECK(IsNavigationStarted());
   DCHECK(response_head);
   TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationRequest", this,
                                "OnResponseStarted");
@@ -1869,7 +1870,8 @@ void NavigationRequest::OnRequestFailedInternal(
     bool skip_throttles,
     const base::Optional<std::string>& error_page_content,
     bool collapse_frame) {
-  DCHECK(state_ == STARTED || state_ == RESPONSE_STARTED);
+  DCHECK(state_ == WILL_START_NAVIGATION || state_ == WILL_START_REQUEST ||
+         state_ == RESPONSE_STARTED || state_ == CANCELING);
   DCHECK(!(status.error_code == net::ERR_ABORTED &&
            error_page_content.has_value()));
 
@@ -2918,12 +2920,12 @@ void NavigationRequest::OnNavigationEventProcessed(
 
 void NavigationRequest::OnWillStartRequestProcessed(
     NavigationThrottle::ThrottleCheckResult result) {
-  DCHECK_EQ(WILL_START_REQUEST, handle_state_);
+  DCHECK_EQ(WILL_START_REQUEST, state_);
   DCHECK_NE(NavigationThrottle::BLOCK_RESPONSE, result.action());
   DCHECK(processing_navigation_throttle_);
   processing_navigation_throttle_ = false;
   if (result.action() != NavigationThrottle::PROCEED)
-    handle_state_ = CANCELING;
+    state_ = CANCELING;
 
   // TODO(zetamoo): Remove CompleteCallback, and call NavigationRequest methods
   // directly.
@@ -2941,7 +2943,7 @@ void NavigationRequest::OnWillRedirectRequestProcessed(
     if (GetDelegate())
       GetDelegate()->DidRedirectNavigation(this);
   } else {
-    handle_state_ = CANCELING;
+    state_ = CANCELING;
   }
   RunCompleteCallback(result);
 }
@@ -2956,7 +2958,7 @@ void NavigationRequest::OnWillFailRequestProcessed(
     result = NavigationThrottle::ThrottleCheckResult(
         NavigationThrottle::PROCEED, net_error_);
   } else {
-    handle_state_ = CANCELING;
+    state_ = CANCELING;
   }
   RunCompleteCallback(result);
 }
@@ -2975,7 +2977,7 @@ void NavigationRequest::OnWillProcessResponseProcessed(
     if (render_frame_host_)
       ReadyToCommitNavigation(false);
   } else {
-    handle_state_ = CANCELING;
+    state_ = CANCELING;
   }
   RunCompleteCallback(result);
 }
@@ -3031,12 +3033,12 @@ void NavigationRequest::CancelDeferredNavigationInternal(
          result.action() == NavigationThrottle::CANCEL ||
          result.action() == NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE);
   DCHECK(result.action() != NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE ||
-         handle_state_ == WILL_START_REQUEST ||
+         state_ == WILL_START_REQUEST ||
          handle_state_ == WILL_REDIRECT_REQUEST);
 
   TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationRequest", this,
                                "CancelDeferredNavigation");
-  handle_state_ = CANCELING;
+  state_ = CANCELING;
   RunCompleteCallback(result);
 }
 
@@ -3044,18 +3046,12 @@ void NavigationRequest::WillStartRequest(
     ThrottleChecksFinishedCallback callback) {
   TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationRequest", this,
                                "WillStartRequest");
-  // WillStartRequest should only be called once.
-  if (handle_state_ != INITIAL) {
-    handle_state_ = CANCELING;
-    RunCompleteCallback(NavigationThrottle::CANCEL);
-    return;
-  }
+  DCHECK_EQ(state_, WILL_START_REQUEST);
 
-  handle_state_ = WILL_START_REQUEST;
   complete_callback_ = std::move(callback);
 
   if (IsSelfReferentialURL()) {
-    handle_state_ = CANCELING;
+    state_ = CANCELING;
     RunCompleteCallback(NavigationThrottle::CANCEL);
     return;
   }
@@ -3088,7 +3084,7 @@ void NavigationRequest::WillRedirectRequest(
   UpdateSiteURL(post_redirect_process);
 
   if (IsSelfReferentialURL()) {
-    handle_state_ = CANCELING;
+    state_ = CANCELING;
     RunCompleteCallback(NavigationThrottle::CANCEL);
     return;
   }
@@ -3472,8 +3468,7 @@ void NavigationRequest::RemoveRequestHeader(const std::string& header_name) {
 
 void NavigationRequest::SetRequestHeader(const std::string& header_name,
                                          const std::string& header_value) {
-  DCHECK(handle_state_ == INITIAL ||
-         handle_state_ == WILL_START_REQUEST ||
+  DCHECK(state_ == WILL_START_REQUEST ||
          handle_state_ == WILL_REDIRECT_REQUEST);
   modified_request_headers_.SetHeader(header_name, header_value);
 }
@@ -3530,10 +3525,7 @@ bool NavigationRequest::IsSignedExchangeInnerResponse() {
 }
 
 net::IPEndPoint NavigationRequest::GetSocketAddress() {
-  // This is CANCELING because although the data comes in after
-  // WILL_PROCESS_RESPONSE, it's possible for the navigation to be cancelled
-  // after and the caller might want this value.
-  DCHECK_GE(handle_state_, CANCELING);
+  DCHECK_GE(handle_state_, WILL_PROCESS_RESPONSE);
   return response() ? response()->head.remote_endpoint : net::IPEndPoint();
 }
 
@@ -3812,7 +3804,7 @@ NavigationRequest* NavigationRequest::From(NavigationHandle* handle) {
 }
 
 bool NavigationRequest::IsNavigationStarted() const {
-  return handle_state_ >= INITIAL;
+  return navigation_handle_id_;
 }
 
 bool NavigationRequest::RequiresSourceSiteInstance() const {

content/browser/frame_host/navigation_request.h

@@ -87,14 +87,26 @@ class CONTENT_EXPORT NavigationRequest : public NavigationHandle,
     // request is created, this stage is skipped.
     WAITING_FOR_RENDERER_RESPONSE,
 
-    // The request was sent to the IO thread and the NavigatorDelegate has been
-    // notified.
-    STARTED,
+    // TODO(zetamoo): Merge this state with WILL_START_REQUEST.
+    // Temporary state where:
+    //  - Before unload handlers have run and this navigation is allowed to
+    //    start.
+    //  - The navigation is still not visible to embedders (via
+    //    NavigationHandle).
+    WILL_START_NAVIGATION,
+
+    // The navigation is visible to embedders (via NavigationHandle). Wait for
+    // the NavigationThrottles to finish running the WillStartRequest event.
+    // This is potentially asynchronous.
+    WILL_START_REQUEST,
 
     // The response started on the IO thread and is ready to be committed. This
     // is one of the two final states for the request.
     RESPONSE_STARTED,
 
+    // The request is being canceled.
+    CANCELING,
+
     // The request failed on the IO thread and an error page should be
     // displayed. This is one of the two final states for the request.
     FAILED,
@@ -108,11 +120,8 @@ class CONTENT_EXPORT NavigationRequest : public NavigationHandle,
   //                the duplicates.
   enum NavigationHandleState {
     NOT_CREATED = 0,
-    INITIAL,
-    WILL_START_REQUEST,
     WILL_REDIRECT_REQUEST,
     WILL_FAIL_REQUEST,
-    CANCELING,
     WILL_PROCESS_RESPONSE,
     READY_TO_COMMIT,
     DID_COMMIT,
@@ -484,12 +493,10 @@ class CONTENT_EXPORT NavigationRequest : public NavigationHandle,
   NavigatorDelegate* GetDelegate() const;
 
   blink::mojom::RequestContextType request_context_type() const {
-    DCHECK_GE(handle_state_, WILL_START_REQUEST);
     return begin_params_->request_context_type;
   }
 
   blink::WebMixedContentContextType mixed_content_context_type() const {
-    DCHECK_GE(handle_state_, WILL_START_REQUEST);
     return begin_params_->mixed_content_context_type;
   }
 

content/browser/frame_host/navigation_request_unittest.cc

@@ -161,6 +161,10 @@ class NavigationRequestTest : public RenderViewHostImplTestHarness {
     return request_->handle_state();
   }
 
+  NavigationRequest::NavigationState request_state() {
+    return request_->state();
+  }
+
   bool call_counts_match(TestNavigationThrottle* throttle,
                          int start,
                          int redirect,
@@ -309,19 +313,19 @@ TEST_F(NavigationRequestTest, SimpleDataChecksFailure) {
 TEST_F(NavigationRequestTest, MAYBE_CancelDeferredWillStart) {
   TestNavigationThrottle* test_throttle =
       CreateTestNavigationThrottle(NavigationThrottle::DEFER);
-  EXPECT_EQ(NavigationRequest::INITIAL, state());
+  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, request_state());
   EXPECT_TRUE(call_counts_match(test_throttle, 0, 0, 0, 0));
 
   // Simulate WillStartRequest. The request should be deferred. The callback
   // should not have been called.
   SimulateWillStartRequest();
-  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, state());
+  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, request_state());
   EXPECT_FALSE(was_callback_called());
   EXPECT_TRUE(call_counts_match(test_throttle, 1, 0, 0, 0));
 
   // Cancel the request. The callback should have been called.
   CancelDeferredNavigation(NavigationThrottle::CANCEL_AND_IGNORE);
-  EXPECT_EQ(NavigationRequest::CANCELING, state());
+  EXPECT_EQ(NavigationRequest::CANCELING, request_state());
   EXPECT_TRUE(was_callback_called());
   EXPECT_EQ(NavigationThrottle::CANCEL_AND_IGNORE, callback_result());
   EXPECT_TRUE(call_counts_match(test_throttle, 1, 0, 0, 0));
@@ -339,7 +343,7 @@ TEST_F(NavigationRequestTest, MAYBE_CancelDeferredWillStart) {
 TEST_F(NavigationRequestTest, MAYBE_CancelDeferredWillRedirect) {
   TestNavigationThrottle* test_throttle =
       CreateTestNavigationThrottle(NavigationThrottle::DEFER);
-  EXPECT_EQ(NavigationRequest::INITIAL, state());
+  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, request_state());
   EXPECT_TRUE(call_counts_match(test_throttle, 0, 0, 0, 0));
 
   // Simulate WillRedirectRequest. The request should be deferred. The callback
@@ -351,7 +355,7 @@ TEST_F(NavigationRequestTest, MAYBE_CancelDeferredWillRedirect) {
 
   // Cancel the request. The callback should have been called.
   CancelDeferredNavigation(NavigationThrottle::CANCEL_AND_IGNORE);
-  EXPECT_EQ(NavigationRequest::CANCELING, state());
+  EXPECT_EQ(NavigationRequest::CANCELING, request_state());
   EXPECT_TRUE(was_callback_called());
   EXPECT_EQ(NavigationThrottle::CANCEL_AND_IGNORE, callback_result());
   EXPECT_TRUE(call_counts_match(test_throttle, 0, 1, 0, 0));
@@ -369,7 +373,7 @@ TEST_F(NavigationRequestTest, MAYBE_CancelDeferredWillRedirect) {
 TEST_F(NavigationRequestTest, MAYBE_CancelDeferredWillFail) {
   TestNavigationThrottle* test_throttle = CreateTestNavigationThrottle(
       TestNavigationThrottle::WILL_FAIL_REQUEST, NavigationThrottle::DEFER);
-  EXPECT_EQ(NavigationRequest::INITIAL, state());
+  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, request_state());
   EXPECT_TRUE(call_counts_match(test_throttle, 0, 0, 0, 0));
 
   // Simulate WillStartRequest.
@@ -385,7 +389,7 @@ TEST_F(NavigationRequestTest, MAYBE_CancelDeferredWillFail) {
 
   // Cancel the request. The callback should have been called.
   CancelDeferredNavigation(NavigationThrottle::CANCEL_AND_IGNORE);
-  EXPECT_EQ(NavigationRequest::CANCELING, state());
+  EXPECT_EQ(NavigationRequest::CANCELING, request_state());
   EXPECT_TRUE(was_callback_called());
   EXPECT_EQ(NavigationThrottle::CANCEL_AND_IGNORE, callback_result());
   EXPECT_TRUE(call_counts_match(test_throttle, 1, 0, 1, 0));
@@ -404,19 +408,19 @@ TEST_F(NavigationRequestTest, MAYBE_CancelDeferredWillFail) {
 TEST_F(NavigationRequestTest, MAYBE_CancelDeferredWillRedirectNoIgnore) {
   TestNavigationThrottle* test_throttle =
       CreateTestNavigationThrottle(NavigationThrottle::DEFER);
-  EXPECT_EQ(NavigationRequest::INITIAL, state());
+  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, request_state());
   EXPECT_TRUE(call_counts_match(test_throttle, 0, 0, 0, 0));
 
   // Simulate WillStartRequest. The request should be deferred. The callback
   // should not have been called.
   SimulateWillStartRequest();
-  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, state());
+  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, request_state());
   EXPECT_TRUE(call_counts_match(test_throttle, 1, 0, 0, 0));
 
   // Cancel the request. The callback should have been called with CANCEL, and
   // not CANCEL_AND_IGNORE.
   CancelDeferredNavigation(NavigationThrottle::CANCEL);
-  EXPECT_EQ(NavigationRequest::CANCELING, state());
+  EXPECT_EQ(NavigationRequest::CANCELING, request_state());
   EXPECT_TRUE(was_callback_called());
   EXPECT_EQ(NavigationThrottle::CANCEL, callback_result());
   EXPECT_TRUE(call_counts_match(test_throttle, 1, 0, 0, 0));
@@ -435,7 +439,7 @@ TEST_F(NavigationRequestTest, MAYBE_CancelDeferredWillRedirectNoIgnore) {
 TEST_F(NavigationRequestTest, MAYBE_CancelDeferredWillFailNoIgnore) {
   TestNavigationThrottle* test_throttle = CreateTestNavigationThrottle(
       TestNavigationThrottle::WILL_FAIL_REQUEST, NavigationThrottle::DEFER);
-  EXPECT_EQ(NavigationRequest::INITIAL, state());
+  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, request_state());
   EXPECT_TRUE(call_counts_match(test_throttle, 0, 0, 0, 0));
 
   // Simulate WillStartRequest.
@@ -452,7 +456,7 @@ TEST_F(NavigationRequestTest, MAYBE_CancelDeferredWillFailNoIgnore) {
   // Cancel the request. The callback should have been called with CANCEL, and
   // not CANCEL_AND_IGNORE.
   CancelDeferredNavigation(NavigationThrottle::CANCEL);
-  EXPECT_EQ(NavigationRequest::CANCELING, state());
+  EXPECT_EQ(NavigationRequest::CANCELING, request_state());
   EXPECT_TRUE(was_callback_called());
   EXPECT_EQ(NavigationThrottle::CANCEL, callback_result());
   EXPECT_TRUE(call_counts_match(test_throttle, 1, 0, 1, 0));

content/browser/frame_host/navigator_impl_unittest.cc

@@ -89,7 +89,7 @@ TEST_F(NavigatorTest, SimpleBrowserInitiatedNavigationFromNonLiveRenderer) {
 
   // As there's no live renderer the navigation should not wait for a
   // beforeUnload ACK from the renderer and start right away.
-  EXPECT_EQ(NavigationRequest::STARTED, request->state());
+  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, request->state());
   ASSERT_TRUE(GetLoaderForNavigationRequest(request));
   EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
 
@@ -142,7 +142,7 @@ TEST_F(NavigatorTest, SimpleRendererInitiatedSameSiteNavigation) {
 
   // The navigation is immediately started as there's no need to wait for
   // beforeUnload to be executed.
-  EXPECT_EQ(NavigationRequest::STARTED, request->state());
+  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, request->state());
   EXPECT_FALSE(request->common_params().has_user_gesture);
   EXPECT_EQ(kUrl2, request->common_params().url);
   EXPECT_FALSE(request->browser_initiated());
@@ -189,7 +189,7 @@ TEST_F(NavigatorTest, SimpleRendererInitiatedCrossSiteNavigation) {
 
   // The navigation is immediately started as there's no need to wait for
   // beforeUnload to be executed.
-  EXPECT_EQ(NavigationRequest::STARTED, request->state());
+  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, request->state());
   EXPECT_EQ(kUrl2, request->common_params().url);
   EXPECT_FALSE(request->browser_initiated());
   if (AreAllSitesIsolatedForTesting() ||
@@ -285,7 +285,7 @@ TEST_F(NavigatorTest, BeginNavigation) {
   TestNavigationURLLoader* subframe_loader =
       GetLoaderForNavigationRequest(subframe_request);
   ASSERT_TRUE(subframe_loader);
-  EXPECT_EQ(NavigationRequest::STARTED, subframe_request->state());
+  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, subframe_request->state());
   EXPECT_EQ(kUrl2, subframe_request->common_params().url);
   EXPECT_EQ(kUrl2, subframe_loader->request_info()->common_params->url);
   // First party for cookies url should be that of the main frame.
@@ -334,7 +334,7 @@ TEST_F(NavigatorTest, BeginNavigation) {
   EXPECT_TRUE(main_request->browser_initiated());
   // BeforeUnloadACK was received from the renderer so the navigation should
   // have started.
-  EXPECT_EQ(NavigationRequest::STARTED, main_request->state());
+  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, main_request->state());
   EXPECT_TRUE(GetSpeculativeRenderFrameHost(root_node));
 
   // As the main frame hasn't yet committed the subframe still exists. Thus, the

content/test/test_render_frame_host.cc

@@ -402,7 +402,8 @@ void TestRenderFrameHost::PrepareForCommitInternal(
   if (!have_to_make_network_request)
     return;  // |request| is destructed by now.
 
-  CHECK(request->state() == NavigationRequest::STARTED);
+  CHECK(request->state() >= NavigationRequest::WILL_START_NAVIGATION &&
+        request->state() < NavigationRequest::RESPONSE_STARTED);
 
   if (!request->loader_for_testing()) {
     base::RunLoop loop;