Don't crash if write cannot be persisted (#2938)

schmidt-sebastian · web-flow · commit a36b51b4897f · 2020-04-20T18:28:04.000-07:00
diff --git a/packages/firestore/CHANGELOG.md b/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.
 
diff --git a/packages/firestore/src/core/sync_engine.ts b/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';
@@ -371,7 +375,24 @@ 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 (e.name === 'IndexedDbTransactionError') {
+        // 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.
+        logError(LOG_TAG, 'Dropping write that cannot be persisted: ' + e);
+        userCallback.reject(
+          new FirestoreError(Code.UNAVAILABLE, 'Failed to persist write: ' + e)
+        );
+        return;
+      } else {
+        throw e;
+      }
+    }
+
     this.sharedClientState.addPendingMutation(result.batchId);
     this.addMutationCallback(result.batchId, userCallback);
     await this.emitNewSnapsAndNotifyLocalStore(result.changes);
diff --git a/packages/firestore/test/integration/bootstrap.ts b/packages/firestore/test/integration/bootstrap.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.
@@ -26,7 +26,11 @@ import '../../index';
 
 // 'context()' definition requires additional dependency on webpack-env package.
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
-const testsContext = (require as any).context('.', true, /^((?!node).)*\.test$/);
+const testsContext = (require as any).context(
+  '.',
+  true,
+  /^((?!node).)*\.test$/
+);
 const browserTests = testsContext
   .keys()
   .filter((file: string) => !file.match(/([\/.])node([\/.])/));
diff --git a/packages/firestore/test/unit/bootstrap.ts b/packages/firestore/test/unit/bootstrap.ts
@@ -26,7 +26,11 @@ import '../../src/platform_browser/browser_init';
 
 // 'context()' definition requires additional dependency on webpack-env package.
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
-const testsContext = (require as any).context('.', true, /^((?!node).)*\.test$/);
+const testsContext = (require as any).context(
+  '.',
+  true,
+  /^((?!node).)*\.test$/
+);
 const browserTests = testsContext
   .keys()
   .filter((file: string) => !file.match(/([\/.])node([\/.])/));
diff --git a/packages/firestore/test/unit/remote/serializer.helper.ts b/packages/firestore/test/unit/remote/serializer.helper.ts
@@ -96,7 +96,7 @@ const protoJsReader = testUserDataReader(/* useProto3Json= */ false);
  */
 export function serializerTest(
   protobufJsVerifier: (jsonValue: api.Value) => void = () => {}
-) : void {
+): void {
   describe('Serializer', () => {
     const partition = new DatabaseId('p', 'd');
     const s = new JsonProtoSerializer(partition, { useProto3Json: false });
diff --git a/packages/firestore/test/unit/specs/recovery_spec.test.ts b/packages/firestore/test/unit/specs/recovery_spec.test.ts
@@ -16,98 +16,150 @@
  */
 
 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',
-  ['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']
-            })
-        );
-      }
-    );
+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)
+          .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'));
 
-    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, {});
+    }
+  );
 
-        return client(0)
+  specTest(
+    'Query is listened to by primary (with recovery)',
+    ['multi-client'],
+    () => {
+      const query = Query.atPath(path('collection'));
+
+      return (
+        client(0)
           .expectPrimaryState(true)
+          .failDatabase()
           .client(1)
-          .expectPrimaryState(false)
           .userListens(query)
-          .failDatabase()
           .client(0)
+          // The primary client 0 receives a WebStorage notification about the
+          // new query, but it cannot load the target from IndexedDB. The
+          // query will only be listened to once we recover the database.
+          .recoverDatabase()
+          .runTimer(TimerId.AsyncQueueRetry)
           .expectListen(query)
-          .watchAcksFull(query, 1000)
+          .failDatabase()
           .client(1)
+          .userUnlistens(query)
+          .client(0)
+          // The primary client 0 receives a notification that the query can
+          // be released, but it can only process the change after we recover
+          // the database.
+          .expectActiveTargets({ query })
           .recoverDatabase()
           .runTimer(TimerId.AsyncQueueRetry)
-          .expectEvents(query, {});
-      }
-    );
+          .expectActiveTargets()
+      );
+    }
+  );
 
-    specTest(
-      'Query is listened to by primary (with recovery)',
-      ['multi-client'],
-      () => {
-        const query = Query.atPath(path('collection'));
+  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);
+  });
 
-        return (
-          client(0)
-            .expectPrimaryState(true)
-            .failDatabase()
-            .client(1)
-            .userListens(query)
-            .client(0)
-            // The primary client 0 receives a WebStorage notification about the
-            // new query, but it cannot load the target from IndexedDB. The
-            // query will only be listened to once we recover the database.
-            .recoverDatabase()
-            .runTimer(TimerId.AsyncQueueRetry)
-            .expectListen(query)
-            .failDatabase()
-            .client(1)
-            .userUnlistens(query)
-            .client(0)
-            // The primary client 0 receives a notification that the query can
-            // be released, but it can only process the change after we recover
-            // the database.
-            .expectActiveTargets({ query })
-            .recoverDatabase()
-            .runTimer(TimerId.AsyncQueueRetry)
-            .expectActiveTargets()
-        );
-      }
+  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] });
+  });
+});
diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/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);
