Solidify Entry discarding logic (NavigationHandle keeps its ID)

content/browser/frame_host/navigator_impl.cc

Index: content/browser/frame_host/navigator_impl.cc
diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc
index 960ef9150e66aee0847045a0bcaaf7df3e28bf35..24365cf3d173739f5f654b6693e6836b7502f281 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -152,7 +152,8 @@ void NavigatorImpl::DidStartProvisionalLoad(
   }
 
   render_frame_host->SetNavigationHandle(NavigationHandleImpl::Create(
-      validated_url, render_frame_host->frame_tree_node(), navigation_start));
+      validated_url, render_frame_host->frame_tree_node(), navigation_start,
+      controller_->GetPendingEntry()));
 }
 
 void NavigatorImpl::DidFailProvisionalLoadWithError(
@@ -199,29 +200,44 @@ void NavigatorImpl::DidFailProvisionalLoadWithError(
     // TODO(creis): Find a way to cancel any pending RFH here.
   }
 
-  // We usually clear the pending entry when it fails, so that an arbitrary URL
-  // isn't left visible above a committed page.  This must be enforced when
-  // the pending entry isn't visible (e.g., renderer-initiated navigations) to
-  // prevent URL spoofs for in-page navigations that don't go through
-  // DidStartProvisionalLoadForFrame.
-  //
-  // However, we do preserve the pending entry in some cases, such as on the
-  // initial navigation of an unmodified blank tab.  We also allow the delegate
-  // to say when it's safe to leave aborted URLs in the omnibox, to let the user
-  // edit the URL and try again.  This may be useful in cases that the committed
-  // page cannot be attacker-controlled.  In these cases, we still allow the
-  // view to clear the pending entry and typed URL if the user requests
-  // (e.g., hitting Escape with focus in the address bar).
-  //
-  // Note: don't touch the transient entry, since an interstitial may exist.
-  bool should_preserve_entry = controller_->IsUnmodifiedBlankTab() ||
-      delegate_->ShouldPreserveAbortedURLs();
-  if (controller_->GetPendingEntry() != controller_->GetVisibleEntry() ||
-      !should_preserve_entry) {
-    controller_->DiscardPendingEntry(true);
-
-    // Also force the UI to refresh.
-    controller_->delegate()->NotifyNavigationStateChanged(INVALIDATE_TYPE_URL);
+  NavigationHandleImpl* handle = render_frame_host->navigation_handle();

[Charlie Reis, 2016/02/06 00:54:58]

nit: Please put these declarations below the big comment, since the comment
helps explain them.

[Charlie Harrison, 2016/02/08 15:06:42]

Done.

+  NavigationEntry* pending_entry = controller_->GetPendingEntry();
+  bool pending_matches_fail_msg =
+      handle && pending_entry &&
+      handle->pending_nav_entry_id() == pending_entry->GetUniqueID();
+  // Another complicated case here is the fact that racy conditions can cause a

[Charlie Reis, 2016/02/06 00:54:58]

nit: Drop "Another complicated case here is the fact that"

(This comes before the rest of the explanation, so using "Another" doesn't make
sense to the reader.  Clearer to just start with "Racy conditions can cause...")

[Charlie Harrison, 2016/02/08 15:06:42]

Done.

+  // fail message to arrive after its corresponding pending entry has been
+  // replaced by another navigation. If |DiscardPendingEntry| is called in this
+  // case, then the completely valid entry for the new navigation would be
+  // discarded. See crbug.com/513742. To catch this case, the current pending
+  // entry is compared against the current navigation handle's entry id, which
+  // should correspond to the failed load.
+  if (pending_matches_fail_msg) {

[Charlie Reis, 2016/02/06 00:54:58]

Sanity check: I'm a bit nervous about unintentionally allowing URL spoofs if an
attacker can somehow clear the handle or make it not match the pending entry. 
In that case, the pending entry would be left around when we otherwise would
have discarded it.

(I'm also trying to remember the attack that's possible if the pending entry
doesn't match the visible entry and we leave the pending entry around.  I might
have to dig through source history to remind myself.)

I'm hoping we can verify that there isn't an attack possible here.

+    // We usually clear the pending entry when it fails, so that an arbitrary
+    // URL isn't left visible above a committed page.  This must be enforced
+    // when the pending entry isn't visible (e.g., renderer-initiated
+    // navigations) to prevent URL spoofs for in-page navigations that don't go
+    // through DidStartProvisionalLoadForFrame.
+    //
+    // However, we do preserve the pending entry in some cases, such as on the
+    // initial navigation of an unmodified blank tab.  We also allow the
+    // delegate to say when it's safe to leave aborted URLs in the omnibox, to
+    // let the user edit the URL and try again.  This may be useful in cases
+    // that the committed page cannot be attacker-controlled.  In these cases,
+    // we still allow the view to clear the pending entry and typed URL if the
+    // user requests (e.g., hitting Escape with focus in the address bar).
+    //
+    // Note: don't touch the transient entry, since an interstitial may exist.
+    bool should_preserve_entry = controller_->IsUnmodifiedBlankTab() ||
+                                 delegate_->ShouldPreserveAbortedURLs();
+    if (pending_entry != controller_->GetVisibleEntry() ||
+        !should_preserve_entry) {
+      controller_->DiscardPendingEntry(true);
+
+      // Also force the UI to refresh.
+      controller_->delegate()->NotifyNavigationStateChanged(
+          INVALIDATE_TYPE_URL);
+    }
   }
 
   if (delegate_)
@@ -773,8 +789,6 @@ void NavigatorImpl::OnBeginNavigation(
           controller_->GetLastCommittedEntryIndex(),
           controller_->GetEntryCount()));
   NavigationRequest* navigation_request = frame_tree_node->navigation_request();
-  navigation_request->CreateNavigationHandle();
-
   if (frame_tree_node->IsMainFrame()) {
     // Renderer-initiated main-frame navigations that need to swap processes
     // will go to the browser via a OpenURL call, and then be handled by the
@@ -789,6 +803,10 @@ void NavigatorImpl::OnBeginNavigation(
     navigation_data_.reset();
   }
 
+  // The NavigationHandle must be created after the call to
+  // |DidStartMainFrameNavigation|, so it receives the most up to date pending
+  // entry from the NavigationController.
+  navigation_request->CreateNavigationHandle(controller_->GetPendingEntry());
   navigation_request->BeginNavigation();
 }
 
@@ -941,7 +959,7 @@ void NavigatorImpl::RequestNavigation(
           navigation_type, is_same_document_history_load, navigation_start,
           controller_));
   NavigationRequest* navigation_request = frame_tree_node->navigation_request();
-  navigation_request->CreateNavigationHandle();
+  navigation_request->CreateNavigationHandle(&entry);
 
   // Have the current renderer execute its beforeunload event if needed. If it
   // is not needed (when beforeunload dispatch is not needed or this navigation