Reland v3 of Session refactor

components/sync_sessions/synced_session_tracker.cc

 // Copyright (c) 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.
 
 #include "components/sync_sessions/synced_session_tracker.h"
 
 #include <utility>
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
@@ skipping 35 matching lines @@
 
 SyncedSessionTracker::SyncedSessionTracker(SyncSessionsClient* sessions_client)
     : sessions_client_(sessions_client) {}
 
 SyncedSessionTracker::~SyncedSessionTracker() {
   Clear();
 }
 
 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;
 }
 
 bool SyncedSessionTracker::LookupAllForeignSessions(
     std::vector<const SyncedSession*>* sessions,
     SessionLookup lookup) const {
   DCHECK(sessions);
   sessions->clear();
   for (const auto& session_pair : synced_session_map_) {
     SyncedSession* foreign_session = session_pair.second.get();
@@ skipping 17 matching lines @@
   for (const auto& window_pair : iter->second->windows)
     windows->push_back(window_pair.second.get());
 
   return true;
 }
 
 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()) {
     // We have no record of this session.
     *tab = nullptr;
     return false;
   }
   auto tab_iter = tab_map_iter->second.find(tab_id);
   if (tab_iter == tab_map_iter->second.end()) {
     // We have no record of this tab.
     *tab = nullptr;
     return false;
   }
   *tab = tab_iter->second;
   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()) {
     tab_node_ids->insert(session_iter->second->tab_node_ids.begin(),
                          session_iter->second->tab_node_ids.end());
   }
   // In case an invalid node id was included, remove it.
   tab_node_ids->erase(TabNodePool::kInvalidTabNodeID);
 }
 
@@ skipping 15 matching lines @@
   std::unique_ptr<SyncedSession> synced_session =
       base::MakeUnique<SyncedSession>();
   DVLOG(1) << "Creating new session with tag " << session_tag << " at "
            << synced_session.get();
   synced_session->session_tag = session_tag;
   synced_session_map_[session_tag] = std::move(synced_session);
 
   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);
 
   bool header_existed = false;
   auto iter = synced_session_map_.find(session_tag);
   if (iter != synced_session_map_.end()) {
     // An implicitly created session that has children tabs but no header node
     // will have never had the device_type changed from unset.
     header_existed = iter->second->device_type != SyncedSession::TYPE_UNSET;
     // SyncedSession's destructor will trigger deletion of windows which will in
@@ skipping 23 matching lines @@
 
     // Then unmap the window itself.
     unmapped_windows_[session_tag][window_pair.first] =
         std::move(window_pair.second);
   }
   session->windows.clear();
 }
 
 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();
 
   for (const auto& tab_pair : unmapped_tabs_[session_tag])
     synced_tab_map_[session_tag].erase(tab_pair.first);
   unmapped_tabs_[session_tag].clear();
 }
 
 void SyncedSessionTracker::PutWindowInSession(const std::string& session_tag,
@@ skipping 18 matching lines @@
                                                             : session_tag);
   }
   DCHECK_EQ(window->window_id.id(), window_id);
   DCHECK(GetSession(session_tag)->windows.end() ==
          GetSession(session_tag)->windows.find(window_id));
   GetSession(session_tag)->windows[window_id] = std::move(window);
 }
 
 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

[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;
   auto it = unmapped_tabs_[session_tag].find(tab_id);
   if (it != unmapped_tabs_[session_tag].end()) {
     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));
+}
+
+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::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) {
-  DCHECK_NE(TabNodePool::kInvalidTabNodeID, tab_node_id);
-  return GetTabImpl(session_tag, tab_id, tab_node_id);
-}
-
-sessions::SessionTab* SyncedSessionTracker::GetTabImpl(
-    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;
       if (tab_ptr->navigations.size() > 0) {
         title =
             " (" + base::UTF16ToUTF8(tab_ptr->navigations.back().title()) + ")";
       }
       DVLOG(1) << "Getting "
                << (session_tag == local_session_tag_ ? "local session"
                                                      : session_tag)
                << "'s seen tab " << tab_id << " at " << tab_ptr << " " << title;
     }
   } else {
     std::unique_ptr<sessions::SessionTab> tab =
         base::MakeUnique<sessions::SessionTab>();
     tab_ptr = tab.get();
     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)
              << "'s new tab " << tab_id << " at " << tab_ptr;
   }
   DCHECK(tab_ptr);
   DCHECK_EQ(tab_ptr->tab_id.id(), tab_id);
   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();
   unmapped_tabs_.clear();
 
   // Delete SyncedSession objects (which also deletes all their windows/tabs).
   synced_session_map_.clear();
 
   // Get rid of our convenience maps (does not delete the actual Window/Tabs
   // themselves; they should have all been deleted above).
   synced_window_map_.clear();
   synced_tab_map_.clear();
 
+  local_tab_pool_.Clear();
   local_session_tag_.clear();
 }
 
 }  // namespace sync_sessions