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..5efea93bb82e82f63a796e2205d7249d2a879de6 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -460,9 +460,21 @@ 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_);

[Charlie Reis, 2015/04/02 03:18:36]

DCHECK_IMPLIES(should_reuse_web_ui_, web_ui_);

(This is a new macro I just learned about, which makes the intention a bit
clearer.)

[carlosk, 2015/04/07 14:42:31]

Done.

+      // 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.
+      // TODO(carlosk): this code might not handle interwoven navigation
+      // requests properly. For example, if two navigations to the current
+      // RenderFrameHost are requested closely to each other, one requiring a
+      // new WebUI and the other requiring none, it might happen that a WebUI is

[Charlie Reis, 2015/04/02 03:18:36]

Can there actually be two navigations in the same RenderFrameHost where one has
a WebUI and one doesn't?  It sounds like a security bug to me to lose WebUI but
stay in the same RenderFrameHost and process.  Maybe it's possible to have a
different WebUI object in the same RFH but not none?

In the reverse direction, RenderViewHostImpl::AllowBindings prevents us from
granting WebUI bindings to a process that has already been used for non-WebUI
pages.

[carlosk, 2015/04/07 14:42:31]

I looked through the code and had the impression from
WebUIControllerFactoryRegistry::IsURLAcceptableForWebUI that it would be
possible (as it for instance allows about:blank pages to reuse a RFH with a
WebUI). But I just tried and was unable to create a test with that behavior (a
speculative RFH was always created).

So I removed that comment and following your suggestion below I merged the
PlzNavigate and regular navigation cases.

+      // set when it shouldn't or the other way around.

[Charlie Reis, 2015/04/02 03:18:36]

If this is a real issue, what's the plan for fixing this TODO?  Setting a WebUI
when there shouldn't be one could be a critical severity security bug (i.e.,
potential sandbox escape), so we can't turn on PlzNavigate while this is
possible.

(If this is real issue, let's mention a bug number here so we can track the work
to fix it.)

[carlosk, 2015/04/07 14:42:31]

My previous comment covers this. 

+      if (speculative_web_ui_ && !speculative_render_frame_host_) {
+        CommitPending();

[Charlie Reis, 2015/04/02 03:18:36]

I'm noticing that this ends up echoing a lot of the code below for the
non-PlzNavigate case.  For example, we call CommitPending() when there's a
pending_web_ui() and no pending_render_frame_host_ on line 494.

Is there a reasonable way to reuse the code rather than having two separate
implementations each addressing subtle cases like this?  I worry about them
diverging if we fix a bug in one but not the other.

(Or is there a good reason to keep the paths separate?)

[carlosk, 2015/04/07 14:42:31]

Done. As we plan to eliminate |cross_navigation_pending_| I added a note on that
for the PlzNavigate case.

I used to keep them separate to minimize the code paths that would touch
independent members from either navigation implementation. So that when we would
have to eliminate the current implementation we'd basically delete the other
side and remove the overhanging if-PlzNavigate.

But I agree your concern on code maintenance is more relevant.

+      } else {
+        CleanUpNavigation();
+      }
     } else {
       // No one else should be sending us a DidNavigate in this state.
       DCHECK(false);
@@ -1637,9 +1649,17 @@ void RenderFrameHostManager::CommitPending() {
     }
   } else {
     // PlzNavigate
-    if (!should_reuse_web_ui_)
+    if (speculative_web_ui_) {

[Charlie Reis, 2015/04/02 03:18:36]

Again, these new cases look a lot like lines 1642-1649, with different names and
slightly different guards.  Is there a way to share the logic so that we don't
diverge, or does that lead to problems?

[carlosk, 2015/04/07 14:42:31]

Done. No problems just the need for some extra PlzNavigate-enabled checks.

+      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_);
+    DCHECK(!should_reuse_web_ui_);
   }
 
   // It's possible for the pending_render_frame_host_ to be nullptr when we