[Sync] Moved tab_node_id tracking to session object and improved foreign session garbage collection.

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 0b9eb7e56112aa0ea852985ad699a5ec55a7435e..b72e5357ee672963699b4b2b53128e1ca384cdd7 100644
--- a/components/sync_sessions/sessions_sync_manager.cc
+++ b/components/sync_sessions/sessions_sync_manager.cc
@@ -43,7 +43,7 @@ const char kNTPOpenTabSyncURL[] = "chrome://newtab/#open_tabs";
 
 // Default number of days without activity after which a session is considered
 // stale and becomes a candidate for garbage collection.
-const size_t kDefaultStaleSessionThresholdDays = 14;  // 2 weeks.
+const int kDefaultStaleSessionThresholdDays = 14;  // 2 weeks.
 
 // Comparator function for use with std::sort that will sort tabs by
 // descending timestamp (i.e., most recent first).
@@ -515,6 +515,17 @@ syncer::SyncError SessionsSyncManager::ProcessSyncChanges(
           // deletions, the header node will be updated and foreign tab will
           // get deleted.
           DisassociateForeignSession(session.session_tag());
+        } else if (session.has_tab() &&
+                   current_machine_tag() != session.session_tag()) {
+          // The challenge here is that we don't know if this tab deletion is
+          // being processes before or after the parent was updated to no longer
+          // reference us, or even more extreme, the parent has been deleted as
+          // well. Tell the tracker to do what it can. The header's update will
+          // mostly get us into the correct state, the only thing this deletion
+          // needs to accomplish is make sure we never tell sync to delete this
+          // tab later during garbage collection.
+          session_tracker_.DeleteForeignTab(session.session_tag(),
+                                            session.tab_node_id());
         }
         break;
       case syncer::SyncChange::ACTION_ADD:
@@ -523,9 +534,6 @@ syncer::SyncError SessionsSyncManager::ProcessSyncChanges(
           // 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.
-          // TODO(skym): Is it really safe to return here? Why not continue?
-          // Couldn't there be multiple SessionSpecifics in the SyncChangeList
-          // that contain different session tags?
           LOG(WARNING) << "Dropping modification to local session.";
           return syncer::SyncError();
         }
@@ -558,7 +566,7 @@ syncer::SyncChange SessionsSyncManager::TombstoneTab(
 
 bool SessionsSyncManager::GetAllForeignSessions(
     std::vector<const sync_driver::SyncedSession*>* sessions) {
-  if (!session_tracker_.LookupAllForeignSessions(sessions))
+  if (!session_tracker_.LookupAllForeignSessions(sessions, true))
     return false;
   std::sort(sessions->begin(), sessions->end(), SessionsRecencyComparator);
   return true;
@@ -571,8 +579,6 @@ bool SessionsSyncManager::InitFromSyncModel(
   bool found_current_header = false;
   for (syncer::SyncDataList::const_iterator it = sync_data.begin();
        it != sync_data.end(); ++it) {
-    // TODO(skym): Why don't we ever look at data.change_type()? Why is this
-    // code path so much different from ProcessSyncChanges?
     const syncer::SyncData& data = *it;
     DCHECK(data.GetSpecifics().has_session());
     const sync_pb::SessionSpecifics& specifics = data.GetSpecifics().session();
@@ -608,6 +614,15 @@ bool SessionsSyncManager::InitFromSyncModel(
       }
     }
   }
+
+  // Cleanup all foreign sessions, since orphaned tabs may have been added after
+  // the header.
+  std::vector<const sync_driver::SyncedSession*> sessions;
+  session_tracker_.LookupAllForeignSessions(&sessions, false);
+  for (const auto* session : sessions) {
+    session_tracker_.CleanupSession(session->session_tag);
+  }
+
   return found_current_header;
 }
 
@@ -619,6 +634,7 @@ void SessionsSyncManager::UpdateTrackerWithForeignSession(
 
   sync_driver::SyncedSession* foreign_session =
       session_tracker_.GetSession(foreign_session_tag);
+
   if (specifics.has_header()) {
     // 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
@@ -635,9 +651,8 @@ void SessionsSyncManager::UpdateTrackerWithForeignSession(
     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).
+    // Reset the tab/window tracking for this session before adding them all
+    // back. This allows us to avoid a two way diff.
     session_tracker_.ResetSessionTracking(foreign_session_tag);
 
     // Process all the windows and their tab information.
@@ -663,6 +678,11 @@ void SessionsSyncManager::UpdateTrackerWithForeignSession(
     if (session_tracker_.LookupSessionTab(foreign_session_tag, tab_id,
                                           &existing_tab) &&
         existing_tab->timestamp > modification_time) {
+      // Remember that there is more sync data behind this tab_id if we decide
+      // to garbage collection this session. We never do granular garbage
+      // collection because we don't want to fight with the owning device.
+      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;
@@ -740,7 +760,8 @@ void SessionsSyncManager::PopulateSessionHeaderFromSpecifics(
         break;
     }
   }
-  session_header->modified_time = mtime;
+  session_header->modified_time =
+      std::max(mtime, session_header->modified_time);
 }
 
 // static
@@ -811,16 +832,12 @@ void SessionsSyncManager::DeleteForeignSessionInternal(
 
   std::set<int> tab_node_ids_to_delete;
   session_tracker_.LookupTabNodeIds(tag, &tab_node_ids_to_delete);
-  if (!DisassociateForeignSession(tag)) {
-    // We don't have any data for this session, our work here is done!
-    return;
+  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)));
   }
-
-  // Prepare deletes for the meta-node as well as individual tab nodes.
-  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(
@@ -834,11 +851,7 @@ void SessionsSyncManager::DeleteForeignSessionInternal(
 
 bool SessionsSyncManager::DisassociateForeignSession(
     const std::string& foreign_session_tag) {
-  if (foreign_session_tag == current_machine_tag()) {
-    DVLOG(1) << "Local session deleted! Doing nothing until a navigation is "
-             << "triggered.";
-    return false;
-  }
+  DCHECK_NE(foreign_session_tag, current_machine_tag());
   DVLOG(1) << "Disassociating session " << foreign_session_tag;
   return session_tracker_.DeleteSession(foreign_session_tag);
 }
@@ -1030,30 +1043,22 @@ SessionsSyncManager::synced_window_delegates_getter() const {
 
 void SessionsSyncManager::DoGarbageCollection() {
   std::vector<const sync_driver::SyncedSession*> sessions;
-  if (!GetAllForeignSessions(&sessions))
-    return;  // No foreign sessions.
-
-  // Iterate through all the sessions and delete any with age older than
-  // |stale_session_threshold_days_|.
+  session_tracker_.LookupAllForeignSessions(&sessions, false);
   syncer::SyncChangeList changes;
-  for (std::vector<const sync_driver::SyncedSession*>::const_iterator iter =
-           sessions.begin();
-       iter != sessions.end(); ++iter) {
-    const sync_driver::SyncedSession* session = *iter;
+  for (const auto* session : sessions) {
     int session_age_in_days =
         (base::Time::Now() - session->modified_time).InDays();
-    std::string session_tag = session->session_tag;
-    if (session_age_in_days > 0 &&  // If false, local clock is not trustworty.
-        static_cast<size_t>(session_age_in_days) >
-            stale_session_threshold_days_) {
+    if (session_age_in_days > stale_session_threshold_days_) {
+      std::string session_tag = session->session_tag;
       DVLOG(1) << "Found stale session " << session_tag << " with age "
                << session_age_in_days << ", deleting.";
       DeleteForeignSessionInternal(session_tag, &changes);
     }
   }
 
-  if (!changes.empty())
+  if (!changes.empty()) {
     sync_processor_->ProcessSyncChanges(FROM_HERE, changes);
+  }
 }
 
 };  // namespace browser_sync