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

components/sync_sessions/synced_session_tracker.h

Index: components/sync_sessions/synced_session_tracker.h
diff --git a/components/sync_sessions/synced_session_tracker.h b/components/sync_sessions/synced_session_tracker.h
index a45a80ee8493bd47074b08ccdd6100f27c30541a..5dc0a60c47c8c2c67fc1a3b0f957c71a3520c00b 100644
--- a/components/sync_sessions/synced_session_tracker.h
+++ b/components/sync_sessions/synced_session_tracker.h
@@ -40,10 +40,13 @@ class SyncedSessionTracker {
 
   // Fill a preallocated vector with all foreign sessions we're tracking (skips
   // the local session object). SyncedSession ownership remains within the
-  // SyncedSessionTracker.
+  // SyncedSessionTracker. Can specify if you want only presentable sessions,
+  // which means they have at least one window with at least one tab that has
+  // syncable content.
   // Returns true if we had foreign sessions to fill it with, false otherwise.
   bool LookupAllForeignSessions(
-      std::vector<const sync_driver::SyncedSession*>* sessions) const;
+      std::vector<const sync_driver::SyncedSession*>* sessions,
+      bool presentable) const;

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

readability nit: prefer defining an enum and using that here rather than passing
a bool.
enum SessionLookup {
  RAW,  /// ....
  PRESENTABLE,  // Have one window with at least one tab with syncable content,
}
  

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

Done.

 
   // Attempts to look up the session windows associatd with the session given
   // by |session_tag|. Ownership Of SessionWindows stays within the
@@ -89,8 +92,21 @@ class SyncedSessionTracker {
   // windows and tabs not owned.
   void ResetSessionTracking(const std::string& session_tag);
 
+  // Deletes the given tab, primarily by |tab_node_id|. |tab_id| is needed to
+  // find the given tab, but isn't the primary identifier in the case, since

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

"in the case" -> "in this case"

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

Comment is different now.

+  // there may be multiple tabs shadowing eachother for the given |tab_id|. In

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

eachother -> each other

Also, it's not clear ot me what you mean when you say multiple tabs may be
shadowing each other for the given |tab_id|?

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

Comment is different now.

I've mostly stopped using the term shadowing, but we still have the same thing
happening. tab_ids are not stable across restarts, and it's possible we may have
multiple sync tab objects, each with a different tab_node_id value, that all
claim to have the same tab_id. SessionsSyncManager uses the data from the one
with the most recent modification time. And so all of losers here, the tabs with
the older modification times, are "shadowing" the winner.

The reason that we care, is that if we decide to garbage collect/delete this
foreign session, we want to issue deletes for all of the tab node ids, even ones
that the SessionsSyncManager ignored before they had old modification times and
the same tab id as other tabs.

+  // several scenarios we cannot actually delete the tab object, and calling
+  // this method will simply remove the given |tab_node_id|. If the tab is

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

will remove |tab_node_id| from where?

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

In this patch set it was from the SessionTabWrapper but now I've moved it to the
SyncedSession. Updated the current comment to make this clear.

+  // completely unowned and shadowing no other tab_node_ids, then this will
+  // result in an actual deletion.
+  void TryDeleteTab(const std::string& session_tag,
+                    SessionID::id_type tab_id,
+                    int tab_node_id);
+
   // Deletes those windows and tabs associated with |session_tag| that are no
-  // longer owned.
+  // longer owned. Should typically only be used for local data, since the pool
+  // will still be tracking identifiers. If this is ever run on a foreign
+  // session we lose the ability to correctly garbage collect.
   // See ResetSessionTracking(...).
   void CleanupSession(const std::string& session_tag);
 
@@ -181,16 +197,14 @@ class SyncedSessionTracker {
   // SessionTab for sync purposes, such as |tab_node_id|.
   // IsOwned is used as a wrapper constructor parameter for readability.
   struct SessionTabWrapper {
-    SessionTabWrapper()
-        : tab_ptr(NULL),
-          owned(false),
-          tab_node_id(TabNodePool::kInvalidTabNodeID) {}
+    SessionTabWrapper();
+
     SessionTabWrapper(sessions::SessionTab* tab_ptr,
                       OwnedState owned,
-                      int tab_node_id)
-        : tab_ptr(tab_ptr),
-          owned(owned == IS_OWNED),
-          tab_node_id(tab_node_id) {}
+                      int tab_node_id);
+
+    ~SessionTabWrapper();
+
     sessions::SessionTab* tab_ptr;
 
     // This is used as part of a mark-and-sweep approach to garbage
@@ -201,8 +215,13 @@ class SyncedSessionTracker {
 
     // This lets us identify the sync node that is "backing" this tab in the
     // sync model, whether it is part of a local or foreign session. The
-    // "tab node id" is described in session_specifics.proto.
-    int tab_node_id;
+    // "tab node id" is described in session_specifics.proto. Sometimes there
+    // may be multiple pieces of sync data that claim to have the same tab_id.
+    // In this case we remember each tab_node_id in this set in case we need
+    // to modify them. It's possible for multiple different tab_ids to use the
+    // same tab_node_id when the remote client is restarted during the local
+    // clients lifetime.

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

I find this paragraph pretty tough to parse. Could you provide a more concrete
example?

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

Moved this to SyncedSession. Added a comment pointing towards
SyncedSessionTracker::GetTabImpl for concrete example

+    std::set<int> tab_node_ids;
   };
   typedef std::map<SessionID::id_type, SessionTabWrapper> IDToSessionTabMap;
   typedef std::map<std::string, IDToSessionTabMap> SyncedTabMap;
@@ -221,8 +240,9 @@ class SyncedSessionTracker {
   typedef std::map<std::string, sync_driver::SyncedSession*> SyncedSessionMap;
 
   // Helper methods for deleting SessionWindows and SessionTabs without owners.
-  bool DeleteOldSessionWindowIfNecessary(SessionWindowWrapper window_wrapper);
-  bool DeleteOldSessionTabIfNecessary(SessionTabWrapper tab_wrapper);
+  bool DeleteOldSessionWindowIfNecessary(
+      const SessionWindowWrapper& window_wrapper);
+  bool DeleteOldSessionTabIfNecessary(const SessionTabWrapper& tab_wrapper);
 
   // Implementation for GetTab(...) above, permits invalid tab_node_id.
   sessions::SessionTab* GetTabImpl(const std::string& session_tag,