Don't crash if Target cannot be persisted

schmidt-sebastian · schmidt-sebastian · commit a72091ce415c · 2020-04-20T18:41:07.000-07:00
diff --git a/packages/firestore/.idea/runConfigurations/Unit_Tests.xml b/packages/firestore/.idea/runConfigurations/Unit_Tests.xml
diff --git a/packages/firestore/CHANGELOG.md b/packages/firestore/CHANGELOG.md
@@ -1,4 +1,6 @@
 # Unreleased
+- [fixed] Firestore now rejects `onSnapshot()` listeners if they cannot be
+  registered in IndexedDB. Previously, these errors crashed the client.
 - [fixed] Firestore now rejects write operations if they cannot be persisted
   in IndexedDB. Previously, these errors crashed the client.
 - [fixed] Fixed a source of IndexedDB-related crashes for tabs that receive 
diff --git a/packages/firestore/src/core/event_manager.ts b/packages/firestore/src/core/event_manager.ts
@@ -21,14 +21,18 @@ import { ObjectMap } from '../util/obj_map';
 import { Query } from './query';
 import { SyncEngine, SyncEngineListener } from './sync_engine';
 import { OnlineState, TargetId } from './types';
-import { DocumentViewChange, ChangeType, ViewSnapshot } from './view_snapshot';
+import { ChangeType, DocumentViewChange, ViewSnapshot } from './view_snapshot';
+import { logError } from '../util/log';
+import { Code, FirestoreError } from '../util/error';
+
+const LOG_TAG = 'EventManager';
 
 /**
  * Holds the listeners and the last received ViewSnapshot for a query being
  * tracked by EventManager.
  */
 class QueryListenersInfo {
-  viewSnap: ViewSnapshot | null = null;
+  viewSnap: ViewSnapshot | undefined = undefined;
   targetId: TargetId = 0;
   listeners: QueryListener[] = [];
 }
@@ -59,16 +63,37 @@ export class EventManager implements SyncEngineListener {
     this.syncEngine.subscribe(this);
   }
 
-  listen(listener: QueryListener): Promise<TargetId> {
+  async listen(listener: QueryListener): Promise<void> {
     const query = listener.query;
     let firstListen = false;
 
     let queryInfo = this.queries.get(query);
     if (!queryInfo) {
       firstListen = true;
       queryInfo = new QueryListenersInfo();
-      this.queries.set(query, queryInfo);
     }
+
+    if (firstListen) {
+      try {
+        const { targetId, snapshot } = await this.syncEngine.listen(query);
+        queryInfo.targetId = targetId;
+        queryInfo.viewSnap = snapshot;
+      } catch (e) {
+        if (e.name === 'IndexedDbTransactionError') {
+          logError(LOG_TAG, `Failed to register query: ${e}`);
+          listener.onError(
+            new FirestoreError(
+              Code.UNAVAILABLE,
+              'Failed to register query: ' + e
+            )
+          );
+          return;
+        }
+        throw e;
+      }
+    }
+
+    this.queries.set(query, queryInfo!);
     queryInfo.listeners.push(listener);
 
     // Run global snapshot listeners if a consistent snapshot has been emitted.
@@ -84,15 +109,6 @@ export class EventManager implements SyncEngineListener {
         this.raiseSnapshotsInSyncEvent();
       }
     }
-
-    if (firstListen) {
-      return this.syncEngine.listen(query).then(targetId => {
-        queryInfo!.targetId = targetId;
-        return targetId;
-      });
-    } else {
-      return Promise.resolve(queryInfo.targetId);
-    }
   }
 
   async unlisten(listener: QueryListener): Promise<void> {
diff --git a/packages/firestore/src/core/sync_engine.ts b/packages/firestore/src/core/sync_engine.ts
@@ -102,6 +102,12 @@ class QueryView {
   ) {}
 }
 
+/** Result type returned during initial query registration. */
+export interface QueryRegistration {
+  targetId: TargetId;
+  snapshot?: ViewSnapshot;
+}
+
 /** Tracks a limbo resolution. */
 class LimboResolution {
   constructor(public key: DocumentKey) {}
@@ -216,7 +222,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
    * server. All the subsequent view snapshots or errors are sent to the
    * subscribed handlers. Returns the targetId of the query.
    */
-  async listen(query: Query): Promise<TargetId> {
+  async listen(query: Query): Promise<QueryRegistration> {
     this.assertSubscribed('listen()');
 
     let targetId;
@@ -250,8 +256,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
       }
     }
 
-    this.syncEngineListener!.onWatchChange([viewSnapshot]);
-    return targetId;
+    return { targetId, snapshot: viewSnapshot };
   }
 
   /**
diff --git a/packages/firestore/test/unit/specs/recovery_spec.test.ts b/packages/firestore/test/unit/specs/recovery_spec.test.ts
@@ -19,6 +19,7 @@ import { describeSpec, specTest } from './describe_spec';
 import { client, spec } from './spec_builder';
 import { TimerId } from '../../../src/util/async_queue';
 import { Query } from '../../../src/core/query';
+import { Code } from '../../../src/util/error';
 import { doc, path } from '../../util/helpers';
 
 describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
@@ -162,4 +163,37 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
       .watchAcksFull(query, 2, doc1, doc3)
       .expectEvents(query, { metadata: [doc1, doc3] });
   });
+
+  specTest('Fails targets that cannot be allocated', [], () => {
+    const query1 = Query.atPath(path('collection1'));
+    const query2 = Query.atPath(path('collection2'));
+    const query3 = Query.atPath(path('collection3'));
+    return spec()
+      .userListens(query1)
+      .watchAcksFull(query1, 1)
+      .expectEvents(query1, {})
+      .failDatabase()
+      .userListens(query2)
+      .expectEvents(query2, { errorCode: Code.UNKNOWN })
+      .recoverDatabase()
+      .userListens(query3)
+      .watchAcksFull(query3, 1)
+      .expectEvents(query3, {});
+  });
+
+  specTest('Can re-add failed target', [], () => {
+    const query1 = Query.atPath(path('collection1'));
+    const query2 = Query.atPath(path('collection2'));
+    return spec()
+      .userListens(query1)
+      .watchAcksFull(query1, 1)
+      .expectEvents(query1, {})
+      .failDatabase()
+      .userListens(query2)
+      .expectEvents(query2, { errorCode: Code.UNKNOWN })
+      .recoverDatabase()
+      .userListens(query2)
+      .watchAcksFull(query2, 1)
+      .expectEvents(query2, {});
+  });
 });
diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts
@@ -89,6 +89,7 @@ export class ClientMemoryState {
   limboMapping: LimboMap = {};
 
   limboIdGenerator: TargetIdGenerator = TargetIdGenerator.forSyncEngine();
+  injectFailures = false;
 
   constructor() {
     this.reset();
@@ -191,6 +192,14 @@ export class SpecBuilder {
     return this.clientState.activeTargets;
   }
 
+  private get injectFailures(): boolean {
+    return this.clientState.injectFailures;
+  }
+
+  private set injectFailures(injectFailures: boolean) {
+    this.clientState.injectFailures = injectFailures;
+  }
+
   /**
    * Exports the spec steps as a JSON object that be used in the spec runner.
    */
@@ -232,18 +241,25 @@ export class SpecBuilder {
 
     const target = query.toTarget();
     let targetId: TargetId = 0;
-    if (this.queryMapping.has(target)) {
-      targetId = this.queryMapping.get(target)!;
+
+    if (this.injectFailures) {
+      this.currentStep = {
+        userListen: [targetId, SpecBuilder.queryToSpec(query)]
+      };
     } else {
-      targetId = this.queryIdGenerator.next(target);
-    }
+      if (this.queryMapping.has(target)) {
+        targetId = this.queryMapping.get(target)!;
+      } else {
+        targetId = this.queryIdGenerator.next(target);
+      }
 
-    this.queryMapping.set(target, targetId);
-    this.addQueryToActiveTargets(targetId, query, resumeToken);
-    this.currentStep = {
-      userListen: [targetId, SpecBuilder.queryToSpec(query)],
-      expectedState: { activeTargets: { ...this.activeTargets } }
-    };
+      this.queryMapping.set(target, targetId);
+      this.addQueryToActiveTargets(targetId, query, resumeToken);
+      this.currentStep = {
+        userListen: [targetId, SpecBuilder.queryToSpec(query)],
+        expectedState: { activeTargets: { ...this.activeTargets } }
+      };
+    }
     return this;
   }
 
@@ -421,6 +437,7 @@ export class SpecBuilder {
   /** Fails all database operations until `recoverDatabase()` is called. */
   failDatabase(): this {
     this.nextStep();
+    this.injectFailures = true;
     this.currentStep = {
       failDatabase: true
     };
@@ -430,6 +447,7 @@ export class SpecBuilder {
   /** Stops failing database operations. */
   recoverDatabase(): this {
     this.nextStep();
+    this.injectFailures = false;
     this.currentStep = {
       failDatabase: false
     };
diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts
@@ -597,7 +597,6 @@ abstract class TestRunner {
   }
 
   private async doListen(listenSpec: SpecUserListen): Promise<void> {
-    const expectedTargetId = listenSpec[0];
     const querySpec = listenSpec[1];
     const query = parseQuery(querySpec);
     const aggregator = new EventAggregator(query, this.pushEvent.bind(this));
@@ -609,14 +608,15 @@ abstract class TestRunner {
     const queryListener = new QueryListener(query, aggregator, options);
     this.queryListeners.set(query, queryListener);
 
-    await this.queue.enqueue(async () => {
-      const targetId = await this.eventManager.listen(queryListener);
-      expect(targetId).to.equal(
-        expectedTargetId,
-        'targetId assigned to listen'
-      );
+    await this.queue.enqueue(() => {
+      return this.eventManager.listen(queryListener);
     });
 
+    if (this.persistence.injectFailures) {
+      // The Watch stream won't open if we are injecting failures.
+      return;
+    }
+
     // Skip the backoff that may have been triggered by a previous call to
     // `watchStreamCloses()`.
     if (
