Fix race when reloading original URL.

Fixes an issue where the browser's last committed entry and the renderer can become out of sync when reloading the original requested URL, which causes the browser to kill the renderer process.

BUG=491861
TBR=jam@chromium.org for chromium.fyi.json

Review URL: https://codereview.chromium.org/1189413005

Cr-Commit-Position: refs/heads/master@{#338864}

12 files changed, 137 insertions, 40 deletions

diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index b873319..35a1d07 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -327,16 +327,6 @@ void NavigationControllerImpl::ReloadInternal(bool check_for_repost,
   if (!entry)
     return;
 
-  if (reload_type == NavigationControllerImpl::RELOAD_ORIGINAL_REQUEST_URL &&
-      entry->GetOriginalRequestURL().is_valid() && !entry->GetHasPostData()) {
-    // We may have been redirected when navigating to the current URL.
-    // Use the URL the user originally intended to visit, if it's valid and if a
-    // POST wasn't involved; the latter case avoids issues with sending data to
-    // the wrong page.
-    entry->SetURL(entry->GetOriginalRequestURL());
-    entry->SetReferrer(Referrer());
-  }
-
   if (g_check_for_repost && check_for_repost &&
       entry->GetHasPostData()) {
     // The user is asking to reload a page with POST data. Prompt to make sure
diff --git a/content/browser/frame_host/navigation_controller_impl_browsertest.cc b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
index ba4a49d..c85e5c5 100644
--- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
@@ -5,6 +5,7 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
 #include "content/browser/frame_host/frame_navigation_entry.h"
 #include "content/browser/frame_host/frame_tree.h"
 #include "content/browser/frame_host/navigation_controller_impl.h"
@@ -2130,4 +2131,79 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   ResourceDispatcherHost::Get()->SetDelegate(nullptr);
 }
 
+namespace {
+class RenderProcessKilledObserver : public WebContentsObserver {
+ public:
+  RenderProcessKilledObserver(WebContents* web_contents)
+      : WebContentsObserver(web_contents) {}
+  ~RenderProcessKilledObserver() override {}
+
+  void RenderProcessGone(base::TerminationStatus status) override {
+    CHECK_NE(status,
+             base::TerminationStatus::TERMINATION_STATUS_PROCESS_WAS_KILLED);
+  }
+};
+}
+
+// This tests a race in ReloadOriginalRequest, where a cross-origin reload was
+// causing an in-flight replaceState to look like a cross-origin navigation,
+// even though it's in-page.  (The reload should not modify the underlying last
+// committed entry.)  Not crashing means that the test is successful.
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, ReloadOriginalRequest) {
+  GURL original_url(embedded_test_server()->GetURL(
+      "/navigation_controller/simple_page_1.html"));
+  NavigateToURL(shell(), original_url);
+  FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+                            ->GetFrameTree()
+                            ->root();
+  RenderProcessKilledObserver kill_observer(shell()->web_contents());
+
+  // Redirect so that we can use ReloadOriginalRequest.
+  GURL redirect_url(embedded_test_server()->GetURL(
+      "foo.com", "/navigation_controller/simple_page_1.html"));
+  {
+    std::string script = "location.replace('" + redirect_url.spec() + "');";
+    FrameNavigateParamsCapturer capturer(root);
+    EXPECT_TRUE(ExecuteScript(shell()->web_contents(), script));
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_LINK | ui::PAGE_TRANSITION_CLIENT_REDIRECT,
+              capturer.params().transition);
+    EXPECT_EQ(NAVIGATION_TYPE_EXISTING_PAGE, capturer.details().type);
+  }
+
+  // Modify an entry in the session history and reload the original request.
+  {
+    // We first send a replaceState() to the renderer, which will cause the
+    // renderer to send back a DidCommitProvisionalLoad. Immediately after,
+    // we send a ReloadOriginalRequest (which in this case is a different
+    // origin) and will also cause the renderer to commit the frame. In the
+    // end we verify that both navigations committed and that the URLs are
+    // correct.
+    std::string script = "history.replaceState({}, '', 'foo');";
+    root->render_manager()
+        ->current_frame_host()
+        ->ExecuteJavaScriptWithUserGestureForTests(base::UTF8ToUTF16(script));
+    EXPECT_FALSE(shell()->web_contents()->IsLoading());
+    shell()->web_contents()->GetController().ReloadOriginalRequestURL(false);
+    EXPECT_TRUE(shell()->web_contents()->IsLoading());
+    EXPECT_EQ(redirect_url, shell()->web_contents()->GetLastCommittedURL());
+
+    // Wait until there's no more navigations.
+    GURL modified_url(embedded_test_server()->GetURL(
+        "foo.com", "/navigation_controller/foo"));
+    FrameNavigateParamsCapturer capturer(root);
+    capturer.set_wait_for_load(false);
+    capturer.set_navigations_remaining(2);
+    capturer.Wait();
+    EXPECT_EQ(2U, capturer.all_details().size());
+    EXPECT_EQ(modified_url, capturer.all_params()[0].url);
+    EXPECT_EQ(original_url, capturer.all_params()[1].url);
+    EXPECT_EQ(original_url, shell()->web_contents()->GetLastCommittedURL());
+  }
+
+  // Make sure the renderer is still alive.
+  EXPECT_TRUE(
+      ExecuteScript(shell()->web_contents(), "console.log('Success');"));
+}
+
 }  // namespace content
diff --git a/content/browser/frame_host/navigation_entry_impl.cc b/content/browser/frame_host/navigation_entry_impl.cc
index 5e47912..904a9bf 100644
--- a/content/browser/frame_host/navigation_entry_impl.cc
+++ b/content/browser/frame_host/navigation_entry_impl.cc
@@ -435,6 +435,8 @@ scoped_ptr<NavigationEntryImpl> NavigationEntryImpl::CloneAndReplace(
 }
 
 CommonNavigationParams NavigationEntryImpl::ConstructCommonNavigationParams(
+    const GURL& dest_url,
+    const Referrer& dest_referrer,
     const FrameNavigationEntry& frame_entry,
     FrameMsg_Navigate_Type::Value navigation_type) const {
   FrameMsg_UILoadMetricsReportType::Value report_type =
@@ -447,9 +449,9 @@ CommonNavigationParams NavigationEntryImpl::ConstructCommonNavigationParams(
 #endif
 
   return CommonNavigationParams(
-      frame_entry.url(), frame_entry.referrer(), GetTransitionType(),
-      navigation_type, !IsViewSourceMode(), ui_timestamp, report_type,
-      GetBaseURLForDataURL(), GetHistoryURLForDataURL());
+      dest_url, dest_referrer, GetTransitionType(), navigation_type,
+      !IsViewSourceMode(), ui_timestamp, report_type, GetBaseURLForDataURL(),
+      GetHistoryURLForDataURL());
 }
 
 StartNavigationParams NavigationEntryImpl::ConstructStartNavigationParams()
diff --git a/content/browser/frame_host/navigation_entry_impl.h b/content/browser/frame_host/navigation_entry_impl.h
index e6c7d7c..4254e6a 100644
--- a/content/browser/frame_host/navigation_entry_impl.h
+++ b/content/browser/frame_host/navigation_entry_impl.h
@@ -144,6 +144,8 @@ class CONTENT_EXPORT NavigationEntryImpl
   // Helper functions to construct NavigationParameters for a navigation to this
   // NavigationEntry.
   CommonNavigationParams ConstructCommonNavigationParams(
+      const GURL& dest_url,
+      const Referrer& dest_referrer,
       const FrameNavigationEntry& frame_entry,
       FrameMsg_Navigate_Type::Value navigation_type) const;
   StartNavigationParams ConstructStartNavigationParams() const;
diff --git a/content/browser/frame_host/navigation_request.cc b/content/browser/frame_host/navigation_request.cc
index 1c13a71..37c48d1 100644
--- a/content/browser/frame_host/navigation_request.cc
+++ b/content/browser/frame_host/navigation_request.cc
@@ -53,6 +53,8 @@ int LoadFlagFromNavigationType(FrameMsg_Navigate_Type::Value navigation_type) {
 // static
 scoped_ptr<NavigationRequest> NavigationRequest::CreateBrowserInitiated(
     FrameTreeNode* frame_tree_node,
+    const GURL& dest_url,
+    const Referrer& dest_referrer,
     const FrameNavigationEntry& frame_entry,
     const NavigationEntryImpl& entry,
     FrameMsg_Navigate_Type::Value navigation_type,
@@ -82,7 +84,8 @@ scoped_ptr<NavigationRequest> NavigationRequest::CreateBrowserInitiated(
 
   scoped_ptr<NavigationRequest> navigation_request(new NavigationRequest(
       frame_tree_node,
-      entry.ConstructCommonNavigationParams(frame_entry, navigation_type),
+      entry.ConstructCommonNavigationParams(dest_url, dest_referrer,
+                                            frame_entry, navigation_type),
       BeginNavigationParams(method, headers.ToString(),
                             LoadFlagFromNavigationType(navigation_type), false),
       entry.ConstructRequestNavigationParams(
diff --git a/content/browser/frame_host/navigation_request.h b/content/browser/frame_host/navigation_request.h
index 133f698..5d2aa03 100644
--- a/content/browser/frame_host/navigation_request.h
+++ b/content/browser/frame_host/navigation_request.h
@@ -57,6 +57,8 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate {
   // Creates a request for a browser-intiated navigation.
   static scoped_ptr<NavigationRequest> CreateBrowserInitiated(
       FrameTreeNode* frame_tree_node,
+      const GURL& dest_url,
+      const Referrer& dest_referrer,
       const FrameNavigationEntry& frame_entry,
       const NavigationEntryImpl& entry,
       FrameMsg_Navigate_Type::Value navigation_type,
diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc
index 9e01343..4a4ae71 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -231,9 +231,22 @@ bool NavigatorImpl::NavigateToEntry(
     bool is_same_document_history_load) {
   TRACE_EVENT0("browser,navigation", "NavigatorImpl::NavigateToEntry");
 
+  GURL dest_url = frame_entry.url();
+  Referrer dest_referrer = frame_entry.referrer();
+  if (reload_type ==
+          NavigationController::ReloadType::RELOAD_ORIGINAL_REQUEST_URL &&
+      entry.GetOriginalRequestURL().is_valid() && !entry.GetHasPostData()) {
+    // We may have been redirected when navigating to the current URL.
+    // Use the URL the user originally intended to visit, if it's valid and if a
+    // POST wasn't involved; the latter case avoids issues with sending data to
+    // the wrong page.
+    dest_url = entry.GetOriginalRequestURL();
+    dest_referrer = Referrer();
+  }
+
   // 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()) {
+  if (dest_url.spec().size() > GetMaxURLChars()) {
     LOG(WARNING) << "Refusing to load URL as it exceeds " << GetMaxURLChars()
                  << " characters.";
     return false;
@@ -250,22 +263,21 @@ bool NavigatorImpl::NavigateToEntry(
   // 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()));
-    RequestNavigation(frame_tree_node, frame_entry, entry, reload_type,
-                      is_same_document_history_load, navigation_start);
+    navigation_data_.reset(new NavigationMetricsData(navigation_start, dest_url,
+                                                     entry.restore_type()));
+    RequestNavigation(frame_tree_node, dest_url, dest_referrer, 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);
-    }
+    if (delegate_)
+      delegate_->DidStartNavigationToPendingEntry(dest_url, reload_type);
 
     return true;
   }
 
   RenderFrameHostImpl* dest_render_frame_host =
-      manager->Navigate(frame_entry, entry);
+      manager->Navigate(dest_url, frame_entry, entry);
   if (!dest_render_frame_host)
     return false;  // Unable to create the desired RenderFrameHost.
 
@@ -274,8 +286,7 @@ bool NavigatorImpl::NavigateToEntry(
 
   // 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());
+  CheckWebUIRendererDoesNotDisplayNormalURL(dest_render_frame_host, dest_url);
 
   // Notify observers that we will navigate in this RenderFrame.
   if (delegate_) {
@@ -292,13 +303,14 @@ bool NavigatorImpl::NavigateToEntry(
       entry.transferred_global_request_id().child_id ==
           dest_render_frame_host->GetProcess()->GetID();
   if (!is_transfer_to_same) {
-    navigation_data_.reset(new NavigationMetricsData(
-        navigation_start, frame_entry.url(), entry.restore_type()));
+    navigation_data_.reset(new NavigationMetricsData(navigation_start, dest_url,
+                                                     entry.restore_type()));
     // Create the navigation parameters.
     FrameMsg_Navigate_Type::Value navigation_type =
         GetNavigationType(controller_->GetBrowserContext(), entry, reload_type);
     dest_render_frame_host->Navigate(
-        entry.ConstructCommonNavigationParams(frame_entry, navigation_type),
+        entry.ConstructCommonNavigationParams(dest_url, dest_referrer,
+                                              frame_entry, navigation_type),
         entry.ConstructStartNavigationParams(),
         entry.ConstructRequestNavigationParams(
             frame_entry, navigation_start, is_same_document_history_load,
@@ -317,7 +329,7 @@ bool NavigatorImpl::NavigateToEntry(
   CHECK_EQ(controller_->GetPendingEntry(), &entry);
 
   if (controller_->GetPendingEntryIndex() == -1 &&
-      frame_entry.url().SchemeIs(url::kJavaScriptScheme)) {
+      dest_url.SchemeIs(url::kJavaScriptScheme)) {
     // 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.
@@ -331,7 +343,7 @@ bool NavigatorImpl::NavigateToEntry(
 
   // Notify observers about navigation.
   if (delegate_) {
-    delegate_->DidStartNavigationToPendingEntry(frame_entry.url(), reload_type);
+    delegate_->DidStartNavigationToPendingEntry(dest_url, reload_type);
   }
 
   return true;
@@ -788,6 +800,8 @@ void NavigatorImpl::CheckWebUIRendererDoesNotDisplayNormalURL(
 // PlzNavigate
 void NavigatorImpl::RequestNavigation(
     FrameTreeNode* frame_tree_node,
+    const GURL& dest_url,
+    const Referrer& dest_referrer,
     const FrameNavigationEntry& frame_entry,
     const NavigationEntryImpl& entry,
     NavigationController::ReloadType reload_type,
@@ -805,8 +819,9 @@ void NavigatorImpl::RequestNavigation(
       GetNavigationType(controller_->GetBrowserContext(), entry, reload_type);
   frame_tree_node->CreatedNavigationRequest(
       NavigationRequest::CreateBrowserInitiated(
-          frame_tree_node, frame_entry, entry, navigation_type,
-          is_same_document_history_load, navigation_start, controller_));
+          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();
 
   // Have the current renderer execute its beforeunload event if needed. If it
diff --git a/content/browser/frame_host/navigator_impl.h b/content/browser/frame_host/navigator_impl.h
index 8f60522..2d2e163 100644
--- a/content/browser/frame_host/navigator_impl.h
+++ b/content/browser/frame_host/navigator_impl.h
@@ -114,6 +114,8 @@ class CONTENT_EXPORT NavigatorImpl : public Navigator {
   // to execute the beforeUnload event. Otherwise, the navigation request will
   // be started.
   void RequestNavigation(FrameTreeNode* frame_tree_node,
+                         const GURL& dest_url,
+                         const Referrer& dest_referrer,
                          const FrameNavigationEntry& frame_entry,
                          const NavigationEntryImpl& entry,
                          NavigationController::ReloadType reload_type,
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
index c570bf5..67f66fa 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -235,13 +235,14 @@ scoped_ptr<WebUIImpl> RenderFrameHostManager::CreateWebUI(const GURL& url,
 }
 
 RenderFrameHostImpl* RenderFrameHostManager::Navigate(
+    const GURL& dest_url,
     const FrameNavigationEntry& frame_entry,
     const NavigationEntryImpl& entry) {
   TRACE_EVENT1("navigation", "RenderFrameHostManager:Navigate",
                "FrameTreeNode id", frame_tree_node_->frame_tree_node_id());
   // Create a pending RenderFrameHost to use for the navigation.
   RenderFrameHostImpl* dest_render_frame_host = UpdateStateForNavigate(
-      frame_entry.url(),
+      dest_url,
       // TODO(creis): Move source_site_instance to FNE.
       entry.source_site_instance(), frame_entry.site_instance(),
       entry.GetTransitionType(),
diff --git a/content/browser/frame_host/render_frame_host_manager.h b/content/browser/frame_host/render_frame_host_manager.h
index 9386e70..e3cfb0f 100644
--- a/content/browser/frame_host/render_frame_host_manager.h
+++ b/content/browser/frame_host/render_frame_host_manager.h
@@ -292,8 +292,11 @@ class CONTENT_EXPORT RenderFrameHostManager : public NotificationObserver {
   // Called when we want to instruct the renderer to navigate to the given
   // navigation entry. It may create a new RenderFrameHost or re-use an existing
   // one. The RenderFrameHost to navigate will be returned. Returns NULL if one
-  // could not be created.
-  RenderFrameHostImpl* Navigate(const FrameNavigationEntry& frame_entry,
+  // could not be created. |dest_url| takes precedence over the |frame_entry|'s
+  // url (this is necessary because ReloadOriginalRequest navigates to a
+  // different URL than the last committed entry, without modifying it).
+  RenderFrameHostImpl* Navigate(const GURL& dest_url,
+                                const FrameNavigationEntry& frame_entry,
                                 const NavigationEntryImpl& entry);
 
   // Instructs the various live views to stop. Called when the user directed the
diff --git a/content/browser/frame_host/render_frame_host_manager_unittest.cc b/content/browser/frame_host/render_frame_host_manager_unittest.cc
index ad04a4a..79a2c58 100644
--- a/content/browser/frame_host/render_frame_host_manager_unittest.cc
+++ b/content/browser/frame_host/render_frame_host_manager_unittest.cc
@@ -423,7 +423,8 @@ class RenderFrameHostManagerTest : public RenderViewHostImplTestHarness {
         switches::kEnableBrowserSideNavigation)) {
       scoped_ptr<NavigationRequest> navigation_request =
           NavigationRequest::CreateBrowserInitiated(
-              manager->frame_tree_node_, *frame_entry, entry,
+              manager->frame_tree_node_, frame_entry->url(),
+              frame_entry->referrer(), *frame_entry, entry,
               FrameMsg_Navigate_Type::NORMAL, false, base::TimeTicks::Now(),
               static_cast<NavigationControllerImpl*>(&controller()));
       TestRenderFrameHost* frame_host = static_cast<TestRenderFrameHost*>(
@@ -433,7 +434,7 @@ class RenderFrameHostManagerTest : public RenderViewHostImplTestHarness {
       return frame_host;
     }
 
-    return manager->Navigate(*frame_entry, entry);
+    return manager->Navigate(frame_entry->url(), *frame_entry, entry);
   }
 
   // Returns the pending RenderFrameHost.
diff --git a/testing/buildbot/chromium.fyi.json b/testing/buildbot/chromium.fyi.json
index 4a78390..2f7ac0c 100644
--- a/testing/buildbot/chromium.fyi.json
+++ b/testing/buildbot/chromium.fyi.json
@@ -3997,7 +3997,7 @@
       {
         "args": [
           "--site-per-process",
-          "--gtest_filter=-*.PreventSpoofFromSubframeAndReplace:SessionHistoryTest.CrossFrameFormBackForward:SessionHistoryTest.FrameBackForward:*.SupportCrossProcessPostMessageWithMessagePort:DevToolsProtocolTest.NavigationPreservesMessages"
+          "--gtest_filter=-*.PreventSpoofFromSubframeAndReplace:SessionHistoryTest.CrossFrameFormBackForward:SessionHistoryTest.FrameBackForward:*.SupportCrossProcessPostMessageWithMessagePort:DevToolsProtocolTest.NavigationPreservesMessages:NavigationControllerBrowserTest.ReloadOriginalRequest"
         ],
         "test": "content_browsertests"
       },
@@ -4027,7 +4027,7 @@
       {
         "args": [
           "--site-per-process",
-          "--gtest_filter=-*.PreventSpoofFromSubframeAndReplace:SessionHistoryTest.CrossFrameFormBackForward:SessionHistoryTest.FrameBackForward:*.SupportCrossProcessPostMessageWithMessagePort:DevToolsProtocolTest.NavigationPreservesMessages"
+          "--gtest_filter=-*.PreventSpoofFromSubframeAndReplace:SessionHistoryTest.CrossFrameFormBackForward:SessionHistoryTest.FrameBackForward:*.SupportCrossProcessPostMessageWithMessagePort:DevToolsProtocolTest.NavigationPreservesMessages:NavigationControllerBrowserTest.ReloadOriginalRequest"
         ],
         "test": "content_browsertests"
       },