[Sync] Fix Session's handling of windows.

chrome/browser/sync/glue/session_model_associator.cc

Index: chrome/browser/sync/glue/session_model_associator.cc
diff --git a/chrome/browser/sync/glue/session_model_associator.cc b/chrome/browser/sync/glue/session_model_associator.cc
index 6b2ca7d99511da53f61a6cebbfa115fa8172a203..a60300babc5820c2e026c09dd4f89b572ce29560 100644
--- a/chrome/browser/sync/glue/session_model_associator.cc
+++ b/chrome/browser/sync/glue/session_model_associator.cc
@@ -13,6 +13,7 @@
 #include "base/sys_info.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sessions/session_id.h"
 #include "chrome/browser/sessions/session_service_factory.h"
 #include "chrome/browser/sync/api/sync_error.h"
 #include "chrome/browser/sync/glue/synced_session.h"
@@ -144,10 +145,9 @@ void SessionModelAssociator::ReassociateWindows(bool reload_tabs) {
   header_s->set_device_type(sync_pb::SessionHeader_DeviceType_TYPE_OTHER);
 #endif
 
-  size_t window_num = 0;
+  synced_session_tracker_.ResetSessionTracking(local_tag);
   std::set<SyncedWindowDelegate*> windows =
       SyncedWindowDelegate::GetSyncedWindowDelegates();
-  current_session->windows.reserve(windows.size());
   for (std::set<SyncedWindowDelegate*>::const_iterator i =
        windows.begin(); i != windows.end(); ++i) {
     // Make sure the window has tabs and a viewable window. The viewable window
@@ -156,8 +156,7 @@ void SessionModelAssociator::ReassociateWindows(bool reload_tabs) {
     // for us to get a handle to a browser that is about to be removed. If
     // the tab count is 0 or the window is NULL, the browser is about to be
     // deleted, so we ignore it.
-    if (ShouldSyncWindow(*i) && (*i)->GetTabCount() &&
-        (*i)->HasWindow()) {
+    if (ShouldSyncWindow(*i) && (*i)->GetTabCount() && (*i)->HasWindow()) {
       sync_pb::SessionWindow window_s;
       SessionID::id_type window_id = (*i)->GetSessionId();
       VLOG(1) << "Reassociating window " << window_id << " with " <<
@@ -191,19 +190,20 @@ void SessionModelAssociator::ReassociateWindows(bool reload_tabs) {
         *header_window = window_s;
 
         // Update this window's representation in the synced session tracker.
-        if (window_num >= current_session->windows.size()) {
-          // This a new window, create it.
-          current_session->windows.push_back(new SessionWindow());
-        }
+        SessionWindow* window_ptr = synced_session_tracker_.GetWindow(
+            local_tag, window_id);
+        current_session->windows[window_id] = window_ptr;
         PopulateSessionWindowFromSpecifics(
             local_tag,
             window_s,
             base::Time::Now(),
-            current_session->windows[window_num++],
+            window_ptr,
             &synced_session_tracker_);
       }
     }
   }
+  // Free memory for closed windows and tabs.
+  synced_session_tracker_.CleanupSession(local_tag);
 
   sync_api::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare());
   sync_api::WriteNode header_node(&trans);
@@ -334,9 +334,8 @@ bool SessionModelAssociator::WriteTabContentsToSyncModel(
 
   // Convert to a local representation and store in synced session tracker.
   SessionTab* session_tab =
-      synced_session_tracker_.GetSessionTab(GetCurrentMachineTag(),
-                                            tab_s->tab_id(),
-                                            false);
+      synced_session_tracker_.GetTab(GetCurrentMachineTag(),
+                                     tab_s->tab_id());
   PopulateSessionTabFromSpecifics(*tab_s,
                                   base::Time::Now(),
                                   session_tab);
@@ -634,37 +633,39 @@ bool SessionModelAssociator::AssociateForeignSpecifics(
     // Load (or create) the SyncedSession object for this client.
     SyncedSession* foreign_session =
         synced_session_tracker_.GetSession(foreign_session_tag);
-
     const sync_pb::SessionHeader& header = specifics.header();
     PopulateSessionHeaderFromSpecifics(header, foreign_session);
-    foreign_session->windows.reserve(header.window_size());
+
+    // Reset the tab/window tracking for this session (must do this before
+    // we start calling GetWindow and GetTabForWindow so that all
+    // unused tabs/windows get cleared by the CleanupSession(...) call).
+    synced_session_tracker_.ResetSessionTracking(foreign_session_tag);
+
+    // Process all the windows and their tab information.
+    size_t i, num_windows;

[akalin, 2011/09/26 18:34:44]

int num_windows = header.window_size();
...
for (int i = 0; i < num_windows; ++i) {
 ...

[Nicolas Zea, 2011/09/26 19:47:36]

Done.

+    num_windows = static_cast<size_t>(header.window_size());
     VLOG(1) << "Associating " << foreign_session_tag << " with " <<
-        header.window_size() << " windows.";
-    size_t i;
-    for (i = 0; i < static_cast<size_t>(header.window_size()); ++i) {
-      if (i >= foreign_session->windows.size()) {
-        // This a new window, create it.
-        foreign_session->windows.push_back(new SessionWindow());
-      }
+        num_windows << " windows.";
+    for (i = 0; i < num_windows; ++i) {
       const sync_pb::SessionWindow& window_s = header.window(i);
+      SessionID::id_type window_id = window_s.window_id();
+      SessionWindow* window_ptr =
+          synced_session_tracker_.GetWindow(foreign_session_tag, window_id);
+      foreign_session->windows[window_id] = window_ptr;
       PopulateSessionWindowFromSpecifics(foreign_session_tag,
                                          window_s,
                                          modification_time,
-                                         foreign_session->windows[i],
+                                         window_ptr,
                                          &synced_session_tracker_);
     }
-    // Remove any remaining windows (in case windows were closed)
-    for (; i < foreign_session->windows.size(); ++i) {
-      delete foreign_session->windows[i];
-    }
-    foreign_session->windows.resize(header.window_size());
+
+    // Delete any closed windows and unused tabs as necessary.
+    synced_session_tracker_.CleanupSession(foreign_session_tag);
   } else if (specifics.has_tab()) {
     const sync_pb::SessionTab& tab_s = specifics.tab();
     SessionID::id_type tab_id = tab_s.tab_id();
     SessionTab* tab =
-        synced_session_tracker_.GetSessionTab(foreign_session_tag,
-                                              tab_id,
-                                              false);
+        synced_session_tracker_.GetTab(foreign_session_tag, tab_id);
     PopulateSessionTabFromSpecifics(tab_s, modification_time, tab);
   } else {
     NOTREACHED();
@@ -734,7 +735,9 @@ void SessionModelAssociator::PopulateSessionWindowFromSpecifics(
   for (int i = 0; i < specifics.tab_size(); i++) {
     SessionID::id_type tab_id = specifics.tab(i);
     session_window->tabs[i] =
-        tracker->GetSessionTab(session_tag, tab_id, true);
+        tracker->GetTabForWindow(session_tag,
+                                 session_window->window_id.id(),
+                                 tab_id);
   }
 }
 
@@ -743,6 +746,7 @@ void SessionModelAssociator::PopulateSessionTabFromSpecifics(
     const sync_pb::SessionTab& specifics,
     const base::Time& mtime,
     SessionTab* tab) {
+  DCHECK_EQ(tab->tab_id.id(), specifics.tab_id());
   if (specifics.has_tab_id())
     tab->tab_id.set_id(specifics.tab_id());
   if (specifics.has_window_id())
@@ -918,8 +922,7 @@ bool SessionModelAssociator::GetLocalSession(
   DCHECK(CalledOnValidThread());
   if (current_machine_tag_.empty())
     return false;
-  *local_session =
-      synced_session_tracker_.GetSession(GetCurrentMachineTag());
+  *local_session = synced_session_tracker_.GetSession(GetCurrentMachineTag());
   return true;
 }