Deflake RaceCrossOriginNavigationAndSamePageHistoryNavigation test.

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 d732bd929d9166d72bc251883dd149cc68316481..81721d248fb8c7db27d8525876b5f44ecaf9aee2 100644
--- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
@@ -5968,17 +5968,29 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   EXPECT_EQ(start_url.GetOrigin().spec(), origin + "/");
 }
 
-// A BrowserMessageFilter that drops FrameHostMsg_DidCommitProvisionaLoad IPC
-// message for a specified URL and runs a callback on the UI thread.
+// A BrowserMessageFilter that delays FrameHostMsg_DidCommitProvisionaLoad IPC
+// message for a specified URL, navigates the WebContents back and then
+// processes the commit message.
 class CommitMessageFilter : public BrowserMessageFilter {

[Charlie Reis, 2016/09/26 20:58:38]

nit: Let's rename this now that it isn't general (e.g., GoBackAndCommitFilter?).

[nasko, 2016/09/26 22:03:49]

Done.

  public:
-  CommitMessageFilter(const GURL& url, base::Closure on_commit)
-      : BrowserMessageFilter(FrameMsgStart), url_(url), on_commit_(on_commit) {}
+  CommitMessageFilter(const GURL& url, WebContentsImpl* web_contents)
+      : BrowserMessageFilter(FrameMsgStart),
+        url_(url),
+        web_contents_(web_contents) {}
 
  protected:
   ~CommitMessageFilter() override {}
 
  private:
+  static void NavigateBackAndCommit(const IPC::Message& message,
+                                    WebContentsImpl* web_contents) {
+    web_contents->GetController().GoBack();
+
+    RenderFrameHostImpl* rfh = web_contents->GetMainFrame();
+    DCHECK_EQ(rfh->routing_id(), message.routing_id());
+    rfh->OnMessageReceived(message);
+  }
+
   // BrowserMessageFilter:
   bool OnMessageReceived(const IPC::Message& message) override {
     if (message.type() != FrameHostMsg_DidCommitProvisionalLoad::ID)
@@ -5996,12 +6008,14 @@ class CommitMessageFilter : public BrowserMessageFilter {
     if (validated_params.url != url_)
       return false;
 
-    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, on_commit_);
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&NavigateBackAndCommit, message, web_contents_));
     return true;
   }
 
   GURL url_;
-  base::Closure on_commit_;
+  WebContentsImpl* web_contents_;
 
   DISALLOW_COPY_AND_ASSIGN(CommitMessageFilter);
 };
@@ -6014,13 +6028,11 @@ class CommitMessageFilter : public BrowserMessageFilter {
 // the cross-origin navigation and updates the URL, but not the origin of the
 // document. This results in mismatch between the two and causes the renderer
 // process to be killed. See https://crbug.com/630103.
-// TODO(nasko): Investigate why this test is flaky, likely related to
-// https://crbug.com/638089, and enable once resolved.
-IN_PROC_BROWSER_TEST_F(
-    NavigationControllerBrowserTest,
-    DISABLED_RaceCrossOriginNavigationAndSamePageHistoryNavigation) {
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
+                       RaceCrossOriginNavigationAndSamePageHistoryNavigation) {
   WebContentsImpl* web_contents =
       static_cast<WebContentsImpl*>(shell()->web_contents());
+  FrameTreeNode* root = web_contents->GetFrameTree()->root();
 
   // Navigate to a simple page and then perform an in-page navigation.
   GURL start_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
@@ -6031,19 +6043,24 @@ IN_PROC_BROWSER_TEST_F(
   EXPECT_TRUE(NavigateToURL(shell(), same_page_url));
   EXPECT_EQ(2, web_contents->GetController().GetEntryCount());
 
-  // Create a CommitMessageFilter, which will drop the commit IPC for a
+  // Create a CommitMessageFilter, which will delay the commit IPC for a
   // cross-origin, same process navigation and will perform a GoBack.
   GURL cross_origin_url(
       embedded_test_server()->GetURL("suborigin.a.com", "/title2.html"));
-  scoped_refptr<CommitMessageFilter> filter = new CommitMessageFilter(
-      cross_origin_url,
-      base::Bind(&NavigationControllerImpl::GoBack,
-                 base::Unretained(&web_contents->GetController())));
+  scoped_refptr<CommitMessageFilter> filter =
+      new CommitMessageFilter(cross_origin_url, web_contents);
   web_contents->GetMainFrame()->GetProcess()->AddFilter(filter.get());
 
-  // Navigate cross-origin, which will fail and the back navigation should have
-  // succeeded.
-  EXPECT_FALSE(NavigateToURL(shell(), cross_origin_url));
+  // Navigate cross-origin, waiting for the commit to occur.
+  UrlCommitObserver cross_origin_commit_observer(root, cross_origin_url);
+  shell()->LoadURL(cross_origin_url);
+  cross_origin_commit_observer.Wait();
+  EXPECT_EQ(cross_origin_url, web_contents->GetLastCommittedURL());
+  EXPECT_EQ(2, web_contents->GetController().GetLastCommittedEntryIndex());
+
+  // Wait for the back navigation to commit as well.
+  UrlCommitObserver history_commit_observer(root, start_url);

[Charlie Reis, 2016/09/26 20:58:38]

Should we be declaring this before the LoadURL call?  I worry that it might hang
if the second commit occurs before we start listening (e.g., during the previous
Wait).

[nasko, 2016/09/26 22:03:49]

Done.

+  history_commit_observer.Wait();
   EXPECT_EQ(start_url, web_contents->GetLastCommittedURL());
   EXPECT_EQ(0, web_contents->GetController().GetLastCommittedEntryIndex());