Closed Bug 1113827 Opened 5 years ago Closed 5 years ago

webidl codegen should provide ErrorResult to any method returning a Promise

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: bkelly, Assigned: bzbarsky)

Details

Attachments

(2 files)

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.
I think this is reasonable.  Boris, Smaug, what do you think?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
Why special casing Promise even more?
If there is a possible exception, use [Throws].
Flags: needinfo?(bugs)
I'm with smaug.
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.
Or give me some other methods (Bindings.conf?) to get an ErrorResult arg without polluting the spec webidl.
Give me some example here please.
(Sounds like we're somewhat misusing the exception-isn't-exception-with-promise-special-case)
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...
Flags: needinfo?(bobbyholley)
(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!
(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.
Flags: needinfo?(bobbyholley)
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.
That sounds good to me.
Sounds good to me as well.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Flags: needinfo?(bzbarsky)
Attachment #8546402 - Flags: review?(peterv) → review+
Attachment #8546403 - Flags: review?(peterv) → review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.