Add sync integration tests for closed tabs

There are certain requirements wrt. how sync should behave when a tab
is closed. This patch introduces the first related integration tests,
which reflect current behavior and most notably include the need to sync
history for navigations issued immediately before closing a tab.

Bug: 882489
Change-Id: I255a5082f4a75a24d91ef4be443324094427bd1e
Reviewed-on: https://chromium-review.googlesource.com/c/1355127
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612372}

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

@@ -30,21 +30,21 @@ using ScopedWindowMap =
 
 // Copies the local session windows of profile at |index| to |local_windows|.
 // Returns true if successful.
-bool GetLocalWindows(int index, ScopedWindowMap* local_windows);
+bool GetLocalWindows(int browser_index, ScopedWindowMap* local_windows);
 
 // Checks that window count and foreign session count are 0.
-bool CheckInitialState(int index);
+bool CheckInitialState(int browser_index);
 
 // Returns number of open windows for a profile.
-int GetNumWindows(int index);
+int GetNumWindows(int browser_index);
 
 // Returns number of foreign sessions for a profile.
-int GetNumForeignSessions(int index);
+int GetNumForeignSessions(int browser_index);
 
 // Fills the sessions vector with the SyncableService's foreign session data.
 // Caller owns |sessions|, but not SyncedSessions objects within.
 // Returns true if foreign sessions were found, false otherwise.
-bool GetSessionData(int index, SyncedSessionVector* sessions);
+bool GetSessionData(int browser_index, SyncedSessionVector* sessions);
 
 // Compares a foreign session based on the first session window.
 // Returns true based on the comparison of the session windows.
@@ -74,75 +74,70 @@ bool WindowsMatch(const SessionWindowMap& win1, const ScopedWindowMap& win2);
 // with a reference SessionWindow list.
 // Returns true if the session windows of the foreign session matches the
 // reference.
-bool CheckForeignSessionsAgainst(int index,
+bool CheckForeignSessionsAgainst(int browser_index,
                                  const std::vector<ScopedWindowMap>& windows);
 
-// Open a single tab  in the browser at |index| and block until the
-// session SyncableService is aware of it. Returns true upon success, false
-// otherwise.
-bool OpenTab(int index, const GURL& url);
+// Opens (appends) a single tab  in the browser at |index| and block until the
+// sessions bridge is aware of it. Returns true upon success, false otherwise.
+bool OpenTab(int browser_index, const GURL& url);
 
 // See OpenTab, except that the tab is opened in position |tab_index|.
 // If |tab_index| is -1 or greater than the number of tabs, the tab will be
 // appended to the end of the strip. i.e. if tab_index is 3 for a tab strip of
 // size 1, the new tab will be in position 1.
-bool OpenTabAtIndex(int index, int tab_index, const GURL& url);
+bool OpenTabAtIndex(int browser_index, int tab_index, const GURL& url);
 
 // Like OpenTab, but opens |url| from the tab at |index_of_source_tab| using
 // |disposition|.
-bool OpenTabFromSourceIndex(int index,
+bool OpenTabFromSourceIndex(int browser_index,
                             int index_of_source_tab,
                             const GURL& url,
                             WindowOpenDisposition disposition);
 
-// Open multiple tabs and block until the session SyncableService is aware
-// of all of them.  Returns true on success, false on failure.
-bool OpenMultipleTabs(int index, const std::vector<GURL>& urls);
+// Opens multiple tabs and blocks until the sessions bridge is aware of all of
+// them. Returns true on success, false on failure.
+bool OpenMultipleTabs(int browser_index, const std::vector<GURL>& urls);
+
+// Closes the tab |tab_index| in the browser at |index|.
+void CloseTab(int browser_index, int tab_index);
 
 // Moves the tab in position |tab_index| in the TabStrip for browser at
-// |from_index| to the TabStrip for browser at |to_index|.
-void MoveTab(int from_index, int to_index, int tab_index);
+// |from_browser_index| to the TabStrip for browser at |to_browser_index|.
+void MoveTab(int from_browser_index, int to_browser_index, int tab_index);
 
 // Navigate the active tab for browser in position |index| to the given
-// url, and blocks until the session SyncableService is aware of it.
-// WARNING: it's dangerous to assume this will return for any arbitrary URL.
-// For URLs that don't resolve to a valid server response, this can block
-// indefinitely. Use a data uri or the embedded_test_server to ensure that this
-// doesn't happen.
-bool NavigateTab(int index, const GURL& url);
+// URL.
+void NavigateTab(int browser_index, const GURL& url);
 
 // Navigate the active tab for browser in position |index| back by one;
 // if this isn't possible, does nothing
-void NavigateTabBack(int index);
+void NavigateTabBack(int browser_index);
 
 // Navigate the active tab for browser in position |index| forward by
 // one; if this isn't possible, does nothing
-void NavigateTabForward(int index);
+void NavigateTabForward(int browser_index);
 
 // Wait for a session change to |web_contents| to propagate to the model
 // associator. Will return true once |url| has been found, or false if it times
 // out while waiting.
-bool WaitForTabToLoad(int index,
+bool WaitForTabToLoad(int browser_index,
                       const GURL& url,
                       content::WebContents* web_contents);
 
 // Wait for each url in |urls| to load. The ordering of |urls| is assumed to
 // match the ordering of the corresponding tabs.
-bool WaitForTabsToLoad(int index, const std::vector<GURL>& urls);
-
-// Check if the session SyncableService knows that the current open tab
-// has this url.
-bool SessionsSyncManagerHasTabWithURL(int index, const GURL& url);
+bool WaitForTabsToLoad(int browser_index, const std::vector<GURL>& urls);
 
 // Stores a pointer to the local session for a given profile in |session|.
 // Returns true on success, false on failure.
-bool GetLocalSession(int index, const sync_sessions::SyncedSession** session);
+bool GetLocalSession(int browser_index,
+                     const sync_sessions::SyncedSession** session);
 
 // Deletes the foreign session with tag |session_tag| from the profile specified
 // by |index|. This will affect all synced clients.
 // Note: We pass the session_tag in by value to ensure it's not a reference
 // to the session tag within the SyncedSession we plan to delete.
-void DeleteForeignSession(int index, std::string session_tag);
+void DeleteForeignSession(int browser_index, std::string session_tag);
 
 }  // namespace sessions_helper
 
@@ -151,7 +146,7 @@ void DeleteForeignSession(int index, std::string session_tag);
 class ForeignSessionsMatchChecker : public MultiClientStatusChangeChecker {
  public:
   ForeignSessionsMatchChecker(
-      int index,
+      int browser_index,
       const std::vector<sessions_helper::ScopedWindowMap>& windows);
 
   // StatusChangeChecker implementation.
@@ -159,7 +154,7 @@ class ForeignSessionsMatchChecker : public MultiClientStatusChangeChecker {
   std::string GetDebugMessage() const override;
 
  private:
-  int index_;
+  int browser_index_;
   const std::vector<sessions_helper::ScopedWindowMap>& windows_;
 };
 

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

@@ -34,9 +34,9 @@ using base::HistogramSamples;
 using base::HistogramTester;
 using fake_server::SessionsHierarchy;
 using sessions_helper::CheckInitialState;
+using sessions_helper::CloseTab;
 using sessions_helper::GetLocalWindows;
 using sessions_helper::GetSessionData;
-using sessions_helper::SessionsSyncManagerHasTabWithURL;
 using sessions_helper::MoveTab;
 using sessions_helper::NavigateTab;
 using sessions_helper::NavigateTabBack;
@@ -53,6 +53,7 @@ using typed_urls_helper::GetUrlFromClient;
 static const char* kURL1 = "data:text/html,<html><title>Test</title></html>";
 static const char* kURL2 = "data:text/html,<html><title>Test2</title></html>";
 static const char* kURL3 = "data:text/html,<html><title>Test3</title></html>";
+static const char* kURL4 = "data:text/html,<html><title>Test4</title></html>";
 static const char* kBaseFragmentURL =
     "data:text/html,<html><title>Fragment</title><body></body></html>";
 static const char* kSpecifiedFragmentURL =
@@ -69,6 +70,39 @@ void ExpectUniqueSampleGE(const HistogramTester& histogram_tester,
   EXPECT_EQ(sample_count, samples->TotalCount());
 }
 
+class IsUrlSyncedChecker : public SingleClientStatusChangeChecker {
+ public:
+  IsUrlSyncedChecker(const std::string& url,
+                     fake_server::FakeServer* fake_server,
+                     browser_sync::ProfileSyncService* service)
+      : SingleClientStatusChangeChecker(service),
+        url_(url),
+        fake_server_(fake_server) {}
+
+  // StatusChangeChecker implementation.
+  bool IsExitConditionSatisfied() override {
+    std::vector<sync_pb::SyncEntity> entities =
+        fake_server_->GetSyncEntitiesByModelType(syncer::SESSIONS);
+    for (const sync_pb::SyncEntity& entity : entities) {
+      for (const auto& navigation :
+           entity.specifics().session().tab().navigation()) {
+        if (navigation.virtual_url() == url_) {
+          return true;
+        }
+      }
+    }
+    return false;
+  }
+
+  std::string GetDebugMessage() const override {
+    return "Waiting for URLs to be commited to the server";
+  }
+
+ private:
+  const std::string url_;
+  fake_server::FakeServer* fake_server_;
+};
+
 class SingleClientSessionsSyncTest : public SyncTest {
  public:
   SingleClientSessionsSyncTest() : SyncTest(SINGLE_CLIENT) {}
@@ -181,6 +215,30 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, ChromeHistory) {
   WaitForURLOnServer(GURL(chrome::kChromeUIHistoryURL));
 }
 
+IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, NavigateThenCloseTab) {
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(CheckInitialState(0));
+
+  // Two tabs are opened initially.
+  ASSERT_TRUE(OpenTab(0, GURL(kURL1)));
+  ASSERT_TRUE(OpenTab(0, GURL(kURL2)));
+  WaitForHierarchyOnServer(SessionsHierarchy({{kURL1, kURL2}}));
+
+  // Close one of the two tabs immediately after issuing an navigation. We also
+  // issue another navigation to make sure association logic kicks in.
+  NavigateTab(0, GURL(kURL3));
+  CloseTab(/*index=*/0, /*tab_index=*/1);
+  NavigateTab(0, GURL(kURL4));
+
+  ASSERT_TRUE(
+      IsUrlSyncedChecker(kURL4, GetFakeServer(), GetSyncService(0)).Wait());
+
+  // All URLs should be synced, for synced history to be complete. In
+  // particular, |kURL3| should be synced despite the tab being closed.
+  EXPECT_TRUE(
+      IsUrlSyncedChecker(kURL3, GetFakeServer(), GetSyncService(0)).Wait());
+}
+
 IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, TimestampMatchesHistory) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
@@ -284,7 +342,7 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest,
   WaitForURLOnServer(base_url);
 
   GURL first_url = GURL(kURL2);
-  ASSERT_TRUE(NavigateTab(0, first_url));
+  NavigateTab(0, first_url);
   WaitForURLOnServer(first_url);
 
   // Check that the navigation chain matches the above sequence of {base_url,
@@ -448,7 +506,7 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest,
 
     // Trigger a sync and wait for it.
     GURL url = GURL(kURL2);
-    ASSERT_TRUE(NavigateTab(0, url));
+    NavigateTab(0, url);
     WaitForURLOnServer(url);
 
     // Verify the cookie jar mismatch bool is set to false.

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

@@ -19,6 +19,7 @@
 namespace {
 
 using sessions_helper::CheckInitialState;
+using sessions_helper::CloseTab;
 using sessions_helper::DeleteForeignSession;
 using sessions_helper::GetLocalWindows;
 using sessions_helper::GetSessionData;
@@ -75,6 +76,30 @@ IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest,
   WaitForForeignSessionsToSync(0, 1);
 }
 
+IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, SingleClientClosed) {
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+
+  // Open two tabs on client 0.
+  OpenTab(0, GURL(kURL1));
+  OpenTab(0, GURL(kURL2));
+  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);
+
+  std::vector<sync_pb::SyncEntity> entities =
+      GetFakeServer()->GetSyncEntitiesByModelType(syncer::SESSIONS);
+  // Two header entities and two tab entities (one of the two has been closed
+  // but considered "free" for future recycling, i.e. not deleted).
+  ASSERT_EQ(4U, entities.size());
+  for (const auto& entity : entities) {
+    EXPECT_FALSE(entity.deleted());
+  }
+}
+
 IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, E2E_ENABLED(AllChanged)) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 

components/sync_sessions/session_sync_bridge_unittest.cc

@@ -932,12 +932,20 @@ TEST_F(SessionSyncBridgeTest, ShouldRestoreLocalSessionWithFreedTab) {
   InitializeBridge();
   StartSyncing();
 
-  ASSERT_THAT(GetData(header_storage_key),
-              EntityDataHasSpecifics(
-                  MatchesHeader(kLocalSessionTag, {kWindowId2}, {kTabId3})));
+  // One tab node de should be free at this point. In the current implementation
+  // (subject to change), this is |kTabNodeId1|. This is because |kTabId3| is
+  // assigned |kTabNodeId2|.
+  ASSERT_THAT(
+      GetAllData(),
+      UnorderedElementsAre(
+          Pair(header_storage_key,
+               EntityDataHasSpecifics(
+                   MatchesHeader(kLocalSessionTag, {kWindowId2}, {kTabId3}))),
+          Pair(tab_storage_key2, EntityDataHasSpecifics(MatchesTab(
+                                     kLocalSessionTag, kWindowId2, kTabId3,
+                                     kTabNodeId2, {"http://qux.com/"})))));
 
-  // |kTabNodeId1| should be free at this point. When a new tab is opened
-  // (|kTabId4|), it should be reused.
+  // When a new tab is opened (|kTabId4|), |kTabNodeId1| should be reused.
   AddTab(kWindowId2, "http://quux.com/", kTabId4);
   EXPECT_THAT(
       GetAllData(),