Simplify remapping of session IDs for session sync

The remapping is no longer needed after
https://chromium-review.googlesource.com/1074750, which guarantees that
session IDs do not collide across restarts of the browser.

This reduces the number of IO operations, makes the sync protocol itself
more robust, and greatly simplifies a code that is believed to be
problematic.

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

components/sync_sessions/local_session_event_handler_impl.cc

@@ -220,32 +220,6 @@ void LocalSessionEventHandlerImpl::AssociateWindows(ReloadTabsOption option,
     DVLOG(1) << "Found no tabbed windows. Reloading "
              << current_session->windows.size()
              << " windows from previous session.";
-
-    // A copy of the specifics must be made because |current_session| will be
-    // updated in place and therefore can't be relied on as the source of truth.
-    sync_pb::SessionSpecifics specifics;
-    specifics.set_session_tag(current_session_tag_);
-    current_session->ToSessionHeaderProto().Swap(specifics.mutable_header());
-    UpdateTrackerWithSpecifics(specifics, base::Time::Now(), session_tracker_);
-
-    // The tab entities stored in sync have outdated SessionId values. Go
-    // through and update them to the new SessionIds.
-    for (auto& win_iter : current_session->windows) {
-      for (auto& tab : win_iter.second->wrapped_window.tabs) {
-        int sync_id = session_tracker_->LookupTabNodeFromTabId(
-            current_session_tag_, tab->tab_id);
-        // In rare cases, the local model could contain a tab that doesn't
-        // have a sync ID, if the restored header referenced a tab ID but the
-        // tab entity didn't exist (e.g. was unsyncable).
-        if (sync_id == TabNodePool::kInvalidTabNodeID) {
-          continue;
-        }
-
-        DVLOG(1) << "Rewriting tab node " << sync_id << " with tab id "
-                 << tab->tab_id.id();
-        AppendChangeForExistingTab(sync_id, *tab, batch);
-      }
-    }
   }
 
   for (auto& window_iter_pair : windows) {
@@ -537,19 +511,12 @@ void LocalSessionEventHandlerImpl::AssociateRestoredPlaceholderTab(
     return;
   }
 
-  AppendChangeForExistingTab(tab_node_id, *local_tab, batch);
-}
-
-void LocalSessionEventHandlerImpl::AppendChangeForExistingTab(
-    int sync_id,
-    const sessions::SessionTab& tab,
-    WriteBatch* batch) const {
   // Rewrite the specifics based on the reassociated SessionTab to preserve
   // the new tab and window ids.
   auto specifics = std::make_unique<sync_pb::SessionSpecifics>();
-  tab.ToSyncData().Swap(specifics->mutable_tab());
+  local_tab->ToSyncData().Swap(specifics->mutable_tab());
   specifics->set_session_tag(current_session_tag_);
-  specifics->set_tab_node_id(sync_id);
+  specifics->set_tab_node_id(tab_node_id);
   batch->Put(std::move(specifics));
 }
 

components/sync_sessions/local_session_event_handler_impl.h

@@ -111,12 +111,6 @@ class LocalSessionEventHandlerImpl : public LocalSessionEventHandler {
                                        SessionID new_window_id,
                                        WriteBatch* batch);
 
-  // Appends an ACTION_UPDATE for a sync tab entity onto |batch| to
-  // reflect the contents of |tab|, given the tab node id |sync_id|.
-  void AppendChangeForExistingTab(int sync_id,
-                                  const sessions::SessionTab& tab,
-                                  WriteBatch* batch) const;
-
   // Set |session_tab| from |tab_delegate|.
   sync_pb::SessionTab GetTabSpecificsFromDelegate(
       const SyncedTabDelegate& tab_delegate) const;

components/sync_sessions/local_session_event_handler_impl_unittest.cc

@@ -451,25 +451,14 @@ TEST_F(LocalSessionEventHandlerImplTest, AssociateCustomTab) {
   AddTab(kWindowId3, kFoo1, kTabId3)->SetSyncId(kCustomTabNodeId);
 
   auto mock_batch = std::make_unique<StrictMock<MockWriteBatch>>();
-
-  {
-    testing::InSequence seq;
-    EXPECT_CALL(*mock_batch,
-                Put(Pointee(MatchesTab(kSessionTag, kWindowId1, kTabId1,
-                                       kRegularTabNodeId, /*urls=*/{}))));
-    // Overriden by the Put() below, so we don't care about the args.
-    EXPECT_CALL(*mock_batch,
-                Put(Pointee(MatchesTab(kSessionTag, _, _, kCustomTabNodeId,
-                                       /*urls=*/_))));
-    EXPECT_CALL(*mock_batch, Put(Pointee(MatchesTab(kSessionTag, kWindowId3,
-                                                    kTabId3, kCustomTabNodeId,
-                                                    /*urls=*/{kFoo1}))));
-    EXPECT_CALL(*mock_batch,
-                Put(Pointee(MatchesHeader(kSessionTag,
-                                          {kWindowId1, kWindowId2, kWindowId3},
-                                          {kTabId1, kTabId3}))));
-    EXPECT_CALL(*mock_batch, Commit());
-  }
+  EXPECT_CALL(*mock_batch, Put(Pointee(MatchesTab(kSessionTag, kWindowId3,
+                                                  kTabId3, kCustomTabNodeId,
+                                                  /*urls=*/{kFoo1}))));
+  EXPECT_CALL(*mock_batch,
+              Put(Pointee(MatchesHeader(kSessionTag,
+                                        {kWindowId1, kWindowId2, kWindowId3},
+                                        {kTabId1, kTabId3}))));
+  EXPECT_CALL(*mock_batch, Commit());
 
   EXPECT_CALL(mock_delegate_, CreateLocalSessionWriteBatch())
       .WillOnce(Return(ByMove(std::move(mock_batch))));
@@ -654,12 +643,7 @@ TEST_F(LocalSessionEventHandlerImplTest,
   EXPECT_CALL(
       *update_mock_batch,
       Put(Pointee(MatchesTab(kSessionTag, kWindowId1, kTabId1, kTabNodeId1,
-                             /*urls=*/{kFoo1, kBaz1}))))
-      .Times(2);
-  EXPECT_CALL(
-      *update_mock_batch,
-      Put(Pointee(MatchesTab(kSessionTag, kWindowId2, kTabId2, kTabNodeId2,
-                             /*urls=*/{kBar1}))));
+                             /*urls=*/{kFoo1, kBaz1}))));
   EXPECT_CALL(*update_mock_batch, Commit());
 
   EXPECT_CALL(mock_delegate_, CreateLocalSessionWriteBatch())

components/sync_sessions/session_store.cc

@@ -449,13 +449,6 @@ SessionStore::SessionStore(
                                     local_session_info.client_name,
                                     local_session_info.device_type);
 
-  // Map of all rewritten local ids. Because ids are reset on each restart,
-  // and id generation happens outside of Sync, all ids from a previous local
-  // session must be rewritten in order to be valid (i.e not collide with
-  // newly assigned IDs). Otherwise, SyncedSessionTracker could mix up IDs.
-  // Key: previous session id. Value: new session id.
-  std::map<SessionID::id_type, SessionID> session_id_map;
-
   bool found_local_header = false;
 
   for (auto& storage_key_and_specifics : initial_data) {
@@ -498,26 +491,8 @@ SessionStore::SessionStore(
       DCHECK(!found_local_header);
       found_local_header = true;
 
-      // Go through and generate new tab and window ids as necessary, updating
-      // the specifics in place.
-      for (auto& window : *specifics.mutable_header()->mutable_window()) {
-        session_id_map.emplace(window.window_id(), SessionID::NewUnique());
-        window.set_window_id(session_id_map.at(window.window_id()).id());
-
-        for (int& tab_id : *window.mutable_tab()) {
-          if (session_id_map.count(tab_id) == 0) {
-            session_id_map.emplace(tab_id, SessionID::NewUnique());
-          }
-          tab_id = session_id_map.at(tab_id).id();
-          // Note: the tab id of the SessionTab will be updated when the tab
-          // node itself is processed.
-        }
-      }
-
       UpdateTrackerWithSpecifics(specifics, mtime, &session_tracker_);
-
-      DVLOG(1) << "Loaded local header and rewrote " << session_id_map.size()
-               << " ids.";
+      DVLOG(1) << "Loaded local header.";
     } else {
       DCHECK(specifics.has_tab());
 
@@ -526,20 +501,12 @@ SessionStore::SessionStore(
       DVLOG(1) << "Associating local tab " << specifics.tab().tab_id()
                << " with node " << specifics.tab_node_id();
 
-      // Now file the tab under the new tab id.
-      SessionID new_tab_id = SessionID::InvalidValue();
-      auto iter = session_id_map.find(specifics.tab().tab_id());
-      if (iter != session_id_map.end()) {
-        new_tab_id = iter->second;
-      } else {
-        new_tab_id = SessionID::NewUnique();
-        session_id_map.emplace(specifics.tab().tab_id(), new_tab_id);
-      }
-      DVLOG(1) << "Remapping tab " << specifics.tab().tab_id() << " to "
-               << new_tab_id;
-
-      specifics.mutable_tab()->set_tab_id(new_tab_id.id());
-      session_tracker_.ReassociateLocalTab(specifics.tab_node_id(), new_tab_id);
+      // TODO(mastiz): Move call to ReassociateLocalTab() into
+      // UpdateTrackerWithSpecifics(), possibly merge with OnTabNodeSeen(). Also
+      // consider merging this branch with processing of foreign tabs above.
+      session_tracker_.ReassociateLocalTab(
+          specifics.tab_node_id(),
+          SessionID::FromSerializedValue(specifics.tab().tab_id()));
       UpdateTrackerWithSpecifics(specifics, mtime, &session_tracker_);
     }
   }

components/sync_sessions/sessions_sync_manager.cc

@@ -417,12 +417,6 @@ syncer::SyncChange SessionsSyncManager::TombstoneTab(
 bool SessionsSyncManager::InitFromSyncModel(
     const syncer::SyncDataList& sync_data,
     syncer::SyncChangeList* new_changes) {
-  // Map of all rewritten local ids. Because ids are reset on each restart,
-  // and id generation happens outside of Sync, all ids from a previous local
-  // session must be rewritten in order to be valid.
-  // Key: previous session id. Value: new session id.
-  std::map<int32_t, SessionID> session_id_map;
-
   bool found_current_header = false;
   int bad_foreign_hash_count = 0;
   for (syncer::SyncDataList::const_iterator it = sync_data.begin();
@@ -464,37 +458,9 @@ bool SessionsSyncManager::InitFromSyncModel(
         // This is our previous header node, reuse it.
         found_current_header = true;
 
-        // The specifics from the SyncData are immutable. Create a mutable copy
-        // to hold the rewritten ids.
-        sync_pb::SessionSpecifics rewritten_specifics(specifics);
-
-        // Go through and generate new tab and window ids as necessary, updating
-        // the specifics in place.
-        for (auto& window :
-             *rewritten_specifics.mutable_header()->mutable_window()) {
-          session_id_map.emplace(window.window_id(), SessionID::NewUnique());
-          window.set_window_id(session_id_map.at(window.window_id()).id());
-
-          google::protobuf::RepeatedField<int>* tab_ids = window.mutable_tab();
-          for (int i = 0; i < tab_ids->size(); i++) {
-            auto tab_iter = session_id_map.find(tab_ids->Get(i));
-            if (tab_iter == session_id_map.end()) {
-              // SessionID::SessionID() automatically increments a static
-              // variable, forcing a new id to be generated each time.
-              session_id_map.emplace(tab_ids->Get(i), SessionID::NewUnique());
-            }
-            *(tab_ids->Mutable(i)) = session_id_map.at(tab_ids->Get(i)).id();
-            // Note: the tab id of the SessionTab will be updated when the tab
-            // node itself is processed.
-          }
-        }
-
-        UpdateTrackerWithSpecifics(rewritten_specifics,
-                                   remote.GetModifiedTime(), &session_tracker_);
-
-        DVLOG(1) << "Loaded local header and rewrote " << session_id_map.size()
-                 << " ids.";
-
+        UpdateTrackerWithSpecifics(specifics, remote.GetModifiedTime(),
+                                   &session_tracker_);
+        DVLOG(1) << "Loaded local header.";
       } else {
         if (specifics.has_header() || !specifics.has_tab()) {
           LOG(WARNING) << "Found more than one session header node with local "
@@ -513,26 +479,11 @@ bool SessionsSyncManager::InitFromSyncModel(
           DVLOG(1) << "Associating local tab " << specifics.tab().tab_id()
                    << " with node " << specifics.tab_node_id();
 
-          // Now file the tab under the new tab id.
-          SessionID new_tab_id = SessionID::InvalidValue();
-          auto iter = session_id_map.find(specifics.tab().tab_id());
-          if (iter != session_id_map.end()) {
-            new_tab_id = iter->second;
-          } else {
-            new_tab_id = SessionID::NewUnique();
-            session_id_map.emplace(specifics.tab().tab_id(), new_tab_id);
-          }
-          DVLOG(1) << "Remapping tab " << specifics.tab().tab_id() << " to "
-                   << new_tab_id;
-
-          // The specifics from the SyncData are immutable. Create a mutable
-          // copy to hold the rewritten ids.
-          sync_pb::SessionSpecifics rewritten_specifics(specifics);
-          rewritten_specifics.mutable_tab()->set_tab_id(new_tab_id.id());
           session_tracker_.ReassociateLocalTab(
-              rewritten_specifics.tab_node_id(), new_tab_id);
-          UpdateTrackerWithSpecifics(
-              rewritten_specifics, remote.GetModifiedTime(), &session_tracker_);
+              specifics.tab_node_id(),
+              SessionID::FromSerializedValue(specifics.tab().tab_id()));
+          UpdateTrackerWithSpecifics(specifics, remote.GetModifiedTime(),
+                                     &session_tracker_);
         }
       }
     }

components/sync_sessions/sessions_sync_manager_unittest.cc

@@ -444,21 +444,14 @@ TEST_F(SessionsSyncManagerTest, PreserveTabbedDataNoWindows) {
   ResetWindows();
   InitWithSyncDataTakeOutput(ConvertToRemote(in), &out);
 
-  // There should be two changes: the rewritten tab (to update the tab id), and
-  // the rewritten header.
-  ASSERT_TRUE(ChangeTypeMatches(
-      out, {SyncChange::ACTION_UPDATE, SyncChange::ACTION_UPDATE}));
-  VerifyLocalTabChange(out[0], 2, kFoo2);
-  VerifyLocalHeaderChange(out[1], 1, 1);
+  // There should be one change to the rewritten header.
+  ASSERT_TRUE(ChangeTypeMatches(out, {SyncChange::ACTION_UPDATE}));
+  VerifyLocalHeaderChange(out[0], 1, 1);
 
-  // Verify the tab id of the restored tab is updated and consistent.
+  // SessionId should not be rewritten on restore.
   int restored_tab_id =
-      out[0].sync_data().GetSpecifics().session().tab().tab_id();
-  // SessionId should be rewritten on restore.
-  ASSERT_NE(tab->GetSessionId().id(), restored_tab_id);
-  ASSERT_EQ(
-      restored_tab_id,
-      out[1].sync_data().GetSpecifics().session().header().window(0).tab(0));
+      out[0].sync_data().GetSpecifics().session().header().window(0).tab(0);
+  EXPECT_EQ(tab->GetSessionId().id(), restored_tab_id);
   out.clear();
 
   // Now actually resurrect the native data, which will end up having different
@@ -500,10 +493,8 @@ TEST_F(SessionsSyncManagerTest, PreserveTabbedDataCustomTab) {
   // synced because we do not have enough local data to ensure that we wouldn't
   // vend the same sync id if our persistent storage didn't match upon the last
   // shutdown.
-  ASSERT_TRUE(ChangeTypeMatches(
-      out, {SyncChange::ACTION_UPDATE, SyncChange::ACTION_UPDATE}));
-  VerifyLocalTabChange(out[0], 2, kFoo2);
-  VerifyLocalHeaderChange(out[1], 1, 1);
+  ASSERT_TRUE(ChangeTypeMatches(out, {SyncChange::ACTION_UPDATE}));
+  VerifyLocalHeaderChange(out[0], 1, 1);
   out.clear();
 
   // Now re-create local data and modify it.
@@ -2055,24 +2046,18 @@ TEST_F(SessionsSyncManagerTest, PlaceholderConflictAcrossWindows) {
   InitWithSyncDataTakeOutput(in, &out);
 
   // The two tabs have the same sync id, which is not allowed. One will have
-  // its ID stripped and re-generated, which manifests as a newly created sync
-  // entity.
-  ASSERT_TRUE(
-      ChangeTypeMatches(out, {SyncChange::ACTION_ADD, SyncChange::ACTION_UPDATE,
-                              SyncChange::ACTION_UPDATE}));
+  // its id stripped, but being a placeholder tab it won't be vended a new sync
+  // ID so it won't get updated.
+  ASSERT_TRUE(ChangeTypeMatches(
+      out, {SyncChange::ACTION_UPDATE, SyncChange::ACTION_UPDATE}));
   VerifyLocalTabChange(out[0], 1, kFoo1);
-  VerifyLocalTabChange(out[1], 1, kFoo1);
-  VerifyLocalHeaderChange(out[2], 2, 2);
+  VerifyLocalHeaderChange(out[1], 1, 1);
 
-  EXPECT_NE(tab1->GetSessionId(), tab2.GetSessionId());
+  EXPECT_NE(tab1->GetSyncId(), tab2.GetSyncId());
   EXPECT_EQ(tab1->GetSessionId().id(),
             out[0].sync_data().GetSpecifics().session().tab().tab_id());
   EXPECT_EQ(tab1->GetSyncId(),
             out[0].sync_data().GetSpecifics().session().tab_node_id());
-  EXPECT_EQ(tab2.GetSessionId().id(),
-            out[1].sync_data().GetSpecifics().session().tab().tab_id());
-  EXPECT_EQ(tab2.GetSyncId(),
-            out[1].sync_data().GetSpecifics().session().tab_node_id());
 }
 
 // Tests that task ids are generated for navigations on local tabs.

components/sync_sessions/synced_session_tracker.h

@@ -194,10 +194,8 @@ class SyncedSessionTracker {
   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
-  // 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
+  // any previous SessionTab object the node was associated with. 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 new_tab_id);