Make vending of sync IDs (tab_node_id) more explicit

Prior to this patch, it was buried in GetTabNodeFromLocalTabId() which
sometimes made a lookup only (if the tab ID was already associated) and
some other times vended a new sync ID (via
TabNodePool::AssociateWithFreeTabNode()).

We refactor the APIs to surface this logic more explicitly in the only
calling site, in LocalSessionEventHandlerImpl.

As a bonus point, a related test is added to SessionSyncBridgeTest,
which also passes prior to this patch.

Bug: 843554
Change-Id: Ie9a2b136dbfb631e43d3abf2c345c07036670eb9
Reviewed-on: https://chromium-review.googlesource.com/1071572Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561478}

components/sync_sessions/local_session_event_handler_impl.cc

@@ -348,15 +348,17 @@ void LocalSessionEventHandlerImpl::AssociateTab(
   DVLOG(1) << "Syncing tab " << tab_id.id() << " from window "
            << tab_delegate->GetWindowId().id();
 
-  int tab_node_id = tab_delegate->GetSyncId();
+  int tab_node_id =
+      session_tracker_->LookupTabNodeFromTabId(current_session_tag_, tab_id);
+
   if (tab_node_id != TabNodePool::kInvalidTabNodeID) {
-    DCHECK_EQ(tab_id, session_tracker_->LookupTabIdFromTabNodeId(
-                          current_session_tag_, tab_node_id));
+    DCHECK(tab_delegate->GetSyncId() == TabNodePool::kInvalidTabNodeID ||
+           tab_delegate->GetSyncId() == tab_node_id);
   } else if (has_tabbed_window) {
-    session_tracker_->GetTabNodeFromLocalTabId(tab_id, &tab_node_id);
-    CHECK_NE(TabNodePool::kInvalidTabNodeID, tab_node_id)
+    // Allocate a new (or reused) sync node for this tab.
+    tab_node_id = session_tracker_->AssociateLocalTabWithFreeTabNode(tab_id);
+    DCHECK_NE(TabNodePool::kInvalidTabNodeID, tab_node_id)
         << "https://crbug.com/639009";
-    tab_delegate->SetSyncId(tab_node_id);
   } else {
     // Only allowed to allocate sync ids when we have native data, which is only
     // true when we have a tabbed window. Without a sync id we cannot sync this
@@ -367,6 +369,8 @@ void LocalSessionEventHandlerImpl::AssociateTab(
     return;
   }
 
+  tab_delegate->SetSyncId(tab_node_id);
+
   // Get the previously synced url.
   sessions::SessionTab* session_tab =
       session_tracker_->GetTab(current_session_tag_, tab_id);

components/sync_sessions/session_sync_bridge_unittest.cc

@@ -770,6 +770,57 @@ TEST_F(SessionSyncBridgeTest, ShouldIgnoreIfCustomTabOnlyOnStartup) {
                                                kLocalSessionTag, _, _)))));
 }
 
+// Ensure that all tabs are exposed in a scenario where only a custom tab
+// (without tabbed windows) was present during startup, and later tabbed windows
+// appear (browser started).
+TEST_F(SessionSyncBridgeTest, ShouldExposeTabbedWindowAfterCustomTabOnly) {
+  const int kWindowId1 = 1000001;
+  const int kWindowId2 = 1000002;
+  const int kTabId1 = 1000003;
+  const int kTabId2 = 1000004;
+
+  AddWindow(kWindowId1, sync_pb::SessionWindow_BrowserType_TYPE_CUSTOM_TAB);
+  TestSyncedTabDelegate* custom_tab =
+      AddTab(kWindowId1, "http://foo.com/", kTabId1);
+
+  InitializeBridge();
+  StartSyncing();
+
+  ASSERT_THAT(GetAllData(),
+              UnorderedElementsAre(Pair(_, EntityDataHasSpecifics(MatchesHeader(
+                                               kLocalSessionTag, _, _)))));
+
+  // Load the actual tabbed window, now that we're syncing.
+  AddWindow(kWindowId2);
+  AddTab(kWindowId2, "http://bar.com/", kTabId2);
+
+  // The local change should be created and tracked correctly. This doesn't
+  // actually start syncing the custom tab yet, because the tab itself isn't
+  // associated yet.
+  EXPECT_THAT(GetAllData(),
+              UnorderedElementsAre(
+                  Pair(_, EntityDataHasSpecifics(MatchesHeader(
+                              kLocalSessionTag, {kWindowId2}, {kTabId2}))),
+                  Pair(_, EntityDataHasSpecifics(MatchesTab(
+                              kLocalSessionTag, kWindowId2, kTabId2,
+                              /*tab_node_id=*/0, {"http://bar.com/"})))));
+
+  // Now trigger OnLocalTabModified() for the custom tab again, it should sync.
+  custom_tab->Navigate("http://baz.com/");
+  EXPECT_THAT(GetAllData(),
+              UnorderedElementsAre(
+                  Pair(_, EntityDataHasSpecifics(MatchesHeader(
+                              kLocalSessionTag, {kWindowId1, kWindowId2},
+                              {kTabId1, kTabId2}))),
+                  Pair(_, EntityDataHasSpecifics(MatchesTab(
+                              kLocalSessionTag, kWindowId2, kTabId2,
+                              /*tab_node_id=*/0, {"http://bar.com/"}))),
+                  Pair(_, EntityDataHasSpecifics(MatchesTab(
+                              kLocalSessionTag, kWindowId1, kTabId1,
+                              /*tab_node_id=*/1,
+                              {"http://foo.com/", "http://baz.com/"})))));
+}
+
 // Ensure that newly assigned tab node IDs do not conflict with IDs provided
 // by the delegate, for IDs the tracker might not know about. This is possible
 // for example if an Android client gets killed after Android's tab restore

components/sync_sessions/synced_session_tracker.cc

@@ -515,8 +515,7 @@ SessionID SyncedSessionTracker::LookupTabIdFromTabNodeId(
   return session->tab_node_pool.GetTabIdFromTabNodeId(tab_node_id);
 }
 
-bool SyncedSessionTracker::GetTabNodeFromLocalTabId(SessionID tab_id,
-                                                    int* tab_node_id) {
+int SyncedSessionTracker::AssociateLocalTabWithFreeTabNode(SessionID tab_id) {
   DCHECK(!local_session_tag_.empty());
   TrackedSession* session = GetTrackedSession(local_session_tag_);
 
@@ -527,17 +526,7 @@ bool SyncedSessionTracker::GetTabNodeFromLocalTabId(SessionID tab_id,
   // sync and as consistent as possible.
   GetTab(local_session_tag_, tab_id);  // Ignore result.
 
-  *tab_node_id = session->tab_node_pool.GetTabNodeIdFromTabId(tab_id);
-  if (*tab_node_id != TabNodePool::kInvalidTabNodeID) {
-    return true;  // Reused existing tab.
-  }
-
-  // Could not reuse an existing tab so create a new one.
-  bool reused_existing_tab = false;
-  *tab_node_id = session->tab_node_pool.AssociateWithFreeTabNode(
-      tab_id, &reused_existing_tab);
-  DCHECK_NE(TabNodePool::kInvalidTabNodeID, *tab_node_id);
-  return reused_existing_tab;
+  return session->tab_node_pool.AssociateWithFreeTabNode(tab_id);
 }
 
 void SyncedSessionTracker::ReassociateLocalTab(int tab_node_id,

components/sync_sessions/synced_session_tracker.h

@@ -189,10 +189,9 @@ class SyncedSessionTracker {
   SessionID LookupTabIdFromTabNodeId(const std::string& session_tag,
                                      int tab_node_id) const;
 
-  // Fills |tab_node_id| with a tab node for |tab_id|. Returns true if an
-  // existing tab node was found, false if there was none and one had to be
-  // created.
-  bool GetTabNodeFromLocalTabId(SessionID tab_id, int* tab_node_id);
+  // Returns a valid tab node for |tab_id|. Will reuse an existing tab node if
+  // possible, and otherwise create a new one.
+  int AssociateLocalTabWithFreeTabNode(SessionID tab_id);
 
   // Reassociates the tab denoted by |tab_node_id| with a new tab id, preserving
   // any previous SessionTab object the node was associated with. This is useful

components/sync_sessions/synced_session_tracker_unittest.cc

@@ -498,7 +498,6 @@ TEST_F(SyncedSessionTrackerTest, DeleteForeignTab) {
 
 TEST_F(SyncedSessionTrackerTest, CleanupLocalTabs) {
   std::set<int> free_node_ids;
-  int tab_node_id = TabNodePool::kInvalidTabNodeID;
 
   GetTracker()->InitLocalSession(kTag, kSessionName, kDeviceType);
 
@@ -519,7 +518,7 @@ TEST_F(SyncedSessionTrackerTest, CleanupLocalTabs) {
   GetTracker()->ResetSessionTracking(kTag);
   GetTracker()->PutWindowInSession(kTag, kWindow1);
   GetTracker()->PutTabInWindow(kTag, kWindow1, kTab1);
-  EXPECT_TRUE(GetTracker()->GetTabNodeFromLocalTabId(kTab1, &tab_node_id));
+  EXPECT_EQ(kTabNode1, GetTracker()->AssociateLocalTabWithFreeTabNode(kTab1));
   GetTracker()->CleanupLocalTabs(&free_node_ids);
   EXPECT_TRUE(free_node_ids.empty());
 
@@ -529,20 +528,20 @@ TEST_F(SyncedSessionTrackerTest, CleanupLocalTabs) {
   EXPECT_FALSE(GetLocalTabNodePool()->Full());
 
   // Simulate a tab opening, which should use the last free tab node.
-  EXPECT_TRUE(GetTracker()->GetTabNodeFromLocalTabId(kTab2, &tab_node_id));
+  EXPECT_EQ(kTabNode2, GetTracker()->AssociateLocalTabWithFreeTabNode(kTab2));
+  EXPECT_EQ(kTabNode2, GetTracker()->LookupTabNodeFromTabId(kTag, kTab2));
   EXPECT_TRUE(GetLocalTabNodePool()->Empty());
 
   // Simulate another tab opening, which should create a new associated tab
   // node.
-  EXPECT_FALSE(GetTracker()->GetTabNodeFromLocalTabId(kTab3, &tab_node_id));
-  EXPECT_EQ(kTabNode3, tab_node_id);
+  EXPECT_EQ(kTabNode3, GetTracker()->AssociateLocalTabWithFreeTabNode(kTab3));
+  EXPECT_EQ(kTabNode3, GetTracker()->LookupTabNodeFromTabId(kTag, kTab3));
   EXPECT_EQ(3U, GetLocalTabNodePool()->Capacity());
   EXPECT_TRUE(GetLocalTabNodePool()->Empty());
 
-  // Fetching the same tab should return the same tab node id.
-  EXPECT_TRUE(GetTracker()->GetTabNodeFromLocalTabId(kTab3, &tab_node_id));
-  EXPECT_EQ(kTabNode3, tab_node_id);
-  EXPECT_TRUE(GetLocalTabNodePool()->Empty());
+  // Previous tabs should still be associated.
+  EXPECT_EQ(kTabNode1, GetTracker()->LookupTabNodeFromTabId(kTag, kTab1));
+  EXPECT_EQ(kTabNode2, GetTracker()->LookupTabNodeFromTabId(kTag, kTab2));
 
   // Associate with no tabs. All tabs should be freed again, and the pool
   // should now be full.
@@ -664,8 +663,8 @@ TEST_F(SyncedSessionTrackerTest, ReassociateTabMappedTwice) {
   // Attempting to access the original tab will create a new SessionTab object.
   EXPECT_NE(GetTracker()->GetTab(kTag, kTab1),
             GetTracker()->GetTab(kTag, kTab2));
-  int tab_node_id = -1;
-  EXPECT_FALSE(GetTracker()->GetTabNodeFromLocalTabId(kTab1, &tab_node_id));
+  EXPECT_EQ(TabNodePool::kInvalidTabNodeID,
+            GetTracker()->LookupTabNodeFromTabId(kTag, kTab1));
   ASSERT_TRUE(VerifyTabIntegrity(kTag));
 }
 

components/sync_sessions/tab_node_pool.cc

@@ -72,22 +72,17 @@ void TabNodePool::FreeTab(SessionID tab_id) {
   free_nodes_pool_.insert(tab_node_id);
 }
 
-int TabNodePool::AssociateWithFreeTabNode(SessionID tab_id,
-                                          bool* reused_existing_node) {
+int TabNodePool::AssociateWithFreeTabNode(SessionID tab_id) {
+  DCHECK_EQ(0U, tabid_nodeid_map_.count(tab_id));
+
   int tab_node_id;
   if (free_nodes_pool_.empty()) {
     // Tab pool has no free nodes, allocate new one.
     tab_node_id = ++max_used_tab_node_id_;
     AddTabNode(tab_node_id);
-    if (reused_existing_node) {
-      *reused_existing_node = false;
-    }
   } else {
     // Return the next free node.
     tab_node_id = *free_nodes_pool_.begin();
-    if (reused_existing_node) {
-      *reused_existing_node = true;
-    }
   }
 
   AssociateTabNode(tab_node_id, tab_id);

components/sync_sessions/tab_node_pool.h

@@ -51,12 +51,9 @@ class TabNodePool {
   SessionID GetTabIdFromTabNodeId(int tab_node_id) const;
 
   // Gets the next free tab node (or creates a new one if needed) and associates
-  // it to |tab_id|. Returns the tab node ID associated to |tab_id|. If
-  // |reused_existing_node| is not null, it will be set with a value that
-  // represents if an existing free tab node was reused or a new one created.
-  // TODO(crbug.com/681921): Remove all logic related to |reused_existing_node|
-  // once the migration to USS is finished.
-  int AssociateWithFreeTabNode(SessionID tab_id, bool* reused_existing_node);
+  // it to |tab_id|. Returns the tab node ID associated to |tab_id|. |tab_id|
+  // must not be previously associated.
+  int AssociateWithFreeTabNode(SessionID tab_id);
 
   // Reassociates |tab_node_id| with |tab_id|. If |tab_node_id| is not already
   // known, it is added to the tab node pool before being associated.

components/sync_sessions/tab_node_pool_unittest.cc

@@ -62,8 +62,7 @@ TEST_F(SyncTabNodePoolTest, TabNodeIdIncreases) {
     ASSERT_EQ(TabNodePool::kInvalidTabNodeID,
               pool_.GetTabNodeIdFromTabId(tab_id));
     EXPECT_NE(TabNodePool::kInvalidTabNodeID,
-              pool_.AssociateWithFreeTabNode(tab_id,
-                                             /*reused_existing_tab=*/nullptr));
+              pool_.AssociateWithFreeTabNode(tab_id));
     EXPECT_EQ(kTabNodeId3, GetMaxUsedTabNodeId());
   }
   pool_.CleanupTabNodes(&deleted_node_ids);
@@ -146,8 +145,7 @@ TEST_F(SyncTabNodePoolTest, ReassociateThenFree) {
     const SessionID tab_id = SessionID::FromSerializedValue(i);
     EXPECT_EQ(TabNodePool::kInvalidTabNodeID,
               pool_.GetTabNodeIdFromTabId(tab_id));
-    sync_ids.push_back(pool_.AssociateWithFreeTabNode(
-        tab_id, /*reused_existing_tab=*/nullptr));
+    sync_ids.push_back(pool_.AssociateWithFreeTabNode(tab_id));
   }
 
   EXPECT_TRUE(pool_.Empty());
@@ -166,16 +164,14 @@ TEST_F(SyncTabNodePoolTest, AddGet) {
   EXPECT_EQ(2U, pool_.Capacity());
   ASSERT_EQ(TabNodePool::kInvalidTabNodeID,
             pool_.GetTabNodeIdFromTabId(kTabId1));
-  EXPECT_EQ(5, pool_.AssociateWithFreeTabNode(kTabId1,
-                                              /*reused_existing_tab=*/nullptr));
+  EXPECT_EQ(5, pool_.AssociateWithFreeTabNode(kTabId1));
   EXPECT_FALSE(pool_.Empty());
   EXPECT_FALSE(pool_.Full());
   EXPECT_EQ(2U, pool_.Capacity());
   ASSERT_EQ(TabNodePool::kInvalidTabNodeID,
             pool_.GetTabNodeIdFromTabId(kTabId2));
   // 5 is now used, should return 10.
-  EXPECT_EQ(10, pool_.AssociateWithFreeTabNode(
-                    kTabId2, /*reused_existing_tab=*/nullptr));
+  EXPECT_EQ(10, pool_.AssociateWithFreeTabNode(kTabId2));
 }
 
 TEST_F(SyncTabNodePoolTest, AssociateWithFreeTabNode) {
@@ -183,18 +179,14 @@ TEST_F(SyncTabNodePoolTest, AssociateWithFreeTabNode) {
             pool_.GetTabNodeIdFromTabId(kTabId1));
   ASSERT_EQ(TabNodePool::kInvalidTabNodeID,
             pool_.GetTabNodeIdFromTabId(kTabId2));
-  bool reused_existing_tab = false;
-  EXPECT_EQ(0, pool_.AssociateWithFreeTabNode(kTabId1, &reused_existing_tab));
-  EXPECT_FALSE(reused_existing_tab);
+  EXPECT_EQ(0, pool_.AssociateWithFreeTabNode(kTabId1));
   EXPECT_EQ(0, pool_.GetTabNodeIdFromTabId(kTabId1));
   ASSERT_EQ(TabNodePool::kInvalidTabNodeID,
             pool_.GetTabNodeIdFromTabId(kTabId2));
-  EXPECT_EQ(1, pool_.AssociateWithFreeTabNode(kTabId2, &reused_existing_tab));
-  EXPECT_FALSE(reused_existing_tab);
+  EXPECT_EQ(1, pool_.AssociateWithFreeTabNode(kTabId2));
   EXPECT_EQ(1, pool_.GetTabNodeIdFromTabId(kTabId2));
   pool_.FreeTab(kTabId1);
-  EXPECT_EQ(0, pool_.AssociateWithFreeTabNode(kTabId3, &reused_existing_tab));
-  EXPECT_TRUE(reused_existing_tab);
+  EXPECT_EQ(0, pool_.AssociateWithFreeTabNode(kTabId3));
 }
 
 TEST_F(SyncTabNodePoolTest, TabPoolFreeNodeLimits) {
@@ -205,8 +197,8 @@ TEST_F(SyncTabNodePoolTest, TabPoolFreeNodeLimits) {
   // kFreeNodesLowWatermark.
   std::vector<int> used_sync_ids;
   for (size_t i = 1; i <= TabNodePool::kFreeNodesHighWatermark + 1; ++i) {
-    used_sync_ids.push_back(pool_.AssociateWithFreeTabNode(
-        SessionID::FromSerializedValue(i), /*reused_existing_tab=*/nullptr));
+    used_sync_ids.push_back(
+        pool_.AssociateWithFreeTabNode(SessionID::FromSerializedValue(i)));
   }
 
   // Free all except one node.