PlzNavigate: Improvements to RFHM commit logic.

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..0871632ba34d4e5c12b77abef334bbb868d35c8c 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -460,9 +460,18 @@ void RenderFrameHostManager::CommitPendingIfNecessary(
     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);
@@ -1637,8 +1646,15 @@ void RenderFrameHostManager::CommitPending() {
     }
   } 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.

   }