[Sync] Add logic to reassociate tab nodes after restart.

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 32b09dd43aa22d1b14e599051aa48dd41b808eed..57d8133102b914f05f41c3191dac9853b337bfb5 100644
--- a/chrome/browser/sync/glue/session_model_associator.cc
+++ b/chrome/browser/sync/glue/session_model_associator.cc
@@ -180,6 +180,7 @@ bool SessionModelAssociator::AssociateWindows(bool reload_tabs,
   synced_session_tracker_.ResetSessionTracking(local_tag);
   std::set<SyncedWindowDelegate*> windows =
       SyncedWindowDelegate::GetSyncedWindowDelegates();
+  std::set<int64> used_sync_ids;
   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
@@ -207,16 +208,25 @@ bool SessionModelAssociator::AssociateWindows(bool reload_tabs,
       }
 
       // Store the order of tabs.
-      bool found_tabs = false;
       for (int j = 0; j < (*i)->GetTabCount(); ++j) {
         SessionID::id_type tab_id = (*i)->GetTabIdAt(j);
+        SyncedTabDelegate* syncedTab = (*i)->GetTabAt(j);

[Nicolas Zea, 2013/06/06 21:27:39]

Given your other patch, you probably need to handle the !syncedTab case.

Also, this should be named synced_tab per style guide

[shashi, 2013/06/11 19:15:06]

Done.

+        if (!syncedTab->HasWebContents()) {
+          // For tabs without webcontents update the tab_id, tab_id could have
+          // changed after a session restore.
+          if (syncedTab->GetSyncId() > 0 &&

[Nicolas Zea, 2013/06/06 21:27:39]

perhaps have GetSyncId() return syncer::kInvalidId and check against that
instead?

[shashi, 2013/06/11 19:15:06]

kInvalidId, is declared in base_node.h and adding it to
tab_contents_synced_tab_delegate fails a deps check.
On 2013/06/06 21:27:39, Nicolas Zea wrote:

+              UpdateTabIdIfNeccessary(syncedTab->GetSyncId(), tab_id)) {

[Nicolas Zea, 2013/06/06 21:27:39]

Remind me why we need to update the sync node's id, rather than the android
tab's id? The sync node's id corresponds to the id encoded in the unique tag, so
should never change.

[shashi, 2013/06/07 01:44:08]

There are two different fields: tab_node_id and tab_id. tab_id is
SyncedTabDelegate->GetSessionID essentially, tab_node_id is unique and used by
the unique tag.
We only update tab_id of the tab. We do not update tab_node_id, which still
remains unique. tab_id is updated because SessionID changes with restarts even
for tabs without web contents (TWW).
In short:
1. For normal tabs : update everything, use the normal
SMA::WriteTabContentsToSyncModel flow.
2. For TWW: just update tab_id, since it may have changed after the restart,
cannot update the rest because webcontents are available.

[Nicolas Zea, 2013/06/07 14:42:28]

Ah, right, that makes sense.

+            used_sync_ids.insert(syncedTab->GetSyncId());

[Nicolas Zea, 2013/06/06 21:27:39]

shouldn't this always be called? Even if you generated a new sync id?

[shashi, 2013/06/11 19:15:06]

It is also called when we look up for session tab:
synced_session_tracker_.LookupSessionTab is successful.

+            window_s.add_tab(tab_id);
+          }
+          continue;
+        }
 
         if (reload_tabs) {
-          SyncedTabDelegate* tab = (*i)->GetTabAt(j);
           // It's possible for GetTabAt to return a tab which has no web
           // contents. We can assume this means the tab already existed but
           // hasn't changed, so no need to reassociate.
-          if (tab->HasWebContents() && !AssociateTab(*tab, error)) {
+          if (syncedTab->HasWebContents() && !AssociateTab(syncedTab, error)) {
             // Association failed. Either we need to re-associate, or this is an
             // unrecoverable error.
             return false;
@@ -230,15 +240,14 @@ bool SessionModelAssociator::AssociateWindows(bool reload_tabs,
         // the tab's presence in the tracker.
         const SessionTab* tab = NULL;
         if (synced_session_tracker_.LookupSessionTab(local_tag, tab_id, &tab)) {
-          found_tabs = true;
+          used_sync_ids.insert(syncedTab->GetSyncId());
           window_s.add_tab(tab_id);
         }
       }
       // Only add a window if it contains valid tabs.
-      if (found_tabs) {
+      if (window_s.tab_size() > 0) {

[Nicolas Zea, 2013/06/06 21:27:39]

note that this is going to change behavior. Previously we would only add the
window if valid non-chrome:// tabs were found. Now we'll add the window, even
though it will show up with no tabs (unless you also chnage the definition of
valid tab)

[shashi, 2013/06/07 01:44:08]

Oh, I thought the found_tabs flag was redundant, it seemed to be set whenever we
add a tab to the window.

On 2013/06/06 21:27:39, Nicolas Zea wrote:

[Nicolas Zea, 2013/06/07 14:42:28]

LookupSessionTab actually does a validity check for whether we should sync the
tab or not. So found_tab is set to true only when "valid" tabs are found.

On 2013/06/07 01:44:08, shashi wrote:

[shashi, 2013/06/11 19:15:06]

Reverted.
On 2013/06/07 14:42:28, Nicolas Zea wrote:

         sync_pb::SessionWindow* header_window = header_s->add_window();
         *header_window = window_s;
-
         // Update this window's representation in the synced session tracker.
         synced_session_tracker_.PutWindowInSession(local_tag, window_id);
         PopulateSessionWindowFromSpecifics(
@@ -250,6 +259,9 @@ bool SessionModelAssociator::AssociateWindows(bool reload_tabs,
       }
     }
   }
+
+  // Free old sync nodes.
+  tab_pool_.FreeUnusedOldSyncNodes(used_sync_ids);

[Nicolas Zea, 2013/06/06 21:27:39]

UnusedOld seems a bit redundant. Perhaps FreeUnsyncedSynceNodes is enough

[shashi, 2013/06/11 19:15:06]

Done.

   // Free memory for closed windows and tabs.
   synced_session_tracker_.CleanupSession(local_tag);
 
@@ -285,19 +297,21 @@ bool SessionModelAssociator::AssociateTabs(
   for (std::vector<SyncedTabDelegate*>::const_iterator i = tabs.begin();
        i != tabs.end();
        ++i) {
-    if (!AssociateTab(**i, error))
+    if (!AssociateTab(*i, error))
       return false;
   }
   if (waiting_for_change_) QuitLoopForSubtleTesting();
   return true;
 }
 
-bool SessionModelAssociator::AssociateTab(const SyncedTabDelegate& tab,
+bool SessionModelAssociator::AssociateTab(SyncedTabDelegate* tab,
                                           syncer::SyncError* error) {
   DCHECK(CalledOnValidThread());
+  CHECK(tab);
+  DCHECK(tab->HasWebContents());
   int64 sync_id;
-  SessionID::id_type tab_id = tab.GetSessionId();
-  if (tab.IsBeingDestroyed()) {
+  SessionID::id_type tab_id = tab->GetSessionId();
+  if (tab->IsBeingDestroyed()) {
     // This tab is closing.
     TabLinksMap::iterator tab_iter = tab_map_.find(tab_id);
     if (tab_iter == tab_map_.end()) {
@@ -309,37 +323,43 @@ bool SessionModelAssociator::AssociateTab(const SyncedTabDelegate& tab,
     return true;
   }
 
-  if (!ShouldSyncTab(tab))
+  if (!ShouldSyncTab(*tab))
     return true;
 
   TabLinksMap::iterator tab_map_iter = tab_map_.find(tab_id);
   TabLink* tab_link = NULL;
   if (tab_map_iter == tab_map_.end()) {
-    // This is a new tab, get a sync node for it.
-    sync_id = tab_pool_.GetFreeTabNode();
-    if (sync_id == syncer::kInvalidId) {
-      if (error) {
-        *error = error_handler_->CreateAndUploadError(
-            FROM_HERE,
-            "Received invalid tab node from tab pool.",
-            model_type());
+    // if there is an old sync node for the tab, reuse it.
+    if (tab_pool_.RemoveIfOldSyncNodeExists(tab->GetSyncId())) {

[Nicolas Zea, 2013/06/06 21:27:39]

This name isn't very clear about what's going on. How about
ReassociateOldTabNode

[shashi, 2013/06/11 19:15:06]

Hopefully more clear now.

+      sync_id = tab->GetSyncId();
+    } else {
+      // This is a new tab, get a sync node for it.
+      sync_id = tab_pool_.GetFreeTabNode();
+      if (sync_id == syncer::kInvalidId) {
+        if (error) {
+          *error = error_handler_->CreateAndUploadError(
+              FROM_HERE,
+              "Received invalid tab node from tab pool.",
+              model_type());
+        }
+        return false;
       }
-      return false;
     }
-    tab_link = new TabLink(sync_id, &tab);
+    tab_link = new TabLink(sync_id, tab);
     tab_map_[tab_id] = make_linked_ptr<TabLink>(tab_link);
+    tab->SetSyncId(sync_id);
   } else {
     // This tab is already associated with a sync node, reuse it.
     // Note: on some platforms the tab object may have changed, so we ensure
     // the tab link is up to date.
     tab_link = tab_map_iter->second.get();
-    tab_map_iter->second->set_tab(&tab);
+    tab_map_iter->second->set_tab(tab);
   }
   DCHECK(tab_link);
   DCHECK_NE(tab_link->sync_id(), syncer::kInvalidId);
 
   DVLOG(1) << "Reloading tab " << tab_id << " from window "
-           << tab.GetWindowId();
+           << tab->GetWindowId();
   return WriteTabContentsToSyncModel(tab_link, error);
 }
 
@@ -731,24 +751,20 @@ bool SessionModelAssociator::UpdateAssociationsFromSyncModel(
           current_session_name_ = specifics.header().client_name();
         }
       } else {
-        if (specifics.has_header()) {
-          LOG(WARNING) << "Found more than one session header node with local "
-                       << " tag.";
-        } else if (!specifics.has_tab()) {
-          LOG(WARNING) << "Found local node with no header or tag field.";
+        if (specifics.has_header() || !specifics.has_tab() ||
+            !specifics.has_tab_node_id()) {
+          LOG(WARNING) << "Found invalid session node, deleting.";
+          sync_node.Tombstone();
+        } else {
+          // This is a valid old tab node, add it to the pool so it can be
+          // reused for reassociation.
+          tab_pool_.AddOldTabNode(id, specifics.tab_node_id());
         }
-
-        // TODO(zea): fix this once we add support for reassociating
-        // pre-existing tabs with pre-existing tab nodes. We'll need to load
-        // the tab_node_id and ensure the tab_pool_ keeps track of them.
-        sync_node.Tombstone();
       }
     }
     id = next_id;
   }
 
-  // After updating from sync model all tabid's should be free.
-  DCHECK(tab_pool_.full());
   return true;
 }
 
@@ -1151,4 +1167,28 @@ bool SessionModelAssociator::CryptoReadyIfNecessary() {
          sync_service_->IsCryptographerReady(&trans);
 }
 
+bool SessionModelAssociator::UpdateTabIdIfNeccessary(
+    int64 sync_id,
+    SessionID::id_type new_tab_id) {
+  DCHECK_GT(sync_id, 0);
+
+  syncer::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare());

[Yaron, 2013/06/07 17:08:35]

On Android, you're more likely than not to take this route and you'd do it for
every swapped out tab. Nicolas: is there a lot of overhead to the transaction
such that it should be done once per associate or not much overhead and better
to keep it as narrow as possible?

[Nicolas Zea, 2013/06/07 17:24:04]

All transactions involve locking, so yes, they should be minimized as much as
possible. I think one way to do that would be to update the tab id only as part
of AssociateTab (where we'll be opening a transaction anyways). 

This might involve ensuring we store the local tab information in the
SyncedSessionTracker. That way we don't have to load a tab node to check the tab
id, it'll already be part of the synced session tracker's information. To do
that, as part of UpdateAssociationsFromSyncModel, we'd pass the local tab node's
information to the SyncedSessionTracker.

[shashi, 2013/06/07 19:08:35]

Thanks, I am going to try to implement Nicolas' suggestion, a map of (sync_id ->
tab_id), in SyncedSessionTracker, that way we open a transaction only when we
have to update the tab_id.

On 2013/06/07 17:24:04, Nicolas Zea wrote:

[shashi, 2013/06/11 19:15:06]

Instead of adding to SessionTracker, tab_node_pool keeps track of mapping of
sync_id -> tab_id, this way we only open a transaction if needed.
On 2013/06/07 19:08:35, shashi wrote:

+  syncer::WriteNode tab_node(&trans);
+  // Rewrite tab id if required.
+  if (tab_node.InitByIdLookup(sync_id) == syncer::BaseNode::INIT_OK) {
+    sync_pb::SessionSpecifics session_specifics =
+        tab_node.GetSessionSpecifics();
+    DCHECK(session_specifics.has_tab());
+    if (session_specifics.has_tab()) {
+      if (session_specifics.tab().tab_id() != new_tab_id) {
+        sync_pb::SessionTab* tab_s = session_specifics.mutable_tab();
+        tab_s->set_tab_id(new_tab_id);
+        tab_node.SetSessionSpecifics(session_specifics);
+      }
+      return true;
+    }
+  }
+  return false;
+}
+
 }  // namespace browser_sync