[Sync] Fix Session's handling of windows.

chrome/browser/sync/glue/synced_session_tracker.cc

Index: chrome/browser/sync/glue/synced_session_tracker.cc
diff --git a/chrome/browser/sync/glue/synced_session_tracker.cc b/chrome/browser/sync/glue/synced_session_tracker.cc
index 5d2c1081a253b270b76fbafeb8eb8ceedf14a194..569c626c4368c9e64ba2fc59db9d2e244f41b102 100644
--- a/chrome/browser/sync/glue/synced_session_tracker.cc
+++ b/chrome/browser/sync/glue/synced_session_tracker.cc
@@ -2,12 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/string_util.h"
 #include "chrome/browser/sync/glue/synced_session_tracker.h"
 #include "chrome/browser/sync/glue/session_model_associator.h"
 
 namespace browser_sync {
 
-
 SyncedSessionTracker::SyncedSessionTracker() {
 }
 
@@ -29,10 +29,19 @@ bool SyncedSessionTracker::LookupAllForeignSessions(
     // Only include foreign sessions with open tabs.
     SyncedSession* foreign_session = i->second;
     if (i->first != local_session_tag_ &&
-        !foreign_session->windows.empty() &&
-        !SessionModelAssociator::SessionWindowHasNoTabsToSync(
-            *foreign_session->windows[0])) {
-      sessions->push_back(foreign_session);
+        !foreign_session->windows.empty()) {
+      bool found_tabs = false;
+      for (SyncedSession::SyncedWindowMap::const_iterator iter =
+               foreign_session->windows.begin();
+           iter != foreign_session->windows.end(); ++iter) {
+        if (!SessionModelAssociator::SessionWindowHasNoTabsToSync(
+                *(iter->second))) {
+          found_tabs = true;
+          break;
+        }
+      }
+      if (found_tabs)
+        sessions->push_back(foreign_session);
     }
   }
 
@@ -46,7 +55,11 @@ bool SyncedSessionTracker::LookupSessionWindows(
   SyncedSessionMap::iterator iter = synced_session_map_.find(session_tag);
   if (iter == synced_session_map_.end())
     return false;
-  *windows = iter->second->windows;
+  SyncedSession::SyncedWindowMap::iterator window_iter =
+      iter->second->windows.begin();
+  windows->clear();
+  while (window_iter != iter->second->windows.end())
+    windows->push_back((window_iter++)->second);
   return true;
 }
 
@@ -65,7 +78,7 @@ bool SyncedSessionTracker::LookupSessionTab(
     *tab = NULL;
     return false;
   }
-  *tab = (*synced_tab_map_[tag])[tab_id];
+  *tab = (*synced_tab_map_[tag])[tab_id].tab_ptr;
   return true;
 }
 
@@ -97,39 +110,183 @@ bool SyncedSessionTracker::DeleteSession(
   }
 }
 
-SessionTab* SyncedSessionTracker::GetSessionTab(
+void SyncedSessionTracker::ResetSessionTracking(
+    const std::string& session_tag) {
+  // Reset window tracking.
+  SyncedWindowMap::iterator window_iter = synced_window_map_.find(session_tag);
+  if (window_iter != synced_window_map_.end()) {
+    IDToSessionWindowMap::iterator window_map_iter;
+    for (window_map_iter = window_iter->second->begin();
+         window_map_iter != window_iter->second->end(); ++window_map_iter) {
+      window_map_iter->second.used = false;
+    }
+  }
+
+  // Reset tab tracking.
+  SyncedTabMap::iterator tab_iter = synced_tab_map_.find(session_tag);
+  if (tab_iter != synced_tab_map_.end()) {
+    IDToSessionTabMap::iterator tab_map_iter;
+    for (tab_map_iter = tab_iter->second->begin();
+         tab_map_iter != tab_iter->second->end(); ++tab_map_iter) {
+      tab_map_iter->second.used = false;
+    }
+  }
+}
+
+bool SyncedSessionTracker::ClearUnusedSessionWindow(
+    SessionWindowWrapper window_wrapper,
+    SyncedSession* session) {
+   // Clear the tabs first, since we don't want the destructor to destroy
+   // them. Their deletion will be handled by ClearUnusedSessionTab below.
+  if (!window_wrapper.used) {
+    if (session) {
+      // If we have a session object, erase the window from that object (does
+      // not free any memory).
+      session->windows.erase(window_wrapper.window_ptr->window_id.id());
+    }
+    VLOG(1) << "Deleting closed window "
+            << window_wrapper.window_ptr->window_id.id()
+            << (session ? " from both" : "from memory");
+    window_wrapper.window_ptr->tabs.clear();
+    delete window_wrapper.window_ptr;

[akalin, 2011/09/27 19:44:24]

the more I look over this CL and the more I think about it, the more I think
that this manual memory management should be replaced with linked_ptrs.

The one problem I see is that a SessionWindow owns its contained tabs, which is
why you have to clear it before you release it above.  Does that mean we have to
track tab ownership from this class?  Since we own any tabs we create, but then
they may be taken over by windows?

Also, we also have SyncedSessions, which owns its contained windows -- how does
that interact with this stuff, too?

Maybe I'm just unfamiliar with this code but I'm confused by the ownership. :(

[Nicolas Zea, 2011/09/27 20:12:13]

Yes, we do we have to track the tab ownership. The thing that makes this
complicated is that tab's are created in one of two cases. First, if the tab
node arrives before the meta node (that contains the windows). We then create an
unmapped tab node. In the second case, where we get a request for a tab we
haven't received yet while processing the window information in a meta node, we
create an empty SessionTab object, which will get filled when the tab node does
arrive.

In other words, tab ownership is indeed closely tracked, and can be owned by one
of two objects (when we're not within a tracking phase). When the tab is in a
temporary state because we haven't received the meta node with the window's that
hold them, they are owned by unmapped_tabs. While the tab has a window, it's
owned by the window itself (similarly the window is owned by the SyncedSession).

Now, that said, during the tracking phase we're in the middle of moving
ownership around (since we need to support tabs being moved between windows). As
a result, we always keep pointers to all the tabs and windows through the
synced_window_map_ and synced_tab_map_. Those are just containers and don't own
the tabs/windows. They're just for keeping track of if something is in use and
aid in quick lookups.

+    return true;
+  }
+  return false;
+}
+
+bool SyncedSessionTracker::ClearUnusedSessionTab(
+    SessionTabWrapper tab_wrapper) {
+  if (!tab_wrapper.used) {
+    if (VLOG_IS_ON(1)) {
+      SessionTab* tab_ptr = tab_wrapper.tab_ptr;
+      std::string title = "";
+      if (tab_ptr->navigations.size() > 0) {
+        title = " (" + UTF16ToASCII(
+            tab_ptr->navigations[tab_ptr->navigations.size()-1].title()) + ")";
+      }
+      VLOG(1) << "Deleting closed tab " << tab_ptr->tab_id.id() << title
+              << " from window " << tab_ptr->window_id.id();
+    }
+    unmapped_tabs_.erase(tab_wrapper.tab_ptr);
+    delete tab_wrapper.tab_ptr;
+    return true;
+  }
+  return false;
+}
+
+void SyncedSessionTracker::CleanupSession(const std::string& session_tag) {
+  SyncedSession* synced_session = NULL;

[akalin, 2011/09/27 19:44:24]

To elaborate on my previous comment, I was thinking something like:

void ...CleanupSession() {
  SyncedWindowMap::iterator ...;
  if (window_iter != end()) {
    SyncedSession* synced_session = NULL;
    {
      it = synced_session_map_.find(session_tag);
      if (it != ..end()) {
        synced_session = *it;
      }
    }
    for (IDToSessionWindowMap::iterator iter = ...) {
      ...
    }
  }

as I don't see synced_session used in the 2nd if statement below

[Nicolas Zea, 2011/09/27 20:12:13]

You're right, it's not used in the if statement for the SessionTabs. But I
wanted to delete the SessionWindows even if there is no SyncedSession (if there
is we additionally delete it from the session itself though).  That said, I was
thinking I should have that functionality just for robustness, but I suppose it
doesn't make sense to be doing session tracking if you haven't created the
session yet. I've changed to code to reflect this.

+  if (synced_session_map_.find(session_tag) !=
+      synced_session_map_.end()) {
+    synced_session = synced_session_map_[session_tag];
+  }
+
+  // Go through and delete any windows or tabs with a false value in their
+  // tracking boolean.
+  SyncedWindowMap::iterator window_iter = synced_window_map_.find(session_tag);
+  if (window_iter != synced_window_map_.end()) {
+    // Go through and remove those windows from the session first.
+    IDToSessionWindowMap::iterator iter;
+    for (iter = window_iter->second->begin();
+         iter != window_iter->second->end();) {
+      SessionWindowWrapper window_wrapper = iter->second;
+      if (ClearUnusedSessionWindow(window_wrapper, synced_session))
+        window_iter->second->erase(iter++);
+      else
+        ++iter;
+    }
+  }
+
+  SyncedTabMap::iterator tab_iter = synced_tab_map_.find(session_tag);
+  if (tab_iter != synced_tab_map_.end()) {
+    IDToSessionTabMap::iterator iter;
+    for (iter = tab_iter->second->begin();
+         iter != tab_iter->second->end();) {
+      SessionTabWrapper tab_wrapper = iter->second;
+      if (ClearUnusedSessionTab(tab_wrapper))
+        tab_iter->second->erase(iter++);
+      else
+        ++iter;
+    }
+  }
+}
+
+SessionWindow* SyncedSessionTracker::GetWindow(const std::string& session_tag,
+                                               SessionID::id_type window_id) {
+  SessionWindow* window_ptr = NULL;
+  if (synced_window_map_.find(session_tag) == synced_window_map_.end())
+    synced_window_map_[session_tag] = new IDToSessionWindowMap;
+  IDToSessionWindowMap::iterator iter =
+      synced_window_map_[session_tag]->find(window_id);
+  if (iter != synced_window_map_[session_tag]->end()) {
+    iter->second.used = true;
+    window_ptr = iter->second.window_ptr;
+    VLOG(1) << "Getting "
+            << (session_tag == local_session_tag_ ?
+                "local session" : session_tag)
+            << "'s seen window " << window_id  << " at " << window_ptr;
+  } else {
+    // Create the window.
+    window_ptr = new SessionWindow();
+    window_ptr->window_id.set_id(window_id);
+    (*synced_window_map_[session_tag])[window_id] =
+        SessionWindowWrapper(window_ptr, true);
+    VLOG(1) << "Getting "
+            << (session_tag == local_session_tag_ ?
+                "local session" : session_tag)
+            << "'s new window " << window_id  << " at " << window_ptr;
+  }
+  DCHECK(window_ptr);
+  DCHECK_EQ(window_ptr->window_id.id(), window_id);
+  return window_ptr;
+}
+
+SessionTab* SyncedSessionTracker::GetTabForWindow(
     const std::string& session_tag,
-    SessionID::id_type tab_id,
-    bool has_window) {
+    SessionID::id_type window_id,
+    SessionID::id_type tab_id) {
+  SessionTab* tab_ptr = GetTab(session_tag, tab_id);
+  unmapped_tabs_.erase(tab_ptr);
+  (*synced_tab_map_[session_tag])[tab_id].used = true;
+  tab_ptr->window_id.set_id(window_id);
+  VLOG(1) << "  - tab " << tab_id << " noted as part of window "<< window_id;
+  return tab_ptr;
+}
+
+SessionTab* SyncedSessionTracker::GetTab(
+    const std::string& session_tag,
+    SessionID::id_type tab_id) {
   if (synced_tab_map_.find(session_tag) == synced_tab_map_.end())
     synced_tab_map_[session_tag] = new IDToSessionTabMap;
-  scoped_ptr<SessionTab> tab;
+  SessionTab* tab_ptr = NULL;
   IDToSessionTabMap::iterator iter =
       synced_tab_map_[session_tag]->find(tab_id);
   if (iter != synced_tab_map_[session_tag]->end()) {
-    tab.reset(iter->second);
-    if (has_window)  // This tab is linked to a window, so it's not an orphan.
-      unmapped_tabs_.erase(tab.get());
-    if (tab->navigations.size() > 0) {
+    tab_ptr = iter->second.tab_ptr;
+    if (VLOG_IS_ON(1)) {
+      std::string title = "";
+      if (tab_ptr->navigations.size() > 0) {
+        title = " (" + UTF16ToASCII(
+            tab_ptr->navigations[tab_ptr->navigations.size()-1].title()) + ")";
+      }
       VLOG(1) << "Getting "
               << (session_tag == local_session_tag_ ?
                   "local session" : session_tag)
-              << "'s seen tab " << tab_id  << " ("
-              << tab->navigations[tab->navigations.size()-1].title()
-              << ")";
+              << "'s seen tab " << tab_id  << " at " << tab_ptr << title;
     }
   } else {
-    tab.reset(new SessionTab());
-    (*synced_tab_map_[session_tag])[tab_id] = tab.get();
-    if (!has_window)  // This tab is not linked to a window, it's an orphan.
-    unmapped_tabs_.insert(tab.get());
+    tab_ptr = new SessionTab();
+    tab_ptr->tab_id.set_id(tab_id);
+    (*synced_tab_map_[session_tag])[tab_id] = SessionTabWrapper(tab_ptr, false);
+    unmapped_tabs_.insert(tab_ptr);
     VLOG(1) << "Getting "
             << (session_tag == local_session_tag_ ?
                 "local session" : session_tag)
-            << "'s new tab " << tab_id  << " at " << tab.get();
+            << "'s new tab " << tab_id  << " at " << tab_ptr;
   }
-  DCHECK(tab.get());
-  return tab.release();
+  DCHECK(tab_ptr);
+  DCHECK_EQ(tab_ptr->tab_id.id(), tab_id);
+  return tab_ptr;
 }
 
 void SyncedSessionTracker::clear() {
@@ -138,7 +295,13 @@ void SyncedSessionTracker::clear() {
       synced_session_map_.end());
   synced_session_map_.clear();
 
-  // Delete IDToSessTab maps. Does not delete the SessionTab objects, because
+  // Delete IDToSessionWindow maps. Does not delete the SessionWindow objects,
+  // because they should already be referenced through synced_session_map_.

[akalin, 2011/09/27 19:44:24]

Confused by this comment -- we delete the synced sessions above, which should
delete the windows, right?  But we're deleting more windows here, too?

[Nicolas Zea, 2011/09/27 20:12:13]

This just deletes the maps, not the windows themselves (synced_window_map_ has a
session tag for a key and a map for a value).

+  STLDeleteContainerPairSecondPointers(synced_window_map_.begin(),
+      synced_window_map_.end());
+  synced_window_map_.clear();
+
+  // Delete IDToSessionTab maps. Does not delete the SessionTab objects, because
   // they should already be referenced through synced_session_map_.
   STLDeleteContainerPairSecondPointers(synced_tab_map_.begin(),
       synced_tab_map_.end());