1300071 - Create an API to access DOM Promise's state {pending, resolved or rejected}

Status: RESOLVED FIXED

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

Description

[Tzuhao Kuo [:kaku]]

Follow bug 1244768 comment 57.

We want to add a method to the Promise class for accessing its states (pending, resolved, rejected) so that we can check the status of a returned promise.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Wouldn't this open the door to potentially-racy code?
E.g.: You read the status as pending, but right then something happens in another thread that resolves or rejects the promise, but in the first thread you will do something based on the now-obsolete 'pending' status.

(It's very possible I'm imagining impossible things, as I don't have a deep knowledge of MozPromise; if unsure you may want to check with Bobby Holley, who wrote it.)

Comment 2

[Boris Zbarsky [:bzbarsky]]

This is about Promise, not MozPromise, right?

Comment 3

[Tzuhao Kuo [:kaku]]

(In reply to Boris Zbarsky [:bz] from comment #2)
> This is about Promise, not MozPromise, right?
Yes.

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Oops, this is embarrassing. Sorry for the confusion.

Comment 5

[Tzuhao Kuo [:kaku]]

(In reply to Gerald Squelart [:gerald] from comment #4)
> Oops, this is embarrassing. Sorry for the confusion.
It's okay. I didn't indicate it in the title and comment and we rarely use DOM promise in playback.

Comment 6

[Tzuhao Kuo [:kaku]]

So, after brief code tracing, I suppose that what I need to do is casting the dom::Promise::mPromiseObj into type js::PromiseObject and then call the object's state() method, right?

Does js::PromiseObject handle the possible racing of accessing/writing its state?

Comment 7

[Tzuhao Kuo [:kaku]]

Or maybe just take advantage of the JS::GetPromiseState() utility?

http://searchfox.org/mozilla-central/source/js/src/jsapi.cpp#4680

Comment 8

[Boris Zbarsky [:bzbarsky]]

You want JS::GetPromiseState.  You should most emphatically NOT be casting anything yourself.

> Does js::PromiseObject handle the possible racing of accessing/writing its state?

There is no possible racing to handle.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Oh, and in general, you should not be poking into any JSAPI headers that are not in js/public, aren't jsapi.h, and aren't jsfriendapi.h looking for APIs to use.  If you don't find what you need there, it may be worth poking at implementation details to see what API could be exposed, of course.  ;)

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

This should be in the DOM bugzilla component.

Comment 11

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1300071 - Create an API to access DOM Promise's state;

Review commit: https://reviewboard.mozilla.org/r/76886/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76886/

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8788390 [details]
Bug 1300071 - Create an API to access DOM Promise's state;

https://reviewboard.mozilla.org/r/76886/#review75098

::: dom/promise/Promise.cpp:3192
(Diff revision 1)
>  #endif // SPIDERMONKEY_PROMISE
>  
> +Promise::PromiseState
> +Promise::State() const
> +{
> +  AutoJSAPI jsapi;

You don't need this AutoJSAPI as far as I can tell.  You only use it for rooting, and that's what RootingCx() is for.

::: dom/promise/Promise.cpp:3196
(Diff revision 1)
> +{
> +  AutoJSAPI jsapi;
> +  MOZ_ASSERT(!jsapi.Init(mGlobal));
> +  JS::Rooted<JSObject*> p(jsapi.cx(), PromiseObj());
> +
> +  switch (JS::GetPromiseState(p)) {

This will do very bad things if SPIDERMONKEY_PROMISE is not defined.  Please have a separate implementation of this function for the !SPIDERMONKEY_PROMISE case.

Comment 13

[Tzuhao Kuo [:kaku]]

Comment on attachment 8788390 [details]
Bug 1300071 - Create an API to access DOM Promise's state;

https://reviewboard.mozilla.org/r/76886/#review75098

> You don't need this AutoJSAPI as far as I can tell.  You only use it for rooting, and that's what RootingCx() is for.

Ok, thank you, that's what I am looking for!

> This will do very bad things if SPIDERMONKEY_PROMISE is not defined.  Please have a separate implementation of this function for the !SPIDERMONKEY_PROMISE case.

So, if SPIDERMONKEY_PROMISE is not defined, there is a mState member which keeps the state of the promise and initialized to pending; it will be settled each time the promise is resolved or rejected. 
I think what I need to do is just returning this mState member if SPIDERMONKEY_PROMISE is not defined.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> I think what I need to do is just returning this mState member if SPIDERMONKEY_PROMISE is not defined.

Correct.

Comment 15

[Tzuhao Kuo [:kaku]]

Comment on attachment 8788390 [details]
Bug 1300071 - Create an API to access DOM Promise's state;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76886/diff/1-2/

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8788390 [details]
Bug 1300071 - Create an API to access DOM Promise's state;

https://reviewboard.mozilla.org/r/76886/#review75466

r=me

Comment 17

[Tzuhao Kuo [:kaku]]

Thanks for the review!

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=30972bb59a42

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad2e3708134a
Create an API to access DOM Promise's state. r=bz

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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