PlzNavigate: track pending commits

content/browser/frame_host/render_frame_host_manager.cc

Index: content/browser/frame_host/render_frame_host_manager.cc
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
index 29771b2ab65da73c748a60643a0634acce2ec5a2..86ec929606b873d108c635750f63b0177eb29f40 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -63,6 +63,8 @@ RenderFrameHostManager::RenderFrameHostManager(
       render_widget_delegate_(render_widget_delegate),
       interstitial_page_(nullptr),
       should_reuse_web_ui_(false),
+      pending_commit_current_frame_(false),
+      pending_commit_speculative_frame_(false),
       weak_factory_(this) {
   DCHECK(frame_tree_node_);
 }
@@ -458,15 +460,19 @@ void RenderFrameHostManager::CommitPendingIfNecessary(
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableBrowserSideNavigation)) {
     if (render_frame_host == speculative_render_frame_host_.get()) {
+      DCHECK(pending_commit_speculative_frame_);
       CommitPending();
     } else if (render_frame_host == render_frame_host_.get()) {
       // TODO(carlosk): this code doesn't properly handle in-page navigation or
       // interwoven navigation requests.
+      DCHECK(pending_commit_current_frame_);

[Charlie Reis, 2015/04/03 06:02:58]

This appears to be the only place that pending_commit_current_frame_ is used.  I
don't see the benefit of tracking it solely for a DCHECK here.  It's better to
avoid adding state when we can, since it's easy for it to become wrong due to
races, etc.  Please remove it.

(I'd like to say the same about pending_commit_speculative_frame_, but I see the
benefit to being able to check it while testing.  Would it work to create a
pending_commit_ on TestRenderFrameHost and manage it solely in test code
instead?  That might let you enforce both of these DCHECKs when
TestRenderFrameHost commits.)

[clamy, 2015/04/07 16:29:21]

So I ended up moving the state tracking to RenderFrameHostImpl (rather than
TestRenderFrameHost), for two reasons:
1) I think it is clearer when we set it.
2) This allow us to do some checking in browser tests as well.

Note that I don't mind moving it to TestRenderFrameHost when we get closer to
launching, but I think that having DCHECKS in place for tracking whether commits
were expected would help us in the development process.

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

Hmm, ok.  It would be nice to not need this in shipping builds of Chrome, but I
don't see a good way to pull that off.  Not even #if defined(UNIT_TEST) can help
us there.  And I agree that having the DCHECK coverage in browser_tests is good,
so TestRFH won't help.

       DCHECK(!speculative_render_frame_host_);
     } else {
       // No one else should be sending us a DidNavigate in this state.
       DCHECK(false);
     }
+    // The current frame either committed or was swapped out.
+    pending_commit_current_frame_ = false;

[Charlie Reis, 2015/04/03 06:02:58]

What if pending_commit_speculative_frame_ was true?  It's not clear why one is
reset here and not the other.

(You can ignore this if you remove this field.)

     DCHECK(!speculative_render_frame_host_);
     return;
   }
@@ -731,13 +737,14 @@ void RenderFrameHostManager::BeginNavigation(const NavigationRequest& request) {
   // navigations.
   CleanUpNavigation();
 
-  RenderFrameHostImpl* dest_rfh = GetFrameHostForNavigation(request);
+  RenderFrameHostImpl* dest_rfh = GetFrameHostForNavigation(request, false);
   DCHECK(dest_rfh);
 }
 
 // PlzNavigate
 RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
-    const NavigationRequest& request) {
+    const NavigationRequest& request,
+    bool is_for_commit) {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
 
@@ -827,7 +834,14 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
     }
   }
 
-  cross_navigation_pending_ = navigation_rfh != render_frame_host_.get();
+  // Register that a commit is pending.
+  if (is_for_commit) {

[Charlie Reis, 2015/04/03 06:02:58]

This is only used at the end of the method and doesn't affect the execution of
the code before it.  That means we could put it in a separate method that is
called afterward in the case of commit, rather than passing a boolean to turn
this piece on or off.

I'd much prefer that, since it reduces the number of tasks this method is
responsible for and makes it easier to understand.

+    if (navigation_rfh == render_frame_host_.get())
+      pending_commit_current_frame_ = true;
+    else
+      pending_commit_speculative_frame_ = true;
+  }
+
   return navigation_rfh;
 }
 
@@ -837,6 +851,7 @@ void RenderFrameHostManager::CleanUpNavigation() {
       switches::kEnableBrowserSideNavigation));
   speculative_web_ui_.reset();
   should_reuse_web_ui_ = false;
+  pending_commit_speculative_frame_ = false;

[Charlie Reis, 2015/04/03 06:02:59]

This is where your comment about it being impossible to cancel
pending_commit_current_frame_ would belong, but I'd rather remove that field
entirely.

   if (speculative_render_frame_host_)
     DiscardUnusedFrame(UnsetSpeculativeRenderFrameHost());
 }
@@ -1671,6 +1686,7 @@ void RenderFrameHostManager::CommitPending() {
     DCHECK(speculative_render_frame_host_);
     old_render_frame_host =
         SetRenderFrameHost(speculative_render_frame_host_.Pass());
+    pending_commit_speculative_frame_ = false;
   }
   cross_navigation_pending_ = false;