Skip to content

Commit

Permalink
Skip IndexedDB cleanup on Safari 14 (#5166)
Browse files Browse the repository at this point in the history
  • Loading branch information
schmidt-sebastian committed Jul 19, 2021
1 parent 02586c9 commit b51be1d
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 6 deletions.
5 changes: 5 additions & 0 deletions .changeset/nine-needles-compare.md
Original file line number Diff line number Diff line change
@@ -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).
10 changes: 10 additions & 0 deletions packages/firestore/src/local/indexeddb_persistence.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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.
Expand Down
7 changes: 6 additions & 1 deletion packages/firestore/src/util/async_queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
24 changes: 21 additions & 3 deletions packages/firestore/src/util/async_queue_impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -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[] = [];

Expand Down Expand Up @@ -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(
Expand All @@ -114,9 +119,22 @@ export class AsyncQueueImpl implements AsyncQueue {
this.verifyNotFailed();
if (this._isShuttingDown) {
// Return a Promise which never resolves.
return new Promise<T>(resolve => {});
return new Promise<T>(() => {});
}
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<T>();
return this.enqueueInternal<unknown>(() => {
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>): void {
Expand Down
20 changes: 18 additions & 2 deletions packages/firestore/test/unit/util/async_queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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);
});
});

Expand Down Expand Up @@ -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<void> =>
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<T>(op: () => T): Promise<T> {
Expand Down

0 comments on commit b51be1d

Please sign in to comment.