USS: Resubmit local session after remote deletion

In the unlikely event that a remote instance deletes our local session,
either via garbage collection or by the user manually requesting a
deletion from the UI, restore a consistent state once a local change
is observed.

This mimics the analogous logic in SessionsSyncManager.

Bug: 681921
Change-Id: Ie3171852abb9a9f1b350535317b73a3fcb907cb6
Reviewed-on: https://chromium-review.googlesource.com/1024036
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552900}

components/sync_sessions/session_sync_bridge.cc

@@ -257,6 +257,7 @@ base::Optional<syncer::ModelError> SessionSyncBridge::ApplySyncChanges(
           // error or a clock is inaccurate). Just ignore the deletion for now.
           DLOG(WARNING) << "Local session data deleted. Ignoring until next "
                         << "local navigation event.";
+          syncing_->local_data_out_of_sync = true;
         } else {
           batch->DeleteForeignEntityAndUpdateTracker(change.storage_key());
         }
@@ -265,19 +266,20 @@ base::Optional<syncer::ModelError> SessionSyncBridge::ApplySyncChanges(
       case syncer::EntityChange::ACTION_UPDATE: {
         const SessionSpecifics& specifics = change.data().specifics.session();
 
-        if (!SessionStore::AreValidSpecifics(specifics) ||
-            change.data().client_tag_hash !=
-                GenerateSyncableHash(syncer::SESSIONS,
-                                     SessionStore::GetClientTag(specifics))) {
-          continue;
-        }
-
         if (syncing_->store->StorageKeyMatchesLocalSession(
                 change.storage_key())) {
           // We should only ever receive a change to our own machine's session
           // info if encryption was turned on. In that case, the data is still
           // the same, so we can ignore.
           DLOG(WARNING) << "Dropping modification to local session.";
+          syncing_->local_data_out_of_sync = true;
+          continue;
+        }
+
+        if (!SessionStore::AreValidSpecifics(specifics) ||
+            change.data().client_tag_hash !=
+                GenerateSyncableHash(syncer::SESSIONS,
+                                     SessionStore::GetClientTag(specifics))) {
           continue;
         }
 
@@ -341,6 +343,19 @@ SessionSyncBridge::ApplyDisableSyncChanges(
 std::unique_ptr<LocalSessionEventHandlerImpl::WriteBatch>
 SessionSyncBridge::CreateLocalSessionWriteBatch() {
   DCHECK(syncing_);
+
+  // If a remote client mangled with our local session (typically deleted
+  // entities due to garbage collection), we resubmit all local entities at this
+  // point (i.e. local changes observed).
+  if (syncing_->local_data_out_of_sync) {
+    syncing_->local_data_out_of_sync = false;
+    // We use PostTask() to avoid interferring with the ongoing handling of
+    // local changes that triggered this function.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(&SessionSyncBridge::ResubmitLocalSession,
+                                  base::AsWeakPtr(this)));
+  }
+
   return std::make_unique<LocalSessionWriteBatch>(
       syncing_->store->local_session_info(), CreateSessionStoreWriteBatch(),
       change_processor());
@@ -473,10 +488,32 @@ void SessionSyncBridge::DeleteForeignSessionWithBatch(
 std::unique_ptr<SessionStore::WriteBatch>
 SessionSyncBridge::CreateSessionStoreWriteBatch() {
   DCHECK(syncing_);
+
   return syncing_->store->CreateWriteBatch(
       base::BindOnce(&SessionSyncBridge::ReportError, base::AsWeakPtr(this)));
 }
 
+void SessionSyncBridge::ResubmitLocalSession() {
+  if (!syncing_) {
+    return;
+  }
+
+  std::unique_ptr<SessionStore::WriteBatch> write_batch =
+      CreateSessionStoreWriteBatch();
+  std::unique_ptr<syncer::DataBatch> read_batch =
+      syncing_->store->GetAllSessionData();
+  while (read_batch->HasNext()) {
+    syncer::KeyAndData key_and_data = read_batch->Next();
+    if (syncing_->store->StorageKeyMatchesLocalSession(key_and_data.first)) {
+      change_processor()->Put(key_and_data.first,
+                              std::move(key_and_data.second),
+                              write_batch->GetMetadataChangeList());
+    }
+  }
+
+  SessionStore::WriteBatch::Commit(std::move(write_batch));
+}
+
 void SessionSyncBridge::ReportError(const syncer::ModelError& error) {
   change_processor()->ReportError(error);
 }

components/sync_sessions/session_sync_bridge.h

@@ -98,6 +98,7 @@ class SessionSyncBridge : public AbstractSessionsSyncManager,
   std::unique_ptr<SessionStore::WriteBatch> CreateSessionStoreWriteBatch();
   void DeleteForeignSessionWithBatch(const std::string& session_tag,
                                      SessionStore::WriteBatch* batch);
+  void ResubmitLocalSession();
   void ReportError(const syncer::ModelError& error);
 
   SyncSessionsClient* const sessions_client_;
@@ -117,6 +118,15 @@ class SessionSyncBridge : public AbstractSessionsSyncManager,
     std::unique_ptr<SessionStore> store;
     std::unique_ptr<OpenTabsUIDelegateImpl> open_tabs_ui_delegate;
     std::unique_ptr<LocalSessionEventHandlerImpl> local_session_event_handler;
+
+    // Tracks whether our local representation of which sync nodes map to what
+    // tabs (belonging to the current local session) is inconsistent.  This can
+    // happen if a foreign client deems our session as "stale" and decides to
+    // delete it. Rather than respond by bullishly re-creating our nodes
+    // immediately, which could lead to ping-pong sequences, we give the benefit
+    // of the doubt and hold off until another local navigation occurs, which
+    // proves that we are still relevant.
+    bool local_data_out_of_sync = false;
   };
 
   base::Optional<SyncingState> syncing_;

components/sync_sessions/session_sync_bridge_unittest.cc

@@ -111,11 +111,15 @@ std::map<std::string, std::unique_ptr<EntityData>> BatchToEntityDataMap(
   return storage_key_to_data;
 }
 
-syncer::EntityDataPtr CreateTombstone(const std::string& client_tag) {
+syncer::UpdateResponseData CreateTombstone(const std::string& client_tag) {
   EntityData tombstone;
   tombstone.client_tag_hash =
       syncer::GenerateSyncableHash(syncer::SESSIONS, client_tag);
-  return tombstone.PassToPtr();
+
+  syncer::UpdateResponseData data;
+  data.entity = tombstone.PassToPtr();
+  data.response_version = 2;
+  return data;
 }
 
 syncer::CommitResponseData CreateSuccessResponse(
@@ -608,6 +612,64 @@ TEST_F(SessionSyncBridgeTest, ShouldUpdateIdsDuringRestore) {
                            kTabNodeId2, {"http://bar.com/"})))));
 }
 
+// Ensure that tabbed windows from a previous session are preserved if no
+// windows are present on startup.
+TEST_F(SessionSyncBridgeTest, ShouldRestoreTabbedDataIfNoWindowsDuringStartup) {
+  const int kWindowId = 1000001;
+  const int kTabNodeId = 0;
+
+  AddWindow(kWindowId);
+  TestSyncedTabDelegate* tab = AddTab(kWindowId, "http://foo.com/");
+
+  const std::string header_storage_key =
+      SessionStore::GetHeaderStorageKey(kLocalSessionTag);
+  const std::string tab_storage_key =
+      SessionStore::GetTabStorageKey(kLocalSessionTag, kTabNodeId);
+
+  InitializeBridge();
+  StartSyncing();
+
+  ASSERT_THAT(
+      GetAllData(),
+      UnorderedElementsAre(
+          Pair(header_storage_key,
+               EntityDataHasSpecifics(MatchesHeader(kLocalSessionTag, _, _))),
+          Pair(tab_storage_key,
+               EntityDataHasSpecifics(MatchesTab(
+                   kLocalSessionTag, _, _, kTabNodeId, {"http://foo.com/"})))));
+
+  ShutdownBridge();
+
+  // Start the bridge with no local windows/tabs.
+  ResetWindows();
+  InitializeBridge();
+  StartSyncing();
+
+  EXPECT_THAT(
+      GetAllData(),
+      UnorderedElementsAre(
+          Pair(header_storage_key,
+               EntityDataHasSpecifics(MatchesHeader(kLocalSessionTag, _, _))),
+          Pair(tab_storage_key,
+               EntityDataHasSpecifics(MatchesTab(
+                   kLocalSessionTag, _, _, kTabNodeId, {"http://foo.com/"})))));
+
+  // Now actually resurrect the native data, which will end up having different
+  // native ids, but the tab has the same sync id as before.
+  EXPECT_CALL(
+      mock_processor(),
+      DoPut(header_storage_key,
+            EntityDataHasSpecifics(MatchesHeader(kLocalSessionTag, _, _)), _));
+  EXPECT_CALL(mock_processor(),
+              DoPut(tab_storage_key,
+                    EntityDataHasSpecifics(MatchesTab(
+                        kLocalSessionTag, /*window_id=*/_, /*tab_id=*/_,
+                        kTabNodeId, {"http://foo.com/", "http://bar.com/"})),
+                    _));
+  AddWindow(kWindowId)->OverrideTabAt(0, tab);
+  tab->Navigate("http://bar.com/");
+}
+
 TEST_F(SessionSyncBridgeTest, ShouldDisableSyncAndReenable) {
   const int kWindowId = 1000001;
   const int kTabId = 1000002;
@@ -706,12 +768,10 @@ TEST_F(SessionSyncBridgeTest, ShouldHandleRemoteDeletion) {
   // Mimic receiving a remote deletion of the foreign session.
   sync_pb::ModelTypeState state;
   state.set_initial_sync_done(true);
-  syncer::UpdateResponseData deletion;
-  deletion.entity = CreateTombstone(SessionStore::GetClientTag(foreign_header));
-  deletion.response_version = 2;
 
   EXPECT_CALL(mock_foreign_sessions_updated_callback(), Run());
-  real_processor()->OnUpdateReceived(state, {deletion});
+  real_processor()->OnUpdateReceived(
+      state, {CreateTombstone(SessionStore::GetClientTag(foreign_header))});
 
   foreign_session_tab = nullptr;
   EXPECT_FALSE(bridge()->GetOpenTabsUIDelegate()->GetForeignTab(
@@ -723,23 +783,31 @@ TEST_F(SessionSyncBridgeTest, ShouldHandleRemoteDeletion) {
 }
 
 TEST_F(SessionSyncBridgeTest, ShouldIgnoreRemoteDeletionOfLocalTab) {
-  const int kWindowId = 1000001;
-  const int kTabId = 1000002;
-  const int kTabNodeId = 0;
+  const int kWindowId1 = 1000001;
+  const int kTabId1 = 1000002;
+  const int kTabNodeId1 = 0;
 
-  AddWindow(kWindowId);
-  AddTab(kWindowId, "http://foo.com/", kTabId);
+  AddWindow(kWindowId1);
+  AddTab(kWindowId1, "http://foo.com/", kTabId1);
 
   InitializeBridge();
   StartSyncing();
 
+  const std::string header_storage_key =
+      SessionStore::GetHeaderStorageKey(kLocalSessionTag);
+  const std::string tab_storage_key1 =
+      SessionStore::GetTabStorageKey(kLocalSessionTag, kTabNodeId1);
+  const std::string tab_client_tag1 =
+      SessionStore::GetTabClientTagForTest(kLocalSessionTag, kTabNodeId1);
+
   ASSERT_THAT(
       GetAllData(),
-      UnorderedElementsAre(Pair(_, EntityDataHasSpecifics(
-                                       MatchesHeader(kLocalSessionTag, _, _))),
-                           Pair(_, EntityDataHasSpecifics(MatchesTab(
-                                       kLocalSessionTag, kWindowId, kTabId,
-                                       kTabNodeId, {"http://foo.com/"})))));
+      UnorderedElementsAre(
+          Pair(header_storage_key,
+               EntityDataHasSpecifics(MatchesHeader(kLocalSessionTag, _, _))),
+          Pair(tab_storage_key1, EntityDataHasSpecifics(MatchesTab(
+                                     kLocalSessionTag, kWindowId1, kTabId1,
+                                     kTabNodeId1, {"http://foo.com/"})))));
   ASSERT_TRUE(real_processor()->IsTrackingMetadata());
   ASSERT_TRUE(real_processor()->HasLocalChangesForTest());
 
@@ -748,33 +816,81 @@ TEST_F(SessionSyncBridgeTest, ShouldIgnoreRemoteDeletionOfLocalTab) {
   sync_pb::ModelTypeState state;
   state.set_initial_sync_done(true);
   real_processor()->OnCommitCompleted(
-      state, {CreateSuccessResponse(SessionStore::GetTabClientTagForTest(
-                  kLocalSessionTag, kTabNodeId)),
+      state, {CreateSuccessResponse(tab_client_tag1),
               CreateSuccessResponse(kLocalSessionTag)});
   ASSERT_FALSE(real_processor()->HasLocalChangesForTest());
 
-  // Mimic receiving a remote deletion of a local tab.
-  syncer::UpdateResponseData deletion;
-  deletion.entity = CreateTombstone(
-      SessionStore::GetTabClientTagForTest(kLocalSessionTag, kTabNodeId));
-  deletion.response_version = 2;
-  real_processor()->OnUpdateReceived(state, {deletion});
+  // Mimic receiving a remote deletion of both entities.
+  EXPECT_CALL(mock_processor(), DoPut(_, _, _)).Times(0);
+  real_processor()->OnUpdateReceived(state, {CreateTombstone(kLocalSessionTag),
+                                             CreateTombstone(tab_client_tag1)});
 
-  // State should remain unchanged (deletion ignored).
+  // State should remain unchanged (deletions ignored).
   EXPECT_THAT(
       GetAllData(),
-      UnorderedElementsAre(Pair(_, EntityDataHasSpecifics(
-                                       MatchesHeader(kLocalSessionTag, _, _))),
-                           Pair(_, EntityDataHasSpecifics(MatchesTab(
-                                       kLocalSessionTag, kWindowId, kTabId,
-                                       kTabNodeId, {"http://foo.com/"})))));
+      UnorderedElementsAre(
+          Pair(header_storage_key,
+               EntityDataHasSpecifics(MatchesHeader(kLocalSessionTag, _, _))),
+          Pair(tab_storage_key1, EntityDataHasSpecifics(MatchesTab(
+                                     kLocalSessionTag, kWindowId1, kTabId1,
+                                     kTabNodeId1, {"http://foo.com/"})))));
 
-  // Mimic a browser restart.
-  ShutdownBridge();
-  InitializeBridge();
-  StartSyncing();
+  // Creating a new tab locally should trigger Put() calls for *all* entities
+  // (because the local data was out of sync).
+  const int kWindowId2 = 2000001;
+  const int kTabId2 = 2000002;
+  const int kTabNodeId2 = 1;
 
-  EXPECT_THAT(GetAllData(), SizeIs(2));
+  const std::string tab_storage_key2 =
+      SessionStore::GetTabStorageKey(kLocalSessionTag, kTabNodeId2);
+
+  // Window creation already triggers a header update, which will be overriden
+  // later below.
+  testing::Expectation put_transient_header =
+      EXPECT_CALL(mock_processor(), DoPut(header_storage_key, _, _));
+  AddWindow(kWindowId2);
+
+  // In the current implementation, some of the updates are reported to the
+  // processor twice, but that's OK because the processor can detect it.
+  EXPECT_CALL(mock_processor(),
+              DoPut(header_storage_key,
+                    EntityDataHasSpecifics(MatchesHeader(
+                        kLocalSessionTag, ElementsAre(kWindowId1, kWindowId2),
+                        ElementsAre(kTabId1, kTabId2))),
+                    _))
+      .Times(2)
+      .After(put_transient_header);
+  EXPECT_CALL(mock_processor(), DoPut(tab_storage_key1,
+                                      EntityDataHasSpecifics(MatchesTab(
+                                          kLocalSessionTag, kWindowId1, kTabId1,
+                                          kTabNodeId1, {"http://foo.com/"})),
+                                      _));
+  EXPECT_CALL(mock_processor(), DoPut(tab_storage_key2,
+                                      EntityDataHasSpecifics(MatchesTab(
+                                          kLocalSessionTag, kWindowId2, kTabId2,
+                                          kTabNodeId2, {"http://bar.com/"})),
+                                      _))
+      .Times(2);
+
+  AddTab(kWindowId2, "http://bar.com/", kTabId2);
+
+  EXPECT_THAT(
+      GetAllData(),
+      UnorderedElementsAre(
+          Pair(header_storage_key,
+               EntityDataHasSpecifics(MatchesHeader(
+                   kLocalSessionTag, ElementsAre(kWindowId1, kWindowId2),
+                   ElementsAre(kTabId1, kTabId2)))),
+          Pair(tab_storage_key1,
+               EntityDataHasSpecifics(
+                   MatchesTab(kLocalSessionTag, /*window_id=*/_, /*tab_id=*/_,
+                              kTabNodeId1, {"http://foo.com/"}))),
+          Pair(tab_storage_key2, EntityDataHasSpecifics(MatchesTab(
+                                     kLocalSessionTag, kWindowId2, kTabId2,
+                                     kTabNodeId2, {"http://bar.com/"})))));
+
+  // Run until idle because PostTask() is used to invoke ResubmitLocalSession().
+  base::RunLoop().RunUntilIdle();
 }
 
 // Verifies that a foreign session can be deleted by the user from the history