[Sync] Handle reassociation of tabs where the old tab is already mapped.

If ReassociateLocalTab is called with a tab node that is already mapped to a
tab, it's possible to wind up with the synced session tracker holding two
tab objects referring to the same tab id. This can lead to memory corruption.

This CL makes the tracker defensive against that scenario, and adds some more
data verification in the unit tests

BUG=714524, 639009

Review-Url: https://codereview.chromium.org/2856913007
Cr-Commit-Position: refs/heads/master@{#469553}

components/sync_sessions/sessions_sync_manager.cc

@@ -864,9 +864,6 @@ bool SessionsSyncManager::InitFromSyncModel(
               rewritten_specifics.tab_node_id(), new_tab_id);
           UpdateTrackerWithSpecifics(rewritten_specifics,
                                      remote.GetModifiedTime());
-
-          session_tracker_.ReassociateLocalTab(
-              rewritten_specifics.tab_node_id(), new_tab_id);
         }
       }
     }

components/sync_sessions/sessions_sync_manager_unittest.cc

@@ -1695,7 +1695,8 @@ TEST_F(SessionsSyncManagerTest, ProcessForeignDeleteTabsWithReusedNodeIds) {
 
 TEST_F(SessionsSyncManagerTest, AssociationReusesNodes) {
   SyncChangeList changes;
-  AddTab(AddWindow()->GetSessionId(), kFoo1);
+  TestSyncedWindowDelegate* window = AddWindow();
+  TestSyncedTabDelegate* tab = AddTab(window->GetSessionId(), kFoo1);
   InitWithSyncDataTakeOutput(SyncDataList(), &changes);
   ASSERT_TRUE(ChangeTypeMatches(changes,
                                 {SyncChange::ACTION_ADD, SyncChange::ACTION_ADD,
@@ -1728,6 +1729,24 @@ TEST_F(SessionsSyncManagerTest, AssociationReusesNodes) {
   VerifyLocalTabChange(changes[0], 1, kFoo1);
   EXPECT_EQ(tab_node_id,
             changes[0].sync_data().GetSpecifics().session().tab_node_id());
+  changes.clear();
+
+  // Update the original tab. Ensure the same tab node is updated.
+  NavigateTab(tab, kFoo2);
+  FilterOutLocalHeaderChanges(&changes);
+  ASSERT_TRUE(ChangeTypeMatches(changes, {SyncChange::ACTION_UPDATE}));
+  VerifyLocalTabChange(changes[0], 2, kFoo2);
+  EXPECT_EQ(tab_node_id,
+            changes[0].sync_data().GetSpecifics().session().tab_node_id());
+  changes.clear();
+
+  // Add a new tab. It should reuse the second tab node.
+  AddTab(window->GetSessionId(), kBar1);
+  FilterOutLocalHeaderChanges(&changes);
+  ASSERT_TRUE(ChangeTypeMatches(changes, {SyncChange::ACTION_UPDATE}));
+  VerifyLocalTabChange(changes[0], 1, kBar1);
+  EXPECT_EQ(tab_node_id + 1,
+            changes[0].sync_data().GetSpecifics().session().tab_node_id());
 }
 
 // Ensure that the merge process deletes a tab node without a tab id.

components/sync_sessions/synced_session_tracker.cc

@@ -397,33 +397,70 @@ void SyncedSessionTracker::ReassociateLocalTab(int tab_node_id,
 
   SessionID::id_type old_tab_id =
       local_tab_pool_.GetTabIdFromTabNodeId(tab_node_id);
+  if (new_tab_id == old_tab_id) {
+    return;
+  }
+
   local_tab_pool_.ReassociateTabNode(tab_node_id, new_tab_id);
 
   sessions::SessionTab* tab_ptr = nullptr;
 
+  auto new_tab_iter = synced_tab_map_[local_session_tag_].find(new_tab_id);
   auto old_tab_iter = synced_tab_map_[local_session_tag_].find(old_tab_id);
   if (old_tab_id != kInvalidTabID &&
       old_tab_iter != synced_tab_map_[local_session_tag_].end()) {
     tab_ptr = old_tab_iter->second;
+    DCHECK(tab_ptr);
+
     // Remove the tab from the synced tab map under the old id.
     synced_tab_map_[local_session_tag_].erase(old_tab_iter);
-  } else {
+
+    if (new_tab_iter != synced_tab_map_[local_session_tag_].end()) {
+      // If both the old and the new tab already exist, delete the new tab
+      // and use the old tab in its place.
+      auto unmapped_tabs_iter =
+          unmapped_tabs_[local_session_tag_].find(new_tab_id);
+      if (unmapped_tabs_iter != unmapped_tabs_[local_session_tag_].end()) {
+        unmapped_tabs_[local_session_tag_].erase(unmapped_tabs_iter);
+      } else {
+        sessions::SessionTab* new_tab_ptr = new_tab_iter->second;
+        for (auto& window_iter_pair : GetSession(local_session_tag_)->windows) {
+          auto& window_tabs = window_iter_pair.second->wrapped_window.tabs;
+          auto tab_iter = std::find_if(
+              window_tabs.begin(), window_tabs.end(),
+              [&new_tab_ptr](const std::unique_ptr<sessions::SessionTab>& tab) {
+                return tab.get() == new_tab_ptr;
+              });
+          if (tab_iter != window_tabs.end()) {
+            window_tabs.erase(tab_iter);
+            break;
+          }
+        }
+      }
+
+      synced_tab_map_[local_session_tag_].erase(new_tab_iter);
+    }
+
+    // If the old tab is unmapped, update the tab id under which it is
+    // indexed.
+    auto unmapped_tabs_iter =
+        unmapped_tabs_[local_session_tag_].find(old_tab_id);
+    if (old_tab_id != kInvalidTabID &&
+        unmapped_tabs_iter != unmapped_tabs_[local_session_tag_].end()) {
+      std::unique_ptr<sessions::SessionTab> tab =
+          std::move(unmapped_tabs_iter->second);
+      DCHECK_EQ(tab_ptr, tab.get());
+      unmapped_tabs_[local_session_tag_].erase(unmapped_tabs_iter);
+      unmapped_tabs_[local_session_tag_][new_tab_id] = std::move(tab);
+    }
+  }
+
+  if (tab_ptr == nullptr) {
     // It's possible a placeholder is already in place for the new tab. If so,
     // reuse it, otherwise create a new one (which will default to unmapped).
     tab_ptr = GetTab(local_session_tag_, new_tab_id);
   }
 
-  // If the old tab is unmapped, update the tab id under which it is indexed.
-  auto unmapped_tabs_iter = unmapped_tabs_[local_session_tag_].find(old_tab_id);
-  if (old_tab_id != kInvalidTabID &&
-      unmapped_tabs_iter != unmapped_tabs_[local_session_tag_].end()) {
-    std::unique_ptr<sessions::SessionTab> tab =
-        std::move(unmapped_tabs_iter->second);
-    DCHECK_EQ(tab_ptr, tab.get());
-    unmapped_tabs_[local_session_tag_].erase(unmapped_tabs_iter);
-    unmapped_tabs_[local_session_tag_][new_tab_id] = std::move(tab);
-  }
-
   // Update the tab id.
   if (old_tab_id != kInvalidTabID) {
     DVLOG(1) << "Remapped tab " << old_tab_id << " with node " << tab_node_id

components/sync_sessions/synced_session_tracker.h

@@ -174,6 +174,9 @@ class SyncedSessionTracker {
   // any previous SessionTab object the node was associated with. This is useful
   // on restart when sync needs to reassociate tabs from a previous session with
   // newly restored tabs (and can be used in conjunction with PutTabInWindow).
+  // If |new_tab_id| is already associated with a tab object, that tab will be
+  // overwritten. Reassociating a tab with a node it is already mapped to will
+  // have no effect.
   void ReassociateLocalTab(int tab_node_id, SessionID::id_type new_tab_id);
 
   // **** Methods for querying/manipulating overall state ****.