Reland of [Sync] Put session tracker in charge of maintaining local state.

components/sync_sessions/sessions_sync_manager.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/sync_sessions/sessions_sync_manager.h"
 
 #include <algorithm>
 #include <utility>
 
+#include "base/format_macros.h"
+#include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/strings/stringprintf.h"
 #include "build/build_config.h"
 #include "components/sync/base/hash_util.h"
 #include "components/sync/device_info/local_device_info_provider.h"
 #include "components/sync/model/sync_error.h"
 #include "components/sync/model/sync_error_factory.h"
 #include "components/sync/model/sync_merge_result.h"
 #include "components/sync/model/time.h"
 #include "components/sync_sessions/sync_sessions_client.h"
 #include "components/sync_sessions/synced_tab_delegate.h"
 #include "components/sync_sessions/synced_window_delegate.h"
 #include "components/sync_sessions/synced_window_delegates_getter.h"
+#include "components/sync_sessions/tab_node_pool.h"
 #include "components/variations/variations_associated_data.h"
 
 using sessions::SerializedNavigationEntry;
 using syncer::DeviceInfo;
 using syncer::LocalDeviceInfoProvider;
 using syncer::SyncChange;
 using syncer::SyncData;
 
 namespace sync_sessions {
 
@@ skipping 21 matching lines @@
   return t1->timestamp > t2->timestamp;
 }
 
 // Comparator function for use with std::sort that will sort sessions by
 // descending modified_time (i.e., most recent first).
 bool SessionsRecencyComparator(const SyncedSession* s1,
                                const SyncedSession* s2) {
   return s1->modified_time > s2->modified_time;
 }
 
+std::string TabNodeIdToTag(const std::string& machine_tag, int tab_node_id) {
+  return base::StringPrintf("%s %d", machine_tag.c_str(), tab_node_id);
+}
+
 std::string TagFromSpecifics(const sync_pb::SessionSpecifics& specifics) {
   if (specifics.has_header()) {
     return specifics.session_tag();
   } else if (specifics.has_tab()) {
-    return TabNodePool::TabIdToTag(specifics.session_tag(),
-                                   specifics.tab_node_id());
+    return TabNodeIdToTag(specifics.session_tag(), specifics.tab_node_id());
   } else {
     return std::string();
   }
 }
 
+sync_pb::SessionSpecifics SessionTabToSpecifics(
+    const sessions::SessionTab& session_tab,
+    const std::string& local_tag,
+    int tab_node_id) {
+  sync_pb::SessionSpecifics specifics;
+  specifics.mutable_tab()->CopyFrom(session_tab.ToSyncData());
+  specifics.set_session_tag(local_tag);
+  specifics.set_tab_node_id(tab_node_id);
+  return specifics;
+}
+
 }  // namespace
 
 // |local_device| is owned by ProfileSyncService, its lifetime exceeds
 // lifetime of SessionSyncManager.
 SessionsSyncManager::SessionsSyncManager(
     sync_sessions::SyncSessionsClient* sessions_client,
     syncer::SyncPrefs* sync_prefs,
     LocalDeviceInfoProvider* local_device,
     std::unique_ptr<LocalSessionEventRouter> router,
     const base::Closure& sessions_updated_callback,
@@ skipping 23 matching lines @@
   return machine_tag;
 }
 
 syncer::SyncMergeResult SessionsSyncManager::MergeDataAndStartSyncing(
     syncer::ModelType type,
     const syncer::SyncDataList& initial_sync_data,
     std::unique_ptr<syncer::SyncChangeProcessor> sync_processor,
     std::unique_ptr<syncer::SyncErrorFactory> error_handler) {
   syncer::SyncMergeResult merge_result(type);
   DCHECK(session_tracker_.Empty());
-  DCHECK_EQ(0U, local_tab_pool_.Capacity());
 
   error_handler_ = std::move(error_handler);
   sync_processor_ = std::move(sync_processor);
 
   // It's possible(via RebuildAssociations) for lost_navigations_recorder_ to
   // persist between sync being stopped and started. If it did persist, it's
   // already associated with |sync_processor|, so leave it alone.
   if (!lost_navigations_recorder_.get()) {
     lost_navigations_recorder_ =
         base::MakeUnique<sync_sessions::LostNavigationsRecorder>();
@@ skipping 14 matching lines @@
   DCHECK(local_device_);
   const DeviceInfo* local_device_info = local_device_->GetLocalDeviceInfo();
   if (local_device_info) {
     current_session_name_ = local_device_info->client_name();
   } else {
     merge_result.set_error(error_handler_->CreateAndUploadError(
         FROM_HERE, "Failed to get local device info."));
     return merge_result;
   }
 
-  session_tracker_.SetLocalSessionTag(current_machine_tag_);
+  session_tracker_.SetLocalSessionTag(current_machine_tag());
 
   syncer::SyncChangeList new_changes;
 
   // First, we iterate over sync data to update our session_tracker_.
-  syncer::SyncDataList restored_tabs;
-  if (!InitFromSyncModel(initial_sync_data, &restored_tabs, &new_changes)) {
+  if (!InitFromSyncModel(initial_sync_data, &new_changes)) {
     // The sync db didn't have a header node for us. Create one.
     sync_pb::EntitySpecifics specifics;
     sync_pb::SessionSpecifics* base_specifics = specifics.mutable_session();
     base_specifics->set_session_tag(current_machine_tag());
     sync_pb::SessionHeader* header_s = base_specifics->mutable_header();
     header_s->set_client_name(current_session_name_);
     header_s->set_device_type(local_device_info->device_type());
     syncer::SyncData data = syncer::SyncData::CreateLocalData(
         current_machine_tag(), current_session_name_, specifics);
     new_changes.push_back(
         syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_ADD, data));
   }
 
 #if defined(OS_ANDROID)
   std::string sync_machine_tag(
       BuildMachineTag(local_device_->GetLocalSyncCacheGUID()));
-  if (current_machine_tag_.compare(sync_machine_tag) != 0)
+  if (current_machine_tag().compare(sync_machine_tag) != 0)
     DeleteForeignSessionInternal(sync_machine_tag, &new_changes);
 #endif
 
   // Check if anything has changed on the local client side.
-  AssociateWindows(RELOAD_TABS, restored_tabs, &new_changes);
+  AssociateWindows(RELOAD_TABS, &new_changes);
   local_tab_pool_out_of_sync_ = false;
 
   merge_result.set_error(
       sync_processor_->ProcessSyncChanges(FROM_HERE, new_changes));
 
   local_event_router_->StartRoutingTo(this);
   return merge_result;
 }
 
 void SessionsSyncManager::AssociateWindows(
     ReloadTabsOption option,
-    const syncer::SyncDataList& restored_tabs,
     syncer::SyncChangeList* change_output) {
   const std::string local_tag = current_machine_tag();
   sync_pb::SessionSpecifics specifics;
   specifics.set_session_tag(local_tag);
   sync_pb::SessionHeader* header_s = specifics.mutable_header();
   SyncedSession* current_session = session_tracker_.GetSession(local_tag);
   current_session->modified_time = base::Time::Now();
   header_s->set_client_name(current_session_name_);
   // SessionDataTypeController ensures that the local device info
   // is available before activating this datatype.
@@ skipping 61 matching lines @@
 
         if (synced_tab->IsPlaceholderTab()) {
           // For tabs without WebContents update the |tab_id| and |window_id|,
           // as it could have changed after a session restore.
           // Note: We cannot check if a tab is valid if it has no WebContents.
           // We assume any such tab is valid and leave the contents of
           // corresponding sync node unchanged.
           if (synced_tab->GetSyncId() > TabNodePool::kInvalidTabNodeID &&
               tab_id > TabNodePool::kInvalidTabID) {
             AssociateRestoredPlaceholderTab(*synced_tab, tab_id, window_id,
-                                            restored_tabs, change_output);
+                                            change_output);
             found_tabs = true;
             window_s.add_tab(tab_id);
           }
           continue;
         }
 
         if (RELOAD_TABS == option)
           AssociateTab(synced_tab, change_output);
 
         // If the tab is valid, it would have been added to the tracker either
@@ skipping 12 matching lines @@
         *header_window = window_s;
 
         // Update this window's representation in the synced session tracker.
         session_tracker_.PutWindowInSession(local_tag, window_id);
         BuildSyncedSessionFromSpecifics(
             local_tag, window_s, current_session->modified_time,
             current_session->windows[window_id].get());
       }
     }
   }
-  local_tab_pool_.DeleteUnassociatedTabNodes(change_output);
-  session_tracker_.CleanupSession(local_tag);
+  std::set<int> deleted_tab_node_ids;
+  session_tracker_.CleanupLocalTabs(&deleted_tab_node_ids);
+  for (int tab_node_id : deleted_tab_node_ids) {
+    std::string tab_node_tag =
+        TabNodeIdToTag(current_machine_tag(), tab_node_id);
+    change_output->push_back(syncer::SyncChange(
+        FROM_HERE, syncer::SyncChange::ACTION_DELETE,
+        syncer::SyncData::CreateLocalDelete(tab_node_tag, syncer::SESSIONS)));
+  }
 
   // Always update the header.  Sync takes care of dropping this update
   // if the entity specifics are identical (i.e windows, client name did
   // not change).
   sync_pb::EntitySpecifics entity;
   entity.mutable_session()->CopyFrom(specifics);
   syncer::SyncData data = syncer::SyncData::CreateLocalData(
       current_machine_tag(), current_session_name_, entity);
   change_output->push_back(
       syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_UPDATE, data));
 }
 
-void SessionsSyncManager::AssociateTab(SyncedTabDelegate* const tab,
+void SessionsSyncManager::AssociateTab(SyncedTabDelegate* const tab_delegate,
                                        syncer::SyncChangeList* change_output) {
-  DCHECK(!tab->IsPlaceholderTab());
-  SessionID::id_type tab_id = tab->GetSessionId();
+  DCHECK(!tab_delegate->IsPlaceholderTab());
 
-  if (tab->IsBeingDestroyed()) {
-    // This tab is closing.
-    TabLinksMap::iterator tab_iter = local_tab_map_.find(tab_id);
-    if (tab_iter == local_tab_map_.end()) {
-      // We aren't tracking this tab (for example, sync setting page).
-      return;
-    }
-    local_tab_pool_.FreeTabNode(tab_iter->second->tab_node_id(), change_output);
-    local_tab_map_.erase(tab_iter);
+  if (tab_delegate->IsBeingDestroyed()) {
+    // Do nothing. By not proactively adding the tab to the session, it will be
+    // removed if necessary during subsequent cleanup.
     return;
   }
 
-  if (!tab->ShouldSync(sessions_client_))
+  if (!tab_delegate->ShouldSync(sessions_client_))
     return;
 
-  TabLinksMap::iterator local_tab_map_iter = local_tab_map_.find(tab_id);
-  TabLink* tab_link = nullptr;
+  SessionID::id_type tab_id = tab_delegate->GetSessionId();
+  DVLOG(1) << "Syncing tab " << tab_id << " from window "
+           << tab_delegate->GetWindowId();
 
-  if (local_tab_map_iter == local_tab_map_.end()) {
-    int tab_node_id = tab->GetSyncId();
-    // If there is an old sync node for the tab, reuse it.  If this is a new
-    // tab, get a sync node for it.
-    if (!local_tab_pool_.IsUnassociatedTabNode(tab_node_id)) {
-      tab_node_id = local_tab_pool_.GetFreeTabNode(change_output);
-      tab->SetSyncId(tab_node_id);
-    }
-    local_tab_pool_.AssociateTabNode(tab_node_id, tab_id);
-    tab_link = new TabLink(tab_node_id, tab);
-    local_tab_map_[tab_id] = make_linked_ptr<TabLink>(tab_link);
-  } else {
-    // This tab is already associated with a sync node, reuse it.
-    // Note: on some platforms the tab object may have changed, so we ensure
-    // the tab link is up to date.
-    tab_link = local_tab_map_iter->second.get();
-    local_tab_map_iter->second->set_tab(tab);
-  }
-  DCHECK(tab_link);
-  DCHECK_NE(tab_link->tab_node_id(), TabNodePool::kInvalidTabNodeID);
-  DVLOG(1) << "Reloading tab " << tab_id << " from window "
-           << tab->GetWindowId();
+  int tab_node_id = TabNodePool::kInvalidTabNodeID;
+  bool existing_tab_node =
+      session_tracker_.GetTabNodeForLocalTab(tab_id, &tab_node_id);
+  DCHECK_NE(TabNodePool::kInvalidTabNodeID, tab_node_id);
+  sessions::SessionTab* session_tab =
+      session_tracker_.GetTab(current_machine_tag(), tab_id);
 
-  // Write to sync model.
-  sync_pb::EntitySpecifics specifics;
-  LocalTabDelegateToSpecifics(*tab, specifics.mutable_session());
-  syncer::SyncData data = syncer::SyncData::CreateLocalData(
-      TabNodePool::TabIdToTag(current_machine_tag_, tab_link->tab_node_id()),
-      current_session_name_, specifics);
-  change_output->push_back(
-      syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_UPDATE, data));
+  // Get the previously synced url.
+  int old_index = session_tab->normalized_navigation_index();
+  GURL old_url;
+  if (session_tab->navigations.size() > static_cast<size_t>(old_index))
+    old_url = session_tab->navigations[old_index].virtual_url();
 
-  int current_index = tab->GetCurrentEntryIndex();
-  const GURL new_url = tab->GetVirtualURLAtIndex(current_index);
-  if (new_url != tab_link->url()) {
-    tab_link->set_url(new_url);
-    favicon_cache_.OnFaviconVisited(new_url,
-                                    tab->GetFaviconURLAtIndex(current_index));
-    page_revisit_broadcaster_.OnPageVisit(
-        new_url, tab->GetTransitionAtIndex(current_index));
-  }
-
+  // Update the tracker's session representation.
+  SetSessionTabFromDelegate(*tab_delegate, base::Time::Now(), session_tab);
+  SetVariationIds(session_tab);
   session_tracker_.GetSession(current_machine_tag())->modified_time =
       base::Time::Now();
+
+  // Write to the sync model itself.
+  sync_pb::EntitySpecifics specifics;
+  specifics.mutable_session()->CopyFrom(
+      SessionTabToSpecifics(*session_tab, current_machine_tag(), tab_node_id));
+  syncer::SyncData data = syncer::SyncData::CreateLocalData(
+      TabNodeIdToTag(current_machine_tag(), tab_node_id), current_session_name_,
+      specifics);
+  change_output->push_back(syncer::SyncChange(
+      FROM_HERE, existing_tab_node ? syncer::SyncChange::ACTION_UPDATE
+                                   : syncer::SyncChange::ACTION_ADD,
+      data));
+
+  int current_index = tab_delegate->GetCurrentEntryIndex();
+  const GURL new_url = tab_delegate->GetVirtualURLAtIndex(current_index);
+  if (new_url != old_url) {
+    favicon_cache_.OnFaviconVisited(
+        new_url, tab_delegate->GetFaviconURLAtIndex(current_index));
+    page_revisit_broadcaster_.OnPageVisit(
+        new_url, tab_delegate->GetTransitionAtIndex(current_index));
+  }
 }
 
 void SessionsSyncManager::RebuildAssociations() {
   syncer::SyncDataList data(sync_processor_->GetAllSyncData(syncer::SESSIONS));
   std::unique_ptr<syncer::SyncErrorFactory> error_handler(
       std::move(error_handler_));
   std::unique_ptr<syncer::SyncChangeProcessor> processor(
       std::move(sync_processor_));
 
   StopSyncing(syncer::SESSIONS);
@@ skipping 38 matching lines @@
     DCHECK(!local_tab_pool_out_of_sync_);
     return;
   }
 
   syncer::SyncChangeList changes;
   AssociateTab(modified_tab, &changes);
   // Note, we always associate windows because it's possible a tab became
   // "interesting" by going to a valid URL, in which case it needs to be added
   // to the window's tab information. Similarly, if a tab became
   // "uninteresting", we remove it from the window's tab information.
-  AssociateWindows(DONT_RELOAD_TABS, syncer::SyncDataList(), &changes);
+  AssociateWindows(DONT_RELOAD_TABS, &changes);
   sync_processor_->ProcessSyncChanges(FROM_HERE, changes);
 }
 
 void SessionsSyncManager::OnFaviconsChanged(const std::set<GURL>& page_urls,
                                             const GURL& /* icon_url */) {
-  // TODO(zea): consider a separate container for tabs with outstanding favicon
-  // loads so we don't have to iterate through all tabs comparing urls.
-  for (const GURL& page_url : page_urls) {
-    for (TabLinksMap::iterator tab_iter = local_tab_map_.begin();
-         tab_iter != local_tab_map_.end(); ++tab_iter) {
-      if (tab_iter->second->url() == page_url)
-        favicon_cache_.OnPageFaviconUpdated(page_url);
-    }
-  }
+  for (const GURL& page_url : page_urls)
+    favicon_cache_.OnPageFaviconUpdated(page_url);
 }
 
 void SessionsSyncManager::StopSyncing(syncer::ModelType type) {
   local_event_router_->Stop();
   if (sync_processor_.get() && lost_navigations_recorder_.get()) {
     sync_processor_->RemoveLocalChangeObserver(
         lost_navigations_recorder_.get());
     lost_navigations_recorder_.reset();
   }
   sync_processor_.reset(nullptr);
   error_handler_.reset();
   session_tracker_.Clear();
-  local_tab_map_.clear();
-  local_tab_pool_.Clear();
   current_machine_tag_.clear();
   current_session_name_.clear();
   local_session_header_node_id_ = TabNodePool::kInvalidTabNodeID;
 }
 
 syncer::SyncDataList SessionsSyncManager::GetAllSyncData(
     syncer::ModelType type) const {
   syncer::SyncDataList list;
   const SyncedSession* session = nullptr;
   if (!session_tracker_.LookupLocalSession(&session))
     return syncer::SyncDataList();
 
   // First construct the header node.
   sync_pb::EntitySpecifics header_entity;
   header_entity.mutable_session()->set_session_tag(current_machine_tag());
   sync_pb::SessionHeader* header_specifics =
       header_entity.mutable_session()->mutable_header();
   header_specifics->MergeFrom(session->ToSessionHeader());
   syncer::SyncData data = syncer::SyncData::CreateLocalData(
       current_machine_tag(), current_session_name_, header_entity);
   list.push_back(data);
 
-  for (auto win_iter = session->windows.begin();
-       win_iter != session->windows.end(); ++win_iter) {
-    for (auto tabs_iter = win_iter->second->tabs.begin();
-         tabs_iter != win_iter->second->tabs.end(); ++tabs_iter) {
+  for (auto& win_iter : session->windows) {
+    for (auto& tab : win_iter.second->tabs) {
+      // TODO(zea): replace with with the correct tab node id once there's a
+      // sync specific wrapper for SessionTab. This method is only used in
+      // tests though, so it's fine for now. crbug.com/662597
+      int tab_node_id = 0;
       sync_pb::EntitySpecifics entity;
-      sync_pb::SessionSpecifics* specifics = entity.mutable_session();
-      specifics->mutable_tab()->MergeFrom((*tabs_iter)->ToSyncData());
-      specifics->set_session_tag(current_machine_tag_);
-
-      TabLinksMap::const_iterator tab_map_iter =
-          local_tab_map_.find((*tabs_iter)->tab_id.id());
-      DCHECK(tab_map_iter != local_tab_map_.end());
-      specifics->set_tab_node_id(tab_map_iter->second->tab_node_id());
+      entity.mutable_session()->CopyFrom(
+          SessionTabToSpecifics(*tab, current_machine_tag(), tab_node_id));
       syncer::SyncData data = syncer::SyncData::CreateLocalData(
-          TabNodePool::TabIdToTag(current_machine_tag_,
-                                  specifics->tab_node_id()),
+          TabNodeIdToTag(current_machine_tag(), tab_node_id),
           current_session_name_, entity);
       list.push_back(data);
     }
   }
   return list;
 }
 
 bool SessionsSyncManager::GetLocalSession(const SyncedSession** local_session) {
-  if (current_machine_tag_.empty())
+  if (current_machine_tag().empty())
     return false;
   *local_session = session_tracker_.GetSession(current_machine_tag());
   return true;
 }
 
 syncer::SyncError SessionsSyncManager::ProcessSyncChanges(
     const tracked_objects::Location& from_here,
     const syncer::SyncChangeList& change_list) {
   if (!sync_processor_.get()) {
     syncer::SyncError error(FROM_HERE, syncer::SyncError::DATATYPE_ERROR,
@@ skipping 39 matching lines @@
         break;
       case syncer::SyncChange::ACTION_ADD:
       case syncer::SyncChange::ACTION_UPDATE:
         if (current_machine_tag() == session.session_tag()) {
           // We should only ever receive a change to our own machine's session
           // info if encryption was turned on. In that case, the data is still
           // the same, so we can ignore.
           LOG(WARNING) << "Dropping modification to local session.";
           return syncer::SyncError();
         }
-        UpdateTrackerWithForeignSession(
+        UpdateTrackerWithSpecifics(
             session, syncer::SyncDataRemote(it->sync_data()).GetModifiedTime());
         break;
       default:
         NOTREACHED() << "Processing sync changes failed, unknown change type.";
     }
   }
 
   if (!sessions_updated_callback_.is_null())
     sessions_updated_callback_.Run();
   return syncer::SyncError();
 }
 
 syncer::SyncChange SessionsSyncManager::TombstoneTab(
     const sync_pb::SessionSpecifics& tab) {
   if (!tab.has_tab_node_id()) {
     LOG(WARNING) << "Old sessions node without tab node id; can't tombstone.";
     return syncer::SyncChange();
   } else {
     return syncer::SyncChange(
         FROM_HERE, SyncChange::ACTION_DELETE,
         SyncData::CreateLocalDelete(
-            TabNodePool::TabIdToTag(current_machine_tag(), tab.tab_node_id()),
+            TabNodeIdToTag(current_machine_tag(), tab.tab_node_id()),
             syncer::SESSIONS));
   }
 }
 
 bool SessionsSyncManager::GetAllForeignSessions(
     std::vector<const SyncedSession*>* sessions) {
   if (!session_tracker_.LookupAllForeignSessions(
           sessions, SyncedSessionTracker::PRESENTABLE))
     return false;
   std::sort(sessions->begin(), sessions->end(), SessionsRecencyComparator);
   return true;
 }
 
 bool SessionsSyncManager::InitFromSyncModel(
     const syncer::SyncDataList& sync_data,
-    syncer::SyncDataList* restored_tabs,
     syncer::SyncChangeList* new_changes) {
   bool found_current_header = false;
   int bad_foreign_hash_count = 0;
   for (syncer::SyncDataList::const_iterator it = sync_data.begin();
        it != sync_data.end(); ++it) {
     const syncer::SyncData& data = *it;
     DCHECK(data.GetSpecifics().has_session());
     syncer::SyncDataRemote remote(data);
     const sync_pb::SessionSpecifics& specifics = data.GetSpecifics().session();
     if (specifics.session_tag().empty() ||
         (specifics.has_tab() &&
          (!specifics.has_tab_node_id() || !specifics.tab().has_tab_id()))) {
       syncer::SyncChange tombstone(TombstoneTab(specifics));
       if (tombstone.IsValid())
         new_changes->push_back(tombstone);
     } else if (specifics.session_tag() != current_machine_tag()) {
       if (TagHashFromSpecifics(specifics) == remote.GetClientTagHash()) {
-        UpdateTrackerWithForeignSession(specifics, remote.GetModifiedTime());
+        UpdateTrackerWithSpecifics(specifics, remote.GetModifiedTime());
       } else {
         // In the past, like years ago, we believe that some session data was
         // created with bad tag hashes. This causes any change this client makes
         // to that foreign data (like deletion through garbage collection) to
         // trigger a data type error because the tag looking mechanism fails. So
         // look for these and delete via remote SyncData, which uses a server id
         // lookup mechanism instead, see crbug.com/604657.
         bad_foreign_hash_count++;
         new_changes->push_back(
             syncer::SyncChange(FROM_HERE, SyncChange::ACTION_DELETE, remote));
       }
     } else {
       // This is previously stored local session information.
       if (specifics.has_header() && !found_current_header) {
         // This is our previous header node, reuse it.
         found_current_header = true;
         if (specifics.header().has_client_name())
           current_session_name_ = specifics.header().client_name();
+
+        // TODO(zea): crbug.com/639009 update the tracker with the specifics
+        // from the header node as well. This will be necessary to preserve
+        // the set of open tabs when a custom tab is opened.
       } else {
         if (specifics.has_header() || !specifics.has_tab()) {
           LOG(WARNING) << "Found more than one session header node with local "
                        << "tag.";
           syncer::SyncChange tombstone(TombstoneTab(specifics));
           if (tombstone.IsValid())
             new_changes->push_back(tombstone);
         } else {
-          // This is a valid old tab node, add it to the pool so it can be
-          // reused for reassociation.
-          local_tab_pool_.AddTabNode(specifics.tab_node_id());
-          restored_tabs->push_back(*it);
+          // This is a valid old tab node, add it to the tracker and associate
+          // it.
+          DVLOG(1) << "Associating local tab " << specifics.tab().tab_id()
+                   << " with node " << specifics.tab_node_id();
+          session_tracker_.ReassociateLocalTab(specifics.tab_node_id(),
+                                               specifics.tab().tab_id());
+          UpdateTrackerWithSpecifics(specifics, remote.GetModifiedTime());
         }
       }
     }
   }
 
   // Cleanup all foreign sessions, since orphaned tabs may have been added after
   // the header.
   std::vector<const SyncedSession*> sessions;
   session_tracker_.LookupAllForeignSessions(&sessions,
                                             SyncedSessionTracker::RAW);
   for (const auto* session : sessions) {
-    session_tracker_.CleanupSession(session->session_tag);
+    session_tracker_.CleanupForeignSession(session->session_tag);
   }
 
   UMA_HISTOGRAM_COUNTS_100("Sync.SessionsBadForeignHashOnMergeCount",
                            bad_foreign_hash_count);
 
   return found_current_header;
 }
 
-void SessionsSyncManager::UpdateTrackerWithForeignSession(
+void SessionsSyncManager::UpdateTrackerWithSpecifics(
     const sync_pb::SessionSpecifics& specifics,
     const base::Time& modification_time) {
-  std::string foreign_session_tag = specifics.session_tag();
-  DCHECK_NE(foreign_session_tag, current_machine_tag());
-
-  SyncedSession* foreign_session =
-      session_tracker_.GetSession(foreign_session_tag);
+  std::string session_tag = specifics.session_tag();
+  SyncedSession* session = session_tracker_.GetSession(session_tag);
   if (specifics.has_header()) {
-    // Read in the header data for this foreign session. Header data is
+    // Read in the header data for this session. Header data is
     // essentially a collection of windows, each of which has an ordered id list
     // for their tabs.
 
     if (!IsValidSessionHeader(specifics.header())) {
-      LOG(WARNING) << "Ignoring foreign session node with invalid header "
-                   << "and tag " << foreign_session_tag << ".";
+      LOG(WARNING) << "Ignoring session node with invalid header "
+                   << "and tag " << session_tag << ".";
       return;
     }
 
     // Load (or create) the SyncedSession object for this client.
     const sync_pb::SessionHeader& header = specifics.header();
-    PopulateSessionHeaderFromSpecifics(header, modification_time,
-                                       foreign_session);
+    PopulateSessionHeaderFromSpecifics(header, modification_time, session);
 
     // Reset the tab/window tracking for this session (must do this before
     // we start calling PutWindowInSession and PutTabInWindow so that all
     // unused tabs/windows get cleared by the CleanupSession(...) call).
-    session_tracker_.ResetSessionTracking(foreign_session_tag);
+    session_tracker_.ResetSessionTracking(session_tag);
 
     // Process all the windows and their tab information.
     int num_windows = header.window_size();
-    DVLOG(1) << "Associating " << foreign_session_tag << " with " << num_windows
+    DVLOG(1) << "Populating " << session_tag << " with " << num_windows
              << " windows.";
 
     for (int i = 0; i < num_windows; ++i) {
       const sync_pb::SessionWindow& window_s = header.window(i);
       SessionID::id_type window_id = window_s.window_id();
-      session_tracker_.PutWindowInSession(foreign_session_tag, window_id);
-      BuildSyncedSessionFromSpecifics(
-          foreign_session_tag, window_s, modification_time,
-          foreign_session->windows[window_id].get());
+      session_tracker_.PutWindowInSession(session_tag, window_id);
+      BuildSyncedSessionFromSpecifics(session_tag, window_s, modification_time,
+                                      session->windows[window_id].get());
     }
     // Delete any closed windows and unused tabs as necessary.
-    session_tracker_.CleanupSession(foreign_session_tag);
+    session_tracker_.CleanupForeignSession(session_tag);

[skym, 2016/12/06 19:37:21]

This kind of feels like a trap for whoever tries to fix the TODO on line 655.
Though I guess SyncedSessionTracker::CleanupForeignSession DCHECKs so it'll fail
fast.

[Nicolas Zea, 2016/12/06 22:31:38]

Yeah, this is changing in the followup patch.

   } else if (specifics.has_tab()) {
     const sync_pb::SessionTab& tab_s = specifics.tab();
     SessionID::id_type tab_id = tab_s.tab_id();
+    DVLOG(1) << "Populating " << session_tag << "'s tab id " << tab_id
+             << " from node " << specifics.tab_node_id();
 
-    const sessions::SessionTab* existing_tab;
-    if (session_tracker_.LookupSessionTab(foreign_session_tag, tab_id,
-                                          &existing_tab) &&
-        existing_tab->timestamp > modification_time) {
-      // Force the tracker to remember this tab node id, even if it isn't
-      // currently being used.
-      session_tracker_.GetTab(foreign_session_tag, tab_id,
-                              specifics.tab_node_id());
-      DVLOG(1) << "Ignoring " << foreign_session_tag << "'s session tab "
-               << tab_id << " with earlier modification time";
+    // Ensure the tracker is aware of the tab node id. Deleting foreign sessions
+    // requires deleting all relevant tab nodes, and it's easier to track the
+    // tab node ids themselves separately from the tab ids.
+    //
+    // Note that TabIDs are not stable across restarts of a client. Consider
+    // this example with two tabs:
+    //
+    // http://a.com  TabID1 --> NodeIDA
+    // http://b.com  TabID2 --> NodeIDB
+    //
+    // After restart, tab ids are reallocated. e.g, one possibility:
+    // http://a.com TabID2 --> NodeIDA
+    // http://b.com TabID1 --> NodeIDB
+    //
+    // If that happened on a remote client, here we will see an update to
+    // TabID1 with tab_node_id changing from NodeIDA to NodeIDB, and TabID2
+    // with tab_node_id changing from NodeIDB to NodeIDA.
+    //
+    // We can also wind up here if we created this tab as an out-of-order
+    // update to the header node for this session before actually associating
+    // the tab itself, so the tab node id wasn't available at the time and
+    // is currently kInvalidTabNodeID.
+    //
+    // In both cases, we can safely throw it into the set of node ids.
+    session_tracker_.AddTabNode(session_tag, specifics.tab_node_id());
+    sessions::SessionTab* tab = session_tracker_.GetTab(session_tag, tab_id);
+    if (tab->timestamp > modification_time) {
+      DVLOG(1) << "Ignoring " << session_tag << "'s session tab " << tab_id
+               << " with earlier modification time";
       return;
     }
 
-    sessions::SessionTab* tab = session_tracker_.GetTab(
-        foreign_session_tag, tab_id, specifics.tab_node_id());
-
     // Update SessionTab based on protobuf.
     tab->SetFromSyncData(tab_s, modification_time);
 
     // If a favicon or favicon urls are present, load the URLs and visit
     // times into the in-memory favicon cache.
     RefreshFaviconVisitTimesFromForeignTab(tab_s, modification_time);
 
     // Update the last modified time.
-    if (foreign_session->modified_time < modification_time)
-      foreign_session->modified_time = modification_time;
+    if (session->modified_time < modification_time)
+      session->modified_time = modification_time;
   } else {
-    LOG(WARNING) << "Ignoring foreign session node with missing header/tab "
-                 << "fields and tag " << foreign_session_tag << ".";
+    LOG(WARNING) << "Ignoring session node with missing header/tab "
+                 << "fields and tag " << session_tag << ".";
   }
 }
 
 void SessionsSyncManager::InitializeCurrentMachineTag() {
   DCHECK(current_machine_tag_.empty());
   std::string persisted_guid;
   persisted_guid = sync_prefs_->GetSyncSessionsGUID();
   if (!persisted_guid.empty()) {
     current_machine_tag_ = persisted_guid;
     DVLOG(1) << "Restoring persisted session sync guid: " << persisted_guid;
   } else {
     DCHECK(local_device_);
     std::string cache_guid = local_device_->GetLocalSyncCacheGUID();
     DCHECK(!cache_guid.empty());
     current_machine_tag_ = BuildMachineTag(cache_guid);
     DVLOG(1) << "Creating session sync guid: " << current_machine_tag_;
     sync_prefs_->SetSyncSessionsGUID(current_machine_tag_);
   }
-
-  local_tab_pool_.SetMachineTag(current_machine_tag_);
 }
 
 // static
 void SessionsSyncManager::PopulateSessionHeaderFromSpecifics(
     const sync_pb::SessionHeader& header_specifics,
     base::Time mtime,
     SyncedSession* session_header) {
   if (header_specifics.has_client_name())
     session_header->session_name = header_specifics.client_name();
   if (header_specifics.has_device_type()) {
@@ skipping 89 matching lines @@
 void SessionsSyncManager::DeleteForeignSessionInternal(
     const std::string& tag,
     syncer::SyncChangeList* change_output) {
   if (tag == current_machine_tag()) {
     LOG(ERROR) << "Attempting to delete local session. This is not currently "
                << "supported.";
     return;
   }
 
   std::set<int> tab_node_ids_to_delete;
-  session_tracker_.LookupTabNodeIds(tag, &tab_node_ids_to_delete);
+  session_tracker_.LookupForeignTabNodeIds(tag, &tab_node_ids_to_delete);
   if (DisassociateForeignSession(tag)) {
     // Only tell sync to delete the header if there was one.
     change_output->push_back(
         syncer::SyncChange(FROM_HERE, SyncChange::ACTION_DELETE,
                            SyncData::CreateLocalDelete(tag, syncer::SESSIONS)));
   }
   for (std::set<int>::const_iterator it = tab_node_ids_to_delete.begin();
        it != tab_node_ids_to_delete.end(); ++it) {
-    change_output->push_back(syncer::SyncChange(
-        FROM_HERE, SyncChange::ACTION_DELETE,
-        SyncData::CreateLocalDelete(TabNodePool::TabIdToTag(tag, *it),
-                                    syncer::SESSIONS)));
+    change_output->push_back(
+        syncer::SyncChange(FROM_HERE, SyncChange::ACTION_DELETE,
+                           SyncData::CreateLocalDelete(TabNodeIdToTag(tag, *it),
+                                                       syncer::SESSIONS)));
   }
   if (!sessions_updated_callback_.is_null())
     sessions_updated_callback_.Run();
 }
 
 bool SessionsSyncManager::DisassociateForeignSession(
     const std::string& foreign_session_tag) {
   DCHECK_NE(foreign_session_tag, current_machine_tag());
   DVLOG(1) << "Disassociating session " << foreign_session_tag;
-  return session_tracker_.DeleteSession(foreign_session_tag);
+  return session_tracker_.DeleteForeignSession(foreign_session_tag);
 }
 
 bool SessionsSyncManager::GetForeignSession(
     const std::string& tag,
     std::vector<const sessions::SessionWindow*>* windows) {
   return session_tracker_.LookupSessionWindows(tag, windows);
 }
 
 bool SessionsSyncManager::GetForeignSessionTabs(
     const std::string& tag,
@@ skipping 25 matching lines @@
 bool SessionsSyncManager::GetForeignTab(const std::string& tag,
                                         const SessionID::id_type tab_id,
                                         const sessions::SessionTab** tab) {
   const sessions::SessionTab* synced_tab = nullptr;
   bool success = session_tracker_.LookupSessionTab(tag, tab_id, &synced_tab);
   if (success)
     *tab = synced_tab;
   return success;
 }
 
-void SessionsSyncManager::LocalTabDelegateToSpecifics(
-    const SyncedTabDelegate& tab_delegate,
-    sync_pb::SessionSpecifics* specifics) {
-  sessions::SessionTab* session_tab = nullptr;
-  session_tab = session_tracker_.GetTab(current_machine_tag(),
-                                        tab_delegate.GetSessionId(),
-                                        tab_delegate.GetSyncId());
-  SetSessionTabFromDelegate(tab_delegate, base::Time::Now(), session_tab);
-  SetVariationIds(session_tab);
-  sync_pb::SessionTab tab_s = session_tab->ToSyncData();
-  specifics->set_session_tag(current_machine_tag_);
-  specifics->set_tab_node_id(tab_delegate.GetSyncId());
-  specifics->mutable_tab()->CopyFrom(tab_s);
-}
-
 void SessionsSyncManager::AssociateRestoredPlaceholderTab(
     const SyncedTabDelegate& tab_delegate,
     SessionID::id_type new_tab_id,
     SessionID::id_type new_window_id,
-    const syncer::SyncDataList& restored_tabs,
     syncer::SyncChangeList* change_output) {
   DCHECK_NE(tab_delegate.GetSyncId(), TabNodePool::kInvalidTabNodeID);
-  // Rewrite the tab using |restored_tabs| to retrieve the specifics.
-  if (restored_tabs.empty()) {
-    DLOG(WARNING) << "Can't Update tab ID.";
-    return;
-  }
 
-  for (syncer::SyncDataList::const_iterator it = restored_tabs.begin();
-       it != restored_tabs.end(); ++it) {
-    if (it->GetSpecifics().session().tab_node_id() !=
-        tab_delegate.GetSyncId()) {
-      continue;
-    }
+  // Update tracker with the new association (and inform it of the tab node
+  // in the process).
+  session_tracker_.ReassociateLocalTab(tab_delegate.GetSyncId(), new_tab_id);
 
-    sync_pb::EntitySpecifics entity;
-    sync_pb::SessionSpecifics* specifics = entity.mutable_session();
-    specifics->CopyFrom(it->GetSpecifics().session());
-    DCHECK(specifics->has_tab());
+  // Update the window id on the SessionTab itself.
+  sessions::SessionTab* local_tab =
+      session_tracker_.GetTab(current_machine_tag(), new_tab_id);
+  local_tab->window_id.set_id(new_window_id);
 
-    // Update tab node pool with the new association.
-    local_tab_pool_.ReassociateTabNode(tab_delegate.GetSyncId(), new_tab_id);
-    TabLink* tab_link = new TabLink(tab_delegate.GetSyncId(), &tab_delegate);
-    local_tab_map_[new_tab_id] = make_linked_ptr<TabLink>(tab_link);
-
-    if (specifics->tab().tab_id() == new_tab_id &&
-        specifics->tab().window_id() == new_window_id)
-      return;
-
-    // Either the tab_id or window_id changed (e.g due to session restore), so
-    // update the sync node.
-    specifics->mutable_tab()->set_tab_id(new_tab_id);
-    specifics->mutable_tab()->set_window_id(new_window_id);
-    syncer::SyncData data = syncer::SyncData::CreateLocalData(
-        TabNodePool::TabIdToTag(current_machine_tag_, specifics->tab_node_id()),
-        current_session_name_, entity);
-    change_output->push_back(
-        syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_UPDATE, data));
-    return;
-  }
+  // Rewrite the specifics based on the reassociated SessionTab to preserve
+  // the new tab and window ids.
+  sync_pb::EntitySpecifics entity;
+  entity.mutable_session()->CopyFrom(SessionTabToSpecifics(
+      *local_tab, current_machine_tag(), tab_delegate.GetSyncId()));
+  syncer::SyncData data = syncer::SyncData::CreateLocalData(
+      TabNodeIdToTag(current_machine_tag(), tab_delegate.GetSyncId()),
+      current_session_name_, entity);
+  change_output->push_back(
+      syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_UPDATE, data));
 }
 
 // static
 void SessionsSyncManager::SetSessionTabFromDelegate(
     const SyncedTabDelegate& tab_delegate,
     base::Time mtime,
     sessions::SessionTab* session_tab) {
   DCHECK(session_tab);
   session_tab->window_id.set_id(tab_delegate.GetWindowId());
   session_tab->tab_id.set_id(tab_delegate.GetSessionId());
@@ skipping 98 matching lines @@
 }
 
 // static
 std::string SessionsSyncManager::TagHashFromSpecifics(
     const sync_pb::SessionSpecifics& specifics) {
   return syncer::GenerateSyncableHash(syncer::SESSIONS,
                                       TagFromSpecifics(specifics));
 }
 
 };  // namespace sync_sessions