Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix issues with firefox private browsing #472

Merged
merged 3 commits into from
Jun 22, 2017
Merged

Fix issues with firefox private browsing #472

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7fcccad
Fix another round of test failures
richvdh Jun 20, 2017
fa593a7
Treat errors when deleting indexeddb as non-fatal
richvdh Jun 21, 2017
6e6b5c9
indexeddb worker: make clearDatabase work without having connected
richvdh Jun 21, 2017
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
20 changes: 11 additions & 9 deletions src/crypto/store/indexeddb-crypto-store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,9 @@ export default class IndexedDBCryptoStore {
};

req.onblocked = () => {
reject(new Error(
"unable to upgrade indexeddb because it is open elsewhere",
));
console.log(
`can't yet open IndexedDBCryptoStore because it is open elsewhere`,
);
};

req.onerror = (ev) => {
Expand Down Expand Up @@ -110,15 +110,17 @@ export default class IndexedDBCryptoStore {
const req = this._indexedDB.deleteDatabase(this._dbName);

req.onblocked = () => {
reject(new Error(
"unable to delete indexeddb because it is open elsewhere",
));
console.log(
`can't yet delete IndexedDBCryptoStore because it is open elsewhere`,
);
};

req.onerror = (ev) => {
reject(new Error(
"unable to delete indexeddb: " + ev.target.error,
));
// 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}`);
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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...

Copy link
Member Author

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...

Copy link
Member

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

resolve();
};

req.onsuccess = () => {
Expand Down
28 changes: 26 additions & 2 deletions src/store/indexeddb-local-backend.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,32 @@ LocalIndexedDBStoreBackend.prototype = {
* @return {Promise} Resolved when the database is cleared.
*/
clearDatabase: function() {
console.log("Removing indexeddb instance: ", this._dbName);
return promiseifyRequest(this.indexedDB.deleteDatabase(this._dbName));
return new q.Promise((resolve, reject) => {
console.log(`Removing indexeddb instance: ${this._dbName}`);
const req = this.indexedDB.deleteDatabase(this._dbName);

req.onblocked = () => {
console.log(
`can't yet delete indexeddb ${this._dbName}` +
` because it is open elsewhere`,
);
};

req.onerror = (ev) => {
// in firefox, with indexedDB disabled, this fails with a
// DOMError. We treat this as non-fatal, so that we can still
// use the app.
console.warn(
`unable to delete js-sdk store indexeddb: ${ev.target.error}`,
);
resolve();
};

req.onsuccess = () => {
console.log(`Removed indexeddb instance: ${this._dbName}`);
resolve();
};
});
},

/**
Expand Down
12 changes: 7 additions & 5 deletions src/store/indexeddb-remote-backend.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ const RemoteIndexedDBStoreBackend = function RemoteIndexedDBStoreBackend(
};

this._worker.onmessage = this._onWorkerMessage.bind(this);

// tell the worker the db name.
this._startPromise = this._doCmd('_setupWorker', [this._dbName]).then(() => {
console.log("IndexedDB worker is ready");
});
};


Expand All @@ -50,10 +55,7 @@ RemoteIndexedDBStoreBackend.prototype = {
* @return {Promise} Resolves if successfully connected.
*/
connect: function() {
return this._doCmd('_setupWorker', [this._dbName]).then(() => {
console.log("IndexedDB worker is ready");
return this._doCmd('connect');
});
return this._startPromise.then(() => this._doCmd('connect'));
},

/**
Expand All @@ -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'));
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, of course

},

/**
Expand Down