Fix and reenable SingleClientSessionsSyncTest.NavigationChain*

Previously, these tests were reusing TestNavigationObserver instances
for multiple navigations, which apparently doesn't work.
This CL moves the TestNavigationObserver stuff directly into the
NavigateTabBack/Forward helpers, so that individual tests don't have
to care about this.
Note that the similar NavigateTab helper uses ui_test_utils::NavigateToURL
which already does the waiting, so nothing else is required there.

Bug: 972871
Change-Id: Iafa796be4da091afcdb279824c6c873f4e6ae62d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1736682Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#684309}

chrome/browser/sync/test/integration/sessions_helper.cc

@@ -37,6 +37,7 @@
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test_utils.h"
+#include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
 #include "url/gurl.h"
 
@@ -182,21 +183,19 @@ void NavigateTab(int browser_index, const GURL& url) {
 }
 
 void NavigateTabBack(int browser_index) {
-  test()
-      ->GetBrowser(browser_index)
-      ->tab_strip_model()
-      ->GetWebContentsAt(0)
-      ->GetController()
-      .GoBack();
+  content::WebContents* web_contents =
+      test()->GetBrowser(browser_index)->tab_strip_model()->GetWebContentsAt(0);
+  content::TestNavigationObserver observer(web_contents);
+  web_contents->GetController().GoBack();
+  observer.WaitForNavigationFinished();
 }
 
 void NavigateTabForward(int browser_index) {
-  test()
-      ->GetBrowser(browser_index)
-      ->tab_strip_model()
-      ->GetWebContentsAt(0)
-      ->GetController()
-      .GoForward();
+  content::WebContents* web_contents =
+      test()->GetBrowser(browser_index)->tab_strip_model()->GetWebContentsAt(0);
+  content::TestNavigationObserver observer(web_contents);
+  web_contents->GetController().GoForward();
+  observer.WaitForNavigationFinished();
 }
 
 bool ExecJs(int browser_index, int tab_index, const std::string& script) {

chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc

@@ -35,7 +35,6 @@
 #include "components/sync_sessions/session_sync_service.h"
 #include "components/sync_sessions/session_sync_test_helper.h"
 #include "components/sync_sessions/synced_session_tracker.h"
-#include "content/public/test/test_navigation_observer.h"
 #include "google_apis/gaia/gaia_auth_util.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -480,80 +479,55 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, FragmentURLNavigation) {
   WaitForURLOnServer(fragment_url);
 }
 
-// Flakily fails on mac/linux/win. http://crbug.com/972871
 IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest,
-                       DISABLED_NavigationChainForwardBack) {
+                       NavigationChainForwardBack) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(CheckInitialState(0));
 
   GURL first_url = GURL(kURL1);
-  content::TestNavigationObserver first_url_observer(first_url);
-  first_url_observer.StartWatchingNewWebContents();
   ASSERT_TRUE(OpenTab(0, first_url));
-  first_url_observer.WaitForNavigationFinished();
   WaitForURLOnServer(first_url);
 
   GURL second_url = GURL(kURL2);
-  content::TestNavigationObserver second_url_observer(second_url);
-  second_url_observer.WatchExistingWebContents();
   NavigateTab(0, second_url);
-  second_url_observer.WaitForNavigationFinished();
   WaitForURLOnServer(second_url);
 
-  first_url_observer.WatchExistingWebContents();
   NavigateTabBack(0);
-  first_url_observer.WaitForNavigationFinished();
   WaitForURLOnServer(first_url);
 
   ExpectNavigationChain({first_url, second_url});
 
-  second_url_observer.WatchExistingWebContents();
   NavigateTabForward(0);
-  second_url_observer.WaitForNavigationFinished();
   WaitForURLOnServer(second_url);
 
   ExpectNavigationChain({first_url, second_url});
 }
 
-// TODO(crbug.com/972871): Fails or crashes on all platforms flakily.
 IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest,
-                       DISABLED_NavigationChainAlteredDestructively) {
+                       NavigationChainAlteredDestructively) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(CheckInitialState(0));
 
   GURL base_url = GURL(kURL1);
-  content::TestNavigationObserver base_url_observer(base_url);
-  base_url_observer.StartWatchingNewWebContents();
   ASSERT_TRUE(OpenTab(0, base_url));
-  base_url_observer.WaitForNavigationFinished();
   WaitForURLOnServer(base_url);
 
   GURL first_url = GURL(kURL2);
-  content::TestNavigationObserver first_url_observer(first_url);
-  first_url_observer.WatchExistingWebContents();
   NavigateTab(0, first_url);
-  first_url_observer.WaitForNavigationFinished();
   WaitForURLOnServer(first_url);
 
   // Check that the navigation chain matches the above sequence of {base_url,
   // first_url}.
   ExpectNavigationChain({base_url, first_url});
 
-  base_url_observer.WatchExistingWebContents();
   NavigateTabBack(0);
-  base_url_observer.WaitForNavigationFinished();
   WaitForURLOnServer(base_url);
 
   GURL second_url = GURL(kURL3);
-  content::TestNavigationObserver second_url_observer(second_url);
-  second_url_observer.WatchExistingWebContents();
   NavigateTab(0, second_url);
-  first_url_observer.WaitForNavigationFinished();
   WaitForURLOnServer(second_url);
 
-  base_url_observer.WatchExistingWebContents();
   NavigateTabBack(0);
-  base_url_observer.WaitForNavigationFinished();
   WaitForURLOnServer(base_url);
 
   // Check that the navigation chain contains second_url where first_url was