Deflake RaceCrossOriginNavigationAndSamePageHistoryNavigation test.

content/browser/frame_host/navigation_controller_impl_browsertest.cc

 // Copyright 2014 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/navigation_controller_impl.h"
 
 #include <stdint.h>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 5950 matching lines @@
   }
 
   // Verify the expected origin through JavaScript. It also has the additional
   // verification of the process also being still alive.
   std::string origin;
   EXPECT_TRUE(ExecuteScriptAndExtractString(
       web_contents, "domAutomationController.send(document.origin)", &origin));
   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)
       return false;
 
     // Parse the IPC message so the URL can be checked agains the expected one.
     base::PickleIterator iter(message);
     FrameHostMsg_DidCommitProvisionalLoad_Params validated_params;
     if (!IPC::ParamTraits<FrameHostMsg_DidCommitProvisionalLoad_Params>::Read(
             &message, &iter, &validated_params)) {
       return false;
     }
 
     // Only handle the message if the URLs are matching.
     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);
 };
 
 // Test which simulates a race condition between a cross-origin, same-process
 // navigation and a same page session history navigation. When such a race
 // occurs, the renderer will commit the cross-origin navigation, updating its
 // version of the current document sequence number, and will send an IPC to the
 // browser process. The session history navigation comes after the commit for
 // 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"));
   EXPECT_TRUE(NavigateToURL(shell(), start_url));
 
   GURL same_page_url(
       embedded_test_server()->GetURL("a.com", "/title1.html#foo"));
   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());
 
   // Verify the expected origin through JavaScript. It also has the additional
   // verification of the process also being still alive.
   std::string origin;
   EXPECT_TRUE(ExecuteScriptAndExtractString(
       web_contents, "domAutomationController.send(document.origin)", &origin));
   EXPECT_EQ(start_url.GetOrigin().spec(), origin + "/");
 }
@@ skipping 57 matching lines @@
   content::FaviconStatus& favicon_status2 = entry->GetFavicon();
   EXPECT_TRUE(favicon_status2.valid);
 
   ASSERT_TRUE(RendererLocationReplace(shell(), GURL("data:text/html,page2")));
   entry = controller.GetLastCommittedEntry();
   content::FaviconStatus& favicon_status3 = entry->GetFavicon();
   EXPECT_FALSE(favicon_status3.valid);
 }
 
 }  // namespace content