Fix tab nodes not marked as free during restore

When the local session is restored from persistence, if there are orphan
tabs, they should be marked as free, to prevent newly created tabs from
creating new tab nodes, instead of recycling available ones.

The restore flow exercises SyncedSessionTracker::CleanupSession(), which
prior to this patch wasn't smart enough for the local session.

Bug: 875671,847325
Change-Id: I91f9f13ad4aedbbd85bfcc73c606096781b80527
Reviewed-on: https://chromium-review.googlesource.com/1204134
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588827}

components/sync_sessions/session_sync_bridge_unittest.cc

@@ -298,6 +298,10 @@ class SessionSyncBridgeTest : public ::testing::Test {
     return tab;
   }
 
+  void CloseTab(int tab_id) {
+    window_getter_.CloseTab(SessionID::FromSerializedValue(tab_id));
+  }
+
   void SessionRestoreComplete() { window_getter_.SessionRestoreComplete(); }
 
   SessionSyncBridge* bridge() { return bridge_.get(); }
@@ -873,6 +877,77 @@ TEST_F(SessionSyncBridgeTest, ShouldExposeTabbedWindowAfterCustomTabOnly) {
                               /*tab_node_id=*/1, {"http://bar.com/"})))));
 }
 
+TEST_F(SessionSyncBridgeTest, ShouldRestoreLocalSessionWithFreedTab) {
+  const int kWindowId1 = 1000001;
+  const int kWindowId2 = 1000002;
+  const int kTabId1 = 1000003;
+  const int kTabId2 = 1000004;
+  const int kTabId3 = 1000005;
+  const int kTabId4 = 1000006;
+  // Zero is the first assigned tab node ID.
+  const int kTabNodeId1 = 0;
+  const int kTabNodeId2 = 1;
+
+  AddWindow(kWindowId1);
+  TestSyncedTabDelegate* tab1 = AddTab(kWindowId1, "http://foo.com/", kTabId1);
+  AddTab(kWindowId1, "http://bar.com/", kTabId2);
+
+  const std::string header_storage_key =
+      SessionStore::GetHeaderStorageKey(kLocalSessionTag);
+  const std::string tab_storage_key1 =
+      SessionStore::GetTabStorageKey(kLocalSessionTag, kTabNodeId1);
+  const std::string tab_storage_key2 =
+      SessionStore::GetTabStorageKey(kLocalSessionTag, kTabNodeId2);
+
+  InitializeBridge();
+  StartSyncing();
+
+  ASSERT_THAT(GetData(header_storage_key),
+              EntityDataHasSpecifics(MatchesHeader(
+                  kLocalSessionTag, {kWindowId1}, {kTabId1, kTabId2})));
+
+  // Close |kTabId2| and force reassociation by navigating in the remaining open
+  // tab, leading to a freed tab entity.
+  CloseTab(kTabId2);
+  tab1->Navigate("http://baz.com/");
+
+  ASSERT_THAT(GetData(header_storage_key),
+              EntityDataHasSpecifics(
+                  MatchesHeader(kLocalSessionTag, {kWindowId1}, {kTabId1})));
+
+  ShutdownBridge();
+  ResetWindows();
+
+  // The browser gets restarted with a new initial tab, for example because the
+  // user chose "Continue where you left off".
+  AddWindow(kWindowId2);
+  AddTab(kWindowId2, "http://qux.com/", kTabId3);
+
+  // Start the bridge again.
+  InitializeBridge();
+  StartSyncing();
+
+  ASSERT_THAT(GetData(header_storage_key),
+              EntityDataHasSpecifics(
+                  MatchesHeader(kLocalSessionTag, {kWindowId2}, {kTabId3})));
+
+  // |kTabNodeId1| should be free at this point. When a new tab is opened
+  // (|kTabId4|), it should be reused.
+  AddTab(kWindowId2, "http://quux.com/", kTabId4);
+  EXPECT_THAT(
+      GetAllData(),
+      UnorderedElementsAre(
+          Pair(header_storage_key,
+               EntityDataHasSpecifics(MatchesHeader(
+                   kLocalSessionTag, {kWindowId2}, {kTabId3, kTabId4}))),
+          Pair(tab_storage_key2, EntityDataHasSpecifics(MatchesTab(
+                                     kLocalSessionTag, kWindowId2, kTabId3,
+                                     kTabNodeId2, {"http://qux.com/"}))),
+          Pair(tab_storage_key1, EntityDataHasSpecifics(MatchesTab(
+                                     kLocalSessionTag, kWindowId2, kTabId4,
+                                     kTabNodeId1, {"http://quux.com/"})))));
+}
+
 TEST_F(SessionSyncBridgeTest, ShouldDisableSyncAndReenable) {
   const int kWindowId = 1000001;
   const int kTabId = 1000002;

components/sync_sessions/synced_session_tracker.cc

@@ -330,8 +330,12 @@ void SyncedSessionTracker::CleanupSessionImpl(const std::string& session_tag) {
     session->synced_window_map.erase(window_pair.first);
   session->unmapped_windows.clear();
 
-  for (const auto& tab_pair : session->unmapped_tabs)
+  for (const auto& tab_pair : session->unmapped_tabs) {
     session->synced_tab_map.erase(tab_pair.first);
+
+    if (session_tag == local_session_tag_)
+      session->tab_node_pool.FreeTab(tab_pair.first);
+  }
   session->unmapped_tabs.clear();
 }
 
@@ -494,8 +498,6 @@ void SyncedSessionTracker::CleanupSession(const std::string& session_tag) {
 void SyncedSessionTracker::CleanupLocalTabs(std::set<int>* deleted_node_ids) {
   DCHECK(!local_session_tag_.empty());
   TrackedSession* session = GetTrackedSession(local_session_tag_);
-  for (const auto& tab_pair : session->unmapped_tabs)
-    session->tab_node_pool.FreeTab(tab_pair.first);
   CleanupSessionImpl(local_session_tag_);
   session->tab_node_pool.CleanupTabNodes(deleted_node_ids);
 }

components/sync_sessions/synced_session_tracker.h

@@ -172,9 +172,9 @@ class SyncedSessionTracker {
   // Gets the session tag previously set with InitLocalSession().
   const std::string& GetLocalSessionTag() const;
 
-  // Similar to CleanupForeignSession, but also marks any unmapped tabs as free
-  // in the tab node pool and fills |deleted_node_ids| with the set of locally
-  // free tab nodes to be deleted.
+  // Similar to CleanupSession() but also triggers garbage collection of free
+  // tab nodes and consequently fills |deleted_node_ids| with the set of
+  // locally free tab nodes to be deleted.
   void CleanupLocalTabs(std::set<int>* deleted_node_ids);
 
   // Returns the tab node ID for |tab_id| if an existing tab node was found, or
@@ -264,7 +264,7 @@ class SyncedSessionTracker {
       SessionLookup lookup,
       bool exclude_local_session) const;
 
-  // Implementation of CleanupForeignSession/CleanupLocalTabs.
+  // Implementation of CleanupSession()/CleanupLocalTabs().
   void CleanupSessionImpl(const std::string& session_tag);
 
   // The client of the sync sessions datatype.

components/sync_sessions/test_synced_window_delegates_getter.cc

@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/stl_util.h"
 #include "components/sessions/core/serialized_navigation_entry_test_helper.h"
 #include "components/sync_sessions/synced_session.h"
 #include "components/sync_sessions/tab_node_pool.h"
@@ -249,6 +250,13 @@ void TestSyncedWindowDelegate::OverrideTabAt(int index,
   tab_delegates_[index] = delegate;
 }
 
+void TestSyncedWindowDelegate::CloseTab(SessionID tab_id) {
+  base::EraseIf(tab_delegates_,
+                [tab_id](const std::pair<int, SyncedTabDelegate*>& entry) {
+                  return entry.second->GetSessionId() == tab_id;
+                });
+}
+
 void TestSyncedWindowDelegate::SetIsSessionRestoreInProgress(bool value) {
   is_session_restore_in_progress_ = value;
 }
@@ -346,6 +354,14 @@ TestSyncedTabDelegate* TestSyncedWindowDelegatesGetter::AddTab(
   return tabs_.back().get();
 }
 
+void TestSyncedWindowDelegatesGetter::CloseTab(SessionID tab_id) {
+  for (auto& window : windows_) {
+    // CloseTab() will only take effect with the belonging window, the rest will
+    // simply ignore the call.
+    window->CloseTab(tab_id);
+  }
+}
+
 void TestSyncedWindowDelegatesGetter::SessionRestoreComplete() {
   for (auto& window : windows_)
     window->SetIsSessionRestoreInProgress(false);

components/sync_sessions/test_synced_window_delegates_getter.h

@@ -122,6 +122,8 @@ class TestSyncedWindowDelegate : public SyncedWindowDelegate {
   // |delegate| must not be nullptr and must outlive this object.
   void OverrideTabAt(int index, SyncedTabDelegate* delegate);
 
+  void CloseTab(SessionID tab_id);
+
   void SetIsSessionRestoreInProgress(bool value);
 
   // SyncedWindowDelegate overrides.
@@ -143,8 +145,6 @@ class TestSyncedWindowDelegate : public SyncedWindowDelegate {
   const sync_pb::SessionWindow_BrowserType window_type_;
 
   std::map<int, SyncedTabDelegate*> tab_delegates_;
-  std::map<int, SyncedTabDelegate*> tab_overrides_;
-  std::map<int, SessionID> tab_id_overrides_;
   bool is_session_restore_in_progress_;
 
   DISALLOW_COPY_AND_ASSIGN(TestSyncedWindowDelegate);
@@ -164,6 +164,7 @@ class TestSyncedWindowDelegatesGetter : public SyncedWindowDelegatesGetter {
   // TestSyncedTabDelegate (not owned).
   TestSyncedTabDelegate* AddTab(SessionID window_id,
                                 SessionID tab_id = SessionID::NewUnique());
+  void CloseTab(SessionID tab_id);
   void SessionRestoreComplete();
   LocalSessionEventRouter* router();