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

Integrate with window.onerror #49

Open
inexorabletash opened this Issue Oct 7, 2015 · 22 comments

Comments

Projects
None yet
5 participants
@inexorabletash
Member

inexorabletash commented Oct 7, 2015

If errors are not prevented, call window.onerror

  • Already implemented in Firefox
  • Chrome implementation in progress (blocked on performance regression)
@bevis-tseng

This comment has been minimized.

Show comment
Hide comment
@bevis-tseng

bevis-tseng May 31, 2016

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

bevis-tseng commented May 31, 2016

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

This comment has been minimized.

Show comment
Hide comment
@inexorabletash

inexorabletash May 31, 2016

Member

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

Member

inexorabletash commented May 31, 2016

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

This comment has been minimized.

Show comment
Hide comment
@inexorabletash

inexorabletash Aug 1, 2016

Member

@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?

Member

inexorabletash commented Aug 1, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@sicking

sicking Aug 1, 2016

Contributor

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?

Contributor

sicking commented Aug 1, 2016

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?

@inexorabletash inexorabletash added this to the V2 milestone Aug 1, 2016

@bevis-tseng

This comment has been minimized.

Show comment
Hide comment
@bevis-tseng

bevis-tseng Aug 3, 2016

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.

bevis-tseng commented Aug 3, 2016

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

This comment has been minimized.

Show comment
Hide comment
@sicking

sicking Aug 3, 2016

Contributor

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"

Contributor

sicking commented Aug 3, 2016

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

This comment has been minimized.

Show comment
Hide comment
@inexorabletash

inexorabletash Aug 3, 2016

Member

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

Member

inexorabletash commented Aug 3, 2016

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

@inexorabletash

This comment has been minimized.

Show comment
Hide comment
@inexorabletash

inexorabletash Oct 12, 2016

Member

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

Member

inexorabletash commented Oct 12, 2016

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

@brettz9

This comment has been minimized.

Show comment
Hide comment
@brettz9

brettz9 Oct 22, 2016

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 commented Oct 22, 2016

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

This comment has been minimized.

Show comment
Hide comment
@brettz9

brettz9 Oct 22, 2016

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

brettz9 commented Oct 22, 2016

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

@brettz9

This comment has been minimized.

Show comment
Hide comment
@brettz9

brettz9 Oct 24, 2016

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.

brettz9 commented Oct 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@inexorabletash

inexorabletash Mar 9, 2017

Member

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 🥇

Member

inexorabletash commented Mar 9, 2017

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 inexorabletash modified the milestones: V3, V2 Mar 9, 2017

@inexorabletash

This comment has been minimized.

Show comment
Hide comment
@inexorabletash

inexorabletash Mar 13, 2017

Member

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?

Member

inexorabletash commented Mar 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@brettz9

brettz9 Apr 20, 2017

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

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

  1. 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 #140 (comment) , correct (though it can still be prevented from being logged to console)?

brettz9 commented Apr 20, 2017

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

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

  1. 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 #140 (comment) , correct (though it can still be prevented from being logged to console)?
@inexorabletash

This comment has been minimized.

Show comment
Hide comment
@inexorabletash

inexorabletash Apr 20, 2017

Member

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
  };
Member

inexorabletash commented Apr 20, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bevis-tseng

bevis-tseng Apr 20, 2017

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.

bevis-tseng commented Apr 20, 2017

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

This comment has been minimized.

Show comment
Hide comment
@inexorabletash

inexorabletash Apr 20, 2017

Member

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

Member

inexorabletash commented Apr 20, 2017

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

@inexorabletash

This comment has been minimized.

Show comment
Hide comment
@inexorabletash

inexorabletash Jan 23, 2018

Member

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 #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?
Member

inexorabletash commented Jan 23, 2018

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 #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 inexorabletash self-assigned this Jan 23, 2018

@inexorabletash

This comment has been minimized.

Show comment
Hide comment
@inexorabletash

inexorabletash Jan 23, 2018

Member

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?

Member

inexorabletash commented Jan 23, 2018

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

This comment has been minimized.

Show comment
Hide comment
@bevis-tseng

bevis-tseng Jan 24, 2018

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.

bevis-tseng commented Jan 24, 2018

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

This comment has been minimized.

Show comment
Hide comment
@inexorabletash

inexorabletash Sep 18, 2018

Member

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

inexorabletash commented Sep 18, 2018

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

This comment has been minimized.

Show comment
Hide comment
@asutherland

asutherland Sep 18, 2018

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.

asutherland commented Sep 18, 2018

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment