[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;
       SessionID::id_type window_id = (*i)->GetSessionId();
       DVLOG(1) << "Associating window " << window_id << " with "
                << (*i)->GetTabCount() << " tabs.";
       window_s.set_window_id(window_id);
       // Note: We don't bother to set selected tab index anymore. We still
       // consume it when receiving foreign sessions, as reading it is free, but
       // it triggers too many sync cycles with too little value to make setting
       // it worthwhile.
       if ((*i)->IsTypeTabbed()) {
         window_s.set_browser_type(
             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* 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;
           }
         }
 
         // 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(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(
             local_tag,
             window_s,
             base::Time::Now(),
             current_session->windows[window_id],
             &synced_session_tracker_);
       }
     }
   }
+
+  // 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);
 
   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);
+  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());
+    // 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);
 }
 
 // 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()) {
+          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;
 }
 
 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::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