Fix syncing of sessions. Numerous changes have been made. Currently, the model associator does not have a local model to associate with, but instead contains a buffer of protobuf specifics for foreign sessions which gets completely overwritten everytime an update occurs. This buffer is then used to create a vector of foreign sessions for each foreign session handler. As a result, The model associator is slightly different from other datatypes.

The creation of a persistent unique machine tag needs to be resolved still. Something understandable by the user would be good (home, work, etc.), but for now we use the directory kernel's cache_guid. Unfortunately, this gets reset each time sync is enabled/disabled, resulting in stale client session info that remains visible.

BUG=30519
TEST=unit_test

Review URL: http://codereview.chromium.org/3825005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@63266 0039d316-1c4b-4281-b951-d872f2087c98

16 files changed, 209 insertions, 169 deletions

diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd
index 3060453..312425b 100644
--- a/chrome/app/generated_resources.grd
+++ b/chrome/app/generated_resources.grd
@@ -8020,6 +8020,9 @@ Keep your key file in a safe place. You will need it to create new versions of y
       <message name="IDS_SYNC_DATATYPE_APPS" desc="Apps, one of the data types that we allow syncing.">
         Apps
       </message>
+      <message name="IDS_SYNC_DATATYPE_SESSIONS" desc="Sessions, one of the data types that we allow syncing.">
+	Foreign Sessions
+      </message>
 
       <!-- Encryption tab of the configure sync dialog -->
       <message name="IDS_SYNC_ENCRYPTION_INSTRUCTIONS" desc="Instructions for the encryption settings tab.">
diff --git a/chrome/browser/dom_ui/foreign_session_handler.cc b/chrome/browser/dom_ui/foreign_session_handler.cc
index a341f47..e35d8e5 100644
--- a/chrome/browser/dom_ui/foreign_session_handler.cc
+++ b/chrome/browser/dom_ui/foreign_session_handler.cc
@@ -35,24 +35,30 @@ void ForeignSessionHandler::RegisterMessages() {
 
 void ForeignSessionHandler::Init() {
   registrar_.Add(this, NotificationType::SYNC_CONFIGURE_DONE,
-      NotificationService::AllSources());
+                 NotificationService::AllSources());
   registrar_.Add(this, NotificationType::FOREIGN_SESSION_UPDATED,
-      NotificationService::AllSources());
-  registrar_.Add(this, NotificationType::FOREIGN_SESSION_DELETED,
-      NotificationService::AllSources());
+                 NotificationService::AllSources());
+  registrar_.Add(this, NotificationType::FOREIGN_SESSION_DISABLED,
+                 NotificationService::AllSources());
 }
 
 void ForeignSessionHandler::Observe(NotificationType type,
                                     const NotificationSource& source,
                                     const NotificationDetails& details) {
-  if (type != NotificationType::SYNC_CONFIGURE_DONE &&
-      type != NotificationType::FOREIGN_SESSION_UPDATED &&
-      type != NotificationType::FOREIGN_SESSION_DELETED) {
-    NOTREACHED();
-    return;
-  }
   ListValue list_value;
-  HandleGetForeignSessions(&list_value);
+  switch (type.value) {
+    case NotificationType::SYNC_CONFIGURE_DONE:
+    case NotificationType::FOREIGN_SESSION_UPDATED:
+      HandleGetForeignSessions(&list_value);
+      break;
+    case NotificationType::FOREIGN_SESSION_DISABLED:
+      // Calling foreignSessions with empty list will automatically hide
+      // foreign session section.
+      dom_ui_->CallJavascriptFunction(L"foreignSessions", list_value);
+      break;
+    default:
+      NOTREACHED();
+  }
 }
 
 SessionModelAssociator* ForeignSessionHandler::GetModelAssociator() {
@@ -106,36 +112,49 @@ void ForeignSessionHandler::OpenForeignSession(
 
 void ForeignSessionHandler::GetForeignSessions(
     SessionModelAssociator* associator) {
-  ScopedVector<ForeignSession> windows;
-  associator->GetSessionDataFromSyncModel(&windows.get());
+  ScopedVector<ForeignSession> clients;
+  if (!associator->GetSessionData(&clients.get())) {
+    LOG(ERROR) << "ForeignSessionHandler failed to get session data from"
+        "SessionModelAssociator.";
+    return;
+  }
   int added_count = 0;
-  ListValue list_value;
+  ListValue client_list;
   for (std::vector<ForeignSession*>::const_iterator i =
-      windows.begin(); i != windows.end() &&
+      clients->begin(); i != clients->end() &&
       added_count < kMaxSessionsToShow; ++i) {
     ForeignSession* foreign_session = *i;
     std::vector<TabRestoreService::Entry*> entries;
     dom_ui_->GetProfile()->GetTabRestoreService()->CreateEntriesFromWindows(
         &foreign_session->windows, &entries);
+    scoped_ptr<ListValue> window_list(new ListValue());
     for (std::vector<TabRestoreService::Entry*>::const_iterator it =
         entries.begin(); it != entries.end(); ++it) {
       TabRestoreService::Entry* entry = *it;
-      scoped_ptr<DictionaryValue> value(new DictionaryValue());
+      scoped_ptr<DictionaryValue> window_data(new DictionaryValue());
       if (entry->type == TabRestoreService::WINDOW &&
           ValueHelper::WindowToValue(
-              *static_cast<TabRestoreService::Window*>(entry), value.get())) {
+              *static_cast<TabRestoreService::Window*>(entry),
+              window_data.get())) {
         // The javascript checks if the session id is a valid session id,
         // when rendering session information to the new tab page, and it
         // sends the sessionTag back when we need to restore a session.
-        value->SetString("sessionTag", foreign_session->foreign_tession_tag);
-        value->SetInteger("sessionId", entry->id);
-        list_value.Append(value.release());  // Give ownership to |list_value|.
+
+        // TODO(zea): sessionTag is per client, it might be better per window.
+        window_data->SetString("sessionTag",
+            foreign_session->foreign_tession_tag);
+        window_data->SetInteger("sessionId", entry->id);
+
+        // Give ownership to |list_value|.
+        window_list->Append(window_data.release());
       }
     }
     added_count++;
+
+    // Give ownership to |client_list|
+    client_list.Append(window_list.release());
   }
-  dom_ui_->CallJavascriptFunction(L"foreignSessions", list_value);
+  dom_ui_->CallJavascriptFunction(L"foreignSessions", client_list);
 }
 
 }  // namespace browser_sync
-
diff --git a/chrome/browser/dom_ui/foreign_session_handler.h b/chrome/browser/dom_ui/foreign_session_handler.h
index 1c4751d..6532205 100644
--- a/chrome/browser/dom_ui/foreign_session_handler.h
+++ b/chrome/browser/dom_ui/foreign_session_handler.h
@@ -6,6 +6,8 @@
 #define CHROME_BROWSER_DOM_UI_FOREIGN_SESSION_HANDLER_H_
 #pragma once
 
+#include <vector>
+
 #include "chrome/browser/dom_ui/dom_ui.h"
 #include "chrome/browser/sessions/session_service.h"
 #include "chrome/browser/sync/glue/session_model_associator.h"
@@ -59,4 +61,3 @@ class ForeignSessionHandler : public DOMMessageHandler,
 }  // namespace browser_sync
 
 #endif  // CHROME_BROWSER_DOM_UI_FOREIGN_SESSION_HANDLER_H_
-
diff --git a/chrome/browser/dom_ui/ntp_resource_cache.cc b/chrome/browser/dom_ui/ntp_resource_cache.cc
index dca2917..c2e7add 100644
--- a/chrome/browser/dom_ui/ntp_resource_cache.cc
+++ b/chrome/browser/dom_ui/ntp_resource_cache.cc
@@ -245,6 +245,8 @@ void NTPResourceCache::CreateNewTabHTML() {
       l10n_util::GetStringUTF16(IDS_NEW_TAB_RECENTLY_CLOSED));
   localized_strings.SetString("closedwindowsingle",
       l10n_util::GetStringUTF16(IDS_NEW_TAB_RECENTLY_CLOSED_WINDOW_SINGLE));
+  localized_strings.SetString("foreignsessions",
+      l10n_util::GetStringUTF16(IDS_SYNC_DATATYPE_SESSIONS));
   localized_strings.SetString("closedwindowmultiple",
       l10n_util::GetStringUTF16(IDS_NEW_TAB_RECENTLY_CLOSED_WINDOW_MULTIPLE));
   localized_strings.SetString("attributionintro",
diff --git a/chrome/browser/dom_ui/options/sync_options_handler.cc b/chrome/browser/dom_ui/options/sync_options_handler.cc
index 7c19a59..de6c5bf 100644
--- a/chrome/browser/dom_ui/options/sync_options_handler.cc
+++ b/chrome/browser/dom_ui/options/sync_options_handler.cc
@@ -52,6 +52,8 @@ void SyncOptionsHandler::GetLocalizedValues(
       l10n_util::GetStringUTF16(IDS_SYNC_DATATYPE_THEMES));
   localized_strings->SetString("syncapps",
       l10n_util::GetStringUTF16(IDS_SYNC_DATATYPE_APPS));
+  localized_strings->SetString("syncsessions",
+      l10n_util::GetStringUTF16(IDS_SYNC_DATATYPE_SESSIONS));
 }
 
 void SyncOptionsHandler::Initialize() {
diff --git a/chrome/browser/sessions/session_restore.cc b/chrome/browser/sessions/session_restore.cc
index 46047a6..a537943 100644
--- a/chrome/browser/sessions/session_restore.cc
+++ b/chrome/browser/sessions/session_restore.cc
@@ -322,8 +322,8 @@ class SessionRestoreImpl : public NotificationObserver {
       ShowBrowser(browser, initial_tab_count,
           (*i)->selected_tab_index);
       NotifySessionServiceOfRestoredTabs(browser, initial_tab_count);
-      FinishedTabCreation(true, has_tabbed_browser);
     }
+    FinishedTabCreation(true, has_tabbed_browser);
   }
 
   ~SessionRestoreImpl() {
diff --git a/chrome/browser/sessions/session_service.cc b/chrome/browser/sessions/session_service.cc
index b0ba070..9b2db65 100644
--- a/chrome/browser/sessions/session_service.cc
+++ b/chrome/browser/sessions/session_service.cc
@@ -447,7 +447,7 @@ void SessionService::Save() {
         &last_updated_save_time_);
     NotificationService::current()->Notify(
         NotificationType::SESSION_SERVICE_SAVED,
-        NotificationService::AllSources(),
+        Source<Profile>(profile()),
         NotificationService::NoDetails());
   }
 }
@@ -1473,4 +1473,3 @@ void SessionService::RecordUpdatedSaveTime(base::TimeDelta delta,
         50);
   }
 }
-
diff --git a/chrome/browser/sync/glue/autofill_change_processor.cc b/chrome/browser/sync/glue/autofill_change_processor.cc
index edf3d9f..c025e84 100644
--- a/chrome/browser/sync/glue/autofill_change_processor.cc
+++ b/chrome/browser/sync/glue/autofill_change_processor.cc
@@ -41,7 +41,6 @@ AutofillChangeProcessor::AutofillChangeProcessor(
 void AutofillChangeProcessor::Observe(NotificationType type,
                                       const NotificationSource& source,
                                       const NotificationDetails& details) {
-  LOG(INFO) << "Observed autofill change.";
   // Ensure this notification came from our web database.
   WebDataService* wds = Source<WebDataService>(source).ptr();
   if (!wds || wds->GetDatabase() != web_database_)
diff --git a/chrome/browser/sync/glue/session_change_processor.cc b/chrome/browser/sync/glue/session_change_processor.cc
index 4250395..ca0b166 100644
--- a/chrome/browser/sync/glue/session_change_processor.cc
+++ b/chrome/browser/sync/glue/session_change_processor.cc
@@ -6,12 +6,15 @@
 
 #include <sstream>
 #include <string>
+#include <vector>
 
 #include "base/logging.h"
+#include "base/scoped_vector.h"
 #include "chrome/browser/browser_thread.h"
 #include "chrome/browser/sync/engine/syncapi.h"
 #include "chrome/browser/sync/glue/session_model_associator.h"
 #include "chrome/common/notification_details.h"
+#include "chrome/common/notification_service.h"
 #include "chrome/common/notification_source.h"
 
 namespace browser_sync {
@@ -41,8 +44,6 @@ void SessionChangeProcessor::Observe(NotificationType type,
     case NotificationType::SESSION_SERVICE_SAVED: {
       std::string tag = session_model_associator_->GetCurrentMachineTag();
       DCHECK_EQ(Source<Profile>(source).ptr(), profile_);
-      LOG(INFO) << "Got change notification of type " << type.value
-                << " for session " << tag;
       session_model_associator_->UpdateSyncModelDataFromClient();
       break;
     }
@@ -61,40 +62,20 @@ void SessionChangeProcessor::ApplyChangesFromSyncModel(
   if (!running()) {
     return;
   }
-  for (int i = 0; i < change_count; ++i) {
-    const sync_api::SyncManager::ChangeRecord& change = changes[i];
-    switch (change.action) {
-      case sync_api::SyncManager::ChangeRecord::ACTION_ADD:
-        UpdateModel(trans, change, true);
-        break;
-      case sync_api::SyncManager::ChangeRecord::ACTION_UPDATE:
-        UpdateModel(trans, change, true);
-        break;
-      case sync_api::SyncManager::ChangeRecord::ACTION_DELETE:
-        UpdateModel(trans, change, false);
-        break;
-    }
-  }
-}
 
-void SessionChangeProcessor::UpdateModel(const sync_api::BaseTransaction* trans,
-    const sync_api::SyncManager::ChangeRecord& change, bool associate) {
-  sync_api::ReadNode node(trans);
-  if (!node.InitByIdLookup(change.id)) {
-    std::stringstream error;
-    error << "Session node lookup failed for change " << change.id
-      << " of action type " << change.action;
-    error_handler()->OnUnrecoverableError(FROM_HERE, error.str());
-    return;
-  }
-  DCHECK_EQ(node.GetModelType(), syncable::SESSIONS);
   StopObserving();
-  if (associate) {
-    session_model_associator_->Associate(
-      (const sync_pb::SessionSpecifics *) NULL, node.GetId());
-  } else {
-    session_model_associator_->Disassociate(node.GetId());
-  }
+
+  // This currently ignores the tracked changes and rebuilds the sessions from
+  // all the session sync nodes.
+  // TODO(zea): Make use of |changes| to adjust only modified sessions.
+  session_model_associator_->UpdateFromSyncModel(trans);
+
+  // Notify foreign session handlers that there are new sessions.
+  NotificationService::current()->Notify(
+      NotificationType::FOREIGN_SESSION_UPDATED,
+      NotificationService::AllSources(),
+      NotificationService::NoDetails());
+
   StartObserving();
 }
 
@@ -115,7 +96,6 @@ void SessionChangeProcessor::StopImpl() {
 void SessionChangeProcessor::StartObserving() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(profile_);
-  LOG(INFO) << "Observing SESSION_SERVICE_SAVED";
   notification_registrar_.Add(
       this, NotificationType::SESSION_SERVICE_SAVED,
       Source<Profile>(profile_));
@@ -124,9 +104,7 @@ void SessionChangeProcessor::StartObserving() {
 void SessionChangeProcessor::StopObserving() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(profile_);
-  LOG(INFO) << "removing observation of all notifications";
   notification_registrar_.RemoveAll();
 }
 
 }  // namespace browser_sync
-
diff --git a/chrome/browser/sync/glue/session_change_processor.h b/chrome/browser/sync/glue/session_change_processor.h
index 2eadc7a..9d772da 100644
--- a/chrome/browser/sync/glue/session_change_processor.h
+++ b/chrome/browser/sync/glue/session_change_processor.h
@@ -57,8 +57,6 @@ class SessionChangeProcessor : public ChangeProcessor,
  private:
   void StartObserving();
   void StopObserving();
-  void UpdateModel(const sync_api::BaseTransaction* trans,
-      const sync_api::SyncManager::ChangeRecord& change, bool associate);
   SessionModelAssociator* session_model_associator_;
   NotificationRegistrar notification_registrar_;
 
@@ -71,4 +69,3 @@ class SessionChangeProcessor : public ChangeProcessor,
 }  // namespace browser_sync
 
 #endif  // CHROME_BROWSER_SYNC_GLUE_SESSION_CHANGE_PROCESSOR_H_
-
diff --git a/chrome/browser/sync/glue/session_model_associator.cc b/chrome/browser/sync/glue/session_model_associator.cc
index 3524aea..24ff417 100644
--- a/chrome/browser/sync/glue/session_model_associator.cc
+++ b/chrome/browser/sync/glue/session_model_associator.cc
@@ -37,19 +37,22 @@ SessionModelAssociator::~SessionModelAssociator() {
   DCHECK(CalledOnValidThread());
 }
 
-// Sends a notification to ForeignSessionHandler to update the UI, because
-// the session corresponding to the id given has changed state.
-void SessionModelAssociator::Associate(
-    const sync_pb::SessionSpecifics* specifics, int64 sync_id) {
-  DCHECK(CalledOnValidThread());
-  NotificationService::current()->Notify(
-      NotificationType::FOREIGN_SESSION_UPDATED,
-      NotificationService::AllSources(),
-      Details<int64>(&sync_id));
-}
-
 bool SessionModelAssociator::AssociateModels() {
   DCHECK(CalledOnValidThread());
+
+  // Make sure we have a machine tag.
+  if (current_machine_tag_.empty())
+    InitializeCurrentMachineTag();  // Creates a syncable::BaseTransaction.
+
+  {
+    // Do an initial update from sync model (in case we just re-enabled and
+    // already had data).
+    sync_api::ReadTransaction trans(
+        sync_service_->backend()->GetUserShareHandle());
+    UpdateFromSyncModel(&trans);
+  }
+
+  // Check if anything has changed on the client side.
   UpdateSyncModelDataFromClient();
   return true;
 }
@@ -63,13 +66,16 @@ bool SessionModelAssociator::ChromeModelHasUserCreatedNodes(
   return true;
 }
 
-// Sends a notification to ForeignSessionHandler to update the UI, because
-// the session corresponding to the id given has been deleted.
-void SessionModelAssociator::Disassociate(int64 sync_id) {
+bool SessionModelAssociator::DisassociateModels() {
+  specifics_.clear();
+
+  // There is no local model stored with which to disassociate, just notify
+  // foreign session handlers.
   NotificationService::current()->Notify(
-      NotificationType::FOREIGN_SESSION_DELETED,
+      NotificationType::FOREIGN_SESSION_DISABLED,
       NotificationService::AllSources(),
-      Details<int64>(&sync_id));
+      NotificationService::NoDetails());
+  return true;
 }
 
 const sync_pb::SessionSpecifics* SessionModelAssociator::
@@ -120,39 +126,10 @@ bool SessionModelAssociator::SyncModelHasUserCreatedNodes(bool* has_nodes) {
 }
 
 std::string SessionModelAssociator::GetCurrentMachineTag() {
-  if (current_machine_tag_.empty())
-    InitializeCurrentMachineTag();
   DCHECK(!current_machine_tag_.empty());
   return current_machine_tag_;
 }
 
-void SessionModelAssociator::AppendForeignSessionFromSpecifics(
-    const sync_pb::SessionSpecifics* specifics,
-    std::vector<ForeignSession*>* session) {
-  ForeignSession* foreign_session = new ForeignSession();
-  foreign_session->foreign_tession_tag = specifics->session_tag();
-  session->insert(session->end(), foreign_session);
-  for (int i = 0; i < specifics->session_window_size(); i++) {
-    const sync_pb::SessionWindow* window = &specifics->session_window(i);
-    SessionWindow* session_window = new SessionWindow();
-    PopulateSessionWindowFromSpecifics(session_window, window);
-    foreign_session->windows.insert(
-        foreign_session->windows.end(), session_window);
-  }
-}
-
-// Fills the given vector with foreign session windows to restore.
-void SessionModelAssociator::AppendForeignSessionWithID(int64 id,
-    std::vector<ForeignSession*>* session, sync_api::BaseTransaction* trans) {
-  if (id == sync_api::kInvalidId)
-    return;
-  sync_api::ReadNode node(trans);
-  if (!node.InitByIdLookup(id))
-    return;
-  const sync_pb::SessionSpecifics* ref = &node.GetSessionSpecifics();
-  AppendForeignSessionFromSpecifics(ref, session);
-}
-
 void SessionModelAssociator::UpdateSyncModelDataFromClient() {
   DCHECK(CalledOnValidThread());
   SessionService::SessionCallback* callback =
@@ -162,24 +139,40 @@ void SessionModelAssociator::UpdateSyncModelDataFromClient() {
   GetSessionService()->GetCurrentSession(&consumer_, callback);
 }
 
-bool SessionModelAssociator::GetSessionDataFromSyncModel(
-    std::vector<ForeignSession*>* sessions) {
-  std::vector<const sync_pb::SessionSpecifics*> specifics;
+// TODO(zea): Don't recreate sessions_ vector from scratch each time. This
+// will involve knowing which sessions have been changed (a different data
+// structure will probably be better too).
+bool SessionModelAssociator::UpdateFromSyncModel(
+    const sync_api::BaseTransaction* trans) {
   DCHECK(CalledOnValidThread());
-  sync_api::ReadTransaction trans(
-      sync_service_->backend()->GetUserShareHandle());
-  sync_api::ReadNode root(&trans);
+
+  // Rebuild specifics_ vector
+  specifics_.clear();
+  if (!QuerySyncModel(trans, specifics_)) {
+    LOG(ERROR) << "SessionModelAssociator failed to updated from sync model";
+    return false;
+  }
+
+  return true;
+}
+
+bool SessionModelAssociator::QuerySyncModel(
+    const sync_api::BaseTransaction* trans,
+    std::vector<const sync_pb::SessionSpecifics*>& specifics) {
+  DCHECK(CalledOnValidThread());
+  sync_api::ReadNode root(trans);
   if (!root.InitByTagLookup(kSessionsTag)) {
     LOG(ERROR) << kNoSessionsFolderError;
     return false;
   }
-  sync_api::ReadNode current_machine(&trans);
+  sync_api::ReadNode current_machine(trans);
   int64 current_id = (current_machine.InitByClientTagLookup(syncable::SESSIONS,
     GetCurrentMachineTag())) ? current_machine.GetId() : sync_api::kInvalidId;
+
   // Iterate through the nodes and populate the session model.
   int64 id = root.GetFirstChildId();
   while (id != sync_api::kInvalidId) {
-    sync_api::ReadNode sync_node(&trans);
+    sync_api::ReadNode sync_node(trans);
     if (!sync_node.InitByIdLookup(id)) {
       LOG(ERROR) << "Failed to fetch sync node for id " << id;
       return false;
@@ -189,13 +182,49 @@ bool SessionModelAssociator::GetSessionDataFromSyncModel(
     }
     id = sync_node.GetSuccessorId();
   }
+  return true;
+}
+
+bool SessionModelAssociator::GetSessionData(
+    std::vector<ForeignSession*>* sessions) {
+  DCHECK(CalledOnValidThread());
+
+  // Build vector of sessions from specifics data
   for (std::vector<const sync_pb::SessionSpecifics*>::const_iterator i =
-    specifics.begin(); i != specifics.end(); ++i) {
+    specifics_.begin(); i != specifics_.end(); ++i) {
     AppendForeignSessionFromSpecifics(*i, sessions);
   }
+
   return true;
 }
 
+void SessionModelAssociator::AppendForeignSessionFromSpecifics(
+    const sync_pb::SessionSpecifics* specifics,
+    std::vector<ForeignSession*>* session) {
+  ForeignSession* foreign_session = new ForeignSession();
+  foreign_session->foreign_tession_tag = specifics->session_tag();
+  session->insert(session->end(), foreign_session);
+  for (int i = 0; i < specifics->session_window_size(); i++) {
+    const sync_pb::SessionWindow* window = &specifics->session_window(i);
+    SessionWindow* session_window = new SessionWindow();
+    PopulateSessionWindowFromSpecifics(session_window, window);
+    foreign_session->windows.insert(
+        foreign_session->windows.end(), session_window);
+  }
+}
+
+// Fills the given vector with foreign session windows to restore.
+void SessionModelAssociator::AppendForeignSessionWithID(int64 id,
+    std::vector<ForeignSession*>* session, sync_api::BaseTransaction* trans) {
+  if (id == sync_api::kInvalidId)
+    return;
+  sync_api::ReadNode node(trans);
+  if (!node.InitByIdLookup(id))
+    return;
+  const sync_pb::SessionSpecifics* ref = &node.GetSessionSpecifics();
+  AppendForeignSessionFromSpecifics(ref, session);
+}
+
 SessionService* SessionModelAssociator::GetSessionService() {
   DCHECK(sync_service_);
   Profile* profile = sync_service_->profile();
@@ -210,8 +239,16 @@ void SessionModelAssociator::InitializeCurrentMachineTag() {
       GetUserShareHandle());
   syncable::Directory* dir =
       trans.GetWrappedWriteTrans()->directory();
+
+  // TODO(zea): We need a better way of creating a machine tag. The directory
+  // kernel's cache_guid changes every time syncing is turned on and off. This
+  // will result in session's associated with stale machine tags persisting on
+  // the server since that tag will not be reused. Eventually this should
+  // become some string identifiable to the user. (Home, Work, Laptop, etc.)
+  // See issue 59672
   current_machine_tag_ = "session_sync";
   current_machine_tag_.append(dir->cache_guid());
+  LOG(INFO) << "Creating machine tag: " << current_machine_tag_;
 }
 
 // See PopulateSessionSpecificsTab for use.  May add functionality that includes
@@ -525,4 +562,3 @@ bool SessionModelAssociator::UpdateSyncModel(
 }
 
 }  // namespace browser_sync
-
diff --git a/chrome/browser/sync/glue/session_model_associator.h b/chrome/browser/sync/glue/session_model_associator.h
index 3605783..ac7a234 100644
--- a/chrome/browser/sync/glue/session_model_associator.h
+++ b/chrome/browser/sync/glue/session_model_associator.h
@@ -12,6 +12,7 @@
 #include "base/basictypes.h"
 #include "base/gtest_prod_util.h"
 #include "base/observer_list.h"
+#include "base/scoped_vector.h"
 #include "chrome/browser/sessions/session_service.h"
 #include "chrome/browser/sessions/session_types.h"
 #include "chrome/browser/sync/engine/syncapi.h"
@@ -36,8 +37,13 @@ class SessionSpecifics;
 namespace browser_sync {
 
 static const char kSessionsTag[] = "google_chrome_sessions";
+
 // Contains all logic for associating the Chrome sessions model and
 // the sync sessions model.
+// In the case of sessions, our local model is nothing but a buffer (specifics_)
+// that gets overwritten everytime there is an update. From it, we build a new
+// foreign session windows list each time |GetSessionData| is called by the
+// ForeignSessionHandler.
 class SessionModelAssociator : public PerDataTypeAssociatorInterface<
     sync_pb::SessionSpecifics, std::string>, public NonThreadSafe {
  public:
@@ -54,11 +60,13 @@ class SessionModelAssociator : public PerDataTypeAssociatorInterface<
     // thread.
   }
 
-  // Used to re-associate a foreign session.
+  // Dummy method, we do everything all-at-once in UpdateFromSyncModel.
   virtual void Associate(const sync_pb::SessionSpecifics* specifics,
-    int64 sync_id);
+    int64 sync_id) {
+  }
 
-  // Updates the sync model with the local client data.
+  // Updates the sync model with the local client data. (calls
+  // UpdateFromSyncModel)
   virtual bool AssociateModels();
 
   // The has_nodes out parameter is set to true if the chrome model
@@ -66,17 +74,13 @@ class SessionModelAssociator : public PerDataTypeAssociatorInterface<
   // occurred.
   virtual bool ChromeModelHasUserCreatedNodes(bool* has_nodes);
 
-  // Will update the new tab page with current foreign sessions excluding the
-  // one being disassociated.
-  virtual void Disassociate(int64 sync_id);
-
-  // TODO(jerrica): Add functionality to stop rendering foreign sessions to the
-  // new tab page.
-  virtual bool DisassociateModels() {
-    // There is no local model stored with which to disassociate.
-    return true;
+  // Dummy method, we do everything all-at-once in UpdateFromSyncModel.
+  virtual void Disassociate(int64 sync_id) {
   }
 
+  // Clear specifics_ buffer and notify foreign session handlers.
+  virtual bool DisassociateModels();
+
   // Returns the chrome session specifics for the given sync id.
   // Returns NULL if no specifics are found for the given sync id.
   virtual const sync_pb::SessionSpecifics* GetChromeNodeFromSyncId(
@@ -109,10 +113,21 @@ class SessionModelAssociator : public PerDataTypeAssociatorInterface<
   // sync model.
   std::string GetCurrentMachineTag();
 
-  // Pulls the current sync model from the server, and returns true upon update
-  // of the client model.
-  bool GetSessionDataFromSyncModel(std::vector<ForeignSession*>* windows);
+  // Updates the server data based upon the current client session.  If no node
+  // corresponding to this machine exists in the sync model, one is created.
+  void UpdateSyncModelDataFromClient();
+
+  // Pulls the current sync model from the sync database and returns true upon
+  // update of the client model. Called by SessionChangeProcessor.
+  // Note that the specifics_ vector is reset and built from scratch each time.
+  bool UpdateFromSyncModel(const sync_api::BaseTransaction* trans);
 
+  // Helper for UpdateFromSyncModel. Appends sync data to a vector of specifics.
+  bool QuerySyncModel(const sync_api::BaseTransaction* trans,
+      std::vector<const sync_pb::SessionSpecifics*>& specifics);
+
+  // Builds sessions from buffered specifics data
+  bool GetSessionData(std::vector<ForeignSession*>* sessions);
 
   // Helper method for converting session specifics to windows.
   void AppendForeignSessionFromSpecifics(
@@ -129,10 +144,6 @@ class SessionModelAssociator : public PerDataTypeAssociatorInterface<
   // Returns the syncable model type.
   static syncable::ModelType model_type() { return syncable::SESSIONS; }
 
-  // Updates the server data based upon the current client session.  If no node
-  // corresponding to this machine exists in the sync model, one is created.
-  void UpdateSyncModelDataFromClient();
-
  private:
   FRIEND_TEST_ALL_PREFIXES(ProfileSyncServiceSessionTest, WriteSessionToNode);
   FRIEND_TEST_ALL_PREFIXES(ProfileSyncServiceSessionTest,
@@ -142,6 +153,7 @@ class SessionModelAssociator : public PerDataTypeAssociatorInterface<
   SessionService* GetSessionService();
 
   // Initializes the tag corresponding to this machine.
+  // Note: creates a syncable::BaseTransaction.
   void InitializeCurrentMachineTag();
 
   // Populates the navigation portion of the session specifics.
@@ -182,10 +194,14 @@ class SessionModelAssociator : public PerDataTypeAssociatorInterface<
   bool UpdateSyncModel(sync_pb::SessionSpecifics* session_data,
                     sync_api::WriteTransaction* trans,
                     const sync_api::ReadNode* root);
-
   // Stores the machine tag.
   std::string current_machine_tag_;
 
+  // Stores the current set of foreign session specifics.
+  // Used by ForeignSessionHandler through |GetSessionData|.
+  // Built by |QuerySyncModel| via |UpdateFromSyncModel|.
+  std::vector<const sync_pb::SessionSpecifics*> specifics_;
+
   // Weak pointer.
   ProfileSyncService* sync_service_;
 
@@ -198,4 +214,3 @@ class SessionModelAssociator : public PerDataTypeAssociatorInterface<
 }  // namespace browser_sync
 
 #endif  // CHROME_BROWSER_SYNC_GLUE_SESSION_MODEL_ASSOCIATOR_H_
-
diff --git a/chrome/browser/sync/profile_sync_service_session_unittest.cc b/chrome/browser/sync/profile_sync_service_session_unittest.cc
index 23e4885..0a5ee55 100644
--- a/chrome/browser/sync/profile_sync_service_session_unittest.cc
+++ b/chrome/browser/sync/profile_sync_service_session_unittest.cc
@@ -56,8 +56,7 @@ class ProfileSyncServiceSessionTest
  public:
   ProfileSyncServiceSessionTest()
       : window_bounds_(0, 1, 2, 3),
-        notified_of_update_(false),
-        notification_sync_id_(0) {}
+        notified_of_update_(false) {}
 
   ProfileSyncService* sync_service() { return sync_service_.get(); }
 
@@ -76,24 +75,15 @@ class ProfileSyncServiceSessionTest
     service()->SetWindowBounds(window_id_, window_bounds_, false);
     registrar_.Add(this, NotificationType::FOREIGN_SESSION_UPDATED,
         NotificationService::AllSources());
-    registrar_.Add(this, NotificationType::FOREIGN_SESSION_DELETED,
-        NotificationService::AllSources());
   }
 
   void Observe(NotificationType type,
       const NotificationSource& source,
       const NotificationDetails& details) {
     switch (type.value) {
-      case NotificationType::FOREIGN_SESSION_UPDATED: {
-        notified_of_update_ = true;
-        notification_sync_id_ = *Details<int64>(details).ptr();
-        break;
-      }
-      case NotificationType::FOREIGN_SESSION_DELETED: {
+      case NotificationType::FOREIGN_SESSION_UPDATED:
         notified_of_update_ = true;
-        notification_sync_id_ = -1;
         break;
-      }
       default:
         NOTREACHED();
         break;
@@ -147,7 +137,6 @@ class ProfileSyncServiceSessionTest
   TestIdFactory ids_;
   const gfx::Rect window_bounds_;
   bool notified_of_update_;
-  int64 notification_sync_id_;
   NotificationRegistrar registrar_;
 };
 
@@ -236,7 +225,7 @@ TEST_F(ProfileSyncServiceSessionTest, WriteFilledSessionToNode) {
 
   // Check that this machine's data is not included in the foreign windows.
   ScopedVector<ForeignSession> foreign_sessions;
-  model_associator_->GetSessionDataFromSyncModel(&foreign_sessions.get());
+  model_associator_->GetSessionData(&foreign_sessions.get());
   ASSERT_EQ(foreign_sessions.size(), 0U);
 
   // Get the windows for this machine from the node and check that they were
@@ -309,6 +298,7 @@ TEST_F(ProfileSyncServiceSessionTest, WriteForeignSessionToNode) {
     sync_api::ReadNode root(&trans);
     ASSERT_TRUE(root.InitByTagLookup(kSessionsTag));
     model_associator_->UpdateSyncModel(&specifics, &trans, &root);
+    model_associator_->UpdateFromSyncModel(&trans);
   }
 
   // Check that the foreign session was written to a node and retrieve the data.
@@ -320,8 +310,8 @@ TEST_F(ProfileSyncServiceSessionTest, WriteForeignSessionToNode) {
       model_associator_->GetChromeNodeFromSyncId(sync_id));
   ASSERT_TRUE(sync_specifics != NULL);
   ScopedVector<ForeignSession> foreign_sessions;
-  model_associator_->GetSessionDataFromSyncModel(&foreign_sessions.get());
-  ASSERT_EQ(foreign_sessions.size(), 1U);
+  model_associator_->GetSessionData(&foreign_sessions.get());
+  ASSERT_EQ(1U, foreign_sessions.size());
   ASSERT_EQ(1U,  foreign_sessions[0]->windows.size());
   ASSERT_EQ(1U, foreign_sessions[0]->windows[0]->tabs.size());
   ASSERT_EQ(1U, foreign_sessions[0]->windows[0]->tabs[0]->navigations.size());
@@ -364,13 +354,11 @@ TEST_F(ProfileSyncServiceSessionTest, UpdatedSyncNodeActionUpdate) {
   scoped_ptr<SyncManager::ChangeRecord> record(new SyncManager::ChangeRecord);
   record->action = SyncManager::ChangeRecord::ACTION_UPDATE;
   record->id = node_id;
-  ASSERT_EQ(notification_sync_id_, 0);
   ASSERT_FALSE(notified_of_update_);
   {
     sync_api::WriteTransaction trans(backend()->GetUserShareHandle());
     change_processor_->ApplyChangesFromSyncModel(&trans, record.get(), 1);
   }
-  ASSERT_EQ(notification_sync_id_, node_id);
   ASSERT_TRUE(notified_of_update_);
 }
 
@@ -385,13 +373,11 @@ TEST_F(ProfileSyncServiceSessionTest, UpdatedSyncNodeActionAdd) {
   scoped_ptr<SyncManager::ChangeRecord> record(new SyncManager::ChangeRecord);
   record->action = SyncManager::ChangeRecord::ACTION_ADD;
   record->id = node_id;
-  ASSERT_EQ(notification_sync_id_, 0);
   ASSERT_FALSE(notified_of_update_);
   {
     sync_api::WriteTransaction trans(backend()->GetUserShareHandle());
     change_processor_->ApplyChangesFromSyncModel(&trans, record.get(), 1);
   }
-  ASSERT_EQ(notification_sync_id_, node_id);
   ASSERT_TRUE(notified_of_update_);
 }
 
@@ -406,13 +392,11 @@ TEST_F(ProfileSyncServiceSessionTest, UpdatedSyncNodeActionDelete) {
   scoped_ptr<SyncManager::ChangeRecord> record(new SyncManager::ChangeRecord);
   record->action = SyncManager::ChangeRecord::ACTION_DELETE;
   record->id = node_id;
-  ASSERT_EQ(notification_sync_id_, 0);
   ASSERT_FALSE(notified_of_update_);
   {
     sync_api::WriteTransaction trans(backend()->GetUserShareHandle());
     change_processor_->ApplyChangesFromSyncModel(&trans, record.get(), 1);
   }
-  ASSERT_EQ(notification_sync_id_, -1);
   ASSERT_TRUE(notified_of_update_);
 }
 
diff --git a/chrome/browser/sync/sync_setup_flow.cc b/chrome/browser/sync/sync_setup_flow.cc
index ee211f5..1dbd60c 100644
--- a/chrome/browser/sync/sync_setup_flow.cc
+++ b/chrome/browser/sync/sync_setup_flow.cc
@@ -128,6 +128,12 @@ static bool GetConfiguration(const std::string& json,
   if (sync_typed_urls)
     config->data_types.insert(syncable::TYPED_URLS);
 
+  bool sync_sessions;
+  if (!result->GetBoolean("syncSessions", &sync_sessions))
+    return false;
+  if (sync_sessions)
+    config->data_types.insert(syncable::SESSIONS);
+
   bool sync_apps;
   if (!result->GetBoolean("syncApps", &sync_apps))
     return false;
diff --git a/chrome/browser/sync/sync_setup_wizard.cc b/chrome/browser/sync/sync_setup_wizard.cc
index 341e42d..147e112 100644
--- a/chrome/browser/sync/sync_setup_wizard.cc
+++ b/chrome/browser/sync/sync_setup_wizard.cc
@@ -115,7 +115,6 @@ void SyncResourcesSource::StartDataRequest(const std::string& path_raw,
     AddString(dict, "success", IDS_SYNC_SUCCESS);
     AddString(dict, "errorsigningin", IDS_SYNC_ERROR_SIGNING_IN);
     AddString(dict, "captchainstructions", IDS_SYNC_GAIA_CAPTCHA_INSTRUCTIONS);
-
     AddString(dict, "invalidaccesscode", IDS_SYNC_INVALID_ACCESS_CODE_LABEL);
     AddString(dict, "enteraccesscode", IDS_SYNC_ENTER_ACCESS_CODE_LABEL);
     AddString(dict, "getaccesscodehelp", IDS_SYNC_ACCESS_CODE_HELP_LABEL);
@@ -141,6 +140,7 @@ void SyncResourcesSource::StartDataRequest(const std::string& path_raw,
     AddString(dict, "extensions", IDS_SYNC_DATATYPE_EXTENSIONS);
     AddString(dict, "typedurls", IDS_SYNC_DATATYPE_TYPED_URLS);
     AddString(dict, "apps", IDS_SYNC_DATATYPE_APPS);
+    AddString(dict, "foreignsessions", IDS_SYNC_DATATYPE_SESSIONS);
     AddString(dict, "synczerodatatypeserror", IDS_SYNC_ZERO_DATA_TYPES_ERROR);
     AddString(dict, "abortederror", IDS_SYNC_SETUP_ABORTED_BY_PENDING_CLEAR);
 
diff --git a/chrome/common/notification_type.h b/chrome/common/notification_type.h
index 1ba2c39..735de96 100644
--- a/chrome/common/notification_type.h
+++ b/chrome/common/notification_type.h
@@ -1064,10 +1064,9 @@ class NotificationType {
     // session data.
     FOREIGN_SESSION_UPDATED,
 
-    // A foreign session has been deleted.  If a new tab page is open, the
-    // foreign session handler needs to update the new tab page's foreign
+    // Foreign sessions has been disabled. New tabs should not display foreign
     // session data.
-    FOREIGN_SESSION_DELETED,
+    FOREIGN_SESSION_DISABLED,
 
     // The syncer requires a passphrase to decrypt sensitive updates. This
     // notification is sent when the first sensitive data type is setup by the