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

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/metrics/field_trial.h"
@@ skipping 25 matching lines @@
 
 // The maximum number of navigations in each direction we care to sync.
 const int kMaxSyncNavigationCount = 6;
 
 // The URL at which the set of synced tabs is displayed. We treat it differently
 // from all other URL's as accessing it triggers a sync refresh of Sessions.
 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).
 bool TabsRecencyComparator(const sessions::SessionTab* t1,
                            const sessions::SessionTab* t2) {
   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).
@@ skipping 451 matching lines @@
           // to avoid any possible ping-pong delete/reassociate sequence, but
           // remember that this happened as our TabNodePool is inconsistent.
           local_tab_pool_out_of_sync_ = true;
           LOG(WARNING) << "Local session data deleted. Ignoring until next "
                        << "local navigation event.";
         } else if (session.has_header()) {
           // Disassociate only when header node is deleted. For tab node
           // deletions, the header node will be updated and foreign tab will
           // get deleted.
           DisassociateForeignSession(session.session_tag());
+        } else if (session.has_tab()) {
+          // The challenge here is that we don't know if this tab deletion is
+          // being processed before or after the parent was updated to no longer
+          // references the tab. 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:
       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.
-          // 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();
         }
         UpdateTrackerWithForeignSession(
             session, syncer::SyncDataRemote(it->sync_data()).GetModifiedTime());
         break;
       default:
         NOTREACHED() << "Processing sync changes failed, unknown change type.";
     }
   }
@@ skipping 12 matching lines @@
     return syncer::SyncChange(
         FROM_HERE, SyncChange::ACTION_DELETE,
         SyncData::CreateLocalDelete(
             TabNodePool::TabIdToTag(current_machine_tag(), tab.tab_node_id()),
             syncer::SESSIONS));
   }
 }
 
 bool SessionsSyncManager::GetAllForeignSessions(
     std::vector<const sync_driver::SyncedSession*>* sessions) {
-  if (!session_tracker_.LookupAllForeignSessions(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;
   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();
     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()) {
@@ skipping 15 matching lines @@
             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);
         }
       }
     }
   }
+
+  // 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,
+                                            SyncedSessionTracker::RAW);
+  for (const auto* session : sessions) {
+    session_tracker_.CleanupSession(session->session_tag);
+  }
+
   return found_current_header;
 }
 
 void SessionsSyncManager::UpdateTrackerWithForeignSession(
     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());
 
   sync_driver::SyncedSession* foreign_session =
@@ skipping 35 matching lines @@
     // Delete any closed windows and unused tabs as necessary.
     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();
 
     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);
@@ skipping 57 matching lines @@
       case sync_pb::SyncEnums_DeviceType_TYPE_TABLET:
         session_header->device_type = sync_driver::SyncedSession::TYPE_TABLET;
         break;
       case sync_pb::SyncEnums_DeviceType_TYPE_OTHER:
       // Intentionally fall-through
       default:
         session_header->device_type = sync_driver::SyncedSession::TYPE_OTHER;
         break;
     }
   }
-  session_header->modified_time = mtime;
+  session_header->modified_time =
+      std::max(mtime, session_header->modified_time);
 }
 
 // static
 void SessionsSyncManager::BuildSyncedSessionFromSpecifics(
     const std::string& session_tag,
     const sync_pb::SessionWindow& specifics,
     base::Time mtime,
     sessions::SessionWindow* session_window) {
   if (specifics.has_window_id())
     session_window->window_id.set_id(specifics.window_id());
@@ skipping 50 matching lines @@
     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);
-  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(
         FROM_HERE, SyncChange::ACTION_DELETE,
         SyncData::CreateLocalDelete(TabNodePool::TabIdToTag(tag, *it),
                                     syncer::SESSIONS)));
   }
   if (!sessions_updated_callback_.is_null())
     sessions_updated_callback_.Run();
 }
 
 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);
 }
 
 bool SessionsSyncManager::GetForeignSession(
     const std::string& tag,
     std::vector<const sessions::SessionWindow*>* windows) {
   return session_tracker_.LookupSessionWindows(tag, windows);
 }
 
@@ skipping 171 matching lines @@
   return &favicon_cache_;
 }
 
 SyncedWindowDelegatesGetter*
 SessionsSyncManager::synced_window_delegates_getter() const {
   return sessions_client_->GetSyncedWindowDelegatesGetter();
 }
 
 void SessionsSyncManager::DoGarbageCollection() {
   std::vector<const sync_driver::SyncedSession*> sessions;
-  if (!GetAllForeignSessions(&sessions))
+  if (!session_tracker_.LookupAllForeignSessions(&sessions,
+                                                 SyncedSessionTracker::RAW))
     return;  // No foreign sessions.
 
   // Iterate through all the sessions and delete any with age older than
   // |stale_session_threshold_days_|.
   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())
     sync_processor_->ProcessSyncChanges(FROM_HERE, changes);
 }
 
 };  // namespace browser_sync