[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 bbdfa2bfcd3b88df946298a47607f0fb320dc31f..1bcfee17346c9d3c34ad9fcf1988c6bbc5ea1e21 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
@@ -210,13 +211,30 @@ bool SessionModelAssociator::AssociateWindows(bool reload_tabs,
       bool found_tabs = false;
       for (int j = 0; j < (*i)->GetTabCount(); ++j) {
         SessionID::id_type tab_id = (*i)->GetTabIdAt(j);
+        SyncedTabDelegate* synced_tab = (*i)->GetTabAt(j);
+
+        // GetTabAt can return a null tab, in case tab is null just skip it.

[Nicolas Zea, 2013/06/19 21:35:33]

"tab; in that case just skip it."

[shashi, 2013/06/20 00:49:32]

Done.

+        if (!synced_tab)
+          continue;
+
+        if (!synced_tab->HasWebContents()) {
+          // For tabs without webcontents update the tab_id, tab_id could have

[Nicolas Zea, 2013/06/19 21:35:33]

"For tabs without WebContents, update the |tab_id|, as it could have changed.."

[shashi, 2013/06/20 00:49:32]

Done.

+          // changed after a session restore.
+          if (synced_tab->GetSyncId() > 0 &&

[Nicolas Zea, 2013/06/19 21:35:33]

!= syncer::kInvalidId
Maybe worth checking whether tab_id is valid as well?

[shashi, 2013/06/20 00:49:32]

I cannot check != syncer::kInvalidId, since TabContentsSyncedDelegate returns a
0 instead of syncer::kInvalidId and I cannot change it to return kInvalidId
because that violates a DEPS rule.
On 2013/06/19 21:35:33, Nicolas Zea wrote:

+              UpdateTabIdIfNecessary(synced_tab->GetSyncId(), tab_id)) {

[Nicolas Zea, 2013/06/19 21:35:33]

why is this part of the condition? Whether the tab id was already accurate or
not should not affect whether the tab is now used or not right?

Also, probably worth commenting that we can't check whether a tab is valid or
not if it has no webcontents, so we assume it's valid

[shashi, 2013/06/20 00:49:32]

Done, removed the condition. I was guarding against the case when
there is no sync node for an old tab, did not want to add it to
used_sync_nodes, but again we wouldn't have added the non existent node to tab
node pool in the first place.
On 2013/06/19 21:35:33, Nicolas Zea wrote:

+            found_tabs = true;
+            used_sync_ids.insert(synced_tab->GetSyncId());
+            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 && tab->HasWebContents() && !AssociateTab(*tab, error)) {
+          if (synced_tab->HasWebContents() &&
+              !AssociateTab(synced_tab, error)) {
             // Association failed. Either we need to re-associate, or this is an
             // unrecoverable error.
             return false;
@@ -231,6 +249,7 @@ bool SessionModelAssociator::AssociateWindows(bool reload_tabs,
         const SessionTab* tab = NULL;
         if (synced_session_tracker_.LookupSessionTab(local_tag, tab_id, &tab)) {
           found_tabs = true;
+          used_sync_ids.insert(synced_tab->GetSyncId());
           window_s.add_tab(tab_id);
         }
       }
@@ -250,6 +269,9 @@ bool SessionModelAssociator::AssociateWindows(bool reload_tabs,
       }
     }
   }
+
+  // Free old sync nodes.
+  tab_pool_.FreeUnusedTabNodes(used_sync_ids);
   // Free memory for closed windows and tabs.
   synced_session_tracker_.CleanupSession(local_tag);
 
@@ -285,19 +307,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);

[Nicolas Zea, 2013/06/19 21:35:33]

nit: I don't think this Check is necessary. We'd still null-ref crash two lines
lower without it.

[shashi, 2013/06/20 00:49:32]

Done. Sometimes though, the stack trace of a crash can get messed up in case of
SIGSEGV but with a CHECK we always get a proper stack trace.

+  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 +333,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());
+    sync_id = tab->GetSyncId();
+    // if there is an old sync node for the tab, reuse it.
+    if (!tab_pool_.ReassociateTabNode(sync_id, tab_id)) {
+      // 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_pool_.AssociateTabNode(sync_id, tab_id);
     }
-    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);

[Nicolas Zea, 2013/06/19 21:35:33]

move this into the if block? It's redundant for successful reassociations right?

[shashi, 2013/06/20 00:49:32]

Done. You are right, it is redundant for successful reassociations.

   } 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 +761,22 @@ 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() || !specifics.tab().has_tab_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.
+          SessionID tab_id;
+          tab_id.set_id(specifics.tab().tab_id());
+          tab_pool_.AddTabNode(id, tab_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 +1179,32 @@ bool SessionModelAssociator::CryptoReadyIfNecessary() {
          sync_service_->IsCryptographerReady(&trans);
 }
 
+bool SessionModelAssociator::UpdateTabIdIfNecessary(
+    int64 sync_id,
+    SessionID::id_type new_tab_id) {
+  DCHECK_GT(sync_id, 0);
+  SessionID::id_type old_tab_id = tab_pool_.TabIDForSyncID(sync_id);
+  if (old_tab_id != TabNodePool::kInvalidTabID && old_tab_id == new_tab_id) {

[Nicolas Zea, 2013/06/19 21:35:33]

Why does it matter whether the tab id changed? Shouldn't the return value just
describe whether we succeeded in finding the sync id or not?

[shashi, 2013/06/20 00:49:32]

Done, in fact removed the return value.
On 2013/06/19 21:35:33, Nicolas Zea wrote:

+    return true;
+  }
+
+  // Rewrite tab id if required.
+  syncer::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare());
+  syncer::WriteNode tab_node(&trans);
+  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()) {
+      sync_pb::SessionTab* tab_s = session_specifics.mutable_tab();
+      tab_s->set_tab_id(new_tab_id);
+      tab_node.SetSessionSpecifics(session_specifics);
+      // Update tab node pool with the new association.
+      tab_pool_.ReassociateTabNode(sync_id, new_tab_id);
+      return true;
+    }
+  }
+  return false;
+}
+
 }  // namespace browser_sync