Deflake TwoClientSessionsSyncTest.NoHistoryIfEncryptionEnabled

ForeignSessionsMatchChecker used to check foreign sessions against
the *snapshot* of local sessions on other profiles. If these local
sessions change again after taking the snapshot (due to postponed
events) before check has been completed, then check fails.

This might not affect other sessions test because they complete
sessions sync faster (before new local changes).

The solution is to grab local sessions in every
IsExitConditionSatisfied() call.

ForeignSessionsMatchChecker now compares foreign sessions only against
single profile, instead of all given windows.

Bug: 1028968
Change-Id: Ib8c3ea1c71f90e8a5dfc22609eac08c5e3681daa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1942128
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#720002}

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

@@ -414,37 +414,6 @@ bool WindowsMatch(const SessionWindowMap& win1, const ScopedWindowMap& win2) {
   return WindowsMatchImpl(win1, win2);
 }
 
-bool CheckForeignSessionsAgainst(int browser_index,
-                                 const std::vector<ScopedWindowMap>& windows) {
-  SyncedSessionVector sessions;
-
-  if (!GetSessionData(browser_index, &sessions)) {
-    LOG(ERROR) << "Cannot get session data";
-    return false;
-  }
-
-  for (size_t w_index = 0; w_index < windows.size(); ++w_index) {
-    // Skip the client's local window
-    if (static_cast<int>(w_index) == browser_index) {
-      continue;
-    }
-
-    size_t s_index = 0;
-
-    for (; s_index < sessions.size(); ++s_index) {
-      if (WindowsMatch(sessions[s_index]->windows, windows[w_index]))
-        break;
-    }
-
-    if (s_index == sessions.size()) {
-      LOG(ERROR) << "Cannot find window #" << w_index;
-      return false;
-    }
-  }
-
-  return true;
-}
-
 void DeleteForeignSession(int browser_index, std::string session_tag) {
   SessionSyncServiceFactory::GetInstance()
       ->GetForProfile(test()->GetProfile(browser_index))
@@ -452,17 +421,40 @@ void DeleteForeignSession(int browser_index, std::string session_tag) {
       ->DeleteForeignSession(session_tag);
 }
 
-}  // namespace sessions_helper
-
 ForeignSessionsMatchChecker::ForeignSessionsMatchChecker(
-    int browser_index,
-    const std::vector<sessions_helper::ScopedWindowMap>& windows)
+    int profile_index,
+    int foreign_profile_index)
     : MultiClientStatusChangeChecker(
           sync_datatype_helper::test()->GetSyncServices()),
-      browser_index_(browser_index),
-      windows_(windows) {}
+      profile_index_(profile_index),
+      foreign_profile_index_(foreign_profile_index) {}
 
 bool ForeignSessionsMatchChecker::IsExitConditionSatisfied(std::ostream* os) {
   *os << "Waiting for matching foreign sessions";
-  return sessions_helper::CheckForeignSessionsAgainst(browser_index_, windows_);
+
+  const sync_sessions::SyncedSession* foreign_local_sessions;
+  if (!GetLocalSession(foreign_profile_index_, &foreign_local_sessions)) {
+    *os << "Cannot get local sessions from profile " << foreign_profile_index_
+        << ".";
+    return false;
+  }
+  DCHECK(foreign_local_sessions);
+
+  SyncedSessionVector sessions;
+  if (!GetSessionData(profile_index_, &sessions)) {
+    *os << "Cannot get foreign sessions on profile " << profile_index_ << ".";
+    return false;
+  }
+
+  for (const sync_sessions::SyncedSession* remote_session : sessions) {
+    if (WindowsMatch(remote_session->windows,
+                     foreign_local_sessions->windows)) {
+      return true;
+    }
+  }
+
+  *os << "Can't match sessions for profile " << foreign_profile_index_ << ".";
+  return false;
 }
+
+}  // namespace sessions_helper

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

@@ -142,22 +142,20 @@ bool GetLocalSession(int browser_index,
 // to the session tag within the SyncedSession we plan to delete.
 void DeleteForeignSession(int browser_index, std::string session_tag);
 
-}  // namespace sessions_helper
-
 // Checker to block until the foreign sessions for a particular profile matches
-// the reference windows.
+// the local sessions from another profile.
 class ForeignSessionsMatchChecker : public MultiClientStatusChangeChecker {
  public:
-  ForeignSessionsMatchChecker(
-      int browser_index,
-      const std::vector<sessions_helper::ScopedWindowMap>& windows);
+  ForeignSessionsMatchChecker(int profile_index, int foreign_profile_index);
 
   // StatusChangeChecker implementation.
   bool IsExitConditionSatisfied(std::ostream* os) override;
 
  private:
-  int browser_index_;
-  const std::vector<sessions_helper::ScopedWindowMap>& windows_;
+  int profile_index_;
+  int foreign_profile_index_;
 };
 
+}  // namespace sessions_helper
+
 #endif  // CHROME_BROWSER_SYNC_TEST_INTEGRATION_SESSIONS_HELPER_H_

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

@@ -23,6 +23,7 @@ namespace {
 using sessions_helper::CheckInitialState;
 using sessions_helper::CloseTab;
 using sessions_helper::DeleteForeignSession;
+using sessions_helper::ForeignSessionsMatchChecker;
 using sessions_helper::GetLocalWindows;
 using sessions_helper::GetSessionData;
 using sessions_helper::NavigateTab;
@@ -39,16 +40,8 @@ class TwoClientSessionsSyncTest : public SyncTest {
   TwoClientSessionsSyncTest() : SyncTest(TWO_CLIENT) {}
   ~TwoClientSessionsSyncTest() override {}
 
-  void WaitForWindowsInForeignSession(int index, ScopedWindowMap windows) {
-    std::vector<ScopedWindowMap> expected_windows(1);
-    expected_windows[0] = std::move(windows);
-    EXPECT_TRUE(ForeignSessionsMatchChecker(index, expected_windows).Wait());
-  }
-
-  void WaitForForeignSessionsToSync(int local_index, int non_local_index) {
-    ScopedWindowMap client_windows;
-    ASSERT_TRUE(GetLocalWindows(local_index, &client_windows));
-    WaitForWindowsInForeignSession(non_local_index, std::move(client_windows));
+  bool WaitForForeignSessionsToSync(int local_index, int non_local_index) {
+    return ForeignSessionsMatchChecker(non_local_index, local_index).Wait();
   }
 
  private:
@@ -77,7 +70,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest,
       base::StringPrintf(kURLTemplate, base::GenerateGUID().c_str());
 
   ASSERT_TRUE(OpenTab(0, GURL(url)));
-  WaitForForeignSessionsToSync(0, 1);
+  EXPECT_TRUE(WaitForForeignSessionsToSync(0, 1));
 }
 
 IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, SingleClientClosed) {
@@ -86,13 +79,13 @@ IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, SingleClientClosed) {
   // Open two tabs on client 0.
   OpenTab(0, GURL(kURL1));
   OpenTab(0, GURL(kURL2));
-  WaitForForeignSessionsToSync(0, 1);
+  ASSERT_TRUE(WaitForForeignSessionsToSync(0, 1));
 
   // Close one of the two tabs. We also issue another navigation to make sure
   // association logic kicks in.
   CloseTab(/*index=*/0, /*tab_index=*/1);
   NavigateTab(0, GURL(kURL3));
-  WaitForForeignSessionsToSync(0, 1);
+  EXPECT_TRUE(WaitForForeignSessionsToSync(0, 1));
 
   std::vector<sync_pb::SyncEntity> entities =
       GetFakeServer()->GetSyncEntitiesByModelType(syncer::SESSIONS);
@@ -105,20 +98,22 @@ IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, E2E_ENABLED(AllChanged)) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
   // Open tabs on all clients and retain window information.
-  std::vector<ScopedWindowMap> client_windows(num_clients());
   for (int i = 0; i < num_clients(); ++i) {
     ScopedWindowMap windows;
     std::string url =
         base::StringPrintf(kURLTemplate, base::GenerateGUID().c_str());
     ASSERT_TRUE(OpenTab(i, GURL(url)));
-    ASSERT_TRUE(GetLocalWindows(i, &windows));
-    client_windows[i] = std::move(windows);
   }
 
   // Get foreign session data from all clients and check it against all
-  // client_windows.
+  // local sessions.
   for (int i = 0; i < num_clients(); ++i) {
-    ASSERT_TRUE(ForeignSessionsMatchChecker(i, client_windows).Wait());
+    for (int j = 0; j < num_clients(); ++j) {
+      if (i == j) {
+        continue;
+      }
+      EXPECT_TRUE(WaitForForeignSessionsToSync(i, j));
+    }
   }
 }
 
@@ -131,12 +126,12 @@ IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, BothChanged) {
   ASSERT_TRUE(OpenTab(0, GURL(kURL1)));
   ASSERT_TRUE(OpenTab(1, GURL(kURL2)));
 
-  WaitForForeignSessionsToSync(0, 1);
-  WaitForForeignSessionsToSync(1, 0);
+  EXPECT_TRUE(WaitForForeignSessionsToSync(0, 1));
+  EXPECT_TRUE(WaitForForeignSessionsToSync(1, 0));
 
   // Check that a navigation in client 0 is reflected on client 1.
   NavigateTab(0, GURL(kURL3));
-  WaitForForeignSessionsToSync(0, 1);
+  EXPECT_TRUE(WaitForForeignSessionsToSync(0, 1));
 }
 
 IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, DeleteIdleSession) {
@@ -147,7 +142,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, DeleteIdleSession) {
 
   // Client 0 opened some tabs then went idle.
   ASSERT_TRUE(OpenTab(0, GURL(kURL1)));
-  WaitForForeignSessionsToSync(0, 1);
+  ASSERT_TRUE(WaitForForeignSessionsToSync(0, 1));
 
   // Get foreign session data from client 1.
   SyncedSessionVector sessions1;
@@ -167,7 +162,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, DeleteActiveSession) {
 
   // Client 0 opened some tabs then went idle.
   ASSERT_TRUE(OpenTab(0, GURL(kURL1)));
-  WaitForForeignSessionsToSync(0, 1);
+  ASSERT_TRUE(WaitForForeignSessionsToSync(0, 1));
 
   SyncedSessionVector sessions1;
   ASSERT_TRUE(GetSessionData(1, &sessions1));
@@ -180,7 +175,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, DeleteActiveSession) {
 
   // Client 0 becomes active again with a new tab.
   ASSERT_TRUE(OpenTab(0, GURL(kURL2)));
-  WaitForForeignSessionsToSync(0, 1);
+  EXPECT_TRUE(WaitForForeignSessionsToSync(0, 1));
   EXPECT_TRUE(GetSessionData(1, &sessions1));
 }
 
@@ -198,7 +193,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, MultipleWindowsMultipleTabs) {
   EXPECT_TRUE(OpenTab(2, GURL(kURL3)));
   EXPECT_TRUE(OpenTabAtIndex(2, 1, GURL(kURL4)));
 
-  WaitForForeignSessionsToSync(0, 1);
+  EXPECT_TRUE(WaitForForeignSessionsToSync(0, 1));
 }
 
 IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest,
@@ -216,7 +211,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest,
       "passphrase"));
 
   EXPECT_TRUE(OpenTab(0, GURL(kURL1)));
-  WaitForForeignSessionsToSync(0, 1);
+  EXPECT_TRUE(WaitForForeignSessionsToSync(0, 1));
 
   EXPECT_THAT(GetFakeServer()->GetCommittedHistoryURLs(), IsEmpty());
 }