Fix race when reloading original URL

content/browser/frame_host/navigation_controller_impl_browsertest.cc

Index: content/browser/frame_host/navigation_controller_impl_browsertest.cc
diff --git a/content/browser/frame_host/navigation_controller_impl_browsertest.cc b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
index d676d4504475ba7db098c54adf46a622294a95ec..cbfb2a6e010bda9d89c212b1a04d93252e62aee3 100644
--- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
@@ -2125,4 +2125,64 @@ 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 for a race in ReloadOriginalRequest. Not crashing means that the
+// test is successful.
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, ReloadOriginalRequest) {
+  GURL url(embedded_test_server()->GetURL(
+      "/navigation_controller/simple_page_1.html"));
+  NavigateToURL(shell(), url);
+  FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+                            ->GetFrameTree()
+                            ->root();
+  RenderProcessKilledObserver kill_observer(shell()->web_contents());
+
+  for (int i = 0; i < 1000; i++) {

[Charlie Reis, 2015/07/06 20:56:39]

This doesn't seem like a great strategy for a test.  The loop isn't guaranteed
to test different schedules, so even if the bug exists we'll only fail if we're
lucky.

Can we structure the test so that it deterministically hits the failure without
your fix, without looping?

[lfg, 2015/07/08 22:09:28]

Sorry, this was just a test because I wasn't sure why the test was flaky. Turned
out there was a pending message in the message loop that wasn't being handled.

I've changed the strategy a bit, now I'm sending the ExecuteScript, but not
waiting for it, so I don't need to rely on setTimeout anymore. This should make
the test deterministic.

+    EXPECT_FALSE(shell()->web_contents()->IsLoading());
+    NavigateToURL(shell(), url);
+
+    // Redirect.
+    {
+      GURL foo_url(embedded_test_server()->GetURL(
+          "foo.com", "/navigation_controller/simple_page_1.html"));
+      std::string script = "location.replace('" + foo_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.
+    {
+      std::string script =
+          "setTimeout(function() { history.replaceState({}, '', 'foo') }, 0);";
+      EXPECT_TRUE(ExecuteScript(shell()->web_contents(), script));
+      EXPECT_FALSE(shell()->web_contents()->IsLoading());
+      shell()->web_contents()->GetController().ReloadOriginalRequestURL(false);
+      EXPECT_TRUE(shell()->web_contents()->IsLoading());

[Charlie Reis, 2015/07/06 20:56:39]

It seems like we should be able to verify that the last committed entry has the
modified URL and not the original URL at this point.

[lfg, 2015/07/08 22:09:28]

We can't, because we're not running the message loop, we don't know if the
renderer has executed the javascript or not. I've added a check at the end to
make sure we end up in the original URL.

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

Hmm, ok.  It's unfortunate, since that's the part we'd really like to prevent
regression on, but I don't see a great way to test it.

(Maybe a StallDelegate on the original URL that lets us check state before
allowing the navigation to commit?)

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

I've changed the test to look at the commits instead. We are only indirectly
testing the last committed url, but it may be enough, what do you think?

+      // Wait until there's no more navigations.

[Charlie Reis, 2015/07/06 20:56:39]

nit: Blank line above.

[lfg, 2015/07/08 22:09:28]

Done.

+      while (shell()->web_contents()->GetLastCommittedURL() != url)
+        EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+    }
+  }
+}
+
+
 }  // namespace content