From 4c47bd4fb2cc25aee95ff13b7fb370cc276642b5 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Fri, 16 Jul 2021 15:07:30 -0600 Subject: [PATCH 1/3] Skip IndexedDB cleanup on Safari 14 This PR changes our IndexedDB shutdown to skip all remaining work items on Safari 14. It is believed that writing to IndexedDB during unload can trigger a bug on Safari that prevents IndexedDB from loading during the next page load. --- .../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 | 19 +++++++++++++-- 4 files changed, 54 insertions(+), 6 deletions(-) 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..8a53c131019 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,21 @@ 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.enterRestrictedMode(/* purgeExistingTasks =*/ true); + + await queue.drain(); + expect(completedSteps).to.deep.equal([]); + }); }); function defer(op: () => T): Promise { From 374951358eeffbfe8581e45081cd397e17d83985 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Fri, 16 Jul 2021 15:12:15 -0600 Subject: [PATCH 2/3] Create nine-needles-compare.md --- .changeset/nine-needles-compare.md | 5 +++++ 1 file changed, 5 insertions(+) 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). From 9c7f5b89c55e494224815c49b56fdc2e7da1793e Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 19 Jul 2021 09:19:38 -0600 Subject: [PATCH 3/3] Better test --- packages/firestore/test/unit/util/async_queue.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/firestore/test/unit/util/async_queue.test.ts b/packages/firestore/test/unit/util/async_queue.test.ts index 8a53c131019..b041f76a688 100644 --- a/packages/firestore/test/unit/util/async_queue.test.ts +++ b/packages/firestore/test/unit/util/async_queue.test.ts @@ -427,10 +427,11 @@ describe('AsyncQueue', () => { }); queue.enqueueAndForget(() => doStep(1)); + queue.enqueueAndForgetEvenWhileRestricted(() => doStep(2)); queue.enterRestrictedMode(/* purgeExistingTasks =*/ true); await queue.drain(); - expect(completedSteps).to.deep.equal([]); + expect(completedSteps).to.deep.equal([2]); }); });