Add legacyOutputDidListenersThrowFlag to event dispatch for IDB

[annevk]

Indexed Database API could have done with some architectural design
review.

See w3c/IndexedDB#140 for context.

[domenic]

I'm particularly curious if this matches implementations; I'd be a bit surprised if they all plumbed such a flag through all this way just for the IndexedDB case. Maybe they do something less intrusive that the spec could copy.

[annevk]

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/indexeddb/IDBEventDispatcher.cpp seems like a specialized implementation of event dispatch for Indexed DB, but even there I cannot find a throwing reference. @inexorabletash? (Not reusing EventTarget might lead to other interesting bugs I suppose.)

@smaug---- what does Gecko do here?

[inexorabletash]

Indexed Database API could have done with some architectural design review.

Are we taking the TARDIS or the Delorean this time? :)

---

Blink has EventTarget::uncaughtExceptionInEventHandler called from the bindings code that calls event listeners/handlers - the only non-empty implementation is for Indexed DB's IDBRequest

The specialized dispatch is just used for the propagation logic, since the spec language for "get the parent" doesn't map to common code in Blink between the DOM and IDB. The actual firing/EventTarget code is common, though.

[smaug----]

Gecko has also special code added to event dispatch just because of IDB being weird. bent and sicking were happy about it at that time.

The actual event dispatch code is the same everywhere, it is just that IDB is the only one to check whether exception was thrown.

[domenic]

Sounds to me that in general like what implementations do is not much nicer than this spec patch, so probably fine as-is. But @annevk maybe you feel inspired by the above descriptions and can think of something a bit nicer.

[annevk]

The only other thing I could think of that would be cleaner was changing the return value, but that's much more invasive, especially just for IDB. A set of steps on EventTarget objects that only IDB uses might not be too bad though. @inexorabletash if I invoke a set of steps there when I see an exception that would be enough to fix this in IDB? What if multiple listeners throw?

[inexorabletash]

I'll need to test in implementations. The PR I just put up differs from Blink implementation in that it only looks at the flag after the dispatch is complete, whereas Blink aborts the transaction after the first callback that throws (so which would be in the "inner invoke" steps). In other words, throwing in the first listener would change state before the event made it to the second listener.

(I suspect changing that would be web-compatible, but need tests anyway.)

[inexorabletash]

Re: I'll need to test in implementations.

Gecko behaves per the spec PR, looking at the flag after dispatch is complete. Blink looks at the flag after each handler is called and only on the handlers on the request (not the "parent" for cases with propagation). I'll fix Blink to align with Gecko and the spec PR.

Edit: I was testing code I broke; throwing in parent handlers does work in blink.

[annevk]

It would be kind of nice to enforce that invariant, but that would require changing the return value of dispatch which seems quite disruptive. Also, given the prefix "legacy" hopefully nobody else starts using this.

[inexorabletash]

BTW, in case it wasn't clear: I'm happy with this PR as-is.

[annevk]

Thanks, hopefully "legacy" and the IDB-only note are enough of a tell for editors of new standards.