Revert of Reland v4 of Session 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 c13df7cb44fb3a9571d5c537c3d814652bc32d8b..4b0938e0bfaa7f4c6d87d6deb8872efc3c9cb78c 100644
--- a/components/sync_sessions/sessions_sync_manager.cc
+++ b/components/sync_sessions/sessions_sync_manager.cc
@@ -7,12 +7,9 @@
 #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"
@@ -24,7 +21,6 @@
 #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;
@@ -66,58 +62,15 @@
   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 TabNodeIdToTag(specifics.session_tag(), specifics.tab_node_id());
+    return TabNodePool::TabIdToTag(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 and is not already synced (which
-// can confuse the logic that tracks whether tabs are mapped or unmapped).
-bool ShouldSyncTabId(
-    SessionID::id_type tab_id,
-    const google::protobuf::RepeatedField<int>& synced_tab_ids) {
-  if (tab_id == TabNodePool::kInvalidTabID)
-    return false;
-
-  for (auto synced_tab_id : synced_tab_ids) {
-    if (tab_id == synced_tab_id)
-      return false;
-  }
-
-  return true;
 }
 
 }  // namespace
@@ -164,6 +117,7 @@
     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);
@@ -199,12 +153,13 @@
     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_.
-  if (!InitFromSyncModel(initial_sync_data, &new_changes)) {
+  syncer::SyncDataList restored_tabs;
+  if (!InitFromSyncModel(initial_sync_data, &restored_tabs, &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();
@@ -221,12 +176,12 @@
 #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, &new_changes);
+  AssociateWindows(RELOAD_TABS, restored_tabs, &new_changes);
   local_tab_pool_out_of_sync_ = false;
 
   merge_result.set_error(
@@ -238,6 +193,7 @@
 
 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;
@@ -305,33 +261,30 @@
         if (!synced_tab)
           continue;
 
-        if (!ShouldSyncTabId(tab_id, window_s.tab())) {
-          LOG(ERROR) << "Not syncing invalid/duplicate 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.
-          if (synced_tab->GetSyncId() > TabNodePool::kInvalidTabNodeID) {
+          // 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,
-                                            change_output);
+                                            restored_tabs, change_output);
+            found_tabs = true;
+            window_s.add_tab(tab_id);
           }
-        } else if (RELOAD_TABS == option) {
+          continue;
+        }
+
+        if (RELOAD_TABS == option)
           AssociateTab(synced_tab, change_output);
-        }
-
-        // 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.
+
+        // 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.
         const sessions::SessionTab* tab = nullptr;
         if (session_tracker_.LookupSessionTab(local_tag, tab_id, &tab)) {
           found_tabs = true;
@@ -350,10 +303,8 @@
       }
     }
   }
-  std::set<int> deleted_tab_node_ids;
-  session_tracker_.CleanupLocalTabs(&deleted_tab_node_ids);
-  AppendDeletionsForTabNodes(deleted_tab_node_ids, current_machine_tag(),
-                             change_output);
+  local_tab_pool_.DeleteUnassociatedTabNodes(change_output);
+  session_tracker_.CleanupSession(local_tag);
 
   // Always update the header.  Sync takes care of dropping this update
   // if the entity specifics are identical (i.e windows, client name did
@@ -366,63 +317,73 @@
       syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_UPDATE, data));
 }
 
-void SessionsSyncManager::AssociateTab(SyncedTabDelegate* const tab_delegate,
+void SessionsSyncManager::AssociateTab(SyncedTabDelegate* const tab,
                                        syncer::SyncChangeList* change_output) {
-  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.
+  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);
     return;
   }
 
-  if (!tab_delegate->ShouldSync(sessions_client_))
+  if (!tab->ShouldSync(sessions_client_))
     return;
 
-  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);
+  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();
+
+  // 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));
+
+  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));
+  }
+
   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));
-  }
 }
 
 bool SessionsSyncManager::RebuildAssociations() {
@@ -482,14 +443,21 @@
   // "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, &changes);
+  AssociateWindows(DONT_RELOAD_TABS, syncer::SyncDataList(), &changes);
   sync_processor_->ProcessSyncChanges(FROM_HERE, changes);
 }
 
 void SessionsSyncManager::OnFaviconsChanged(const std::set<GURL>& page_urls,
                                             const GURL& /* icon_url */) {
-  for (const GURL& page_url : page_urls)
-    favicon_cache_.OnPageFaviconUpdated(page_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);
+    }
+  }
 }
 
 void SessionsSyncManager::StopSyncing(syncer::ModelType type) {
@@ -502,6 +470,8 @@
   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;
@@ -524,17 +494,22 @@
       current_machine_tag(), current_session_name_, header_entity);
   list.push_back(data);
 
-  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;
+  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) {
       sync_pb::EntitySpecifics entity;
-      entity.mutable_session()->CopyFrom(
-          SessionTabToSpecifics(*tab, current_machine_tag(), tab_node_id));
+      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());
       syncer::SyncData data = syncer::SyncData::CreateLocalData(
-          TabNodeIdToTag(current_machine_tag(), tab_node_id),
+          TabNodePool::TabIdToTag(current_machine_tag_,
+                                  specifics->tab_node_id()),
           current_session_name_, entity);
       list.push_back(data);
     }
@@ -543,7 +518,7 @@
 }
 
 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;
@@ -603,7 +578,7 @@
           LOG(WARNING) << "Dropping modification to local session.";
           return syncer::SyncError();
         }
-        UpdateTrackerWithSpecifics(
+        UpdateTrackerWithForeignSession(
             session, syncer::SyncDataRemote(it->sync_data()).GetModifiedTime());
         break;
       default:
@@ -625,7 +600,7 @@
     return syncer::SyncChange(
         FROM_HERE, SyncChange::ACTION_DELETE,
         SyncData::CreateLocalDelete(
-            TabNodeIdToTag(current_machine_tag(), tab.tab_node_id()),
+            TabNodePool::TabIdToTag(current_machine_tag(), tab.tab_node_id()),
             syncer::SESSIONS));
   }
 }
@@ -641,6 +616,7 @@
 
 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;
@@ -658,7 +634,7 @@
         new_changes->push_back(tombstone);
     } else if (specifics.session_tag() != current_machine_tag()) {
       if (TagHashFromSpecifics(specifics) == remote.GetClientTagHash()) {
-        UpdateTrackerWithSpecifics(specifics, remote.GetModifiedTime());
+        UpdateTrackerWithForeignSession(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
@@ -677,10 +653,6 @@
         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 "
@@ -688,19 +660,11 @@
           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 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());
+          // 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);
         }
       }
     }
@@ -712,7 +676,7 @@
   session_tracker_.LookupAllForeignSessions(&sessions,
                                             SyncedSessionTracker::RAW);
   for (const auto* session : sessions) {
-    session_tracker_.CleanupForeignSession(session->session_tag);
+    session_tracker_.CleanupSession(session->session_tag);
   }
 
   UMA_HISTOGRAM_COUNTS_100("Sync.SessionsBadForeignHashOnMergeCount",
@@ -721,83 +685,69 @@
   return found_current_header;
 }
 
-void SessionsSyncManager::UpdateTrackerWithSpecifics(
+void SessionsSyncManager::UpdateTrackerWithForeignSession(
     const sync_pb::SessionSpecifics& specifics,
     const base::Time& modification_time) {
-  std::string session_tag = specifics.session_tag();
-  SyncedSession* session = session_tracker_.GetSession(session_tag);
+  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);
   if (specifics.has_header()) {
-    // Read in the header data for this session. Header data is
+    // Read in the header data for this foreign 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 session node with invalid header "
-                   << "and tag " << session_tag << ".";
+      LOG(WARNING) << "Ignoring foreign session node with invalid header "
+                   << "and tag " << foreign_session_tag << ".";
       return;
     }
 
     // Load (or create) the SyncedSession object for this client.
     const sync_pb::SessionHeader& header = specifics.header();
-    PopulateSessionHeaderFromSpecifics(header, modification_time, session);
+    PopulateSessionHeaderFromSpecifics(header, modification_time,
+                                       foreign_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(session_tag);
+    session_tracker_.ResetSessionTracking(foreign_session_tag);
 
     // Process all the windows and their tab information.
     int num_windows = header.window_size();
-    DVLOG(1) << "Populating " << session_tag << " with " << num_windows
+    DVLOG(1) << "Associating " << foreign_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(session_tag, window_id);
-      BuildSyncedSessionFromSpecifics(session_tag, window_s, modification_time,
-                                      session->windows[window_id].get());
+      session_tracker_.PutWindowInSession(foreign_session_tag, window_id);
+      BuildSyncedSessionFromSpecifics(
+          foreign_session_tag, window_s, modification_time,
+          foreign_session->windows[window_id].get());
     }
     // Delete any closed windows and unused tabs as necessary.
-    session_tracker_.CleanupForeignSession(session_tag);
+    session_tracker_.CleanupSession(foreign_session_tag);
   } 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();
-
-    // 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;
+
+    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";
       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);
@@ -807,11 +757,11 @@
     RefreshFaviconVisitTimesFromForeignTab(tab_s, modification_time);
 
     // Update the last modified time.
-    if (session->modified_time < modification_time)
-      session->modified_time = modification_time;
+    if (foreign_session->modified_time < modification_time)
+      foreign_session->modified_time = modification_time;
   } else {
-    LOG(WARNING) << "Ignoring session node with missing header/tab "
-                 << "fields and tag " << session_tag << ".";
+    LOG(WARNING) << "Ignoring foreign session node with missing header/tab "
+                 << "fields and tag " << foreign_session_tag << ".";
   }
 }
 
@@ -829,6 +779,8 @@
     DVLOG(1) << "Creating session sync guid: " << current_machine_tag_;
     sync_prefs_->SetSyncSessionsGUID(current_machine_tag_);
   }
+
+  local_tab_pool_.SetMachineTag(current_machine_tag_);
 }
 
 // static
@@ -892,11 +844,11 @@
     }
   }
   session_window->timestamp = mtime;
-  session_window->tabs.clear();
+  session_window->tabs.resize(specifics.tab_size());
   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);
+                                    tab_id, i);
   }
 }
 
@@ -938,14 +890,20 @@
   }
 
   std::set<int> tab_node_ids_to_delete;
-  session_tracker_.LookupForeignTabNodeIds(tag, &tab_node_ids_to_delete);
+  session_tracker_.LookupTabNodeIds(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)));
   }
-  AppendDeletionsForTabNodes(tab_node_ids_to_delete, tag, change_output);
+  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)));
+  }
   if (!sessions_updated_callback_.is_null())
     sessions_updated_callback_.Run();
 }
@@ -954,7 +912,7 @@
     const std::string& foreign_session_tag) {
   DCHECK_NE(foreign_session_tag, current_machine_tag());
   DVLOG(1) << "Disassociating session " << foreign_session_tag;
-  return session_tracker_.DeleteForeignSession(foreign_session_tag);
+  return session_tracker_.DeleteSession(foreign_session_tag);
 }
 
 bool SessionsSyncManager::GetForeignSession(
@@ -1000,41 +958,66 @@
   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);
-
-  // 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.";
+  // Rewrite the tab using |restored_tabs| to retrieve the specifics.
+  if (restored_tabs.empty()) {
+    DLOG(WARNING) << "Can't Update tab ID.";
     return;
   }
 
-  // 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 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);
-
-  // 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));
+  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 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;
+  }
 }
 
 // static