1016560 - Remove the footguns from Promise::MaybeReject

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

We allow rejecting with anything.

But that's bogus.  People should be rejecting with exceptions.  Going to clean up our existing callers.

Comment 1

[Boris Zbarsky [:bzbarsky]]

The main offender I'm not sure what to do with is Telephony::Callback::NotifyDialError.  It's rejecting promises with strings, which is not something we want to allow.  What should it be doing instead?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove the footgun of rejecting promises with arbitrary objects

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is a question for aknow or htsai.

Comment 4

[Szu-Yu Chen [:aknow] (inactive)]

The string argument of Telephony::Callback::NotifyDialError represents an application error, for example, 'RadioNotAvailable', 'BadNumberError'. I couldn't find an existed error type in DOMError that is suitable for this case. What could be a better way in this condition? 

To be more specific, we have an API, telephony.dial(), which will return a promise. When it fails, we would like to provide the reason as detailed as possible. So. could we create a DOMError with user specific error name which is not included in the spec? Or we have to select a proper error type for each of them.

Comment 5

[Boris Zbarsky [:bzbarsky]]

In JS, I'd think you would reject with something like new Error("Radio not available") or some such, right?

We can pretty easily create DOMExceptions with custom messages here, and same for TypeErrors.  We can also get JSAPI added to let us create raw Errors.

We shouldn't be using DOMError at all.

Comment 6

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Boris Zbarsky [:bz] from comment #5)
> In JS, I'd think you would reject with something like new Error("Radio not
> available") or some such, right?

Yes.

> We can pretty easily create DOMExceptions with custom messages here, and
> same for TypeErrors.  We can also get JSAPI added to let us create raw
> Errors.

So, what should we do for this bug? The strings used in NotifyDialError is a finite set. We could list all of them here.
 
> We shouldn't be using DOMError at all.

How will the change affect the gaia code?

  promise.then(resolveCB, rejectCB(reason) {} )

In our original usage, the type of 'reason' is a string. Then, what will it be after this patch? Is it a DOMException?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> So, what should we do for this bug?

You should decide what you want your rejections to be, as long as they're some subtype of Error.  Then I'll figure out how to make it happen.

> Then, what will it be after this patch? Is it a DOMException?

It's whatever you decide, as long as it's a subtype of Error.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I suppose another alternative is I could grandfather in these strings by adding a new BrokenMaybeReject API that takes a string for just this consumer, and stop worrying about this.  This is fundamentally the same as a function throwing a string instead of an Error object, and while it's crappy API design we do allow people to do that in JS...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove the footgun of rejecting promises with arbitrary objects

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-other failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/223a185a7ee2

https://tbpl.mozilla.org/php/getParsedLog.php?id=40968114&tree=Mozilla-Inbound

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yeah, we were picking up the bool version of ToJSValue with DOMError*. :(

I'm going to make MaybeRejectBrokenly a template with explicit specializations for const nsAString& and const nsRefPtr<DOMError>&.

Comment 12

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f23221132fe9

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f23221132fe9

Comment 14

[Jamin Liu [:jaliu][:Jamin]]

I attempted to mark Bug 1032755 as the blocker of Bug 1036233 yesterday.
However, I typed the incorrect bug number by accident.
Sorry for any inconvenience caused.