diff --git a/.changeset/smart-cars-doubt.md b/.changeset/smart-cars-doubt.md new file mode 100644 index 00000000000..5784db83675 --- /dev/null +++ b/.changeset/smart-cars-doubt.md @@ -0,0 +1,6 @@ +--- +"firebase": patch +"@firebase/firestore": patch +--- + +The SDK no longer crashes if an IndexedDB failure occurs when unsubscribing from a Query. diff --git a/packages/firestore/src/local/local_store.ts b/packages/firestore/src/local/local_store.ts index c996fe532c1..57c80de809f 100644 --- a/packages/firestore/src/local/local_store.ts +++ b/packages/firestore/src/local/local_store.ts @@ -911,7 +911,7 @@ class LocalStoreImpl implements LocalStore { } } - releaseTarget( + async releaseTarget( targetId: number, keepPersistedTargetData: boolean ): Promise { @@ -922,21 +922,34 @@ class LocalStoreImpl implements LocalStore { ); const mode = keepPersistedTargetData ? 'readwrite' : 'readwrite-primary'; - return this.persistence - .runTransaction('Release target', mode, txn => { - if (!keepPersistedTargetData) { + + try { + if (!keepPersistedTargetData) { + await this.persistence.runTransaction('Release target', mode, txn => { return this.persistence.referenceDelegate.removeTarget( txn, targetData! ); - } else { - return PersistencePromise.resolve(); - } - }) - .then(() => { - this.targetDataByTarget = this.targetDataByTarget.remove(targetId); - this.targetIdByTarget.delete(targetData!.target); - }); + }); + } + } catch (e) { + if (isIndexedDbTransactionError(e)) { + // All `releaseTarget` does is record the final metadata state for the + // target, but we've been recording this periodically during target + // activity. If we lose this write this could cause a very slight + // difference in the order of target deletion during GC, but we + // don't define exact LRU semantics so this is acceptable. + logDebug( + LOG_TAG, + `Failed to update sequence numbers for target ${targetId}: ${e}` + ); + } else { + throw e; + } + } + + this.targetDataByTarget = this.targetDataByTarget.remove(targetId); + this.targetIdByTarget.delete(targetData!.target); } executeQuery( diff --git a/packages/firestore/test/unit/specs/recovery_spec.test.ts b/packages/firestore/test/unit/specs/recovery_spec.test.ts index a8f8ea21c31..c96454bd575 100644 --- a/packages/firestore/test/unit/specs/recovery_spec.test.ts +++ b/packages/firestore/test/unit/specs/recovery_spec.test.ts @@ -569,7 +569,10 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => { ); }); - specTest('Recovers when watch rejection cannot be persisted', [], () => { + specTest('Handles rejections that cannot be persisted', [], () => { + // This test verifies that the client ignores failures during the + // 'Release target' transaction. + const doc1Query = Query.atPath(path('collection/key1')); const doc2Query = Query.atPath(path('collection/key2')); const doc1a = doc('collection/key1', 1000, { foo: 'a' }); @@ -593,25 +596,22 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => { doc1Query, new RpcError(Code.PERMISSION_DENIED, 'Simulated target error') ) - // `failDatabaseTransactions()` causes us to go offline. - .expectActiveTargets() - .expectEvents(doc1Query, { fromCache: true }) - .expectEvents(doc2Query, { fromCache: true }) + .expectEvents(doc1Query, { errorCode: Code.PERMISSION_DENIED }) .recoverDatabase() - .runTimer(TimerId.AsyncQueueRetry) - .expectActiveTargets( - { query: doc1Query, resumeToken: 'resume-token-1000' }, - { query: doc2Query, resumeToken: 'resume-token-2000' } - ) - .watchAcksFull(doc1Query, 3000) - .expectEvents(doc1Query, {}) .watchRemoves( doc2Query, new RpcError(Code.PERMISSION_DENIED, 'Simulated target error') ) .expectEvents(doc2Query, { errorCode: Code.PERMISSION_DENIED }) - .watchSends({ affects: [doc1Query] }, doc1b) - .watchSnapshots(4000) + // Verify that `doc1Query` can be listened to again. Note that the + // resume token is slightly outdated since we failed to persist the + // target update during the release. + .userListens(doc1Query, 'resume-token-1000') + .expectEvents(doc1Query, { + added: [doc1a], + fromCache: true + }) + .watchAcksFull(doc1Query, 4000, doc1b) .expectEvents(doc1Query, { modified: [doc1b] }) @@ -797,4 +797,45 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => { ); } ); + + specTest('Unlisten succeeds when target release fails', [], () => { + const query = Query.atPath(path('collection')); + const doc1 = doc('collection/key1', 1, { foo: 'a' }); + return spec() + .userListens(query) + .watchAcksFull(query, 1000, doc1) + .expectEvents(query, { + added: [doc1] + }) + .failDatabaseTransactions('Release target') + .userUnlistens(query) + .expectActiveTargets(); + }); + + specTest('Can re-listen to query when unlisten fails', [], () => { + const query = Query.atPath(path('collection')); + const doc1 = doc('collection/key1', 1, { foo: 'a' }); + const doc2 = doc('collection/key2', 2, { foo: 'b' }); + return spec() + .withGCEnabled(false) + .userListens(query) + .watchAcksFull(query, 1000, doc1) + .expectEvents(query, { + added: [doc1] + }) + .failDatabaseTransactions('Release target') + .userUnlistens(query) + .watchRemoves(query) + .recoverDatabase() + .userListens(query, 'resume-token-1000') + .expectEvents(query, { + added: [doc1], + fromCache: true + }) + .watchAcksFull(query, 2000, doc2) + .expectEvents(query, { + added: [doc2] + }) + .userUnlistens(query); + }); });