From a7255a21558f0c35a4bf517033e32f6aacf859af Mon Sep 17 00:00:00 2001
From: wu-hui <53845758+wu-hui@users.noreply.github.com>
Date: Wed, 28 Sep 2022 14:37:30 -0400
Subject: [PATCH] Fix b/249494921: Secondary Tab time travels with document
 deletes (#6635)

---
 .../firestore/src/core/sync_engine_impl.ts    | 10 +++-
 .../test/unit/specs/listen_spec.test.ts       | 54 +++++++++++++++++++
 2 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/packages/firestore/src/core/sync_engine_impl.ts b/packages/firestore/src/core/sync_engine_impl.ts
index 339865549a7..67405db0b1b 100644
--- a/packages/firestore/src/core/sync_engine_impl.ts
+++ b/packages/firestore/src/core/sync_engine_impl.ts
@@ -1021,13 +1021,19 @@ export async function syncEngineEmitNewSnapsAndNotifyLocalStore(
       syncEngineImpl
         .applyDocChanges(queryView, changes, remoteEvent)
         .then(viewSnapshot => {
-          if (viewSnapshot) {
+          // If there are changes, or we are handling a global snapshot, notify
+          // secondary clients to update query state.
+          if (viewSnapshot || remoteEvent) {
             if (syncEngineImpl.isPrimaryClient) {
               syncEngineImpl.sharedClientState.updateQueryState(
                 queryView.targetId,
-                viewSnapshot.fromCache ? 'not-current' : 'current'
+                viewSnapshot?.fromCache ? 'not-current' : 'current'
               );
             }
+          }
+
+          // Update views if there are actual changes.
+          if (!!viewSnapshot) {
             newSnaps.push(viewSnapshot);
             const docChanges = LocalViewChanges.fromSnapshot(
               queryView.targetId,
diff --git a/packages/firestore/test/unit/specs/listen_spec.test.ts b/packages/firestore/test/unit/specs/listen_spec.test.ts
index 854266735cd..8ebf5d7cbc0 100644
--- a/packages/firestore/test/unit/specs/listen_spec.test.ts
+++ b/packages/firestore/test/unit/specs/listen_spec.test.ts
@@ -950,6 +950,60 @@ describeSpec('Listens:', [], () => {
     }
   );
 
+  // Reproduces b/249494921.
+  specTest(
+    'Secondary client advances query state with global snapshot from primary',
+    ['multi-client'],
+    () => {
+      const query1 = query('collection');
+      const docA = doc('collection/a', 1000, { key: '1' });
+      const docADeleted = deletedDoc('collection/a', 2000);
+      const docARecreated = doc('collection/a', 2000, {
+        key: '2'
+      }).setHasLocalMutations();
+
+      return (
+        client(0)
+          .becomeVisible()
+          .expectPrimaryState(true)
+          .userListens(query1)
+          .watchAcksFull(query1, 1000, docA)
+          .expectEvents(query1, { added: [docA] })
+          .userUnlistens(query1)
+          .watchRemoves(query1)
+          .client(1)
+          .userListens(query1)
+          .expectEvents(query1, { added: [docA], fromCache: true })
+          .client(0)
+          .expectListen(query1, { resumeToken: 'resume-token-1000' })
+          .watchAcksFull(query1, 1500, docA)
+          .client(1)
+          .expectEvents(query1, {})
+          .client(0)
+          .userDeletes('collection/a')
+          .client(1)
+          .expectEvents(query1, {
+            removed: [docA]
+          })
+          .client(0)
+          .writeAcks('collection/a', 2000)
+          .watchAcksFull(query1, 2000, docADeleted)
+          .client(1) // expects no event
+          .client(0)
+          .userSets('collection/a', { key: '2' })
+          .client(1)
+          // Without the fix for b/249494921, two snapshots will be raised: a first
+          // one as show below, and a second one with `docADeleted` because
+          // `client(1)` failed to advance its cursor in remote document cache, and
+          // read a stale document.
+          .expectEvents(query1, {
+            added: [docARecreated],
+            hasPendingWrites: true
+          })
+      );
+    }
+  );
+
   specTest(
     'Mirror queries from same secondary client',
     ['multi-client'],