PlzNavigate: track pending commits

content/browser/frame_host/render_frame_host_impl.cc

Index: content/browser/frame_host/render_frame_host_impl.cc
diff --git a/content/browser/frame_host/render_frame_host_impl.cc b/content/browser/frame_host/render_frame_host_impl.cc
index d4cc6f50c5f352ef7c908d8f5601785b97335ec6..62cdfc284d1a3149504c54e2f6eea1d5522caab4 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -150,6 +150,7 @@ RenderFrameHostImpl::RenderFrameHostImpl(SiteInstance* site_instance,
       is_waiting_for_beforeunload_ack_(false),
       unload_ack_is_for_navigation_(false),
       is_loading_(false),
+      pending_commit_(false),
       accessibility_reset_token_(0),
       accessibility_reset_count_(0),
       no_create_browser_accessibility_manager_for_testing_(false),
@@ -839,6 +840,14 @@ void RenderFrameHostImpl::OnDidCommitProvisionalLoad(const IPC::Message& msg) {
 
   accessibility_reset_count_ = 0;
   frame_tree_node()->navigator()->DidNavigate(this, validated_params);
+
+  // PlzNavigate
+  // Check that a commit is expected.
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableBrowserSideNavigation)) {
+    DCHECK(pending_commit_);

[Charlie Reis, 2015/04/07 17:04:47]

This doesn't seem right to me.

If the renderer process does a in-page navigation (fragment, pushState, etc),
we'll get a DidCommitProvisionalLoad without going through CommitNavigation,
right?

[clamy, 2015/04/08 11:56:23]

I think we want to inform the browser that we're doing a synchronous navigation
first (through BeginNavigation). This allows to cancel an ongoing navigation if
needed. Then the Navigator can see we don't need to go through the network stack
and inform the appropriate RFH to expect a commit.

[Charlie Reis, 2015/04/08 16:45:27]

Ok, so the renderer will tell the browser via BeginNavigation, then later in the
same task commit the navigation via DidCommit.  That part seems ok.

However, it doesn't look like you're setting pending_commit_ to be true in that
case.  NavigatorImpl::BeginNavigation will get false from
NavigationRequest::BeginNavigation if it's a pushState, etc, and it doesn't tell
the current RFH to expect the commit.  It's only set to true in
CommitNavigation, which doesn't look like it's called here.  Am I missing
something?

[clamy, 2015/04/10 14:40:15]

No that it is not implemented yet. We do not currently support same-site
navigations properly.

[Charlie Reis, 2015/04/10 23:04:01]

Ok.  If you're expecting this check to fail, please add a TODO here to fix it
(mentioning why it would fail).

[clamy, 2015/04/14 14:14:16]

Considering that the DCHECK may fail for a certain number of not fully
understood reasons, I'm removing it.

+    pending_commit_ = false;
+  }
 }
 
 void RenderFrameHostImpl::OnDidDropNavigation() {
@@ -938,6 +947,13 @@ void RenderFrameHostImpl::SwapOut(
 
   if (!GetParent())
     delegate_->SwappedOut(this);
+
+  // PlzNavigate
+  // A commit is no longer expected as the frame is swapped out.

[Charlie Reis, 2015/04/07 17:04:47]

I don't understand how this would happen.  When would a frame get swapped out
after a call to CommitNavigation?

[clamy, 2015/04/08 11:56:23]

If the current RFH was supposed to commit a navigation, but for some reason it
took a lot of time, which allowed a new cross-site navigation to commit in the
meantime. Then the current RFH gets swapped out before sending
DidCommitProvisionalLoad.

[Charlie Reis, 2015/04/08 16:45:27]

I thought that the DidCommitProvisionalLoad IPC was sent in the same task as
receiving the OnCommitNavigation IPC, right?  That means that the renderer
process won't have a chance to see the FrameMsg_SwapOut request before sending
the DidCommit.  I think this will cause your DCHECK in DidCommit to fail.

(Looking closer, I see that there's an early return in
RenderFrameImpl::OnCommitNavigation if PrepareRenderViewForNavigation returns
false, but it's not sending a DidDropNavigation IPC the way we do in default
Chrome.  That seems potentially buggy-- don't we need to stop the spinner and
clean up the pending state if we ignore the navigation as stale?  We should
probably clear pending_commit_ in that case as well.)

It sounds like we probably want a test for this case, since it's not clear
whether it works.

[clamy, 2015/04/10 14:40:15]

No the DidCommitProvisionalLoad IPC is sent when the renderer gets data from the
browser from the stream request, which does not happen synchronously.

(Indeed we noticed that as well when trying to get PlzNavigate to send the
proper start/stop loading notifications. Fabrice is working on making the
start/stop loading work with PlzNavigate, so I expect him to fix that in a later
patch).

[Charlie Reis, 2015/04/10 23:04:01]

Oh.  Wouldn't all sorts of things potentially cancel the commit in that case? 
What if a Stop happened, triggering a DidFailProvisionalLoad?

[clamy, 2015/04/14 14:14:16]

Good point. I removed the DCHECK, and I wont reset pending_commit_ here.

+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableBrowserSideNavigation)) {
+    pending_commit_ = false;
+  }
 }
 
 void RenderFrameHostImpl::OnBeforeUnloadACK(
@@ -1714,6 +1730,7 @@ void RenderFrameHostImpl::CommitNavigation(
   // TODO(clamy): Release the stream handle once the renderer has finished
   // reading it.
   stream_handle_ = body.Pass();
+  pending_commit_ = true;
 }
 
 void RenderFrameHostImpl::SetUpMojoIfNeeded() {