-
Notifications
You must be signed in to change notification settings - Fork 963
change(firestore): close Firestore instance more gracefully when "Clear Site Data" button pressed in browser #9118
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
Changes from 5 commits
5563417
cd84a44
9482f20
37803dd
51a279d
2b27201
3f24c15
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@firebase/firestore': patch | ||
--- | ||
|
||
Terminate Firestore more gracefully when "Clear Site Data" button is pressed in a web browser |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,11 +18,16 @@ | |
import { getGlobal, getUA, isIndexedDBAvailable } from '@firebase/util'; | ||
|
||
import { debugAssert } from '../util/assert'; | ||
import { generateUniqueDebugId } from '../util/debug_uid'; | ||
import { Code, FirestoreError } from '../util/error'; | ||
import { logDebug, logError, logWarn } from '../util/log'; | ||
import { logDebug, logError } from '../util/log'; | ||
import { Deferred } from '../util/promise'; | ||
|
||
import { DatabaseDeletedListener } from './persistence'; | ||
import { | ||
ClearSiteDataDatabaseDeletedEvent, | ||
DatabaseDeletedListener, | ||
VersionChangeDatabaseDeletedEvent | ||
} from './persistence'; | ||
import { PersistencePromise } from './persistence_promise'; | ||
|
||
// References to `indexedDB` are guarded by SimpleDb.isAvailable() and getGlobal() | ||
|
@@ -299,9 +304,33 @@ export class SimpleDb { | |
// https://developer.mozilla.org/en-US/docs/Web/API/IDBVersionChangeRequest/setVersion | ||
const request = indexedDB.open(this.name, this.version); | ||
|
||
// Store information about "Clear Site Data" being detected in the | ||
// "onupgradeneeded" event listener and handle it in the "onsuccess" | ||
// event listener, as opposed to throwing directly from the | ||
// "onupgradeneeded" event listener. Do this because throwing from the | ||
// "onupgradeneeded" event listener results in a generic error being | ||
// reported to the "onerror" event listener that cannot be distinguished | ||
// from other errors. | ||
const clearSiteDataEvent: ClearSiteDataDatabaseDeletedEvent[] = []; | ||
|
||
request.onsuccess = (event: Event) => { | ||
let error: unknown; | ||
if (clearSiteDataEvent[0]) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the c++ side of my brain hurts when i see this :) TS playground suggests it's safe to do, but why not use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What, you don't like the looks of accessing uninitialized memory? ;) My memory told me that the TypeScript compiler was failing to infer that |
||
try { | ||
this.databaseDeletedListener?.(clearSiteDataEvent[0]); | ||
} catch (e) { | ||
error = e; | ||
} | ||
} | ||
|
||
const db = (event.target as IDBOpenDBRequest).result; | ||
resolve(db); | ||
|
||
if (error) { | ||
reject(error); | ||
db.close(); | ||
} else { | ||
resolve(db); | ||
} | ||
}; | ||
|
||
request.onblocked = () => { | ||
|
@@ -353,18 +382,14 @@ export class SimpleDb { | |
this.lastClosedDbVersion !== null && | ||
this.lastClosedDbVersion !== event.oldVersion | ||
) { | ||
// This thrown error will get passed to the `onerror` callback | ||
// registered above, and will then be propagated correctly. | ||
throw new Error( | ||
`refusing to open IndexedDB database due to potential ` + | ||
`corruption of the IndexedDB database data; this corruption ` + | ||
`could be caused by clicking the "clear site data" button in ` + | ||
`a web browser; try reloading the web page to re-initialize ` + | ||
`the IndexedDB database: ` + | ||
`lastClosedDbVersion=${this.lastClosedDbVersion}, ` + | ||
`event.oldVersion=${event.oldVersion}, ` + | ||
`event.newVersion=${event.newVersion}, ` + | ||
`db.version=${db.version}` | ||
clearSiteDataEvent.push( | ||
new ClearSiteDataDatabaseDeletedEvent({ | ||
eventId: generateUniqueDebugId(), | ||
lastClosedVersion: this.lastClosedDbVersion, | ||
eventOldVersion: event.oldVersion, | ||
eventNewVersion: event.newVersion, | ||
dbVersion: db.version | ||
}) | ||
); | ||
} | ||
this.schemaConverter | ||
|
@@ -399,11 +424,12 @@ export class SimpleDb { | |
// Notify the listener if another tab attempted to delete the IndexedDb | ||
// database, such as by calling clearIndexedDbPersistence(). | ||
if (event.newVersion === null) { | ||
logWarn( | ||
`Received "versionchange" event with newVersion===null; ` + | ||
'notifying the registered DatabaseDeletedListener, if any' | ||
this.databaseDeletedListener?.( | ||
new VersionChangeDatabaseDeletedEvent({ | ||
eventId: generateUniqueDebugId(), | ||
eventNewVersion: event.newVersion | ||
}) | ||
); | ||
this.databaseDeletedListener?.(); | ||
} | ||
}, | ||
{ passive: true } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment. I presume
onsuccess
always gets called afteronupgradeneeded
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is correct:
onsuccess
is called afteronupgradeneeded
(as long asonupgradeneeded
doesn't throw an exception)