PlzNavigate: track pending commits

content/browser/frame_host/render_frame_host_manager.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_manager.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/logging.h"
@@ skipping 45 matching lines @@
     RenderWidgetHostDelegate* render_widget_delegate,
     Delegate* delegate)
     : frame_tree_node_(frame_tree_node),
       delegate_(delegate),
       cross_navigation_pending_(false),
       render_frame_delegate_(render_frame_delegate),
       render_view_delegate_(render_view_delegate),
       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_);
 }
 
 RenderFrameHostManager::~RenderFrameHostManager() {
   if (pending_render_frame_host_)
     UnsetPendingRenderFrameHost();
 
   if (speculative_render_frame_host_)
     UnsetSpeculativeRenderFrameHost();
@@ skipping 375 matching lines @@
   // prior to navigation take effect.
   CommitPendingSandboxFlags();
 }
 
 void RenderFrameHostManager::CommitPendingIfNecessary(
     RenderFrameHostImpl* render_frame_host,
     bool was_caused_by_user_gesture) {
   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;
   }
 
   if (!cross_navigation_pending_) {
     DCHECK(!pending_render_frame_host_);
 
     // We should only hear this from our current renderer.
     DCHECK_EQ(render_frame_host_, render_frame_host);
 
@@ skipping 244 matching lines @@
 
 // PlzNavigate
 void RenderFrameHostManager::BeginNavigation(const NavigationRequest& request) {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   // Clean up any state in case there's an ongoing navigation.
   // TODO(carlosk): remove this cleanup here once we properly cancel ongoing
   // 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));
 
   SiteInstance* current_site_instance = render_frame_host_->GetSiteInstance();
 
   scoped_refptr<SiteInstance> dest_site_instance = GetSiteInstanceForNavigation(
       request.common_params().url, request.source_site_instance(),
       request.dest_site_instance(), request.common_params().transition,
       request.restore_type() != NavigationEntryImpl::RESTORE_NONE,
       request.is_view_source());
@@ skipping 69 matching lines @@
   if (!navigation_rfh->IsRenderFrameLive()) {
     // Recreate the opener chain.
     int opener_route_id = delegate_->CreateOpenerRenderViewsForRenderManager(
         navigation_rfh->GetSiteInstance());
     if (!InitRenderView(navigation_rfh->render_view_host(), opener_route_id,
                         MSG_ROUTING_NONE, frame_tree_node_->IsMainFrame())) {
       return nullptr;
     }
   }
 
-  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;
 }
 
 // PlzNavigate
 void RenderFrameHostManager::CleanUpNavigation() {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       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());
 }
 
 // PlzNavigate
 scoped_ptr<RenderFrameHostImpl>
 RenderFrameHostManager::UnsetSpeculativeRenderFrameHost() {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   speculative_render_frame_host_->GetProcess()->RemovePendingView();
@@ skipping 814 matching lines @@
   scoped_ptr<RenderFrameHostImpl> old_render_frame_host;
   if (!browser_side_navigation) {
     DCHECK(!speculative_render_frame_host_);
     old_render_frame_host =
         SetRenderFrameHost(pending_render_frame_host_.Pass());
   } else {
     // PlzNavigate
     DCHECK(speculative_render_frame_host_);
     old_render_frame_host =
         SetRenderFrameHost(speculative_render_frame_host_.Pass());
+    pending_commit_speculative_frame_ = false;
   }
   cross_navigation_pending_ = false;
 
   if (is_main_frame)
     render_frame_host_->render_view_host()->AttachToFrameTree();
 
   // The process will no longer try to exit, so we can decrement the count.
   render_frame_host_->GetProcess()->RemovePendingView();
 
   // Show the new view (or a sad tab) if necessary.
@@ skipping 335 matching lines @@
 void RenderFrameHostManager::DeleteRenderFrameProxyHost(
     SiteInstance* instance) {
   RenderFrameProxyHostMap::iterator iter = proxy_hosts_.find(instance->GetId());
   if (iter != proxy_hosts_.end()) {
     delete iter->second;
     proxy_hosts_.erase(iter);
   }
 }
 
 }  // namespace content