[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() &&
+                   current_machine_tag() != session.session_tag()) {

[Nicolas Zea, 2016/04/12 20:38:03]

the current_machine_tag condition isn't necessary here right? (given the
original if statement above)

[skym, 2016/04/12 22:12:14]

Done.

+          // 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

[Nicolas Zea, 2016/04/12 20:38:03]

being processes -> being processed

[skym, 2016/04/12 22:12:14]

Done.

+          // reference us, or even more extreme, the parent has been deleted as

[Nicolas Zea, 2016/04/12 20:38:03]

"reference us, or even" -> "reference the tab. Or, even"

[skym, 2016/04/12 22:12:13]

Done.

+          // 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_.TryDeleteTab(session.session_tag(),
+                                        session.tab().tab_id(),
+                                        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, true))
     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 26 matching lines @@
 }
 
 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 =
       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
     // for their tabs.
 
     if (!IsValidSessionHeader(specifics.header())) {
       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,
                                        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.
     int num_windows = header.window_size();
     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(foreign_session_tag, window_id);
       BuildSyncedSessionFromSpecifics(foreign_session_tag, window_s,
                                       modification_time,
                                       foreign_session->windows[window_id]);
     }
-    // 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) {
+      // Remember that there is more sync data behind this tab_id if we decide
+      // to garbage collection this session. We never do granular garbage

[Nicolas Zea, 2016/04/12 20:38:03]

to garbage collection -> to garbage collect (or to do garbage collection)

[skym, 2016/04/12 22:12:14]

Comment is different now.

+      // 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;
     }
 
     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))
-    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()) {

[Nicolas Zea, 2016/04/12 20:38:03]

nit: rest of file has (I think) been leaving curly braces off for single line if
bodies

[skym, 2016/04/12 22:12:14]

Done.

     sync_processor_->ProcessSyncChanges(FROM_HERE, changes);
+  }
 }
 
 };  // namespace browser_sync