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

chrome/browser/sync/glue/session_model_associator.cc

 // Copyright 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 "chrome/browser/sync/glue/session_model_associator.h"
 
 #include <algorithm>
 #include <set>
 #include <utility>
 
@@ skipping 162 matching lines @@
   sync_pb::SessionHeader* header_s = specifics.mutable_header();
   SyncedSession* current_session =
       synced_session_tracker_.GetSession(local_tag);
   current_session->modified_time = base::Time::Now();
   header_s->set_client_name(current_session_name_);
   header_s->set_device_type(DeviceInfo::GetLocalDeviceType());
 
   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
     // check is necessary because, for example, when a browser is closed the
     // destructor is not necessarily run immediately. This means its possible
     // 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()) {
       sync_pb::SessionWindow window_s;
@@ skipping 10 matching lines @@
             sync_pb::SessionWindow_BrowserType_TYPE_TABBED);
       } else {
         window_s.set_browser_type(
             sync_pb::SessionWindow_BrowserType_TYPE_POPUP);
       }
 
       // 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* 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;
           }
         }
 
         // If the tab is valid, it would have been added to the tracker either
         // by the above AssociateTab call (at association time), or by the
         // change processor calling AssociateTab for all modified tabs.
         // Therefore, we can key whether this window has valid tabs based on
         // 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(synced_tab->GetSyncId());
           window_s.add_tab(tab_id);
         }
       }
       // Only add a window if it contains valid tabs.
       if (found_tabs) {
         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(
             local_tag,
             window_s,
             base::Time::Now(),
             current_session->windows[window_id],
             &synced_session_tracker_);
       }
     }
   }
+
+  // Free old sync nodes.
+  tab_pool_.FreeUnusedTabNodes(used_sync_ids);
   // Free memory for closed windows and tabs.
   synced_session_tracker_.CleanupSession(local_tag);
 
   syncer::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare());
   syncer::WriteNode header_node(&trans);
   if (header_node.InitByIdLookup(local_session_syncid_) !=
           syncer::BaseNode::INIT_OK) {
     if (error) {
       *error = error_handler_->CreateAndUploadError(
            FROM_HERE,
@@ skipping 15 matching lines @@
   return window->IsTypeTabbed() || window->IsTypePopup();
 }
 
 bool SessionModelAssociator::AssociateTabs(
     const std::vector<SyncedTabDelegate*>& tabs,
     syncer::SyncError* error) {
   DCHECK(CalledOnValidThread());
   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()) {
       // We aren't tracking this tab (for example, sync setting page).
       return true;
     }
     tab_pool_.FreeTabNode(tab_iter->second->sync_id());
     tab_map_.erase(tab_iter);
     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);
 }
 
 // static
 GURL SessionModelAssociator::GetCurrentVirtualURL(
     const SyncedTabDelegate& tab_delegate) {
   const int current_index = tab_delegate.GetCurrentEntryIndex();
   const int pending_index = tab_delegate.GetPendingEntryIndex();
   const NavigationEntry* current_entry =
       (current_index == pending_index) ?
@@ skipping 371 matching lines @@
     } else {
       // This is previously stored local session information.
       if (specifics.has_header() &&
           local_session_syncid_ == syncer::kInvalidId) {
         // This is our previous header node, reuse it.
         local_session_syncid_ = id;
         if (specifics.header().has_client_name()) {
           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;
 }
 
 void SessionModelAssociator::AssociateForeignSpecifics(
     const sync_pb::SessionSpecifics& specifics,
     const base::Time& modification_time) {
   DCHECK(CalledOnValidThread());
   std::string foreign_session_tag = specifics.session_tag();
   if (foreign_session_tag == GetCurrentMachineTag() && !setup_for_test_)
     return;
@@ skipping 382 matching lines @@
 }
 
 bool SessionModelAssociator::CryptoReadyIfNecessary() {
   // We only access the cryptographer while holding a transaction.
   syncer::ReadTransaction trans(FROM_HERE, sync_service_->GetUserShare());
   const syncer::ModelTypeSet encrypted_types = trans.GetEncryptedTypes();
   return !encrypted_types.Has(SESSIONS) ||
          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