Reland v3 of Session refactor

components/sync_sessions/synced_session_tracker.cc

Index: components/sync_sessions/synced_session_tracker.cc
diff --git a/components/sync_sessions/synced_session_tracker.cc b/components/sync_sessions/synced_session_tracker.cc
index a74b877c085c11e890af5c51567516596c8d01be..e9b9a8188664d106a470c8df2216e6820c928214 100644
--- a/components/sync_sessions/synced_session_tracker.cc
+++ b/components/sync_sessions/synced_session_tracker.cc
@@ -53,6 +53,8 @@ SyncedSessionTracker::~SyncedSessionTracker() {
 
 void SyncedSessionTracker::SetLocalSessionTag(
     const std::string& local_session_tag) {
+  DCHECK(local_session_tag_.empty());
+  DCHECK(!local_session_tag.empty());
   local_session_tag_ = local_session_tag;
 }
 
@@ -90,6 +92,9 @@ bool SyncedSessionTracker::LookupSessionTab(
     const std::string& tag,
     SessionID::id_type tab_id,
     const sessions::SessionTab** tab) const {
+  if (tab_id == TabNodePool::kInvalidTabID)

[skym, 2017/01/24 20:46:19]

This looks unit testable.

[Nicolas Zea, 2017/01/25 23:55:05]

Done.

+    return false;
+
   DCHECK(tab);
   auto tab_map_iter = synced_tab_map_.find(tag);
   if (tab_map_iter == synced_tab_map_.end()) {
@@ -107,8 +112,9 @@ bool SyncedSessionTracker::LookupSessionTab(
   return true;
 }
 
-void SyncedSessionTracker::LookupTabNodeIds(const std::string& session_tag,
-                                            std::set<int>* tab_node_ids) {
+void SyncedSessionTracker::LookupForeignTabNodeIds(
+    const std::string& session_tag,
+    std::set<int>* tab_node_ids) const {
   tab_node_ids->clear();
   auto session_iter = synced_session_map_.find(session_tag);
   if (session_iter != synced_session_map_.end()) {
@@ -144,7 +150,9 @@ SyncedSession* SyncedSessionTracker::GetSession(
   return synced_session_map_[session_tag].get();
 }
 
-bool SyncedSessionTracker::DeleteSession(const std::string& session_tag) {
+bool SyncedSessionTracker::DeleteForeignSession(
+    const std::string& session_tag) {
+  DCHECK_NE(local_session_tag_, session_tag);
   unmapped_windows_.erase(session_tag);
   unmapped_tabs_.erase(session_tag);
 
@@ -188,13 +196,14 @@ void SyncedSessionTracker::ResetSessionTracking(
 
 void SyncedSessionTracker::DeleteForeignTab(const std::string& session_tag,
                                             int tab_node_id) {
+  DCHECK_NE(local_session_tag_, session_tag);
   auto session_iter = synced_session_map_.find(session_tag);
   if (session_iter != synced_session_map_.end()) {
     session_iter->second->tab_node_ids.erase(tab_node_id);
   }
 }
 
-void SyncedSessionTracker::CleanupSession(const std::string& session_tag) {
+void SyncedSessionTracker::CleanupSessionImpl(const std::string& session_tag) {
   for (const auto& window_pair : unmapped_windows_[session_tag])
     synced_window_map_[session_tag].erase(window_pair.first);
   unmapped_windows_[session_tag].clear();
@@ -233,10 +242,9 @@ void SyncedSessionTracker::PutWindowInSession(const std::string& session_tag,
 
 void SyncedSessionTracker::PutTabInWindow(const std::string& session_tag,
                                           SessionID::id_type window_id,
-                                          SessionID::id_type tab_id,
-                                          size_t tab_index) {
+                                          SessionID::id_type tab_id) {
   // We're called here for two reasons. 1) We've received an update to the
-  // SessionWindow information of a SessionHeader node for a foreign session,
+  // SessionWindow information of a SessionHeader node for a session,
   // and 2) The SessionHeader node for our local session changed. In both cases
   // we need to update our tracking state to reflect the change.
   //
@@ -246,7 +254,7 @@ void SyncedSessionTracker::PutTabInWindow(const std::string& session_tag,
   // We know that we will eventually process (via GetTab) every single tab node
   // in the system, so we permit ourselves to use kInvalidTabNodeID here and

[Patrick Noland, 2017/01/25 19:23:41]

Is the comment still applicable now that you're not passing
TabNodePool::kInvalidTabNodeID ?

[Nicolas Zea, 2017/01/25 23:55:04]

Reworded to be clearer.

   // rely on the later update to build the mapping (or a restart).
-  GetTabImpl(session_tag, tab_id, TabNodePool::kInvalidTabNodeID);
+  GetTab(session_tag, tab_id);
 
   // The tab should be unmapped.
   std::unique_ptr<sessions::SessionTab> tab;
@@ -255,60 +263,30 @@ void SyncedSessionTracker::PutTabInWindow(const std::string& session_tag,
     tab = std::move(it->second);
     unmapped_tabs_[session_tag].erase(it);
   }
-  DCHECK(tab);
+  CHECK(tab) << "crbug.com/673618 Attempting to map tab " << tab_id
+             << " multiple times!";
 
   tab->window_id.set_id(window_id);
   DVLOG(1) << "  - tab " << tab_id << " added to window " << window_id;
   DCHECK(GetSession(session_tag)->windows.find(window_id) !=
          GetSession(session_tag)->windows.end());
   auto& window_tabs = GetSession(session_tag)->windows[window_id]->tabs;
-  if (window_tabs.size() <= tab_index) {
-    window_tabs.resize(tab_index + 1);
-  }
-  DCHECK(!window_tabs[tab_index]);
-  window_tabs[tab_index] = std::move(tab);
+  window_tabs.push_back(std::move(tab));
 }
 
-sessions::SessionTab* SyncedSessionTracker::GetTab(
-    const std::string& session_tag,
-    SessionID::id_type tab_id,
-    int tab_node_id) {
-  DCHECK_NE(TabNodePool::kInvalidTabNodeID, tab_node_id);
-  return GetTabImpl(session_tag, tab_id, tab_node_id);
+void SyncedSessionTracker::OnTabNodeSeen(const std::string& session_tag,
+                                         int tab_node_id) {
+  GetSession(session_tag)->tab_node_ids.insert(tab_node_id);
 }
 
-sessions::SessionTab* SyncedSessionTracker::GetTabImpl(
+sessions::SessionTab* SyncedSessionTracker::GetTab(

[Patrick Noland, 2017/01/25 19:23:41]

This function does two things: it creates a SessionTab for the given session_tag
and tab_id if one doesn't exist, and it returns the SessionTab for the tab_id.
Is there a good way to rename this or else  refactor it to functions that only
do one thing? e.g. AllocateSessionTab(), LookupSessionTab(). This would mean you
don't have to do confusing things like ignore the result of it.

[Nicolas Zea, 2017/01/25 23:55:04]

I think that would actually make things more complicated. Knowing which to call,
Allocate or Lookup, is something that only SyncedSessionTracker can do. Also
keep in mind that LookupSessionTab doesn't provide access to a mutable version
of the SessionTab, only a immutable one.

     const std::string& session_tag,
-    SessionID::id_type tab_id,
-    int tab_node_id) {
+    SessionID::id_type tab_id) {
+  CHECK_NE(TabNodePool::kInvalidTabNodeID, tab_id) << "crbug.com/673618";
   sessions::SessionTab* tab_ptr = nullptr;

[Patrick Noland, 2017/01/25 19:23:41]

Can this use LookupSessionTab ?

[Nicolas Zea, 2017/01/25 23:55:05]

That's specifically a const SessionTab*, and this method is returning a
non-const.

   auto iter = synced_tab_map_[session_tag].find(tab_id);
   if (iter != synced_tab_map_[session_tag].end()) {
     tab_ptr = iter->second;
-    if (tab_node_id != TabNodePool::kInvalidTabNodeID &&
-        tab_id != TabNodePool::kInvalidTabID) {
-      // TabIDs are not stable across restarts of a client. Consider this
-      // example with two tabs:
-      //
-      // http://a.com  TabID1 --> NodeIDA
-      // http://b.com  TabID2 --> NodeIDB
-      //
-      // After restart, tab ids are reallocated. e.g, one possibility:
-      // http://a.com TabID2 --> NodeIDA
-      // http://b.com TabID1 --> NodeIDB
-      //
-      // If that happend on a remote client, here we will see an update to
-      // TabID1 with tab_node_id changing from NodeIDA to NodeIDB, and TabID2
-      // with tab_node_id changing from NodeIDB to NodeIDA.
-      //
-      // We can also wind up here if we created this tab as an out-of-order
-      // update to the header node for this session before actually associating
-      // the tab itself, so the tab node id wasn't available at the time and
-      // is currently kInvalidTabNodeID.
-      //
-      // In both cases, we can safely throw it into the set of node ids.
-      GetSession(session_tag)->tab_node_ids.insert(tab_node_id);
-    }
 
     if (VLOG_IS_ON(1)) {
       std::string title;
@@ -328,7 +306,6 @@ sessions::SessionTab* SyncedSessionTracker::GetTabImpl(
     tab->tab_id.set_id(tab_id);
     synced_tab_map_[session_tag][tab_id] = tab_ptr;
     unmapped_tabs_[session_tag][tab_id] = std::move(tab);
-    GetSession(session_tag)->tab_node_ids.insert(tab_node_id);
     DVLOG(1) << "Getting "
              << (session_tag == local_session_tag_ ? "local session"
                                                    : session_tag)
@@ -339,6 +316,98 @@ sessions::SessionTab* SyncedSessionTracker::GetTabImpl(
   return tab_ptr;
 }
 
+void SyncedSessionTracker::CleanupForeignSession(
+    const std::string& session_tag) {
+  DCHECK_NE(local_session_tag_, session_tag);
+  CleanupSessionImpl(session_tag);
+}
+
+void SyncedSessionTracker::CleanupLocalTabs(std::set<int>* deleted_node_ids) {
+  DCHECK(!local_session_tag_.empty());
+  for (const auto& tab_pair : unmapped_tabs_[local_session_tag_])
+    local_tab_pool_.FreeTab(tab_pair.first);
+  CleanupSessionImpl(local_session_tag_);
+  local_tab_pool_.CleanupTabNodes(deleted_node_ids);
+  for (int tab_node_id : *deleted_node_ids) {
+    GetSession(local_session_tag_)->tab_node_ids.erase(tab_node_id);
+  }
+}
+
+bool SyncedSessionTracker::GetTabNodeForLocalTab(int tab_id, int* tab_node_id) {

[Patrick Noland, 2017/01/25 19:23:41]

Given that GetTab refers to a SessionTab* and this refers to a tab_id, it would
be clearer to name this "GetTabNodeForLocalTabId". Or else refer to SessionTabs
by their full name elsewhere

[Nicolas Zea, 2017/01/25 23:55:04]

Done.

+  DCHECK(!local_session_tag_.empty());
+  // Ensure a placeholder SessionTab is in place, if not already.
+  // Although we don't need a SessionTab to fulfill this request, this forces
+  // the
+  // creation of one if it doesn't already exist. This helps to make sure we're
+  // tracking this |tab_id| if |local_tab_pool_| is, and everyone's data
+  // structures
+  // are kept in sync and as consistent as possible.
+  GetTab(local_session_tag_, tab_id);  // Ignore result.
+
+  bool reused_existing_tab =
+      local_tab_pool_.GetTabNodeForTab(tab_id, tab_node_id);
+  DCHECK_NE(TabNodePool::kInvalidTabNodeID, *tab_node_id);

[Patrick Noland, 2017/01/25 19:23:41]

Why DCHECK here and CHECK at the call site?

[Nicolas Zea, 2017/01/25 23:55:04]

The CHECK at the callsite is just temporary while I verify some assumptions.
This will stay around.

+  GetSession(local_session_tag_)->tab_node_ids.insert(*tab_node_id);
+  return reused_existing_tab;
+}
+
+bool SyncedSessionTracker::IsLocalTabNodeValid(int tab_node_id) {
+  if (tab_node_id <= TabNodePool::kInvalidTabNodeID)

[skym, 2017/01/24 20:46:19]

Why is this condition using <= when everything else is checking equality?

[Nicolas Zea, 2017/01/25 23:55:05]

Done.

+    return false;
+  return local_tab_pool_.GetTabIdFromTabNodeId(tab_node_id) !=
+         TabNodePool::kInvalidTabID;
+}
+
+void SyncedSessionTracker::ReassociateLocalTab(int tab_node_id,
+                                               SessionID::id_type new_tab_id) {

[Patrick Noland, 2017/01/25 19:23:41]

I'm confused by the usage of both SessionID::id_type and int as the type for a
tab_id. Could you make it consistent?

[Nicolas Zea, 2017/01/25 23:55:04]

Good point, done (all tab_id's should use SessionID::id_type).

+  DCHECK(!local_session_tag_.empty());
+  DCHECK_NE(TabNodePool::kInvalidTabNodeID, tab_node_id);
+  DCHECK_NE(TabNodePool::kInvalidTabID, new_tab_id);
+
+  SessionID::id_type old_tab_id =
+      local_tab_pool_.GetTabIdFromTabNodeId(tab_node_id);

[Patrick Noland, 2017/01/25 19:23:41]

I'm also confused by some functions using the term "TabNode" and others using
"TabNodeId" although this issue obviously predates this change

[Nicolas Zea, 2017/01/25 23:55:05]

Fair point, although I think that should probably be in a separate CL since it
would touch a fair amount of code.

+  local_tab_pool_.ReassociateTabNode(tab_node_id, new_tab_id);
+
+  sessions::SessionTab* tab_ptr = nullptr;
+
+  auto old_tab_iter = synced_tab_map_[local_session_tag_].find(old_tab_id);
+  if (old_tab_id != TabNodePool::kInvalidTabID &&

[skym, 2017/01/24 20:46:19]

There should never be an entry for kInvalidTabID though, right? Should this just
be a CHECK?

[Nicolas Zea, 2017/01/25 23:55:05]

If the tab_node_id isn't found (because it wasn't previously associated with
anything), GetTabIdFromTabNodeId will return kInvalidTabId. So we should handle
that.

[skym, 2017/01/26 00:05:36]

Are you saying it's possible to have
synced_tab_map_[local_session_tab_][TabNodePool::kInvalidTabID] actually contain
something?

+      old_tab_iter != synced_tab_map_[local_session_tag_].end()) {
+    tab_ptr = old_tab_iter->second;
+    // Remove the tab from the synced tab map under the old id.
+    synced_tab_map_[local_session_tag_].erase(old_tab_iter);
+  } else {
+    // It's possible a placeholder is already in place for the new tab. If so,
+    // reuse it, otherwise create a new one (which will default to unmapped).
+    tab_ptr = GetTab(local_session_tag_, new_tab_id);
+  }
+
+  // If the old tab is unmapped, update the tab id under which it is indexed.
+  auto unmapped_tabs_iter = unmapped_tabs_[local_session_tag_].find(old_tab_id);
+  if (old_tab_id != TabNodePool::kInvalidTabID &&
+      unmapped_tabs_iter != unmapped_tabs_[local_session_tag_].end()) {
+    std::unique_ptr<sessions::SessionTab> tab =
+        std::move(unmapped_tabs_iter->second);
+    DCHECK_EQ(tab_ptr, tab.get());
+    unmapped_tabs_[local_session_tag_].erase(unmapped_tabs_iter);
+    unmapped_tabs_[local_session_tag_][new_tab_id] = std::move(tab);
+  }
+
+  // Update the tab id.
+  if (old_tab_id != TabNodePool::kInvalidTabID) {
+    DVLOG(1) << "Remapped tab " << old_tab_id << " with node " << tab_node_id
+             << " to tab " << new_tab_id;
+  } else {
+    DVLOG(1) << "Mapped new tab node " << tab_node_id << " to tab "
+             << new_tab_id;
+  }
+  tab_ptr->tab_id.set_id(new_tab_id);
+
+  // Add the tab back into the tab map with the new id.
+  synced_tab_map_[local_session_tag_][new_tab_id] = tab_ptr;
+  GetSession(local_session_tag_)->tab_node_ids.insert(tab_node_id);
+}
+
 void SyncedSessionTracker::Clear() {
   // Cleanup unmapped tabs and windows.
   unmapped_windows_.clear();
@@ -352,6 +421,7 @@ void SyncedSessionTracker::Clear() {
   synced_window_map_.clear();
   synced_tab_map_.clear();
 
+  local_tab_pool_.Clear();
   local_session_tag_.clear();
 }