Don't crash if write cannot be persisted

packages/firestore/CHANGELOG.md

@@ -1,4 +1,6 @@
 # Unreleased
+- [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 
   multi-tab notifications while the file system is locked.
 

packages/firestore/src/core/sync_engine.ts

@@ -16,7 +16,11 @@
  */
 
 import { User } from '../auth/user';
-import { ignoreIfPrimaryLeaseLoss, LocalStore } from '../local/local_store';
+import {
+  ignoreIfPrimaryLeaseLoss,
+  LocalStore,
+  LocalWriteResult
+} from '../local/local_store';
 import { LocalViewChanges } from '../local/local_view_changes';
 import { ReferenceSet } from '../local/reference_set';
 import { TargetData, TargetPurpose } from '../local/target_data';
@@ -34,7 +38,7 @@ import { RemoteStore } from '../remote/remote_store';
 import { RemoteSyncer } from '../remote/remote_syncer';
 import { debugAssert, fail, hardAssert } from '../util/assert';
 import { Code, FirestoreError } from '../util/error';
-import { logDebug } from '../util/log';
+import { logDebug, logError } from '../util/log';
 import { primitiveComparator } from '../util/misc';
 import { ObjectMap } from '../util/obj_map';
 import { Deferred } from '../util/promise';
@@ -373,7 +377,26 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
    */
   async write(batch: Mutation[], userCallback: Deferred<void>): Promise<void> {
     this.assertSubscribed('write()');
-    const result = await this.localStore.localWrite(batch);
+
+    let result: LocalWriteResult;
+    try {
+      result = await this.localStore.localWrite(batch);
+    } catch (e) {
+      // If we can't persist the mutation, we reject the user callback and don't
+      // send the mutation. The user can then retry the write. We currently do
+      // not use `enqueueRetryable()` for writes since this would require us to
+      // move all view computation logic to `enqueueRetryable()`. Otherwise,
+      // this write should be surfaced in all subsequent operations.
+      logError(LOG_TAG, 'Dropping write that cannot be persisted: ' + e);
+      userCallback.reject(
+        new FirestoreError(
+          Code.UNKNOWN,
+          'Query allocation failed with environment error: ' + e
+        )
+      );
+      return;
+    }
+
     this.sharedClientState.addPendingMutation(result.batchId);
     this.addMutationCallback(result.batchId, userCallback);
     await this.emitNewSnapsAndNotifyLocalStore(result.changes);

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

@@ -16,10 +16,10 @@
  */
 
 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 { doc, path } from '../../util/helpers';
 
 describeSpec(
   'Persistence Recovery',
@@ -42,7 +42,7 @@ describeSpec(
             // 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 
+            // make sure that it also does not get processed until
             // `recoverDatabase` is called.
             .runTimer(TimerId.AsyncQueueRetry)
             .recoverDatabase()
@@ -75,5 +75,61 @@ describeSpec(
           .expectEvents(query, {});
       }
     );
+
+    specTest('Recovers when write cannot be persisted', [], () => {
+      return spec()
+        .userSets('collection/key1', { foo: 'a' })
+        .expectNumOutstandingWrites(1)
+        .failDatabase()
+        .userSets('collection/key2', { bar: 'b' })
+        .expectUserCallbacks({ rejected: ['collection/key2'] })
+        .recoverDatabase()
+        .expectNumOutstandingWrites(1)
+        .userSets('collection/key3', { baz: 'c' })
+        .expectNumOutstandingWrites(2)
+        .writeAcks('collection/key1', 1)
+        .writeAcks('collection/key3', 2)
+        .expectNumOutstandingWrites(0);
+    });
+
+    specTest('Does not surface non-persisted writes', [], () => {
+      const query = Query.atPath(path('collection'));
+      const doc1Local = doc(
+        'collection/key1',
+        0,
+        { foo: 'a' },
+        { hasLocalMutations: true }
+      );
+      const doc1 = doc('collection/key1', 1, { foo: 'a' });
+      const doc3Local = doc(
+        'collection/key3',
+        0,
+        { foo: 'c' },
+        { hasLocalMutations: true }
+      );
+      const doc3 = doc('collection/key3', 2, { foo: 'c' });
+      return spec()
+        .userListens(query)
+        .userSets('collection/key1', { foo: 'a' })
+        .expectEvents(query, {
+          added: [doc1Local],
+          fromCache: true,
+          hasPendingWrites: true
+        })
+        .failDatabase()
+        .userSets('collection/key2', { foo: 'b' })
+        .expectUserCallbacks({ rejected: ['collection/key2'] })
+        .recoverDatabase()
+        .userSets('collection/key3', { foo: 'c' })
+        .expectEvents(query, {
+          added: [doc3Local],
+          fromCache: true,
+          hasPendingWrites: true
+        })
+        .writeAcks('collection/key1', 1)
+        .writeAcks('collection/key3', 2)
+        .watchAcksFull(query, 2, doc1, doc3)
+        .expectEvents(query, { metadata: [doc1, doc3] });
+    });
   }
 );

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

@@ -687,7 +687,9 @@ abstract class TestRunner {
       () => this.rejectedDocs.push(...documentKeys)
     );
 
-    this.sharedWrites.push(mutations);
+    if (!this.persistence.injectFailures) {
+      this.sharedWrites.push(mutations);
+    }
 
     return this.queue.enqueue(() => {
       return this.syncEngine.write(mutations, syncEngineCallback);