From b51be1da318a8f79ff159bcb8be9797d192519fd Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 19 Jul 2021 12:06:20 -0600 Subject: [PATCH] Skip IndexedDB cleanup on Safari 14 (#5166) --- .changeset/nine-needles-compare.md | 5 ++++ .../src/local/indexeddb_persistence.ts | 10 ++++++++ packages/firestore/src/util/async_queue.ts | 7 +++++- .../firestore/src/util/async_queue_impl.ts | 24 ++++++++++++++++--- .../test/unit/util/async_queue.test.ts | 20 ++++++++++++++-- 5 files changed, 60 insertions(+), 6 deletions(-) create mode 100644 .changeset/nine-needles-compare.md diff --git a/.changeset/nine-needles-compare.md b/.changeset/nine-needles-compare.md new file mode 100644 index 00000000000..8c246ab998f --- /dev/null +++ b/.changeset/nine-needles-compare.md @@ -0,0 +1,5 @@ +--- +"@firebase/firestore": patch +--- + +The SDK no longer accesses IndexedDB during a page unload event on Safari 14. This aims to reduce the occurrence of an IndexedDB bug in Safari (https://bugs.webkit.org/show_bug.cgi?id=226547). diff --git a/packages/firestore/src/local/indexeddb_persistence.ts b/packages/firestore/src/local/indexeddb_persistence.ts index 859505f8dcf..d4eba6e873c 100644 --- a/packages/firestore/src/local/indexeddb_persistence.ts +++ b/packages/firestore/src/local/indexeddb_persistence.ts @@ -15,6 +15,8 @@ * limitations under the License. */ +import { isSafari } from '@firebase/util'; + import { User } from '../auth/user'; import { DatabaseId } from '../core/database_info'; import { ListenSequence, SequenceNumberSyncer } from '../core/listen_sequence'; @@ -960,6 +962,14 @@ export class IndexedDbPersistence implements Persistence { // to make sure it gets a chance to run. this.markClientZombied(); + if (isSafari() && navigator.appVersion.match(`Version/14`)) { + // On Safari 14, we do not run any cleanup actions as it might trigger + // a bug that prevents Safari from re-opening IndexedDB during the + // next page load. + // See https://bugs.webkit.org/show_bug.cgi?id=226547 + this.queue.enterRestrictedMode(/* purgeExistingTasks= */ true); + } + this.queue.enqueueAndForget(() => { // Attempt graceful shutdown (including releasing our primary lease), // but there's no guarantee it will complete. diff --git a/packages/firestore/src/util/async_queue.ts b/packages/firestore/src/util/async_queue.ts index 0876f080416..24f07e77f9b 100644 --- a/packages/firestore/src/util/async_queue.ts +++ b/packages/firestore/src/util/async_queue.ts @@ -223,8 +223,13 @@ export interface AsyncQueue { * Initialize the shutdown of this queue. Once this method is called, the * only possible way to request running an operation is through * `enqueueEvenWhileRestricted()`. + * + * @param purgeExistingTasks Whether already enqueued tasked should be + * rejected (unless enqueued wih `enqueueEvenWhileRestricted()`). Defaults + * to false. */ - enterRestrictedMode(): void; + enterRestrictedMode(purgeExistingTasks?: boolean): void; + /** * Adds a new operation to the queue. Returns a promise that will be resolved * when the promise returned by the new operation is (with its value). diff --git a/packages/firestore/src/util/async_queue_impl.ts b/packages/firestore/src/util/async_queue_impl.ts index d012df905fc..6d870e3c272 100644 --- a/packages/firestore/src/util/async_queue_impl.ts +++ b/packages/firestore/src/util/async_queue_impl.ts @@ -23,6 +23,7 @@ import { debugAssert, fail } from './assert'; import { AsyncQueue, DelayedOperation, TimerId } from './async_queue'; import { FirestoreError } from './error'; import { logDebug, logError } from './log'; +import { Deferred } from './promise'; const LOG_TAG = 'AsyncQueue'; @@ -49,6 +50,9 @@ export class AsyncQueueImpl implements AsyncQueue { // assertion sanity-checks. private operationInProgress = false; + // Enabled during shutdown on Safari to prevent future access to IndexedDB. + private skipNonRestrictedTasks = false; + // List of TimerIds to fast-forward delays for. private timerIdsToSkip: TimerId[] = []; @@ -97,9 +101,10 @@ export class AsyncQueueImpl implements AsyncQueue { this.enqueueInternal(op); } - enterRestrictedMode(): void { + enterRestrictedMode(purgeExistingTasks?: boolean): void { if (!this._isShuttingDown) { this._isShuttingDown = true; + this.skipNonRestrictedTasks = purgeExistingTasks || false; const document = getDocument(); if (document && typeof document.removeEventListener === 'function') { document.removeEventListener( @@ -114,9 +119,22 @@ export class AsyncQueueImpl implements AsyncQueue { this.verifyNotFailed(); if (this._isShuttingDown) { // Return a Promise which never resolves. - return new Promise(resolve => {}); + return new Promise(() => {}); } - return this.enqueueInternal(op); + + // Create a deferred Promise that we can return to the callee. This + // allows us to return a "hanging Promise" only to the callee and still + // advance the queue even when the operation is not run. + const task = new Deferred(); + return this.enqueueInternal(() => { + if (this._isShuttingDown && this.skipNonRestrictedTasks) { + // We do not resolve 'task' + return Promise.resolve(); + } + + op().then(task.resolve, task.reject); + return task.promise; + }).then(() => task.promise); } enqueueRetryable(op: () => Promise): void { diff --git a/packages/firestore/test/unit/util/async_queue.test.ts b/packages/firestore/test/unit/util/async_queue.test.ts index 2cc65a2a170..b041f76a688 100644 --- a/packages/firestore/test/unit/util/async_queue.test.ts +++ b/packages/firestore/test/unit/util/async_queue.test.ts @@ -87,7 +87,7 @@ describe('AsyncQueue', () => { it('handles failures', () => { const queue = newAsyncQueue() as AsyncQueueImpl; - const expected = new Error('Firit cestore Test Simulated Error'); + const expected = new Error('Firestore Test Simulated Error'); // Disable logging for this test to avoid the assertion being logged const oldLogLevel = getLogLevel(); @@ -142,7 +142,7 @@ describe('AsyncQueue', () => { }).to.throw(/already failed:.*Simulated Error/); // Finally, restore log level. - setLogLevel((oldLogLevel as unknown) as LogLevelString); + setLogLevel(oldLogLevel as unknown as LogLevelString); }); }); @@ -417,6 +417,22 @@ describe('AsyncQueue', () => { await queue.drain(); expect(completedSteps).to.deep.equal([1, 2, 4]); }); + + it('Does not run existing operations if opted out', async () => { + const queue = newAsyncQueue() as AsyncQueueImpl; + const completedSteps: number[] = []; + const doStep = (n: number): Promise => + defer(() => { + completedSteps.push(n); + }); + + queue.enqueueAndForget(() => doStep(1)); + queue.enqueueAndForgetEvenWhileRestricted(() => doStep(2)); + queue.enterRestrictedMode(/* purgeExistingTasks =*/ true); + + await queue.drain(); + expect(completedSteps).to.deep.equal([2]); + }); }); function defer(op: () => T): Promise {