Fix race when reloading original URL

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 "base/command_line.h"
 #include "base/metrics/histogram.h"
 #include "base/time/time.h"
 #include "content/browser/frame_host/frame_tree.h"
@@ skipping 215 matching lines @@
 bool NavigatorImpl::NavigateToEntry(
     FrameTreeNode* frame_tree_node,
     const FrameNavigationEntry& frame_entry,
     const NavigationEntryImpl& entry,
     NavigationController::ReloadType reload_type,
     bool is_same_document_history_load) {
   TRACE_EVENT0("browser,navigation", "NavigatorImpl::NavigateToEntry");
 
   // The renderer will reject IPC messages with URLs longer than
   // this limit, so don't attempt to navigate with a longer URL.
   if (frame_entry.url().spec().size() > GetMaxURLChars()) {

[Charlie Reis, 2015/07/08 23:23:37]

There's a lot of frame_entry.url() uses in here that are all wrong in this case.

Can we move the reloadOriginalURL check from ConstructCommonNavigationParams to
the top of this method and create a dest_url and dest_referrer to use below? 
We'll have to pass them to RequestNavigation and
ConstructCommonNavigationParams, and they'll trump whatever is in frame_entry.

[lfg, 2015/07/14 18:58:31]

Done.

     LOG(WARNING) << "Refusing to load URL as it exceeds " << GetMaxURLChars()
                  << " characters.";
     return false;
   }
 
   // This will be used to set the Navigation Timing API navigationStart
   // parameter for browser navigations in new tabs (intents, tabs opened through
   // "Open link in new tab"). We need to keep it above RFHM::Navigate() call to
   // capture the time needed for the RenderFrameHost initialization.
   base::TimeTicks navigation_start = base::TimeTicks::Now();
 
   RenderFrameHostManager* manager = frame_tree_node->render_manager();
 
   // PlzNavigate: the RenderFrameHosts are no longer asked to navigate.
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableBrowserSideNavigation)) {
     navigation_data_.reset(new NavigationMetricsData(
         navigation_start, frame_entry.url(), entry.restore_type()));

[Charlie Reis, 2015/07/08 23:23:37]

dest_url

[lfg, 2015/07/14 18:58:31]

Done.

     RequestNavigation(frame_tree_node, frame_entry, entry, reload_type,
                       is_same_document_history_load, navigation_start);
 
     // Notify observers about navigation.
     if (delegate_) {
-      delegate_->DidStartNavigationToPendingEntry(frame_entry.url(),
-                                                  reload_type);
+      delegate_->DidStartNavigationToPendingEntry(
+          frame_tree_node->navigation_request()->common_params().url,

[Charlie Reis, 2015/07/08 23:23:37]

dest_url

[lfg, 2015/07/14 18:58:31]

Done.

+          reload_type);
     }
 
     return true;
   }
 
   RenderFrameHostImpl* dest_render_frame_host =
       manager->Navigate(frame_entry, entry);
   if (!dest_render_frame_host)
     return false;  // Unable to create the desired RenderFrameHost.
 
   // Make sure no code called via RFHM::Navigate clears the pending entry.
   CHECK_EQ(controller_->GetPendingEntry(), &entry);
 
   // For security, we should never send non-Web-UI URLs to a Web UI renderer.
   // Double check that here.
   CheckWebUIRendererDoesNotDisplayNormalURL(dest_render_frame_host,
                                             frame_entry.url());

[Charlie Reis, 2015/07/08 23:23:37]

dest_url

[lfg, 2015/07/14 18:58:31]

Done.

 
   // Notify observers that we will navigate in this RenderFrame.
   if (delegate_) {
     delegate_->AboutToNavigateRenderFrame(frame_tree_node->current_frame_host(),
                                           dest_render_frame_host);
   }
 
   // Navigate in the desired RenderFrameHost.
   // We can skip this step in the rare case that this is a transfer navigation
   // which began in the chosen RenderFrameHost, since the request has already
   // been issued.  In that case, simply resume the response.
   bool is_transfer_to_same =
       entry.transferred_global_request_id().child_id != -1 &&
       entry.transferred_global_request_id().child_id ==
           dest_render_frame_host->GetProcess()->GetID();
+  GURL pending_url = frame_entry.url();
   if (!is_transfer_to_same) {
     navigation_data_.reset(new NavigationMetricsData(
         navigation_start, frame_entry.url(), entry.restore_type()));
     // Create the navigation parameters.
     FrameMsg_Navigate_Type::Value navigation_type =
         GetNavigationType(controller_->GetBrowserContext(), entry, reload_type);
+    CommonNavigationParams common_navigation_params =
+        entry.ConstructCommonNavigationParams(frame_entry, navigation_type);
+    pending_url = common_navigation_params.url;
     dest_render_frame_host->Navigate(
-        entry.ConstructCommonNavigationParams(frame_entry, navigation_type),
-        entry.ConstructStartNavigationParams(),
+        common_navigation_params, entry.ConstructStartNavigationParams(),
         entry.ConstructRequestNavigationParams(
             frame_entry, navigation_start, is_same_document_history_load,
             controller_->HasCommittedRealLoad(frame_tree_node),
             controller_->GetPendingEntryIndex() == -1,
             controller_->GetIndexOfEntry(&entry),
             controller_->GetLastCommittedEntryIndex(),
             controller_->GetEntryCount()));
   } else {
     // No need to navigate again.  Just resume the deferred request.
     dest_render_frame_host->GetProcess()->ResumeDeferredNavigation(
         entry.transferred_global_request_id());
   }
 
   // Make sure no code called via RFH::Navigate clears the pending entry.
   CHECK_EQ(controller_->GetPendingEntry(), &entry);
 
   if (controller_->GetPendingEntryIndex() == -1 &&
       frame_entry.url().SchemeIs(url::kJavaScriptScheme)) {

[Charlie Reis, 2015/07/08 23:23:37]

dest_url

(Not sure if javascript: URLs can actually be in redirect chains, but we should
be consistent here.)

[lfg, 2015/07/14 18:58:31]

Done.

     // If the pending entry index is -1 (which means a new navigation rather
     // than a history one), and the user typed in a javascript: URL, don't add
     // it to the session history.
     //
     // This is a hack. What we really want is to avoid adding to the history any
     // URL that doesn't generate content, and what would be great would be if we
     // had a message from the renderer telling us that a new page was not
     // created. The same message could be used for mailto: URLs and the like.
     return false;
   }
 
   // Notify observers about navigation.
   if (delegate_) {
-    delegate_->DidStartNavigationToPendingEntry(frame_entry.url(), reload_type);
+    delegate_->DidStartNavigationToPendingEntry(pending_url, reload_type);
   }
 
   return true;
 }
 
 bool NavigatorImpl::NavigateToPendingEntry(
     FrameTreeNode* frame_tree_node,
     const FrameNavigationEntry& frame_entry,
     NavigationController::ReloadType reload_type,
     bool is_same_document_history_load) {
@@ skipping 559 matching lines @@
       entry->set_should_replace_entry(pending_entry->should_replace_entry());
       entry->SetRedirectChain(pending_entry->GetRedirectChain());
     }
     controller_->SetPendingEntry(entry.Pass());
     if (delegate_)
       delegate_->NotifyChangedNavigationState(content::INVALIDATE_TYPE_URL);
   }
 }
 
 }  // namespace content