Merge

Author: schmidt-sebastian

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

@@ -19,6 +19,7 @@ import { User } from '../auth/user';
 import {
   ignoreIfPrimaryLeaseLoss,
   LocalStore,
+  LocalWriteResult,
   MultiTabLocalStore
 } from '../local/local_store';
 import { LocalViewChanges } from '../local/local_view_changes';
@@ -38,7 +39,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';
@@ -352,7 +353,24 @@ export class SyncEngine implements RemoteSyncer {
    */
   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);
@@ -1209,10 +1227,12 @@ export class MultiTabSyncEngine extends SyncEngine
     }
 
     for (const targetId of added) {
-      debugAssert(
-        !this.queriesByTarget.has(targetId),
-        'Trying to add an already active target'
-      );
+      if (this.queriesByTarget.has(targetId)) {
+        // A target might have been added in a previous attempt
+        logDebug(LOG_TAG, 'Adding an already active target ' + targetId);
+        continue;
+      }
+
       const target = await this.localStore.getTarget(targetId);
       debugAssert(
         !!target,

packages/firestore/src/local/shared_client_state.ts

@@ -29,9 +29,10 @@ import { Platform } from '../platform/platform';
 import { hardAssert, debugAssert } from '../util/assert';
 import { AsyncQueue } from '../util/async_queue';
 import { Code, FirestoreError } from '../util/error';
-import { forEach } from '../util/obj';
 import { logError, logDebug } from '../util/log';
 import { SortedSet } from '../util/sorted_set';
+import { SortedMap } from '../util/sorted_map';
+import { primitiveComparator } from '../util/misc';
 import { isSafeInteger } from '../util/types';
 import {
   QueryTargetState,
@@ -475,12 +476,14 @@ export class WebStorageSharedClientState implements SharedClientState {
   private readonly storage: Storage;
   private readonly localClientStorageKey: string;
   private readonly sequenceNumberKey: string;
-  private readonly activeClients: { [key: string]: ClientState } = {};
   private readonly storageListener = this.handleWebStorageEvent.bind(this);
   private readonly onlineStateKey: string;
   private readonly clientStateKeyRe: RegExp;
   private readonly mutationBatchKeyRe: RegExp;
   private readonly queryTargetKeyRe: RegExp;
+  private activeClients = new SortedMap<string, ClientState>(
+    primitiveComparator
+  );
   private started = false;
   private currentUser: User;
 
@@ -519,7 +522,10 @@ export class WebStorageSharedClientState implements SharedClientState {
     this.sequenceNumberKey = createWebStorageSequenceNumberKey(
       this.persistenceKey
     );
-    this.activeClients[this.localClientId] = new LocalClientState();
+    this.activeClients = this.activeClients.insert(
+      this.localClientId,
+      new LocalClientState()
+    );
 
     this.clientStateKeyRe = new RegExp(
       `^${CLIENT_STATE_KEY_PREFIX}_${escapedPersistenceKey}_([^_]*)$`
@@ -576,7 +582,10 @@ export class WebStorageSharedClientState implements SharedClientState {
           storageItem
         );
         if (clientState) {
-          this.activeClients[clientState.clientId] = clientState;
+          this.activeClients = this.activeClients.insert(
+            clientState.clientId,
+            clientState
+          );
         }
       }
     }
@@ -611,24 +620,17 @@ export class WebStorageSharedClientState implements SharedClientState {
   }
 
   getAllActiveQueryTargets(): TargetIdSet {
-    let activeTargets = targetIdSet();
-    forEach(this.activeClients, (key, value) => {
-      activeTargets = activeTargets.unionWith(value.activeTargetIds);
-    });
-    return activeTargets;
+    return this.extractActiveQueryTargets(this.activeClients);
   }
 
   isActiveQueryTarget(targetId: TargetId): boolean {
-    // This is not using `obj.forEach` since `forEach` doesn't support early
-    // return.
-    for (const clientId in this.activeClients) {
-      if (this.activeClients.hasOwnProperty(clientId)) {
-        if (this.activeClients[clientId].activeTargetIds.has(targetId)) {
-          return true;
-        }
+    let found = false;
+    this.activeClients.forEach((key, value) => {
+      if (value.activeTargetIds.has(targetId)) {
+        found = true;
       }
-    }
-    return false;
+    });
+    return found;
   }
 
   addPendingMutation(batchId: BatchId): void {
@@ -823,7 +825,7 @@ export class WebStorageSharedClientState implements SharedClientState {
   }
 
   private get localClientState(): LocalClientState {
-    return this.activeClients[this.localClientId] as LocalClientState;
+    return this.activeClients.get(this.localClientId) as LocalClientState;
   }
 
   private persistClientState(): void {
@@ -979,26 +981,23 @@ export class WebStorageSharedClientState implements SharedClientState {
     clientId: ClientId,
     clientState: RemoteClientState | null
   ): Promise<void> {
-    const existingTargets = this.getAllActiveQueryTargets();
-
-    if (clientState) {
-      this.activeClients[clientId] = clientState;
-    } else {
-      delete this.activeClients[clientId];
-    }
+    const updatedClients = clientState
+      ? this.activeClients.insert(clientId, clientState)
+      : this.activeClients.remove(clientId);
 
-    const newTargets = this.getAllActiveQueryTargets();
+    const existingTargets = this.extractActiveQueryTargets(this.activeClients);
+    const newTargets = this.extractActiveQueryTargets(updatedClients);
 
     const addedTargets: TargetId[] = [];
     const removedTargets: TargetId[] = [];
 
-    newTargets.forEach(async targetId => {
+    newTargets.forEach(targetId => {
       if (!existingTargets.has(targetId)) {
         addedTargets.push(targetId);
       }
     });
 
-    existingTargets.forEach(async targetId => {
+    existingTargets.forEach(targetId => {
       if (!newTargets.has(targetId)) {
         removedTargets.push(targetId);
       }
@@ -1007,7 +1006,9 @@ export class WebStorageSharedClientState implements SharedClientState {
     return this.syncEngine!.applyActiveTargetsChange(
       addedTargets,
       removedTargets
-    );
+    ).then(() => {
+      this.activeClients = updatedClients;
+    });
   }
 
   private handleOnlineStateEvent(onlineState: SharedOnlineState): void {
@@ -1016,10 +1017,20 @@ export class WebStorageSharedClientState implements SharedClientState {
     // IndexedDb. If a client does not update their IndexedDb client state
     // within 5 seconds, it is considered inactive and we don't emit an online
     // state event.
-    if (this.activeClients[onlineState.clientId]) {
+    if (this.activeClients.get(onlineState.clientId)) {
       this.onlineStateHandler!(onlineState.onlineState);
     }
   }
+
+  private extractActiveQueryTargets(
+    clients: SortedMap<string, ClientState>
+  ): SortedSet<TargetId> {
+    let activeTargets = targetIdSet();
+    clients.forEach((kev, value) => {
+      activeTargets = activeTargets.unionWith(value.activeTargetIds);
+    });
+    return activeTargets;
+  }
 }
 
 function fromWebStorageSequenceNumber(

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([\/.])/));

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([\/.])/));

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