PlzNavigate: Improvements to RFHM commit logic.

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 437 matching lines @@
   CommitPendingIfNecessary(render_frame_host, was_caused_by_user_gesture);
 
   // Make sure any dynamic changes to this frame's sandbox flags that were made
   // 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()) {
-      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(!speculative_render_frame_host_);
-    } else {
-      // No one else should be sending us a DidNavigate in this state.
-      DCHECK(false);
-    }
+  // Note: In PlzNavigate |cross_navigation_pending_| being false means there is
+  // *no* speculative RenderFrameHost set.
+  if (!cross_navigation_pending_) {
     DCHECK(!speculative_render_frame_host_);
-    return;
-  }
-
-  if (!cross_navigation_pending_) {
     DCHECK(!pending_render_frame_host_);
+    DCHECK_IMPLIES(should_reuse_web_ui_, web_ui_);
 
     // We should only hear this from our current renderer.
     DCHECK_EQ(render_frame_host_, render_frame_host);
 
     // Even when there is no pending RVH, there may be a pending Web UI.
-    if (pending_web_ui())
+    if (pending_web_ui() || speculative_web_ui_)
       CommitPending();
     return;
   }
 
-  if (render_frame_host == pending_render_frame_host_) {
+  if (render_frame_host == pending_render_frame_host_ ||
+      render_frame_host == speculative_render_frame_host_) {
     // The pending cross-site navigation completed, so show the renderer.
     CommitPending();
   } else if (render_frame_host == render_frame_host_) {
-    if (was_caused_by_user_gesture) {
-      // A navigation in the original page has taken place.  Cancel the pending
-      // one. Only do it for user gesture originated navigations to prevent
-      // page doing any shenanigans to prevent user from navigating.
-      // See https://code.google.com/p/chromium/issues/detail?id=75195
-      CancelPending();
-      cross_navigation_pending_ = false;
+    if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+            switches::kEnableBrowserSideNavigation)) {
+      CleanUpNavigation();
+    } else {
+      if (was_caused_by_user_gesture) {

[Charlie Reis, 2015/04/07 23:15:05]

Should the PlzNavigate CleanUpNavigation call be inside the
was_caused_by_user_gesture check?

Suppose the current frame does an in-page navigation (e.g., pushState) without a
user gesture, which commits without going through the network stack.  We don't
want that to cancel the user's ongoing navigation.

Does PlzNavigate handle that differently?

[carlosk, 2015/04/08 14:43:45]

It is handled differently by PlzNavigate. The decision of canceling or not on
navigation for that specific case is taken in NavigatorImpl::OnBeginNavigation
(see line 652), which is the entry points for renderer initiated navigation
requests.

This cleanup here is intended to properly handle cases of interwoven navigation
requests: the current RFH committed even though there's an ongoing cross-site
navigation (started after the RFH was already committing). So we cancel the
latter.

+        // A navigation in the original page has taken place.  Cancel the
+        // pending one. Only do it for user gesture originated navigations to
+        // prevent page doing any shenanigans to prevent user from navigating.
+        // See https://code.google.com/p/chromium/issues/detail?id=75195
+        CancelPending();
+        cross_navigation_pending_ = false;
+      }
     }
   } else {
     // No one else should be sending us DidNavigate in this state.
     DCHECK(false);
   }
 }
 
 void RenderFrameHostManager::DidDisownOpener(
     RenderFrameHost* render_frame_host) {
   // Notify all RenderFrameHosts but the one that notified us. This is necessary
@@ skipping 1114 matching lines @@
                "FrameTreeNode id", frame_tree_node_->frame_tree_node_id());
   bool browser_side_navigation =
       base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableBrowserSideNavigation);
   // First check whether we're going to want to focus the location bar after
   // this commit.  We do this now because the navigation hasn't formally
   // committed yet, so if we've already cleared |pending_web_ui_| the call chain
   // this triggers won't be able to figure out what's going on.
   bool will_focus_location_bar = delegate_->FocusLocationBarByDefault();
 
-  if (!browser_side_navigation) {
-    DCHECK(!speculative_web_ui_);
-    // Next commit the Web UI, if any. Either replace |web_ui_| with
-    // |pending_web_ui_|, or clear |web_ui_| if there is no pending WebUI, or
-    // leave |web_ui_| as is if reusing it.
-    DCHECK(!(pending_web_ui_.get() && pending_and_current_web_ui_.get()));
-    if (pending_web_ui_) {
-      web_ui_.reset(pending_web_ui_.release());
-    } else if (!pending_and_current_web_ui_.get()) {
-      web_ui_.reset();
+  // Next commit the Web UI, if any. Either replace |web_ui_| with
+  // |pending_web_ui_|, or clear |web_ui_| if there is no pending WebUI, or
+  // leave |web_ui_| as is if reusing it.
+  DCHECK(!(pending_web_ui_ && pending_and_current_web_ui_));
+  if (pending_web_ui_ || speculative_web_ui_) {
+    DCHECK(!should_reuse_web_ui_);
+    web_ui_.reset(browser_side_navigation ? speculative_web_ui_.release()
+                                          : pending_web_ui_.release());
+  } else if (pending_and_current_web_ui_ || should_reuse_web_ui_) {
+    if (browser_side_navigation) {
+      DCHECK(web_ui_);
+      should_reuse_web_ui_ = false;
     } else {
       DCHECK_EQ(pending_and_current_web_ui_.get(), web_ui_.get());
       pending_and_current_web_ui_.reset();
     }
   } else {
-    // PlzNavigate
-    if (!should_reuse_web_ui_)
-      web_ui_.reset(speculative_web_ui_.release());
-    DCHECK(!speculative_web_ui_);
+    web_ui_.reset();
   }
+  DCHECK(!speculative_web_ui_);
+  DCHECK(!should_reuse_web_ui_);
 
   // It's possible for the pending_render_frame_host_ to be nullptr when we
   // aren't crossing process boundaries. If so, we just needed to handle the Web
   // UI committing above and we're done.
   if (!pending_render_frame_host_ && !speculative_render_frame_host_) {
     if (will_focus_location_bar)
       delegate_->SetFocusToLocationBar(false);
     return;
   }
 
@@ skipping 377 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