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 133 matching lines @@
       render_frame_host->navigation_handle()->set_is_transferring(false);
       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>());
   }
 
+  NavigationEntry* pending_entry = controller_->GetPendingEntry();
   render_frame_host->SetNavigationHandle(NavigationHandleImpl::Create(
       validated_url, render_frame_host->frame_tree_node(),
       false,             // is_synchronous
       is_iframe_srcdoc,  // is_srcdoc
-      navigation_start));
+      navigation_start,
+      pending_entry ? pending_entry->GetUniqueID() : 0));
 }
 
 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 19 matching lines @@
     // commits of the interstitial page.
     FrameTreeNode* root =
         render_frame_host->frame_tree_node()->frame_tree()->root();
     if (root->render_manager()->interstitial_page() != NULL) {
       LOG(WARNING) << "Discarding message during interstitial.";
       return;
     }
 
     // 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
+    // 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);
+  // Racy conditions can cause a 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.
+  NavigationHandleImpl* handle = render_frame_host->navigation_handle();
+  NavigationEntry* pending_entry = controller_->GetPendingEntry();
+  bool pending_matches_fail_msg =
+      handle && pending_entry &&
+      handle->pending_nav_entry_id() == pending_entry->GetUniqueID();
+  if (pending_matches_fail_msg) {
+    // 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() ||

[Charlie Reis, 2016/02/16 21:55:07]

Sanity check: If I remember correctly this line isn't critical anymore because
of the check we've added to RendererDidNavigateToNewPage's clone logic, but
we're keeping this around for now to be safe.  (Maybe add a TODO for me to clean
it up later?)

[Charlie Harrison, 2016/02/16 22:55:22]

Done.

+        !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 532 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. The pending entry is guaranteed to be
+  // non null at this point.
+  DCHECK(controller_->GetPendingEntry());
+  navigation_request->CreateNavigationHandle(
+      controller_->GetPendingEntry()->GetUniqueID());
   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 133 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, lofi_state, is_same_document_history_load,
           navigation_start, controller_));
   NavigationRequest* navigation_request = frame_tree_node->navigation_request();
-  navigation_request->CreateNavigationHandle();
+  navigation_request->CreateNavigationHandle(entry.GetUniqueID());
 
   // 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