Reland v5 of Sessions 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..16ce234109ab77c9648db74ed7142c69bec07608 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)
+    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();
@@ -204,6 +213,11 @@ void SyncedSessionTracker::CleanupSession(const std::string& session_tag) {
   unmapped_tabs_[session_tag].clear();
 }
 
+bool SyncedSessionTracker::IsTabUnmappedForTesting(SessionID::id_type tab_id) {
+  auto it = unmapped_tabs_[local_session_tag_].find(tab_id);
+  return it != unmapped_tabs_[local_session_tag_].end();
+}
+
 void SyncedSessionTracker::PutWindowInSession(const std::string& session_tag,
                                               SessionID::id_type window_id) {
   std::unique_ptr<sessions::SessionWindow> window;
@@ -233,20 +247,19 @@ 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.
   //
   // Because the SessionHeader nodes are separate from the individual tab nodes
   // and we don't store tab_node_ids in the header / SessionWindow specifics,
-  // the tab_node_ids are not always available when processing headers.
-  // 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
-  // rely on the later update to build the mapping (or a restart).
-  GetTabImpl(session_tag, tab_id, TabNodePool::kInvalidTabNodeID);
+  // the tab_node_ids are not always available when processing headers. We know
+  // that we will eventually process (via GetTab) every single tab node in the
+  // system, so we permit ourselves to just call GetTab and ignore the result,
+  // creating a placeholder SessionTab in the process.
+  sessions::SessionTab* tab_ptr = GetTab(session_tag, tab_id);
 
   // The tab should be unmapped.
   std::unique_ptr<sessions::SessionTab> tab;
@@ -254,61 +267,53 @@ void SyncedSessionTracker::PutTabInWindow(const std::string& session_tag,
   if (it != unmapped_tabs_[session_tag].end()) {
     tab = std::move(it->second);
     unmapped_tabs_[session_tag].erase(it);
+  } else {
+    // The tab has already been mapped, possibly because of the sync id being

[skym, 2017/02/27 18:22:38]

"sync id" being the "tab node id"? As nice as it would have been to our two id's
names not contain node, we should probably stick to consistent names. Maybe a
follow up rename CL could make this happen everywhere.

[Nicolas Zea, 2017/02/28 00:09:01]

Yeah, good point. Switched to tab node id.

+    // reused across tabs. Find the existing tab and move it to the right
+    // window.
+    SessionID::id_type old_window_id = -1;
+    for (auto& window_iter_pair : GetSession(session_tag)->windows) {
+      auto tab_iter = std::find_if(
+          window_iter_pair.second->tabs.begin(),
+          window_iter_pair.second->tabs.end(),
+          [&tab_ptr](const std::unique_ptr<sessions::SessionTab>& tab) {
+            return tab.get() == tab_ptr;

[skym, 2017/02/27 18:22:38]

I don't understand why it's safe here to assume that we have the more
significant piece of information right now, and we should replace/overwrite how
it was previously mapped as.

[Nicolas Zea, 2017/02/28 00:09:01]

From my testing, the situations in which this can happen are transient, and in
part due to how the Chrome for Android code handles moving a tab between windows
(which appears to involve a period of time when the tab is duplicated). As such,
the goal is to handle this scenario gracefully, with the assumption that
eventually things will become consistent (with only one open tab being
associated with that sync id).

+          });
+      if (tab_iter != window_iter_pair.second->tabs.end()) {
+        old_window_id = window_iter_pair.first;
+        tab = std::move(*tab_iter);
+        window_iter_pair.second->tabs.erase(tab_iter);
+        break;
+      }
+    }
+    CHECK(tab) << " Unable to find tab " << tab_id

[skym, 2017/02/27 18:22:38]

CHECK should be temporary in this scenario, right? Either switch to DCHECK or
add crbug.com/????? if you want more data for investigating failures.

[Nicolas Zea, 2017/02/28 00:09:01]

Yes, this should be temporary. Added the crbug.

+               << " within unmapped tabs or previously mapped windows.";
+
+    DVLOG(1) << "Moving tab " << tab_id << " from window " << old_window_id

[skym, 2017/02/27 18:22:38]

Why do this after the break? Why not immediately before the break? Would remove
the need for the temp variable.

[Nicolas Zea, 2017/02/28 00:09:01]

Good point, done.

+             << " to " << window_id;
   }
-  DCHECK(tab);
 
   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(
     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;
   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 +333,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 +343,99 @@ 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::GetTabNodeFromLocalTabId(SessionID::id_type tab_id,
+                                                    int* tab_node_id) {
+  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);
+  GetSession(local_session_tag_)->tab_node_ids.insert(*tab_node_id);
+  return reused_existing_tab;
+}
+
+bool SyncedSessionTracker::IsLocalTabNodeAssociated(int tab_node_id) {
+  if (tab_node_id == TabNodePool::kInvalidTabNodeID)
+    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) {
+  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);
+  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 &&
+      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 +449,7 @@ void SyncedSessionTracker::Clear() {
   synced_window_map_.clear();
   synced_tab_map_.clear();
 
+  local_tab_pool_.Clear();
   local_session_tag_.clear();
 }