From c8e93d974280cf11df1b6fe8ba5def48ba376cd2 Mon Sep 17 00:00:00 2001
From: Michael Lehenbauer <mikelehen@google.com>
Date: Fri, 26 Jan 2018 12:02:35 -0800
Subject: [PATCH] Fix b/72502745: OnlineState changes cause limbo document
 crash.

Context: I made a previous change to raise isFromCache=true events when the
client goes offline. As part of this change I added an assert for
"OnlineState should not affect limbo documents", which it turns out was not
valid because:
* When we go offline, we set the view to non-current and call
  View.applyChanges().
* View.applyChanges() calls applyTargetChange() even though there's no target
  change and it recalculates limbo documents.
* When the view is not current, we consider no documents to be in limbo.
* Therefore all limbo documents are removed and so applyChanges() ends up
  returning unexpected LimboDocumentChanges.

Fix: I've modified the View logic so that we don't recalculate limbo documents
(and generate LimboDocumentChanges) when the View is not current, so now my
assert holds and there should be less spurious removal / re-adding of limbo
documents.
---
 packages/firestore/src/core/view.ts           | 27 +++++++++-------
 .../test/unit/specs/offline_spec.test.ts      | 31 +++++++++++++++++++
 2 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/packages/firestore/src/core/view.ts b/packages/firestore/src/core/view.ts
index 225db0ee139..8da76d74faa 100644
--- a/packages/firestore/src/core/view.ts
+++ b/packages/firestore/src/core/view.ts
@@ -244,7 +244,8 @@ export class View {
       );
     });
 
-    const limboChanges = this.applyTargetChange(targetChange);
+    this.applyTargetChange(targetChange);
+    const limboChanges = this.updateLimboDocuments();
     const synced = this.limboDocuments.size === 0 && this.current;
     const newSyncState = synced ? SyncState.Synced : SyncState.Local;
     const syncStateChanged = newSyncState !== this.syncState;
@@ -320,9 +321,7 @@ export class View {
    * Updates syncedDocuments, current, and limbo docs based on the given change.
    * Returns the list of changes to which docs are in limbo.
    */
-  private applyTargetChange(
-    targetChange?: TargetChange
-  ): LimboDocumentChange[] {
+  private applyTargetChange(targetChange?: TargetChange): void {
     if (targetChange) {
       const targetMapping = targetChange.mapping;
       if (targetMapping instanceof ResetMapping) {
@@ -348,19 +347,23 @@ export class View {
           );
       }
     }
+  }
+
+  private updateLimboDocuments(): LimboDocumentChange[] {
+    // We can only determine limbo documents when we're in-sync with the server.
+    if (!this.current) {
+      return [];
+    }
 
-    // Recompute the set of limbo docs.
     // TODO(klimt): Do this incrementally so that it's not quadratic when
     // updating many documents.
     const oldLimboDocuments = this.limboDocuments;
     this.limboDocuments = documentKeySet();
-    if (this.current) {
-      this.documentSet.forEach(doc => {
-        if (this.shouldBeInLimbo(doc.key)) {
-          this.limboDocuments = this.limboDocuments.add(doc.key);
-        }
-      });
-    }
+    this.documentSet.forEach(doc => {
+      if (this.shouldBeInLimbo(doc.key)) {
+        this.limboDocuments = this.limboDocuments.add(doc.key);
+      }
+    });
 
     // Diff the new limbo docs with the old limbo docs.
     const changes: LimboDocumentChange[] = [];
diff --git a/packages/firestore/test/unit/specs/offline_spec.test.ts b/packages/firestore/test/unit/specs/offline_spec.test.ts
index 00e34bb1f4f..d832400e034 100644
--- a/packages/firestore/test/unit/specs/offline_spec.test.ts
+++ b/packages/firestore/test/unit/specs/offline_spec.test.ts
@@ -115,4 +115,35 @@ describeSpec('Offline:', [], () => {
         .expectEvents(query, { fromCache: false })
     );
   });
+
+  specTest('Queries with limbo documents handle going offline.', [], () => {
+    const query = Query.atPath(path('collection'));
+    const docA = doc('collection/a', 1000, { key: 'a' });
+    const docB = doc('collection/b', 1005, { key: 'b' });
+    const limboQuery = Query.atPath(docA.key.path);
+    return (
+      spec()
+        .userListens(query)
+        .watchAcksFull(query, 1000, docA)
+        .expectEvents(query, { added: [docA] })
+        .watchResets(query)
+        // No more documents
+        .watchCurrents(query, 'resume-token-1001')
+        .watchSnapshots(1001)
+        // docA will now be in limbo (triggering fromCache=true)
+        .expectLimboDocs(docA.key)
+        .expectEvents(query, { fromCache: true })
+        // first error triggers unknown state
+        .watchStreamCloses(Code.UNAVAILABLE)
+        .restoreListen(query, 'resume-token-1001')
+        // getting two more errors triggers offline state.
+        .watchStreamCloses(Code.UNAVAILABLE)
+        .watchStreamCloses(Code.UNAVAILABLE)
+        .watchAcksFull(query, 1001)
+        .watchAcksFull(limboQuery, 1001)
+        // Limbo document is resolved. No longer from cache.
+        .expectEvents(query, { removed: [docA], fromCache: false })
+        .expectLimboDocs()
+    );
+  });
 });