Remove IsSynchronous API from NavigationHandle.

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 b95c691e3975168323163d65aa0197dbfcc15462..a7c1d9f459045fe6f3d22f800926e2010c0a7411 100644
--- a/content/browser/frame_host/navigation_handle_impl.cc
+++ b/content/browser/frame_host/navigation_handle_impl.cc
@@ -39,21 +39,20 @@ 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)
@@ -64,8 +63,7 @@ NavigationHandleImpl::NavigationHandleImpl(
       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),
@@ -128,10 +126,6 @@ bool NavigationHandleImpl::IsRendererInitiated() {
   return is_renderer_initiated_;
 }
 
-bool NavigationHandleImpl::IsSynchronousNavigation() {
-  return is_synchronous_;
-}
-
 bool NavigationHandleImpl::IsSrcdoc() {
   return is_srcdoc_;
 }
@@ -200,10 +194,7 @@ RenderFrameHostImpl* NavigationHandleImpl::GetRenderFrameHost() {
 }
 
 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() {
@@ -453,13 +444,13 @@ void NavigationHandleImpl::DidCommitNavigation(
     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;
 }