Support iOS 13 with offline persistence (#2271)

* Add transaction retries (#2250)

* Marking SimpleDb calls as idempotent (#2251)

* Mark mostly readonly calls as idempotent (#2252)

* Fix test failure (#2256)

* Make handleUserChange idempotent (#2257)

* Temporarily disable CountingQueryEngine tests (#2258)

*  Improve test hack (#2259)

* Improve test hack

* Comment in test hack

* Make getNewDocumentChanges() idempotent (#2255)

* Add onCommitted listeners for transactions (#2265)

* Fix build

* Fix Lint

* Make applyRemoteEvent idempotent (#2263)

* Make notifyLocalViewChanges idempotent (#2268)

* Make releaseQuery idempotent (#2266)

* Mark acknowledgeBatch and rejectBatch idempotent (#2269)

* idempotent `allocateQuery` and `notifyLocalViewChanges` (#2264)

* Mark collectGarbage idempotent (#2267)

* Idempotency: Address TODOs, add Changelog (#2270)

Author: schmidt-sebastian

packages/firestore/CHANGELOG.md

@@ -1,9 +1,15 @@
 # Unreleased
+- [changed] Fixed a crash on iOS 13 that occurred when persistence was enabled
+  in a background tab (#2232).
+- [fixed] Fixed an issue in the interaction with the Firestore Emulator that
+  caused requests with timestamps to fail.
+- [feature] Added a `Firestore.onSnapshotsInSync()` method that notifies you
+  when all your snapshot listeners are in sync with each other.
+
+# 1.6.0
 - [fixed] Fixed a regression that caused queries with nested field filters to
   crash the client if the field was not present in the local copy of the
   document. 
-- [fixed] Fixed an issue that caused requests with timestamps to Firestore
-  Emulator to fail.
 
 # 1.5.0
 - [feature] Added a `Firestore.waitForPendingWrites()` method that
@@ -17,8 +23,6 @@
   small subset of the documents in a collection.
 - [fixed] Fixed a race condition between authenticating and initializing
   Firestore that could result in initial writes to the database being dropped.
-- [feature] Added a `Firestore.onSnapshotsInSync()` method that notifies you
-  when all your snapshot listeners are in sync with each other.
 
 # 1.4.10
 - [changed] Transactions now perform exponential backoff before retrying.
@@ -37,7 +41,6 @@
   match the query (https://github.com/firebase/firebase-android-sdk/issues/155).
 
 # 1.4.4
->>>>>>> master
 - [fixed] Fixed an internal assertion that was triggered when an update
    with a `FieldValue.serverTimestamp()` and an update with a
   `FieldValue.increment()` were pending for the same document.

packages/firestore/src/core/firestore_client.ts

@@ -400,6 +400,8 @@ export class FirestoreClient {
         // TODO(index-free): Use IndexFreeQueryEngine/IndexedQueryEngine as appropriate.
         const queryEngine = new SimpleQueryEngine();
         this.localStore = new LocalStore(this.persistence, queryEngine, user);
+        await this.localStore.start();
+
         if (maybeLruGc) {
           // We're running LRU Garbage collection. Set up the scheduler.
           this.lruScheduler = new LruScheduler(

packages/firestore/src/local/indexeddb_index_manager.ts

@@ -40,15 +40,28 @@ export class IndexedDbIndexManager implements IndexManager {
    */
   private collectionParentsCache = new MemoryCollectionParentIndex();
 
+  /**
+   * Adds a new entry to the collection parent index.
+   *
+   * Repeated calls for the same collectionPath should be avoided within a
+   * transaction as IndexedDbIndexManager only caches writes once a transaction
+   * has been committed.
+   */
   addToCollectionParentIndex(
     transaction: PersistenceTransaction,
     collectionPath: ResourcePath
   ): PersistencePromise<void> {
     assert(collectionPath.length % 2 === 1, 'Expected a collection path.');
-    if (this.collectionParentsCache.add(collectionPath)) {
-      assert(collectionPath.length >= 1, 'Invalid collection path.');
+    if (!this.collectionParentsCache.has(collectionPath)) {
       const collectionId = collectionPath.lastSegment();
       const parentPath = collectionPath.popLast();
+
+      transaction.addOnCommittedListener(() => {
+        // Add the collection to the in memory cache only if the transaction was
+        // successfully committed.
+        this.collectionParentsCache.add(collectionPath);
+      });
+
       return collectionParentsStore(transaction).put({
         collectionId,
         parent: encode(parentPath)

packages/firestore/src/local/indexeddb_mutation_queue.ts

@@ -178,26 +178,33 @@ export class IndexedDbMutationQueue implements MutationQueue {
       );
       const dbBatch = this.serializer.toDbMutationBatch(this.userId, batch);
 
-      this.documentKeysByBatchId[batchId] = batch.keys();
-
       const promises: Array<PersistencePromise<void>> = [];
+      let collectionParents = new SortedSet<ResourcePath>((l, r) =>
+        primitiveComparator(l.canonicalString(), r.canonicalString())
+      );
       for (const mutation of mutations) {
         const indexKey = DbDocumentMutation.key(
           this.userId,
           mutation.key.path,
           batchId
         );
+        collectionParents = collectionParents.add(mutation.key.path.popLast());
         promises.push(mutationStore.put(dbBatch));
         promises.push(
           documentStore.put(indexKey, DbDocumentMutation.PLACEHOLDER)
         );
+      }
+
+      collectionParents.forEach(parent => {
         promises.push(
-          this.indexManager.addToCollectionParentIndex(
-            transaction,
-            mutation.key.path.popLast()
-          )
+          this.indexManager.addToCollectionParentIndex(transaction, parent)
         );
-      }
+      });
+
+      transaction.addOnCommittedListener(() => {
+        this.documentKeysByBatchId[batchId] = batch.keys();
+      });
+
       return PersistencePromise.waitFor(promises).next(() => batch);
     });
   }
@@ -492,7 +499,9 @@ export class IndexedDbMutationQueue implements MutationQueue {
       this.userId,
       batch
     ).next(removedDocuments => {
-      this.removeCachedMutationKeys(batch.batchId);
+      transaction.addOnCommittedListener(() => {
+        this.removeCachedMutationKeys(batch.batchId);
+      });
       return PersistencePromise.forEach(
         removedDocuments,
         (key: DocumentKey) => {

packages/firestore/src/local/indexeddb_persistence.ts

@@ -62,6 +62,7 @@ import { MutationQueue } from './mutation_queue';
 import {
   Persistence,
   PersistenceTransaction,
+  PersistenceTransactionMode,
   PrimaryStateListener,
   ReferenceDelegate
 } from './persistence';
@@ -316,22 +317,16 @@ export class IndexedDbPersistence implements Persistence {
 
         this.scheduleClientMetadataAndPrimaryLeaseRefreshes();
 
-        return this.startRemoteDocumentCache();
-      })
-      .then(() => {
         return this.simpleDb.runTransaction(
-          'readonly',
+          'readonly-idempotent',
           [DbTargetGlobal.store],
-          txn => {
-            return getHighestListenSequenceNumber(txn).next(
-              highestListenSequenceNumber => {
-                this.listenSequence = new ListenSequence(
-                  highestListenSequenceNumber,
-                  this.sequenceNumberSyncer
-                );
-              }
-            );
-          }
+          txn => getHighestListenSequenceNumber(txn)
+        );
+      })
+      .then(highestListenSequenceNumber => {
+        this.listenSequence = new ListenSequence(
+          highestListenSequenceNumber,
+          this.sequenceNumberSyncer
         );
       })
       .then(() => {
@@ -343,12 +338,6 @@ export class IndexedDbPersistence implements Persistence {
       });
   }
 
-  private startRemoteDocumentCache(): Promise<void> {
-    return this.simpleDb.runTransaction('readonly', ALL_STORES, txn =>
-      this.remoteDocumentCache.start(txn)
-    );
-  }
-
   setPrimaryStateListener(
     primaryStateListener: PrimaryStateListener
   ): Promise<void> {
@@ -645,7 +634,7 @@ export class IndexedDbPersistence implements Persistence {
     this.detachVisibilityHandler();
     this.detachWindowUnloadHook();
     await this.simpleDb.runTransaction(
-      'readwrite',
+      'readwrite-idempotent',
       [DbPrimaryClient.store, DbClientMetadata.store],
       txn => {
         return this.releasePrimaryLeaseIfHeld(txn).next(() =>
@@ -677,7 +666,7 @@ export class IndexedDbPersistence implements Persistence {
 
   getActiveClients(): Promise<ClientId[]> {
     return this.simpleDb.runTransaction(
-      'readonly',
+      'readonly-idempotent',
       [DbClientMetadata.store],
       txn => {
         return clientMetadataStore(txn)
@@ -742,20 +731,39 @@ export class IndexedDbPersistence implements Persistence {
 
   runTransaction<T>(
     action: string,
-    mode: 'readonly' | 'readwrite' | 'readwrite-primary',
+    mode: PersistenceTransactionMode,
     transactionOperation: (
       transaction: PersistenceTransaction
     ) => PersistencePromise<T>
   ): Promise<T> {
     log.debug(LOG_TAG, 'Starting transaction:', action);
 
+    // TODO(schmidt-sebastian): Simplify once all transactions are idempotent.
+    const idempotent = mode.endsWith('idempotent');
+    const readonly = mode.startsWith('readonly');
+    const simpleDbMode = readonly
+      ? idempotent
+        ? 'readonly-idempotent'
+        : 'readonly'
+      : idempotent
+      ? 'readwrite-idempotent'
+      : 'readwrite';
+
+    let persistenceTransaction: PersistenceTransaction;
+
     // Do all transactions as readwrite against all object stores, since we
     // are the only reader/writer.
-    return this.simpleDb.runTransaction(
-      mode === 'readonly' ? 'readonly' : 'readwrite',
-      ALL_STORES,
-      simpleDbTxn => {
-        if (mode === 'readwrite-primary') {
+    return this.simpleDb
+      .runTransaction(simpleDbMode, ALL_STORES, simpleDbTxn => {
+        persistenceTransaction = new IndexedDbTransaction(
+          simpleDbTxn,
+          this.listenSequence.next()
+        );
+
+        if (
+          mode === 'readwrite-primary' ||
+          mode === 'readwrite-primary-idempotent'
+        ) {
           // While we merely verify that we have (or can acquire) the lease
           // immediately, we wait to extend the primary lease until after
           // executing transactionOperation(). This ensures that even if the
@@ -776,12 +784,7 @@ export class IndexedDbPersistence implements Persistence {
                   PRIMARY_LEASE_LOST_ERROR_MSG
                 );
               }
-              return transactionOperation(
-                new IndexedDbTransaction(
-                  simpleDbTxn,
-                  this.listenSequence.next()
-                )
-              );
+              return transactionOperation(persistenceTransaction);
             })
             .next(result => {
               return this.acquireOrExtendPrimaryLease(simpleDbTxn).next(
@@ -790,13 +793,14 @@ export class IndexedDbPersistence implements Persistence {
             });
         } else {
           return this.verifyAllowTabSynchronization(simpleDbTxn).next(() =>
-            transactionOperation(
-              new IndexedDbTransaction(simpleDbTxn, this.listenSequence.next())
-            )
+            transactionOperation(persistenceTransaction)
           );
         }
-      }
-    );
+      })
+      .then(result => {
+        persistenceTransaction.raiseOnCommittedEvent();
+        return result;
+      });
   }
 
   /**

packages/firestore/src/local/indexeddb_query_cache.ts

@@ -164,7 +164,7 @@ export class IndexedDbQueryCache implements QueryCache {
         const queryData = this.serializer.fromDbTarget(value);
         if (
           queryData.sequenceNumber <= upperBound &&
-          activeTargetIds[queryData.targetId] === undefined
+          activeTargetIds.get(queryData.targetId) === null
         ) {
           count++;
           promises.push(this.removeQueryData(txn, queryData));