-
-
Notifications
You must be signed in to change notification settings - Fork 585
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
Fix issues with firefox private browsing #472
Conversation
'blocked' is *not* a fatal situation when opening or deleting databases.
If we get an error when vaping the indexeddb, carry on regardless
... so that we can clear the database during login from a temporary client.
@@ -62,7 +64,7 @@ RemoteIndexedDBStoreBackend.prototype = { | |||
* @return {Promise} Resolved when the database is cleared. | |||
*/ | |||
clearDatabase: function() { | |||
return this._doCmd('clearDatabase'); | |||
return this._startPromise.then(() => this._doCmd('clearDatabase')); |
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.
Should we make the worker not replace the backend if it already has one? otherwise this will make a new worker if you clear the db after having opened it which may cause issues because it's been opened by a different indexedb instance?
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.
I don't think it will make a new worker?
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.
Will it not create another backend object in https://github.com/matrix-org/matrix-js-sdk/blob/master/src/store/indexeddb-store-worker.js#L59 though?
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.
No, because _setupWorker is only called once, from the constructor. We're just piggy-backing on the promise created there.
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.
Oh right, of course
// in firefox, with indexedDB disabled, this fails with a | ||
// DOMError. We treat this as non-fatal, so that people can | ||
// still use the app. | ||
console.warn(`unable to delete IndexedDBCryptoStore: ${ev.target.error}`); |
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.
Would it be sensible to give up on using indexeddb entirely if we fail to clear the database? This would mitigate the possibility of ending up opening the database with spurious data from a different run.
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.
possibly, but how would we implement that, given this will happen on a different MatrixClient to subsequent access?
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.
Yeah, it would have to be a global flag somewhere which is a bit grim - potentially setting the createMatrixClient worker script property to null...
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.
setting the createMatrixClient worker script property to null...
createMatrixClient is in a different project...
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.
yeah, would have to be done in react-sdk in any case I guess, so not one for this PR
Treat errors when deleting indexeddb as non-fatal
also let the webworker be used for deleting the indexeddb without it having previously been used for syncing.
(part of the fix to element-hq/element-web#4340)