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 442 matching lines @@
 }
 
 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_);
+      DCHECK(!should_reuse_web_ui_ || web_ui_);
+      // When the current RenderFrameHost is committing there's still the
+      // possibility that there is a speculative WebUI to be made active.
+      // But if there's also a speculative RenderFrameHost set it means the
+      // WebUI was not meant for the current one.

[clamy, 2015/03/31 15:54:10]

The last sentence is not true. The speculative web ui could be for the
speculative RenderFrameHost, which I guess is a problem in the patch as well.
This should be fixed before landing the patch.

[carlosk, 2015/04/01 14:54:07]

If both the speculative RFH *and* WebUI are set then it's certain the two belong
together. That's what I meant in the comment and that's what the if-check below
intends to detect.

What might still happen is that if two navigations, both to the current RFH,
requested closely to each other, one requiring a new WebUI and the other
requiring none, then we might end up having a WebUI set when we shouldn't or the
other way around. I added that example to the existing TODO.

The same problem can happen with the current navigation implementation so this
patch does not regress on that point. And the TODO is there so that we don't
forget to deal with it.

+      // TODO(carlosk): this code might not handle interwoven navigation
+      // requests properly.
+      if (speculative_web_ui_ && !speculative_render_frame_host_) {
+        CommitPending();
+      } else {
+        CleanUpNavigation();
+      }
     } else {
       // No one else should be sending us a DidNavigate in this state.
       DCHECK(false);
     }
     DCHECK(!speculative_render_frame_host_);
     return;
   }
 
   if (!cross_navigation_pending_) {
     DCHECK(!pending_render_frame_host_);
@@ skipping 1154 matching lines @@
     if (pending_web_ui_) {
       web_ui_.reset(pending_web_ui_.release());
     } else if (!pending_and_current_web_ui_.get()) {
       web_ui_.reset();
     } 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_)
+    if (speculative_web_ui_) {
+      DCHECK(!should_reuse_web_ui_);
       web_ui_.reset(speculative_web_ui_.release());
+    } else if (should_reuse_web_ui_) {
+      DCHECK(web_ui_);
+      should_reuse_web_ui_ = false;
+    } else {
+      web_ui_.reset();
+    }
     DCHECK(!speculative_web_ui_);

[clamy, 2015/03/31 15:54:10]

Could you also add a DCHECK(!should_reuse_web_ui_)?

[carlosk, 2015/04/01 14:54:07]

Done.

   }
 
   // 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 366 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