Don't crash if Target cannot be persisted (#2944)

Author: schmidt-sebastian

packages/firestore/.idea/runConfigurations/Unit_Tests.xml

@@ -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 

packages/firestore/CHANGELOG.md

@@ -20,16 +20,19 @@ import { EventHandler } from '../util/misc';
 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 { OnlineState } from './types';
+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;
-  targetId: TargetId = 0;
+  viewSnap: ViewSnapshot | undefined = undefined;
   listeners: QueryListener[] = [];
 }
 
@@ -59,16 +62,32 @@ 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 {
+        queryInfo.viewSnap = await this.syncEngine.listen(query);
+      } catch (e) {
+        const msg = `Initialization of query '${query}' failed: ${e}`;
+        logError(LOG_TAG, msg);
+        if (e.name === 'IndexedDbTransactionError') {
+          listener.onError(new FirestoreError(Code.UNAVAILABLE, msg));
+        } else {
+          throw e;
+        }
+        return;
+      }
+    }
+
+    this.queries.set(query, queryInfo);
     queryInfo.listeners.push(listener);
 
     // Run global snapshot listeners if a consistent snapshot has been emitted.
@@ -84,15 +103,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> {

packages/firestore/src/core/event_manager.ts

@@ -393,9 +393,7 @@ export class FirestoreClient {
   ): QueryListener {
     this.verifyNotTerminated();
     const listener = new QueryListener(query, observer, options);
-    this.asyncQueue.enqueueAndForget(() => {
-      return this.eventMgr.listen(listener);
-    });
+    this.asyncQueue.enqueueAndForget(() => this.eventMgr.listen(listener));
     return listener;
   }
 

packages/firestore/src/core/firestore_client.ts

@@ -213,9 +213,9 @@ export class SyncEngine implements RemoteSyncer {
   /**
    * Initiates the new listen, resolves promise when listen enqueued to the
    * server. All the subsequent view snapshots or errors are sent to the
-   * subscribed handlers. Returns the targetId of the query.
+   * subscribed handlers. Returns the initial snapshot.
    */
-  async listen(query: Query): Promise<TargetId> {
+  async listen(query: Query): Promise<ViewSnapshot> {
     this.assertSubscribed('listen()');
 
     let targetId;
@@ -249,8 +249,7 @@ export class SyncEngine implements RemoteSyncer {
       }
     }
 
-    this.syncEngineListener!.onWatchChange([viewSnapshot]);
-    return targetId;
+    return viewSnapshot;
   }
 
   /**

packages/firestore/src/core/sync_engine.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.UNAVAILABLE })
+      .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.UNAVAILABLE })
+      .recoverDatabase()
+      .userListens(query2)
+      .watchAcksFull(query2, 1)
+      .expectEvents(query2, {});
+  });
 });

packages/firestore/test/unit/specs/recovery_spec.test.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,26 @@ export class SpecBuilder {
 
     const target = query.toTarget();
     let targetId: TargetId = 0;
-    if (this.queryMapping.has(target)) {
-      targetId = this.queryMapping.get(target)!;
+
+    if (this.injectFailures) {
+      // Return a `userListens()` step but don't advance the target IDs.
+      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 +438,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 +448,7 @@ export class SpecBuilder {
   /** Stops failing database operations. */
   recoverDatabase(): this {
     this.nextStep();
+    this.injectFailures = false;
     this.currentStep = {
       failDatabase: false
     };

packages/firestore/test/unit/specs/spec_builder.ts

@@ -48,8 +48,8 @@ import { Token } from '../../../src/api/credentials';
 import { Observer } from '../../../src/core/event_manager';
 import { ViewSnapshot } from '../../../src/core/view_snapshot';
 import { Query } from '../../../src/core/query';
-import { expectFirestoreError } from '../../util/helpers';
 import { Mutation } from '../../../src/model/mutation';
+import { expect } from 'chai';
 
 /**
  * A test-only MemoryPersistence implementation that is able to inject
@@ -367,7 +367,7 @@ export class EventAggregator implements Observer<ViewSnapshot> {
   }
 
   error(error: Error): void {
-    expectFirestoreError(error);
+    expect(error.name).to.equal('FirebaseError');
     this.pushEvent({ query: this.query, error: error as FirestoreError });
   }
 }

packages/firestore/test/unit/specs/spec_test_components.ts

@@ -76,7 +76,7 @@ import {
   deletedDoc,
   deleteMutation,
   doc,
-  expectFirestoreError,
+  validateFirestoreError,
   filter,
   key,
   orderBy,
@@ -350,10 +350,16 @@ abstract class TestRunner {
   }
 
   private async doListen(listenSpec: SpecUserListen): Promise<void> {
-    const expectedTargetId = listenSpec[0];
+    let targetFailed = false;
+
     const querySpec = listenSpec[1];
     const query = parseQuery(querySpec);
-    const aggregator = new EventAggregator(query, this.pushEvent.bind(this));
+    const aggregator = new EventAggregator(query, e => {
+      if (e.error) {
+        targetFailed = true;
+      }
+      this.pushEvent(e);
+    });
     // TODO(dimond): Allow customizing listen options in spec tests
     const options = {
       includeMetadataChanges: true,
@@ -362,27 +368,27 @@ 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(() => this.eventManager.listen(queryListener));
 
-    // Skip the backoff that may have been triggered by a previous call to
-    // `watchStreamCloses()`.
-    if (
-      this.queue.containsDelayedOperation(TimerId.ListenStreamConnectionBackoff)
-    ) {
-      await this.queue.runDelayedOperationsEarly(
-        TimerId.ListenStreamConnectionBackoff
-      );
-    }
+    if (targetFailed) {
+      expect(this.persistence.injectFailures).to.be.true;
+    } else {
+      // Skip the backoff that may have been triggered by a previous call to
+      // `watchStreamCloses()`.
+      if (
+        this.queue.containsDelayedOperation(
+          TimerId.ListenStreamConnectionBackoff
+        )
+      ) {
+        await this.queue.runDelayedOperationsEarly(
+          TimerId.ListenStreamConnectionBackoff
+        );
+      }
 
-    if (this.isPrimaryClient && this.networkEnabled) {
-      // Open should always have happened after a listen
-      await this.connection.waitForWatchOpen();
+      if (this.isPrimaryClient && this.networkEnabled) {
+        // Open should always have happened after a listen
+        await this.connection.waitForWatchOpen();
+      }
     }
   }
 
@@ -955,7 +961,10 @@ abstract class TestRunner {
     const expectedQuery = parseQuery(expected.query);
     expect(actual.query).to.deep.equal(expectedQuery);
     if (expected.errorCode) {
-      expectFirestoreError(actual.error!);
+      validateFirestoreError(
+        mapCodeFromRpcCode(expected.errorCode),
+        actual.error!
+      );
     } else {
       const expectedChanges: DocumentViewChange[] = [];
       if (expected.removed) {

packages/firestore/test/unit/specs/spec_test_runner.ts

@@ -92,6 +92,7 @@ import { ByteString } from '../../src/util/byte_string';
 import { PlatformSupport } from '../../src/platform/platform';
 import { JsonProtoSerializer } from '../../src/remote/serializer';
 import { Timestamp } from '../../src/api/timestamp';
+import { Code, FirestoreError } from '../../src/util/error';
 
 /* eslint-disable no-restricted-globals */
 
@@ -815,8 +816,12 @@ export function expectEqualitySets<T>(
   }
 }
 
-export function expectFirestoreError(err: Error): void {
-  expect(err.name).to.equal('FirebaseError');
+export function validateFirestoreError(
+  expectedCode: Code,
+  actualError: Error
+): void {
+  expect(actualError.name).to.equal('FirebaseError');
+  expect((actualError as FirestoreError).code).to.equal(expectedCode);
 }
 
 export function forEachNumber<V>(