1355736 - Return value to stop propagating IndexedDB errors from .onerror handlers is treated inconsistently

Status: NEW

(Core :: Storage: IndexedDB, enhancement, P3)

Description

[Jukka Jylänki]

A common pattern we are seeing is that developers do

> window.onerror = function(e) {
>   console.error('Fatal uncaught error ' + e);
>   telemetryServer.send(e);
>   abortPageExecutionAndShowErrorDialog('Unrecoverable error: ' + e);
> }

to be able to assert fail on any errors that they might have missed at development time and to enforce that all exceptions are caught on the site where they occur.

For example, IndexedDB database open is then performed via

> function openIndexedDB(dbName, dbVersion) {
>   return new Promise(function(resolve, reject) {
>     try {
>       var idb = window.indexedDB || window.mozIndexedDB || window.webkitIndexedDB || window.msIndexedDB;
>       var openRequest = idb.open(dbName, dbVersion);
>       openRequest.onsuccess = function(event) {
>         resolve(event.target.result);
>       };
>       openRequest.onerror = function(error) {
>         reject(openRequest.error.name + ': ' + openRequest.error.message);
>         return false; /* Don't propagate the error to window.onerror handler. */
>       };
>     } catch(e) {
>       return reject(e);
>     }
>   });
> }

to ensure that caller can detect and handle errors via

> openIndexedDB(dbName, dbVersion).catch(function(error) {
>   /* handle error */
> });

to make sure that everything is caught at the site of use and window.onerror() is then reserved as a last catch-all resort to deal with unexpected conditions.

However due to bug 1246615, idb.open() can fail with a random error like UnknownError or InvalidStateError if the user has used the Firefox profile with a future version of Firefox. In this case, it looks like the error is directly short circuited to call window.onerror() rather than throwing an exception from idb.open(), or calling the openRequest.onerror() handler. This means that the /* handle error */ part is not called, but the error goes directly to window.onerror() function.

It would be good if IndexedDB did not short circuit to report its errors in window.onerror(), but either synchronously threw an exception or called the .onerror() handler of the transaction in question, so that developers would be able to avoid having to use the last catch-call handler to deal with IndexedDB errors.

Comment 1

[Jukka Jylänki]

Attempting a hacky workaround, it looks like it is not possible to install a custom window.onerror() handler for the duration of the IndexedDB .open() call, i.e. the following does not work:

> var idb = window.indexedDB || window.mozIndexedDB || window.webkitIndexedDB || window.msIndexedDB;
> idb.prevOpen = idb.open;
> idb.open = function(dbName, dbVersion) {
> 	var prevOnError = window.onerror;
> 	window.onerror = function(e) { /* handle idb error */ }
> 	idb.prevOpen(dbName, dbVersion);
> 	window.onerror = prevOnError;
> }

since IndexedDB open() is asynchronous. This leaves developers with not much more than a

> window.onerror = function(e) {
>   if (e.indexOf('UnknownError) != -1 || e.indexOf('InvalidStateError') != -1) return; /* ignore errors from browser bugs */
>   console.error('Fatal uncaught error ' + e);
>   telemetryServer.send(e);
>   abortPageExecutionAndShowErrorDialog('Unrecoverable error: ' + e);
> }

but that is troublesome as well, since it could mask a real error coming from another source.

Comment 2

[Jan Varga [:janv]]

I'm really surprised that openRequest.onerror is not fired.

Comment 3

[Jukka Jylänki]

Attachment: testidbopen.html

It looks like FF Stable 52.0.1 (64-bit) and FF Nightly 55.0a1 (2017-04-06) (64-bit) is a pair of browsers that have a schema change in between.

STR:
1. Open attached page in new user profile in FF Stable 52, it prints "Opened db".
2. Open same page in FF Nightly 55.0a1 (2017-04-06) (64-bit) in same user profile, it prints "Opened db" again.
3. Open the page then in FF Stable 52 again, and it prints

> window.onerror: UnknownError
> UnknownError

Comment 4

[Jukka Jylänki]

Err, it looks like the return conditions are mixed. That is, in order to not propagate to window.onerror() from IDB open request, one has to return true; from its onerror handler. In order to not propagate to window.onerror() from a IDB put request, one has to return false; instead.

The following avoids IDB open from propagating an error to window.onerror():

> function openIndexedDB() {
>   return new Promise(function(resolve, reject) {
>     var idb = window.indexedDB || window.mozIndexedDB || window.webkitIndexedDB || window.msIndexedDB;
>     var openRequest = idb.open('dbName', 2/*dbVersion*/);
>     openRequest.onupgradeneeded = function(event) {
>       var db = event.target.result;
>       if (db.objectStoreNames.contains('FILES')) db.deleteObjectStore('FILES');
>       db.createObjectStore('FILES');
>     };
>     openRequest.onsuccess = function(event) {
>       resolve(event.target.result);
>     };
>     openRequest.onerror = function(error) {
>       reject(error);
>       return true; /* Don't propagate the error to window.onerror handler. */
>     };
>   });
> }

 
whereas the following avoids a .put() operation from propagating an error to window.onerror():

> function storeToIndexedDB(db, key, value) {
>   return new Promise(function(resolve, reject) {
>     try {
>       var transaction = db.transaction(['FILES'], 'readwrite');
>       var packages = transaction.objectStore('FILES');
>       var putRequest = packages.put(value, key);
>       putRequest.onsuccess = function(event) {
>         console.log('putRequest.onsuccess');
>         resolve(key);
>       };
>       putRequest.onerror = function(error) {
>         console.error('putRequest.onerror');
>         reject(error);
>         return false; /* Don't propagate the error to window.onerror handler. */
>       };
>     } catch(e) {
>       console.error('Exception: ' + e);
>       reject(e);
>     }
>   });
> }

Should one return true or false from the request onerror() handlers to stop propagation?

Comment 5

[Jens Stutte [:jstutte]]

:sgiesecke, is there some appropriate meta-bug we might want to collect this with?

Comment 6

[Simon Giesecke [:sg] [he/him]]

(In reply to Jens Stutte [:jstutte] from comment #5)

:sgiesecke, is there some appropriate meta-bug we might want to collect this with?

As I understand this, this is at least closely related to Bug 1246615. While not an exact duplicate, since it refers to error reporting to the script rather than immediately to the user, I assume that if an unusable profile is detected earlier, we would not normally get to executing a script at all. If the user overrides this, accepting the risk, I think the current behaviour is ok, they should know what they are doing in that case. So this should either be resolved as a duplicate of Bug 1246615 or at least depend on it.