Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
7 changes: 7 additions & 0 deletions .changeset/fix-auth-indexeddb-retry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@firebase/auth': patch
'@firebase/auth-compat': patch
'firebase': patch
---

Updated `_isAvailable()` to use retry logic for the initial IndexedDB availability check, preventing incorrect fallbacks to in-memory persistence in environments where transactions may occasionally drop on startup.
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import {
_clearDatabase,
_openDatabase,
_POLLING_INTERVAL_MS,
_TRANSACTION_RETRY_COUNT,
_putObject
} from './indexed_db';

Expand All @@ -53,6 +54,14 @@ describe('platform_browser/persistence/indexed_db', () => {
indexedDBLocalPersistence
);

beforeEach(() => {
(persistence as any).dbPromise = null;
(persistence as any).listeners = {};
(persistence as any).localCache = {};
(persistence as any).pendingWrites = 0;
(persistence as any).stopPolling();
});

afterEach(sinon.restore);

async function waitUntilPoll(clock: sinon.SinonFakeTimers): Promise<void> {
Expand Down Expand Up @@ -91,7 +100,8 @@ describe('platform_browser/persistence/indexed_db', () => {
expect(await persistence._isAvailable()).to.be.true;
});

it('should return false if db creation errors', async () => {
it('should return false if db creation errors repeatedly', async () => {
(persistence as any).dbPromise = null;
sinon.stub(indexedDB, 'open').returns({
addEventListener(evt: string, cb: () => void) {
if (evt === 'error') {
Expand All @@ -102,6 +112,36 @@ describe('platform_browser/persistence/indexed_db', () => {
} as any);

expect(await persistence._isAvailable()).to.be.false;
expect((indexedDB.open as sinon.SinonStub).callCount).to.eq(
_TRANSACTION_RETRY_COUNT + 2
);
Comment on lines +115 to +117
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The expectation of _TRANSACTION_RETRY_COUNT + 2 (which evaluates to 5 attempts when the retry count is 3) indicates an off-by-one error in the underlying _withRetries helper's loop condition (numAttempts++ > _TRANSACTION_RETRY_COUNT). While _withRetries is not modified in this PR, this magic number in the test makes the logic less intuitive. Consider addressing the discrepancy in _withRetries by changing the condition to numAttempts++ >= _TRANSACTION_RETRY_COUNT so that the number of retries matches the constant's value, and then updating this test to expect _TRANSACTION_RETRY_COUNT + 1 total attempts.

});

it('should retry if db creation errors temporarily and then succeed', async () => {
(persistence as any).dbPromise = null;
const originalOpen = indexedDB.open.bind(indexedDB);
let errorsToThrow = 2;

sinon.stub(indexedDB, 'open').callsFake(((
name: string,
version?: number
) => {
if (errorsToThrow > 0) {
errorsToThrow--;
return {
addEventListener(evt: string, cb: () => void) {
if (evt === 'error') {
cb();
}
},
error: new DOMException('temporary error')
} as any;
}
return originalOpen(name, version);
}) as typeof indexedDB.open);

expect(await persistence._isAvailable()).to.be.true;
expect((indexedDB.open as sinon.SinonStub).callCount).to.eq(3);
});
});

Expand All @@ -116,6 +156,10 @@ describe('platform_browser/persistence/indexed_db', () => {
db = await _openDatabase();
});

after(async () => {
db.close();
});

beforeEach(async () => {
clock = sinon.useFakeTimers();
callback = sinon.spy();
Expand All @@ -134,6 +178,7 @@ describe('platform_browser/persistence/indexed_db', () => {
});

it('should trigger a listener when the key changes', async () => {
await persistence._get(key); // Ensure cache is populated before change
await _putObject(db, key, newValue);

await waitUntilPoll(clock);
Expand All @@ -154,6 +199,7 @@ describe('platform_browser/persistence/indexed_db', () => {
});

it('should not trigger the listener when a different key changes', async () => {
await persistence._get(key); // Ensure cache is populated
await _putObject(db, 'other-key', newValue);

await waitUntilPoll(clock);
Expand All @@ -162,6 +208,7 @@ describe('platform_browser/persistence/indexed_db', () => {
});

it('should not trigger if a write is pending', async () => {
await persistence._get(key); // Ensure cache is populated
await _putObject(db, key, newValue);
(persistence as any)['pendingWrites'] = 1;

Expand All @@ -184,6 +231,7 @@ describe('platform_browser/persistence/indexed_db', () => {
});

it('should trigger both listeners if multiple listeners are registered', async () => {
await persistence._get(key); // Ensure cache is populated
await _putObject(db, key, newValue);

await waitUntilPoll(clock);
Expand Down
27 changes: 16 additions & 11 deletions packages/auth/src/platform_browser/persistence/indexed_db.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ class IndexedDBLocalPersistence implements InternalPersistence {
static type: 'LOCAL' = 'LOCAL';

type = PersistenceType.LOCAL;
db?: IDBDatabase;
private dbPromise: Promise<IDBDatabase> | null = null;
readonly _shouldAllowMigration = true;

private readonly listeners: Record<string, Set<StorageEventListener>> = {};
Expand All @@ -184,11 +184,14 @@ class IndexedDBLocalPersistence implements InternalPersistence {
}

async _openDb(): Promise<IDBDatabase> {
if (this.db) {
return this.db;
if (this.dbPromise) {
return this.dbPromise;
}
this.db = await _openDatabase();
return this.db;
this.dbPromise = _openDatabase();
this.dbPromise.catch(() => {
this.dbPromise = null;
});
return this.dbPromise;
}

async _withRetries<T>(op: (db: IDBDatabase) => Promise<T>): Promise<T> {
Expand All @@ -202,9 +205,10 @@ class IndexedDBLocalPersistence implements InternalPersistence {
if (numAttempts++ > _TRANSACTION_RETRY_COUNT) {
throw e;
}
if (this.db) {
this.db.close();
this.db = undefined;
if (this.dbPromise) {
const db = await this.dbPromise;
db.close();
this.dbPromise = null;
}
// TODO: consider adding exponential backoff
}
Expand Down Expand Up @@ -310,9 +314,10 @@ class IndexedDBLocalPersistence implements InternalPersistence {
if (!indexedDB) {
return false;
}
const db = await _openDatabase();
await _putObject(db, STORAGE_AVAILABLE_KEY, '1');
await _deleteObject(db, STORAGE_AVAILABLE_KEY);
await this._withRetries(async (db: IDBDatabase) => {
await _putObject(db, STORAGE_AVAILABLE_KEY, '1');
await _deleteObject(db, STORAGE_AVAILABLE_KEY);
});
return true;
} catch {}
return false;
Expand Down
Loading