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

components/sync_sessions/synced_session_tracker.h

 // Copyright 2012 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.
 
 #ifndef COMPONENTS_SYNC_SESSIONS_SYNCED_SESSION_TRACKER_H_
 #define COMPONENTS_SYNC_SESSIONS_SYNCED_SESSION_TRACKER_H_
 
 #include <stddef.h>
 
 #include <map>
@@ skipping 22 matching lines @@
  public:
   explicit SyncedSessionTracker(
       sync_sessions::SyncSessionsClient* sessions_client);
   ~SyncedSessionTracker();
 
   // We track and distinguish the local session from foreign sessions.
   void SetLocalSessionTag(const std::string& local_session_tag);
 
   // 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
   // SyncedSessionTracker.
   // If lookup succeeds:
   // - Fills windows with the SessionWindow pointers, returns true.
   // Else
   // - Returns false.
   bool LookupSessionWindows(
       const std::string& session_tag,
@@ skipping 25 matching lines @@
   // Resets the tracking information for the session specified by |session_tag|.
   // This involves clearing all the windows and tabs from the session, while
   // keeping pointers saved in the synced_window_map_ and synced_tab_map_.
   // Once reset, all calls to PutWindowInSession and PutTabInWindow will denote
   // that the requested windows and tabs are owned (by setting the boolean
   // in their SessionWindowWrapper/SessionTabWrapper to true) and add them back
   // to their session. The next call to CleanupSession(...) will delete those
   // 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);
 
   // Adds the window with id |window_id| to the session specified by
   // |session_tag|, and markes the window as being owned. If none existed for
   // that session, creates one. Similarly, if the session did not exist yet,
   // creates it. Ownership of the SessionWindow remains within the
   // SyncedSessionTracker.
   void PutWindowInSession(const std::string& session_tag,
                           SessionID::id_type window_id);
@@ skipping 70 matching lines @@
   // have ownership of the Windows/Tabs, they just provide id-based access to
   // them. The ownership remains within its containing session (for windows and
   // mapped tabs, unmapped tabs are owned by the unmapped_tabs_ container).
   // Note, we pair pointers with bools so that we can track what is owned and
   // what can be deleted (see ResetSessionTracking(..) and CleanupSession(..)
   // above).
   // The wrappers also serve as a convenient place to augment state stored in
   // 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
     // collection for closed tabs that are no longer "in use", or "owned".
     // ResetSessionTracking will clear |owned| bits, and if it is not claimed
     // by a window by the time CleanupSession is called it will be deleted.
     bool owned;
 
     // 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;
 
   struct SessionWindowWrapper {
     SessionWindowWrapper() : window_ptr(NULL), owned(false) {}
     SessionWindowWrapper(sessions::SessionWindow* window_ptr, OwnedState owned)
         : window_ptr(window_ptr), owned(owned == IS_OWNED) {}
     sessions::SessionWindow* window_ptr;
     bool owned;
   };
   typedef std::map<SessionID::id_type, SessionWindowWrapper>
       IDToSessionWindowMap;
   typedef std::map<std::string, IDToSessionWindowMap> SyncedWindowMap;
 
   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,
                                    SessionID::id_type tab_id,
                                    int tab_node_id);
 
   // The client of the sync sessions datatype.
   sync_sessions::SyncSessionsClient* const sessions_client_;
 
   // Per client mapping of tab id's to their SessionTab objects.
@@ skipping 20 matching lines @@
   // The tag for this machine's local session, so we can distinguish the foreign
   // sessions.
   std::string local_session_tag_;
 
   DISALLOW_COPY_AND_ASSIGN(SyncedSessionTracker);
 };
 
 }  // namespace browser_sync
 
 #endif  // COMPONENTS_SYNC_SESSIONS_SYNCED_SESSION_TRACKER_H_