Solidify Entry discarding logic (NavigationHandle keeps its ID)

content/browser/frame_host/navigator_impl.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/navigator_impl.h"
 
 #include <utility>
 
 #include "base/metrics/histogram.h"
 #include "base/time/time.h"
@@ skipping 134 matching lines @@
       return;
     }
 
     // This ensures that notifications about the end of the previous
     // navigation are sent before notifications about the start of the
     // new navigation.
     render_frame_host->SetNavigationHandle(scoped_ptr<NavigationHandleImpl>());
   }
 
   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(
     RenderFrameHostImpl* render_frame_host,
     const FrameHostMsg_DidFailProvisionalLoadWithError_Params& params) {
   VLOG(1) << "Failed Provisional Load: " << params.url.possibly_invalid_spec()
           << ", error_code: " << params.error_code
           << ", error_description: " << params.error_description
           << ", showing_repost_interstitial: " <<
             params.showing_repost_interstitial
@@ skipping 26 matching lines @@
 
     // We used to cancel the pending renderer here for cross-site downloads.
     // However, it's not safe to do that because the download logic repeatedly
     // looks for this WebContents based on a render ID.  Instead, we just
     // leave the pending renderer around until the next navigation event
     // (Navigate, DidNavigate, etc), which will clean it up properly.
     //
     // 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);
+  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);
+      // Also force the UI to refresh.
+      controller_->delegate()->NotifyNavigationStateChanged(
+          INVALIDATE_TYPE_URL);
+    }
   }
 
   if (delegate_)
     delegate_->DidFailProvisionalLoadWithError(render_frame_host, params);
 }
 
 void NavigatorImpl::DidFailLoadWithError(
     RenderFrameHostImpl* render_frame_host,
     const GURL& url,
     int error_code,
@@ skipping 531 matching lines @@
   }
 
   // In all other cases the current navigation, if any, is canceled and a new
   // NavigationRequest is created for the node.
   frame_tree_node->CreatedNavigationRequest(
       NavigationRequest::CreateRendererInitiated(
           frame_tree_node, common_params, begin_params, body,
           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
     // same code path as browser-initiated navigations. For renderer-initiated
     // main frame navigation that start via a BeginNavigation IPC, the
     // RenderFrameHost will not be swapped. Therefore it is safe to call
     // DidStartMainFrameNavigation with the SiteInstance from the current
     // RenderFrameHost.
     DidStartMainFrameNavigation(
         common_params.url,
         frame_tree_node->current_frame_host()->GetSiteInstance());
     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();
 }
 
 // PlzNavigate
 void NavigatorImpl::CommitNavigation(FrameTreeNode* frame_tree_node,
                                      ResourceResponse* response,
                                      scoped_ptr<StreamHandle> body) {
   CHECK(IsBrowserSideNavigationEnabled());
 
   NavigationRequest* navigation_request = frame_tree_node->navigation_request();
@@ skipping 132 matching lines @@
   bool should_dispatch_beforeunload =
       frame_tree_node->current_frame_host()->ShouldDispatchBeforeUnload();
   FrameMsg_Navigate_Type::Value navigation_type =
       GetNavigationType(controller_->GetBrowserContext(), entry, reload_type);
   frame_tree_node->CreatedNavigationRequest(
       NavigationRequest::CreateBrowserInitiated(
           frame_tree_node, dest_url, dest_referrer, frame_entry, entry,
           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
   // is synchronous and same-site) then NavigationRequest::BeginNavigation
   // should be directly called instead.
   if (should_dispatch_beforeunload &&
       ShouldMakeNetworkRequestForURL(
           navigation_request->common_params().url)) {
     navigation_request->SetWaitingForRendererResponse();
     frame_tree_node->current_frame_host()->DispatchBeforeUnload(true);
@@ skipping 83 matching lines @@
       entry->set_should_replace_entry(pending_entry->should_replace_entry());
       entry->SetRedirectChain(pending_entry->GetRedirectChain());
     }
     controller_->SetPendingEntry(std::move(entry));
     if (delegate_)
       delegate_->NotifyChangedNavigationState(content::INVALIDATE_TYPE_URL);
   }
 }
 
 }  // namespace content