Closed Bug 1016560 Opened 5 years ago Closed 5 years ago

Remove the footguns from Promise::MaybeReject

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(1 file, 1 obsolete file)

We allow rejecting with anything.

But that's bogus.  People should be rejecting with exceptions.  Going to clean up our existing callers.
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?
Flags: needinfo?(szchen)
Flags: needinfo?(khuey)
This is a question for aknow or htsai.
Flags: needinfo?(khuey)
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.
Flags: needinfo?(szchen)
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.
(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?
> 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.
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...
Attachment #8429554 - Attachment is obsolete: true
Attachment #8429554 - Flags: feedback?(khuey)
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>&.
https://hg.mozilla.org/mozilla-central/rev/f23221132fe9
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
No longer blocks: 1036233
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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.