1113827 - webidl codegen should provide ErrorResult to any method returning a Promise

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently we are pretty much guaranteed that any promise returning method will need an ErrorResult argument:

1) Promise::Create() requires an ErrorResult to be passed.
2) If you want to auto-reject the promise, you can throw on the ErrorResult and the bindings will create a rejected promise for you.

Currently every Promise returning method has to use [Throws] to get an ErrorResult.  We should avoid this boilerplate (which usually won't be in the spec) and just provide an ErrorResult argument.

Comment 1

[(no longer active)]

I think this is reasonable.  Boris, Smaug, what do you think?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Why special casing Promise even more?
If there is a possible exception, use [Throws].

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I'm with smaug.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Sorry, I was told we had a goal to make our webidl pastable from specs.  As far as I can tell spec webidl generally does not specify [Throws] on promise returning methods.  (Because its expected the promise will be rejected in that case.)

Requiring extra annotations to sprinkled around for all promise returning methods seems at odds with that goal.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Or give me some other methods (Bindings.conf?) to get an ErrorResult arg without polluting the spec webidl.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Give me some example here please.
(Sounds like we're somewhat misusing the exception-isn't-exception-with-promise-special-case)

Comment 7

[Boris Zbarsky [:bzbarsky]]

I can probably spend more time on this when I get back, so leaving needinfo, but some initial thoughts:

1)  Making webidl pastable from specs is sort of a goal, in the sense that in the common case it should mostly work.  We've more or less given up on it as an explicit goal, because experience showed that putting things in Bindings.conf sucks a lot more than putting them in IDL annotations.

2)  There is no [Throws] in the spec at all.  IDL does not annotate whether something can throw.  So the fact that once you can throw you can no longer match the spec IDL is not unique to promise-returning methods.  The premise was that most things in fact cannot throw, and I think this premise still holds.

3)  It's sort of nice to maintain consistency across our IDL, such that you only get the ErrorResult if you're throwing.  If this is causing problems because too many things are throwing, we should be looking into whether they're throwing because the spec requires them to or for internal reasons and in the latter case seeing whether we can eliminate the internal reasons.  For example, maybe Promise::Create should abort on OOM.  Would it be sane for it to abort on jsapi.Init() fail?  If we can do both of those things, we could in fact make it not require an ErrorResult.  Bobby, how reasonable do you think this is?  Basically, we'd make failure to allocate the JSObject here fatal, as well as jsapi.Init() failure.  I suspect if we did this, a lot of the current [Throws] bits could go away.

4)  Reject-via-throw is convenient, I agree.  This is the #1 reason I'm still thinking about this.  Especially if we were to only do this for the [NewObject] case, since I think using the throw-to-reject behavior is not so desirable for the non-NewObject case.  Of course our IDL doesn't actually use [NewObject] too consistently for promise-returning methods, nor do the specs...

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Vacation Dec 15-31 from comment #7)
> For example, maybe Promise::Create should abort on OOM.  Would it be sane
> for it to abort on jsapi.Init() fail?  If we can do both of those things, we
> could in fact make it not require an ErrorResult.  Bobby, how reasonable do
> you think this is?  Basically, we'd make failure to allocate the JSObject
> here fatal, as well as jsapi.Init() failure.  I suspect if we did this, a
> lot of the current [Throws] bits could go away.

This would be nice and solve the issue for me.  I have no problem manually rejecting the promise to avoid [Throws] in my webidl.  I can easily have a local ErrorResult and use Promise::MaybeReject(), now that we support that.

Thanks Vacation!

Comment 9

[Bobby Holley (:bholley)]

(In reply to Vacation Dec 15-31 from comment #7)
> For example, maybe Promise::Create should abort on OOM.  Would it be sane
> for it to abort on jsapi.Init() fail?  If we can do both of those things, we
> could in fact make it not require an ErrorResult.  Bobby, how reasonable do
> you think this is?  Basically, we'd make failure to allocate the JSObject
> here fatal

I'm not wild about it. JS can OOM when the rest of the browser is doing totally fine memory-wise, and we need to have a failure mode other than whole-process abort. We do abort in cases when we hit an OOM during brain transplants etc, but that's (a) much less script-triggerable than promise allocation and (b) much more dire if we get it wrong.

As far as I can tell this is just about the annoyance of writing [Throws] everywhere. If that's the case we should just do comment 0, not abort the process.

> as well as jsapi.Init() failure.

We're converting all of these to fallible precisely because infallibility (which we experimented with with AutoEntryScript) didn't really hold up. nsIGlobalObject::GetGlobalJSObject starts returning null in various CC corner cases, and that's a really bad time (i.e. hard to reproduce) to start randomly aborting.

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK.  So a principled option here would be the following: If a method is marked [NewObject] and its return type requires a JSObject allocation (so return type isSpiderMonkeyInterface(), or isObject() or method returnsPromise()) then mark the method fallible even if there is no [Throws] annotation.

This should presumably cover most of the cases where the fallibility is due solely to promise creation.

Comment 11

[(no longer active)]

That sounds good to me.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Sounds good to me as well.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Automatically mark Web IDL [NewObject] methods that return a JS type as fallible

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Remove some now-unnecessary [Throws] annotations in our IDL

Comment 15

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e3abaf18602
https://hg.mozilla.org/integration/mozilla-inbound/rev/560fcd0679e8

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7e3abaf18602
https://hg.mozilla.org/mozilla-central/rev/560fcd0679e8