Remove IsSynchronous API from NavigationHandle.

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 <utility>
 
 #include "base/logging.h"
 #include "content/browser/browsing_data/clear_site_data_throttle.h"
@@ skipping 21 matching lines @@
   *to_update = result;
 }
 
 }  // namespace
 
 // static
 std::unique_ptr<NavigationHandleImpl> NavigationHandleImpl::Create(
     const GURL& url,
     FrameTreeNode* frame_tree_node,
     bool is_renderer_initiated,
-    bool is_synchronous,
+    bool is_same_page,
     bool is_srcdoc,
     const base::TimeTicks& navigation_start,
     int pending_nav_entry_id) {
-  return std::unique_ptr<NavigationHandleImpl>(
-      new NavigationHandleImpl(url, frame_tree_node, is_renderer_initiated,
-                               is_synchronous, is_srcdoc, navigation_start,
-                               pending_nav_entry_id));
+  return std::unique_ptr<NavigationHandleImpl>(new NavigationHandleImpl(
+      url, frame_tree_node, is_renderer_initiated, is_same_page, is_srcdoc,
+      navigation_start, pending_nav_entry_id));
 }
 
 NavigationHandleImpl::NavigationHandleImpl(
     const GURL& url,
     FrameTreeNode* frame_tree_node,
     bool is_renderer_initiated,
-    bool is_synchronous,
+    bool is_same_page,
     bool is_srcdoc,
     const base::TimeTicks& navigation_start,
     int pending_nav_entry_id)
     : url_(url),
       has_user_gesture_(false),
       transition_(ui::PAGE_TRANSITION_LINK),
       is_external_protocol_(false),
       net_error_code_(net::OK),
       render_frame_host_(nullptr),
       is_renderer_initiated_(is_renderer_initiated),
-      is_same_page_(false),
-      is_synchronous_(is_synchronous),
+      is_same_page_(is_same_page),
       is_srcdoc_(is_srcdoc),
       was_redirected_(false),
       state_(INITIAL),
       is_transferring_(false),
       frame_tree_node_(frame_tree_node),
       next_index_(0),
       navigation_start_(navigation_start),
       pending_nav_entry_id_(pending_nav_entry_id),
       request_context_type_(REQUEST_CONTEXT_TYPE_UNSPECIFIED) {
   DCHECK(!navigation_start.is_null());
@@ skipping 42 matching lines @@
   if (frame_tree_node_->parent())
     return frame_tree_node_->parent()->IsMainFrame();
 
   return false;
 }
 
 bool NavigationHandleImpl::IsRendererInitiated() {
   return is_renderer_initiated_;
 }
 
-bool NavigationHandleImpl::IsSynchronousNavigation() {
-  return is_synchronous_;
-}
-
 bool NavigationHandleImpl::IsSrcdoc() {
   return is_srcdoc_;
 }
 
 bool NavigationHandleImpl::WasServerRedirect() {
   return was_redirected_;
 }
 
 int NavigationHandleImpl::GetFrameTreeNodeId() {
   return frame_tree_node_->frame_tree_node_id();
@@ skipping 48 matching lines @@
   // TODO(mkwst): Change this to check against 'READY_TO_COMMIT' once
   // ReadyToCommitNavigation is available whether or not PlzNavigate is
   // enabled. https://crbug.com/621856
   CHECK_GE(state_, WILL_PROCESS_RESPONSE)
       << "This accessor should only be called after a response has been "
          "delivered for processing.";
   return render_frame_host_;
 }
 
 bool NavigationHandleImpl::IsSamePage() {
-  DCHECK(state_ == DID_COMMIT || state_ == DID_COMMIT_ERROR_PAGE)
-      << "This accessor should not be called before the navigation has "
-         "committed.";
-  return is_same_page_;
+  return is_same_page_ && url_ != GURL(url::kAboutBlankURL);
 }
 
 const net::HttpResponseHeaders* NavigationHandleImpl::GetResponseHeaders() {
   DCHECK_GE(state_, WILL_REDIRECT_REQUEST);
   return response_headers_.get();
 }
 
 bool NavigationHandleImpl::HasCommitted() {
   return state_ == DID_COMMIT || state_ == DID_COMMIT_ERROR_PAGE;
 }
@@ skipping 229 matching lines @@
   if (IsBrowserSideNavigationEnabled())
     GetDelegate()->ReadyToCommitNavigation(this);
 }
 
 void NavigationHandleImpl::DidCommitNavigation(
     const FrameHostMsg_DidCommitProvisionalLoad_Params& params,
     bool same_page,
     RenderFrameHostImpl* render_frame_host) {
   DCHECK(!render_frame_host_ || render_frame_host_ == render_frame_host);
   DCHECK_EQ(frame_tree_node_, render_frame_host->frame_tree_node());
+  DCHECK_EQ(same_page, IsSamePage());

[nasko, 2016/09/16 23:38:11]

Sadly, I think I will have to lose this DCHECK due to
NavigationTypeClassification_On1InPageTo2While2Pending.
It creates page1, while there, it starts a load to page2, but the network is
blocked on it. That creates a NavigationHandle for page2 with IsSamePage set to
false. The test then does a pushState for the exact same URL for page2, which
sends a commit IPC. Since the URLs match exactly, we keep the created
NavigationHandle and commit the URL, however same_page is true because it was a
pushState navigation.

Any ideas on how to disambiguate this case? FWIW, it works correctly in
PlzNavigate mode :).

[Charlie Reis, 2016/09/16 23:59:15]

Aaaa!  This is why comparing URLs is never a good way to track things.  :(

I'm a bit concerned about this case, because we're clearly using a different
navigation's handle for the same-page commit.  Is there any other state on the
handle that might be incorrect when this happens?  For example, what if the slow
one was a POST?  Would the handle's resource_request_body_ get persisted in the
NavEntry for the same-page navigation?  That seems bad.

Seems better if we can create a new NavigationHandle for the pushState here,
even if the URL matches the old handle.  I don't know how to detect that,
though.
How does PlzNavigate get it right?

[nasko, 2016/09/20 23:11:20]

As we discussed in person, I am adding in the mix of checks whether the
NavigationHandle and the renderer process agree on the value of "is same page
navigation". If there is a disagreement, then the old NavigationHandle will be
cleared and a new one will be created.

The eventual commit of the slow URL will create a new NavigationHandle and fire
the set of Start/Finish notifications. However, that does mean that the first
handle that was initially created will get a failed navigation, which isn't
quite true, but probably the safer of the two possible outcomes.
PlzNavigate gets it mostly right, since the slow loading NavigationHandle is
owned by the NavigationRequest until we get the response from the network. At
the time we send the Commit IPC down to the RenderFrame, we transfer the
ownership to the RFH. The race is still possible, but the window for it is much
smaller.

   CHECK_EQ(url_, params.url);
 
   method_ = params.method;
   has_user_gesture_ = (params.gesture == NavigationGestureUser);
   transition_ = params.transition;
   render_frame_host_ = render_frame_host;
-  is_same_page_ = same_page;
 
   state_ = net_error_code_ == net::OK ? DID_COMMIT : DID_COMMIT_ERROR_PAGE;
 }
 
 NavigationThrottle::ThrottleCheckResult
 NavigationHandleImpl::CheckWillStartRequest() {
   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) {
@@ skipping 119 matching lines @@
     throttles_to_register.push_back(std::move(clear_site_data_throttle));
 
   if (throttles_to_register.size() > 0) {
     throttles_.insert(throttles_.begin(), throttles_to_register.begin(),
                       throttles_to_register.end());
     throttles_to_register.weak_clear();
   }
 }
 
 }  // namespace content