PlzNavigate: track pending commits

content/browser/frame_host/render_frame_host_impl.cc

 // Copyright 2013 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/render_frame_host_impl.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/containers/hash_tables.h"
 #include "base/lazy_instance.h"
@@ skipping 132 matching lines @@
       cross_process_frame_connector_(NULL),
       render_frame_proxy_host_(NULL),
       frame_tree_(frame_tree),
       frame_tree_node_(frame_tree_node),
       routing_id_(routing_id),
       render_frame_created_(false),
       navigations_suspended_(false),
       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),
       weak_ptr_factory_(this) {
   bool is_swapped_out = !!(flags & CREATE_RF_SWAPPED_OUT);
   bool hidden = !!(flags & CREATE_RF_HIDDEN);
   frame_tree_->RegisterRenderFrameHost(this);
   GetProcess()->AddRoute(routing_id_, this);
   g_routing_id_frame_map.Get().insert(std::make_pair(
       RenderFrameHostID(GetProcess()->GetID(), routing_id_),
@@ skipping 669 matching lines @@
   // filenames it can't access in a future session restore.
   if (!render_view_host_->CanAccessFilesOfPageState(
           validated_params.page_state)) {
     bad_message::ReceivedBadMessage(
         GetProcess(), bad_message::RFH_CAN_ACCESS_FILES_OF_PAGE_STATE);
     return;
   }
 
   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() {
   // At the end of Navigate(), the delegate's DidStartLoading is called to force
   // the spinner to start, even if the renderer didn't yet begin the load. If it
   // turns out that the renderer dropped the navigation, we need to turn off the
   // spinner.
   delegate_->DidStopLoading();
 }
 
@@ skipping 79 matching lines @@
     replication_state = proxy->frame_tree_node()->current_replication_state();
   }
 
   if (IsRenderFrameLive()) {
     Send(new FrameMsg_SwapOut(routing_id_, proxy_routing_id, is_loading,
                               replication_state));
   }
 
   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(
     bool proceed,
     const base::TimeTicks& renderer_before_unload_start_time,
     const base::TimeTicks& renderer_before_unload_end_time) {
   TRACE_EVENT_ASYNC_END0(
       "navigation", "RenderFrameHostImpl::BeforeUnload", this);
   DCHECK(!GetParent());
   // If this renderer navigated while the beforeunload request was in flight, we
@@ skipping 756 matching lines @@
   const ResourceResponseHead head = response ?
       response->head : ResourceResponseHead();
   Send(new FrameMsg_CommitNavigation(routing_id_, head, body_url, common_params,
                                      request_params));
   // TODO(clamy): Check if we should start the throbber for non javascript urls
   // here.
 
   // TODO(clamy): Release the stream handle once the renderer has finished
   // reading it.
   stream_handle_ = body.Pass();
+  pending_commit_ = true;
 }
 
 void RenderFrameHostImpl::SetUpMojoIfNeeded() {
   if (service_registry_.get())
     return;
 
   service_registry_.reset(new ServiceRegistryImpl());
   if (!GetProcess()->GetServiceRegistry())
     return;
 
@@ skipping 228 matching lines @@
     return;
 
   permission_manager->RegisterPermissionUsage(
       PermissionType::GEOLOCATION,
       GetLastCommittedURL().GetOrigin(),
       frame_tree_node()->frame_tree()->GetMainFrame()
           ->GetLastCommittedURL().GetOrigin());
 }
 
 }  // namespace content