Integrate with window.onerror

[inexorabletash]

If errors are not prevented, call window.onerror

• Already implemented in Firefox
• Chrome implementation in progress (blocked on performance regression)

[bevis-tseng]

FYI: there are 2 web-platform test cases failed after supporting this in Firefox [1]:
idbtransaction_objectStoreNames.html
idbfactory_open10.htm
and the following two cases are suppressed with 'allow_uncaught_exception'
idbobjectstore_createIndex6-event_order.htm
idbobjectstore_createIndex7-event_order.htm

Expecting this to be specified eventually in IDB Spec v2.
@inexorabletash, is there any plan on specifying this in v2?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1274938#c2

[inexorabletash]

Yes, I'll get around to specifying it soon.

(Chrome impl still blocked on performance regression, since capturing source location is not free. I need to push on the v8 team.)

[inexorabletash]

@bevis-tseng - would this capture the FF behavior?

In the definition of request, add the properties script and position which capture the script and position from which the call which created the request was made, the property context which is the global object specified by the script's settings object.

At the end of 5.10 Firing an error event add a step: If the event’s canceled flag is not set, report an error with request's script, position, and context.

Add a similar substep to open() and deleteDatabase()

Any particular notes about ordering? If preventDefault() is called on the ErrorEvent seen in onerror, does that prevent the transaction from aborting?

[sicking]

Does report an error do the right thing when the error happens in a dedicated worker? I.e. does an error event fired against a worker global have a default action of firing an error event on the Worker object in the parent window, etc?

[bevis-tseng]

Yes, this captures the behavior in FF.
Besides, the ordering is IDBRequest -> IDBTransaction -> IDBDatabase -> window global or worker global and the transaction abortion will be prevented if preventDefault() on the error event is called.

[sicking]

Note that in a worker, the propagation path in Firefox is

IDBRequest -> IDBTransaction ->IDBDatabase => Worker global => Worker object => Window global

In the above, "->" indicates a single Event propagation path and "=>" indicates that a new Event is fired unless the previous Event was cancelled.

Since Firefox support nested workers, the propagation path can even be

IDBRequest -> IDBTransaction ->IDBDatabase => Worker 2 global => Worker 2 object => Worker 1 global => Worker 1 object => Window global

Where "Worker 2" is a nested worker inside of "Worker 1"

[inexorabletash]

I don't suppose anyone else wants to take a stab at specifying this and submitting a PR? Fame and glory await...

[inexorabletash]

We should update the definitions error events from open() and deleteDatabase() to ensure the events are cancelable - see #86

[brettz9]

In the specification, I assume both window.onerror and window.addEventListener('error'...) handlers will be triggered, and if so, I am wondering whether both should be passed a single error object (as occurs in Chrome when you dispatch an error event on window) or whether window.onerror should be passed the message, source, lineno, colno, and error object arguments described at https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onerror and apparently at https://html.spec.whatwg.org/multipage/webappapis.html#handler-onerror .

The latter would be harder to faithfully polyfill, however, as one would need to overwrite any user window.onerror with a function which detected avoided firing twice (dispatchEvent would need to be sent by the polyfill to trigger addEventListener events but since that only sends a single error object argument, the polyfill would need to overwrite the user handler to avoid triggering in this instance and instead only trigger when the polyfill directly calls window.onerror with multiple arguments).

[brettz9]

Apologies, I hadn't been aware that there was ErrorEvent which could be dispatched to trigger the handlers appropriately.

[brettz9]

Although this might be covered by the normal DOM behavior with exceptions in handlers triggering the global window.onerror, I wonder whether this might be helpful to spell out, especially for those events like "complete" where some might think it too late to fire.

[inexorabletash]

I'm going to punt this to v3, since we have implementations claiming "v2" support without this and it's a normative addition.

Gecko still gets 🥇

[inexorabletash]

To test/spec:

When IDBCursor's continue() (etc) is called and an error occurs, what source position is reported - the original openCursor() (etc) call that created the request, or the continue() call?

[brettz9]

In the new DOM hook for IndexedDB, legacyOutputDidListenersThrowFlag is set after "report the exception".

7. Call a user object’s operation with listener’s callback, "handleEvent", a list of arguments consisting of event, and event’s currentTarget attribute value as the callback this value. If this throws an exception, then:

   1. Report the exception.

   2. Set legacyOutputDidListenersThrowFlag if given.

This 'reporting the exception' step itself mentions it needing to "report the error" with the "global object" which would thus appear to trigger window.onerror in its later step, the following:

7. Let notHandled be the result of firing an event named error at target, using ErrorEvent, with the cancelable attribute initialized to true, the message attribute initialized to message, the filename attribute initialized to urlString, the lineno attribute initialized to line, the colno attribute initialized to col, and the error attribute initialized to errorValue.

So, it looks like:

1. This window.onerror behavior is already a fait accompli given that IndexedDB is also using the legacyOutputDidListenersThrowFlag hooks, no? Or is this issue being kept open since tests don't yet exist for it and it is not ready to be officially part of V2 for reasons mentioned above? (I'm mostly just interested here in confirming that I am not mistaken that technically this is already spec'd.)
2. Since legacyOutputDidListenersThrowFlag is set after "report the exception", it is apparently too late to prevent the default of aborting within window.onerror, consistent with "If an exception was propagated out from any event handler" is not a thing #140 (comment) , correct (though it can still be prevented from being logged to console)?

[inexorabletash]

This issue is about having errors on requests be reported to global onerror; the "Report the exception" in the DOM algorithm mentioned above would apply if an exception is thrown in a callback.

Contrast:

  store.put(v, k).onsuccess = e => {
    throw Error('hello world'); // will invoke window.onerror handler
  };

and:

  store.add(v, k);
  store.put(v, k).onerror = e => {
    console.warn('this will appear'); // will not invoke window.onerror handler
  };

[bevis-tseng]

Note:
In FF, there is a propagation path as mentioned in #49 (comment)

Hence, for the case of

  store.put(v, k).onerror = e => {
    console.warn('this will appear'); // will not invoke window.onerror handler
  };

window.onerror will still be invoked if e.preventDefault() is not invoked in this onerror callback.

[inexorabletash]

Oh, yes. FF has already taken the leap and implemented (years ago!). I need to spec. :)

[inexorabletash]

It looks like we're unblocked from landing this on the Blink side on the performance front.

To Do list:

• Tests - including the propagation path from workers
• Verify with the HTML editors that the report the error hook does the right thing
• Spec the stuff in Integrate with window.onerror #49 (comment)
• For continue()/advance() calls - do we need to update the request's internal state? The iterate a cursor steps never produce an error, so is this observable?

[inexorabletash]

The propagation from workers to window does not appear to behave as described in #49 (comment) in Firefox 57 for Indexed DB errors.

Given a page with a worker that uses IDB that has handlers for the request, transaction, connection, a global onerror handler in the worker, an onerror handler on the Worker object, and an onerror handler on the window global scope, Firefox sees the following when a ConstraintError is triggered:

in worker: [object IDBRequest]: error, ConstraintError
in worker: [object IDBTransaction]: error, ConstraintError
in worker: [object IDBDatabase]: error, ConstraintError
worker global onerror: ConstraintError, http://localhost:8888/worker.js, 22, 17, null

(i.e. the propagation path is just IDBRequest → IDBTransaction → IDBDatabase ⇒ Worker global)

In contrast, for a non-IDB error, e.g. the script ends with throw Error('oops'):

worker global onerror: Error: oops, http://localhost:8888/worker.js, 33, 0, Error: oops
Worker object onerror: [object ErrorEvent], undefined, undefined, undefined, undefined
window global onerror: Error: oops, http://localhost:8888/worker.js, 33, 0, null

(i.e. the propagation path is Worker global ⇒ Worker object ⇒ Window global)

Further, on the worker's global onerror, the first argument passed is the Error not the Event. That means preventDefault() can't be called.

@bevis-tseng - can you confirm the above, and if this is intentional?

[bevis-tseng]

We disabled it temporary in bug 1389913 since this was not available yet in spec v2.
You can add dom.indexedDB.errorEventToSelfError manually in about:config for testing.

In addition, only the error event originally targeted to an IDBRequest will be propagated in this way.

[inexorabletash]

Is there still interest in pursuing this, either from developers or implementers?

Pro:

• Hidden errors are bad.

Con:

• A new source of global errors could break sites that aren't expecting failures.
• Can already just listen to onerror on each connection.

[asutherland]

I don't think we have a strong opinion on the Firefox side at this time. My inclination would be to stick with the status quo (which translates to "do not pursue this"). My rationale is:

• Needing to call preventDefault() technically makes sense from an HTML/DOM perspective, but is arguably not intuitive for a database API, at least when it jumps to the global. It is handy for the IDBDatabase/IDBTransaction/IDBRequest scenario. And since we've largely moved over to using Promises for new API's like the Cache API, it makes even less sense to start bubbling to the global now.
• The contents of the ErrorEvents are not particularly useful, containing only location information for the most specific frame. In the case where an IDB wrapper is used, the location info will have zero useful information. In the case of direct IDB usage, it's still the case that if the request ever succeeds, then the problem is to be found in the data, not the call. (Although the line number could provide some info.)
  • If attempting to improve the developer UX around IDB in Firefox, it wouldn't involve this error.
  • I would expect any production debugging tools/middleware to wrap the IDB API in order to get a more useful error out, as you say.
  • It'd be more useful to add an "errors" mode of operation on IDBObserver that would allow inspection of the database's state and the failed mutations. This would be fun for middleware and devtools.

[pwnall]

I agree with @asutherland. Let's stick with the status quo.

At this point, I've accepted that any non-trivial use of IndexedDB will be wrapped by a library. Said library can deal with error handling.

[inexorabletash]

Closing out.