Reland of [Sync] Put session tracker in charge of maintaining local state.

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 34 matching lines @@
   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 21 matching lines @@
   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) {
   // 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,

[skym, 2016/12/03 01:20:46]

This #1 case now included local data, right?

[Nicolas Zea, 2016/12/06 01:32:55]

Correct. Updated.

   // 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);
+  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);
 
   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);
 }
 
+void SyncedSessionTracker::AddTabNode(const std::string& session_tag,
+                                      int tab_node_id) {
+  GetSession(session_tag)->tab_node_ids.insert(tab_node_id);
+}
+
 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);
-}
-
-sessions::SessionTab* SyncedSessionTracker::GetTabImpl(
-    const std::string& session_tag,
-    SessionID::id_type tab_id,
-    int tab_node_id) {
+    SessionID::id_type tab_id) {
   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

[skym, 2016/12/05 19:16:57]

This comment block, at least the part about tab ids not being consistent across
restarts, was super helpful to me to understand some of the requirements for the
logic around sessions. Would be nice to have this live on somewhere prominent.

Similarly, would be nice for the concept of placeholders and not trusting the
locally loaded data unless sometimes would be good. Maybe all of these tricky
scenarios could be documented in sessions_sync_manager.h

[Nicolas Zea, 2016/12/06 01:32:55]

Good call. Moved to the SessionSyncManager's UpdateTrackerWithSpecifics method,
which is really where this issue matters.

-      // 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) {
+  DCHECK(!local_session_tag_.empty());
+  // Ensure a placeholder SessionTab is in place, if not already.

[skym, 2016/12/03 01:20:46]

This sounds like GetTab(...) is going to create a SyncedTabDelegate object that
returns true when IsPlaceholderTab() is called. That's not what going on though,
right?

[Nicolas Zea, 2016/12/06 01:32:55]

I'm using placeholder like the colloquial term, unrelated to the
SyncedTabDelegate (which the tracker knows nothing about). This is just to have
consistency by ensuring there's a SessionTab object for each tab node (which
helps with testing).

[skym, 2016/12/06 19:37:21]

Ahh, okay, so you're saying that adding one here if it doesn't exist helps with
tests? It looks like the only time GetTabNodeForLocalTab is called in non-test
code is right before the SyncChange is built, which means there had better be a
real SyncTab here. And very not placeholder-y.

I also worry that using the colloquial definition of placeholder might be
confusing.

I'd prefer to see a comment that talks about this from a consistency
perspective.

// 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.

[Nicolas Zea, 2016/12/06 22:31:37]

Well, it's called right before we fill the SessionTab from the SyncedTabDelegate
(in AssociateTab). As such, at the time we call it, it very well could be a
placeholder (if it's the first time we're filling the tab).

That said, agreed that a comment would be good here. Added.

+  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;
+}
+
+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);

[skym, 2016/12/05 19:16:57]

Three case here, right? It returns kInvalidTabID, it returns the same value as
new_tab_id, or it returns a different value?

[Nicolas Zea, 2016/12/06 01:32:55]

Correct. 

+  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_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

[skym, 2016/12/05 19:16:57]

Placeholder? Like as in SyncedTabDelegate::IsPlaceholderTab()? It seems like you
could have a different tab node id sitting here previously using the
|new_tab_id|, and you're stealing their SessionTab. They can always create a new
one I suppose, but the one you stole now contains someone else's data, which is
wrong for us, right?

[Nicolas Zea, 2016/12/06 01:32:55]

Placeholder like a dummy SessionTab in place to show that the tab exists (which
get's created anytime GetTab is called). It's unrelated to the SyncedTabDelegate
logic.

I'm not sure thinking about it as "someone else's data" is correct. It's still
this device's data for that tab id. It's just that it's possibly stale.  Also
note that that old tab node is automatically freed as part of
ReassociateTabNode, so things remain consistent.

[skym, 2016/12/06 19:37:21]

Similar to above, I'm concerned that using placeholder is confusing (it was to
me). I'd prefer this comment to explain when/why a SyncTab would already exist.

I tried to write down my understanding of what's going on here, but I realized I
don't understand when this method should be called. It seems like
ReassociateLocalTab isn't being called for non-placeholder tabs. Why not? Why do
placeholders need this while non-place holders don't? Shouldn't they both get
set up with initial data from sync, and then at some point in the future (the
length of gap is the only difference) we get data from delegates and need to
worry about correcting tab ids?

[Nicolas Zea, 2016/12/06 22:31:37]

Chatted offline. I've added a comment to SessionsSyncManager to explain when and
where android placeholders are used, and when we reassociate them using their
sync id.

+    // that's the case, reuse the placeholder SessionTab. Otherwise, reuse the
+    // old SessionTab if it's available.
+    auto new_tab_iter = synced_tab_map_[local_session_tag_].find(new_tab_id);
+    if (new_tab_iter != synced_tab_map_[local_session_tag_].end()) {
+      tab_ptr = new_tab_iter->second;
+    } else {
+      // Otherwise there was no old tab in place. Create a new one in an
+      // unmapped state.
+      tab_ptr = GetTab(local_session_tag_, new_tab_id);

[skym, 2016/12/05 19:16:57]

...So there are three cases that GetTab could run into, right? new_tab_id
already exists and is mapped, it already exists and is unmapped, and it doesn't
exist and is created here and thrown into the unmapped_tabs_ w/ the new id?

Except you special case the first (already in synced_tab_map_) in the above
logic, and the outcome is the same?

[Nicolas Zea, 2016/12/06 01:32:55]

Ah, you're right, this conditional is pointless. GetTab is already looking for
the new_tab_id, and will reuse the tab if it exists. Fixed!

+    }
+  }
+
+  // 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 &&

[skym, 2016/12/05 19:16:57]

At this point, I'm wondering, would it have been easier to just erase everything
and then re-add everything?

[Nicolas Zea, 2016/12/06 01:32:55]

I'm not sure I follow. Erase all tabs for this session, just to reassociate one?

[skym, 2016/12/06 19:37:21]

I meant remove everything at tab_node_id, new_tab_id, and old_tab_id. Then
create a new SessionTab every time. But looking at this again today, I'm liking
your approach more. So never mind!

[Nicolas Zea, 2016/12/06 22:31:37]

Acknowledged.

+      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