Reland v5 of Sessions Refactor

components/sync_sessions/sessions_sync_manager.cc

Index: components/sync_sessions/sessions_sync_manager.cc
diff --git a/components/sync_sessions/sessions_sync_manager.cc b/components/sync_sessions/sessions_sync_manager.cc
index 4b0938e0bfaa7f4c6d87d6deb8872efc3c9cb78c..6db29af031be86b47197350469369aced8acf80c 100644
--- a/components/sync_sessions/sessions_sync_manager.cc
+++ b/components/sync_sessions/sessions_sync_manager.cc
@@ -7,9 +7,12 @@
 #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"
@@ -21,6 +24,7 @@
 #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;
@@ -62,17 +66,53 @@ bool SessionsRecencyComparator(const SyncedSession* s1,
   return s1->modified_time > s2->modified_time;
 }
 
+std::string TabNodeIdToTag(const std::string& machine_tag, int tab_node_id) {
+  CHECK_GT(tab_node_id, TabNodePool::kInvalidTabNodeID) << "crbug.com/673618";
+  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;
+}
+
+void AppendDeletionsForTabNodes(const std::set<int>& tab_node_ids,
+                                const std::string& machine_tag,
+                                syncer::SyncChangeList* change_output) {
+  for (std::set<int>::const_iterator it = tab_node_ids.begin();
+       it != tab_node_ids.end(); ++it) {
+    change_output->push_back(syncer::SyncChange(
+        FROM_HERE, SyncChange::ACTION_DELETE,
+        SyncData::CreateLocalDelete(TabNodeIdToTag(machine_tag, *it),
+                                    syncer::SESSIONS)));
+  }
+}
+
+// Ensure that the tab id is not invalid.
+bool ShouldSyncTabId(
+    SessionID::id_type tab_id,
+    const google::protobuf::RepeatedField<int>& synced_tab_ids) {

[skym, 2017/02/27 18:22:38]

Should this param be removed?

[Nicolas Zea, 2017/02/28 00:09:01]

Woops, yes it should. Done.

+  if (tab_id == TabNodePool::kInvalidTabID)
+    return false;
+  return true;
+}
+
 }  // namespace
 
 // |local_device| is owned by ProfileSyncService, its lifetime exceeds
@@ -117,7 +157,6 @@ syncer::SyncMergeResult SessionsSyncManager::MergeDataAndStartSyncing(
     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);
@@ -153,13 +192,12 @@ syncer::SyncMergeResult SessionsSyncManager::MergeDataAndStartSyncing(
     InitializeCurrentMachineTag(local_device_->GetLocalSyncCacheGUID());
   }
 
-  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();
@@ -176,12 +214,12 @@ syncer::SyncMergeResult SessionsSyncManager::MergeDataAndStartSyncing(
 #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(
@@ -193,7 +231,6 @@ syncer::SyncMergeResult SessionsSyncManager::MergeDataAndStartSyncing(
 
 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;
@@ -205,7 +242,7 @@ void SessionsSyncManager::AssociateWindows(
   header_s->set_device_type(current_device_type_);
 
   session_tracker_.ResetSessionTracking(local_tag);
-  std::set<const SyncedWindowDelegate*> windows =
+  SyncedWindowDelegatesGetter::SyncedWindowDelegateMap windows =
       synced_window_delegates_getter()->GetSyncedWindowDelegates();
 
   if (option == RELOAD_TABS) {
@@ -217,7 +254,8 @@ void SessionsSyncManager::AssociateWindows(
     LOG(ERROR) << "No windows present, see crbug.com/639009";
     return;
   }
-  for (const SyncedWindowDelegate* window_delegate : windows) {
+  for (auto window_iter_pair : windows) {
+    const SyncedWindowDelegate* window_delegate = window_iter_pair.second;
     if (option == RELOAD_TABS) {
       UMA_HISTOGRAM_COUNTS("Sync.SessionTabs", window_delegate->GetTabCount());
     }
@@ -261,30 +299,33 @@ void SessionsSyncManager::AssociateWindows(
         if (!synced_tab)
           continue;
 
+        if (!ShouldSyncTabId(tab_id, window_s.tab())) {
+          LOG(ERROR) << "Not syncing invalid tab with id " << tab_id;
+          continue;
+        }
+
+        // Placeholder tabs are those without WebContents, either because they
+        // were never loaded into memory or they were evicted from memory
+        // (typically only on Android devices). They only have a tab id, window
+        // id, and a saved synced id (corresponding to the tab node id). Note
+        // that only placeholders have this sync id, as it's necessary to
+        // properly reassociate the tab with the entity that was backing it.
         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) {
+          if (synced_tab->GetSyncId() > TabNodePool::kInvalidTabNodeID) {
             AssociateRestoredPlaceholderTab(*synced_tab, tab_id, window_id,
-                                            restored_tabs, change_output);
-            found_tabs = true;
-            window_s.add_tab(tab_id);
+                                            change_output);
           }
-          continue;
-        }
-
-        if (RELOAD_TABS == option)
+        } else if (RELOAD_TABS == option) {
           AssociateTab(synced_tab, change_output);
+        }
 
-        // If the tab is valid, it would have been added to the tracker either
-        // by the above AssociateTab call (at association time), or by the
-        // change processor calling AssociateTab for all modified tabs.
-        // Therefore, we can key whether this window has valid tabs based on
-        // the tab's presence in the tracker.
+        // If the tab was syncable, it would have been added to the tracker
+        // either by the above Associate[RestoredPlaceholder]Tab call or by the
+        // OnLocalTabModified method invoking AssociateTab directly. Therefore,
+        // we can key whether this window has valid tabs based on the tab's
+        // presence in the tracker.
         const sessions::SessionTab* tab = nullptr;
         if (session_tracker_.LookupSessionTab(local_tag, tab_id, &tab)) {
           found_tabs = true;
@@ -303,8 +344,10 @@ void SessionsSyncManager::AssociateWindows(
       }
     }
   }
-  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);
+  AppendDeletionsForTabNodes(deleted_tab_node_ids, current_machine_tag(),
+                             change_output);
 
   // Always update the header.  Sync takes care of dropping this update
   // if the entity specifics are identical (i.e windows, client name did
@@ -317,73 +360,64 @@ void SessionsSyncManager::AssociateWindows(
       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();
-
-  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);
+  DCHECK(!tab_delegate->IsPlaceholderTab());
+
+  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;
-
-  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();
+  SessionID::id_type tab_id = tab_delegate->GetSessionId();
+  DVLOG(1) << "Syncing tab " << tab_id << " from window "
+           << tab_delegate->GetWindowId();
+
+  int tab_node_id = TabNodePool::kInvalidTabNodeID;
+  bool existing_tab_node =
+      session_tracker_.GetTabNodeFromLocalTabId(tab_id, &tab_node_id);
+  CHECK_NE(TabNodePool::kInvalidTabNodeID, tab_node_id) << "crbug.com/673618";
+  tab_delegate->SetSyncId(tab_node_id);
+  sessions::SessionTab* session_tab =
+      session_tracker_.GetTab(current_machine_tag(), tab_id);
+
+  // 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();
+
+  // 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 sync model.
+  // Write to the sync model itself.
   sync_pb::EntitySpecifics specifics;
-  LocalTabDelegateToSpecifics(*tab, specifics.mutable_session());
+  specifics.mutable_session()->CopyFrom(
+      SessionTabToSpecifics(*session_tab, current_machine_tag(), tab_node_id));
   syncer::SyncData data = syncer::SyncData::CreateLocalData(
-      TabNodePool::TabIdToTag(current_machine_tag_, tab_link->tab_node_id()),
-      current_session_name_, specifics);
+      TabNodeIdToTag(current_machine_tag(), tab_node_id), current_session_name_,
+      specifics);
   change_output->push_back(
-      syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_UPDATE, data));
-
-  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));
+      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->GetTransitionAtIndex(current_index));
+        new_url, tab_delegate->GetTransitionAtIndex(current_index));
   }
-
-  session_tracker_.GetSession(current_machine_tag())->modified_time =
-      base::Time::Now();
 }
 
 bool SessionsSyncManager::RebuildAssociations() {
@@ -443,21 +477,14 @@ void SessionsSyncManager::OnLocalTabModified(SyncedTabDelegate* modified_tab) {
   // "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) {
@@ -470,8 +497,6 @@ void SessionsSyncManager::StopSyncing(syncer::ModelType type) {
   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;
@@ -494,22 +519,17 @@ syncer::SyncDataList SessionsSyncManager::GetAllSyncData(
       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);
     }
@@ -518,7 +538,7 @@ syncer::SyncDataList SessionsSyncManager::GetAllSyncData(
 }
 
 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;
@@ -578,7 +598,7 @@ syncer::SyncError SessionsSyncManager::ProcessSyncChanges(
           LOG(WARNING) << "Dropping modification to local session.";
           return syncer::SyncError();
         }
-        UpdateTrackerWithForeignSession(
+        UpdateTrackerWithSpecifics(
             session, syncer::SyncDataRemote(it->sync_data()).GetModifiedTime());
         break;
       default:
@@ -600,7 +620,7 @@ syncer::SyncChange SessionsSyncManager::TombstoneTab(
     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));
   }
 }
@@ -616,7 +636,6 @@ bool SessionsSyncManager::GetAllForeignSessions(
 
 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;
@@ -634,7 +653,7 @@ bool SessionsSyncManager::InitFromSyncModel(
         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
@@ -653,6 +672,10 @@ bool SessionsSyncManager::InitFromSyncModel(
         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 "
@@ -660,11 +683,19 @@ bool SessionsSyncManager::InitFromSyncModel(
           syncer::SyncChange tombstone(TombstoneTab(specifics));
           if (tombstone.IsValid())
             new_changes->push_back(tombstone);
+        } else if (specifics.tab().tab_id() == TabNodePool::kInvalidTabID) {
+          LOG(WARNING) << "Found tab node with invalid tab id.";
+          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());
         }
       }
     }
@@ -676,7 +707,7 @@ bool SessionsSyncManager::InitFromSyncModel(
   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",
@@ -685,70 +716,84 @@ bool SessionsSyncManager::InitFromSyncModel(
   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);
   } else if (specifics.has_tab()) {
     const sync_pb::SessionTab& tab_s = specifics.tab();
     SessionID::id_type tab_id = tab_s.tab_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";
+    DVLOG(1) << "Populating " << session_tag << "'s tab id " << tab_id
+             << " from node " << specifics.tab_node_id();
+
+    // 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_.OnTabNodeSeen(session_tag, specifics.tab_node_id());
+    sessions::SessionTab* tab = session_tracker_.GetTab(session_tag, tab_id);
+    if (!tab->timestamp.is_null() && tab->timestamp > modification_time) {
+      DVLOG(1) << "Ignoring " << session_tag << "'s session tab " << tab_id
+               << " with earlier modification time: " << tab->timestamp
+               << " vs " << 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);
 
@@ -757,11 +802,11 @@ void SessionsSyncManager::UpdateTrackerWithForeignSession(
     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 << ".";
   }
 }
 
@@ -779,8 +824,6 @@ void SessionsSyncManager::InitializeCurrentMachineTag(
     DVLOG(1) << "Creating session sync guid: " << current_machine_tag_;
     sync_prefs_->SetSyncSessionsGUID(current_machine_tag_);
   }
-
-  local_tab_pool_.SetMachineTag(current_machine_tag_);
 }
 
 // static
@@ -844,11 +887,11 @@ void SessionsSyncManager::BuildSyncedSessionFromSpecifics(
     }
   }
   session_window->timestamp = mtime;
-  session_window->tabs.resize(specifics.tab_size());
+  session_window->tabs.clear();
   for (int i = 0; i < specifics.tab_size(); i++) {
     SessionID::id_type tab_id = specifics.tab(i);
     session_tracker_.PutTabInWindow(session_tag, session_window->window_id.id(),
-                                    tab_id, i);
+                                    tab_id);
   }
 }
 
@@ -890,20 +933,14 @@ void SessionsSyncManager::DeleteForeignSessionInternal(
   }
 
   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)));
-  }
+  AppendDeletionsForTabNodes(tab_node_ids_to_delete, tag, change_output);
   if (!sessions_updated_callback_.is_null())
     sessions_updated_callback_.Run();
 }
@@ -912,7 +949,7 @@ 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(
@@ -958,66 +995,41 @@ bool SessionsSyncManager::GetForeignTab(const std::string& tag,
   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.";
+
+  // It's possible the placeholder tab is associated with a tab node that's
+  // since been deleted. If that's the case, there's no way to reassociate it,
+  // so just return now without adding the tab to the session tracker.
+  if (!session_tracker_.IsLocalTabNodeAssociated(tab_delegate.GetSyncId())) {
+    DVLOG(1) << "Restored placeholder tab's node " << tab_delegate.GetSyncId()
+             << " deleted.";
     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;
-    }
-
-    sync_pb::EntitySpecifics entity;
-    sync_pb::SessionSpecifics* specifics = entity.mutable_session();
-    specifics->CopyFrom(it->GetSpecifics().session());
-    DCHECK(specifics->has_tab());
+  // 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);
 
-    // 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;
+  // 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);
 
-    // 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