Skip to content

Skip IndexedDB cleanup on Safari 14 #5166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 19, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
19 changes: 17 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,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<void> =>
defer(() => {
completedSteps.push(n);
});

queue.enqueueAndForget(() => doStep(1));
queue.enterRestrictedMode(/* purgeExistingTasks =*/ true);

await queue.drain();
expect(completedSteps).to.deep.equal([]);
});
});

function defer<T>(op: () => T): Promise<T> {
Expand Down