Don't crash if Target cannot be persisted

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 failures crashed the client.
 - [fixed] Fixed a source of IndexedDB-related crashes for tabs that receive 
   multi-tab notifications while the file system is locked.
 

packages/firestore/CHANGELOG.md

@@ -21,7 +21,15 @@ 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,
+  isIndexedDbBrowserException
+} from '../util/error';
+
+const LOG_TAG = 'EventManager';
 
 /**
  * Holds the listeners and the last received ViewSnapshot for a query being
@@ -59,15 +67,14 @@ 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);
     }
     queryInfo.listeners.push(listener);
 
@@ -86,13 +93,24 @@ export class EventManager implements SyncEngineListener {
     }
 
     if (firstListen) {
-      return this.syncEngine.listen(query).then(targetId => {
-        queryInfo!.targetId = targetId;
-        return targetId;
-      });
-    } else {
-      return Promise.resolve(queryInfo.targetId);
+      try {
+        queryInfo!.targetId = await this.syncEngine.listen(query);
+      } catch (e) {
+        if (isIndexedDbBrowserException(e)) {
+          logError(LOG_TAG, `Failed to allocate query target: ${e}`);
+          listener.onError(
+            new FirestoreError(
+              Code.UNKNOWN,
+              'Query allocation failed with environment error'
+            )
+          );
+          return Promise.resolve();
+        }
+        throw e;
+      }
     }
+
+    this.queries.set(query, queryInfo!);
   }
 
   async unlisten(listener: QueryListener): Promise<void> {

packages/firestore/src/core/event_manager.ts

@@ -16,7 +16,7 @@
  */
 
 import { debugAssert, fail } from './assert';
-import { Code, FirestoreError } from './error';
+import { Code, FirestoreError, isIndexedDbBrowserException } from './error';
 import { logDebug, logError } from './log';
 import { CancelablePromise, Deferred } from './promise';
 import { ExponentialBackoff } from '../remote/backoff';
@@ -338,13 +338,7 @@ export class AsyncQueue {
           deferred.resolve();
           this.backoff.reset();
         } catch (e) {
-          // We only retry errors that match the ones thrown by IndexedDB (see
-          // https://developer.mozilla.org/en-US/docs/Web/API/IDBTransaction/error)
-          if (
-            (typeof DOMException !== 'undefined' &&
-              e instanceof DOMException) ||
-            (typeof DOMError !== 'undefined' && e instanceof DOMError)
-          ) {
+          if (isIndexedDbBrowserException(e)) {
             logDebug(LOG_TAG, 'Operation failed with retryable error: ' + e);
             this.backoff.backoffAndRun(retryingOp);
           } else {

packages/firestore/src/util/async_queue.ts

@@ -1,6 +1,6 @@
 /**
  * @license
- * Copyright 2017 Google Inc.
+ * Copyright 2017 Google LLC
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -171,3 +171,18 @@ export class FirestoreError extends Error implements firestore.FirestoreError {
     this.toString = () => `${this.name}: [code=${this.code}]: ${this.message}`;
   }
 }
+
+/**
+ * Returns true if the provided error is a DOMException (as thrown by Chrome
+ * and Firefox for IndexedDB exceptions) or a DOMError (Safari).
+ *
+ * @see https://developer.mozilla.org/en-US/docs/Web/API/IDBTransaction/error
+ */
+export function isIndexedDbBrowserException(
+  e: DOMException | DOMError
+): boolean {
+  return (
+    (typeof DOMException !== 'undefined' && e instanceof DOMException) ||
+    (typeof DOMError !== 'undefined' && e instanceof DOMError)
+  );
+}

packages/firestore/src/util/error.ts

@@ -16,64 +16,94 @@
  */
 
 import { describeSpec, specTest } from './describe_spec';
-import { client } from './spec_builder';
+import { client, spec } from './spec_builder';
 import { TimerId } from '../../../src/util/async_queue';
 import { Query } from '../../../src/core/query';
 import { path } from '../../util/helpers';
+import { Code } from '../../../src/util/error';
 
-describeSpec(
-  'Persistence Recovery',
-  ['durable-persistence', 'no-ios', 'no-android'],
-  () => {
-    specTest(
-      'Write is acknowledged by primary client (with recovery)',
-      ['multi-client'],
-      () => {
-        return (
-          client(0)
-            .expectPrimaryState(true)
-            .client(1)
-            .expectPrimaryState(false)
-            .userSets('collection/a', { v: 1 })
-            .failDatabase()
-            .client(0)
-            .writeAcks('collection/a', 1, { expectUserCallback: false })
-            .client(1)
-            // Client 1 has received the WebStorage notification that the write
-            // has been acknowledged, but failed to process the change. Hence,
-            // we did not get a user callback. We schedule the first retry and
-            // make sure that it also does not get processed until
-            // `recoverDatabase` is called.
-            .runTimer(TimerId.AsyncQueueRetry)
-            .recoverDatabase()
-            .runTimer(TimerId.AsyncQueueRetry)
-            .expectUserCallbacks({
-              acknowledged: ['collection/a']
-            })
-        );
-      }
-    );
-
-    specTest(
-      'Query raises events in secondary client  (with recovery)',
-      ['multi-client'],
-      () => {
-        const query = Query.atPath(path('collection'));
-
-        return client(0)
+describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
+  specTest(
+    'Write is acknowledged by primary client (with recovery)',
+    ['multi-client'],
+    () => {
+      return (
+        client(0)
           .expectPrimaryState(true)
           .client(1)
           .expectPrimaryState(false)
-          .userListens(query)
+          .userSets('collection/a', { v: 1 })
           .failDatabase()
           .client(0)
-          .expectListen(query)
-          .watchAcksFull(query, 1000)
+          .writeAcks('collection/a', 1, { expectUserCallback: false })
           .client(1)
+          // Client 1 has received the WebStorage notification that the write
+          // has been acknowledged, but failed to process the change. Hence,
+          // we did not get a user callback. We schedule the first retry and
+          // make sure that it also does not get processed until
+          // `recoverDatabase` is called.
+          .runTimer(TimerId.AsyncQueueRetry)
           .recoverDatabase()
           .runTimer(TimerId.AsyncQueueRetry)
-          .expectEvents(query, {});
-      }
-    );
-  }
-);
+          .expectUserCallbacks({
+            acknowledged: ['collection/a']
+          })
+      );
+    }
+  );
+
+  specTest(
+    'Query raises events in secondary client  (with recovery)',
+    ['multi-client'],
+    () => {
+      const query = Query.atPath(path('collection'));
+
+      return client(0)
+        .expectPrimaryState(true)
+        .client(1)
+        .expectPrimaryState(false)
+        .userListens(query)
+        .failDatabase()
+        .client(0)
+        .expectListen(query)
+        .watchAcksFull(query, 1000)
+        .client(1)
+        .recoverDatabase()
+        .runTimer(TimerId.AsyncQueueRetry)
+        .expectEvents(query, {});
+    }
+  );
+
+  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, {});
+  });
+});

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,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
     };

packages/firestore/test/unit/specs/spec_builder.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 inject failures.
+      return;
+    }
+
     // Skip the backoff that may have been triggered by a previous call to
     // `watchStreamCloses()`.
     if (