Closed Bug 1300071 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: kaku, Assigned: kaku)

References

Details

Attachments

(1 file)

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.
Blocks: 1244768
QA Contact: kaku
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.)
This is about Promise, not MozPromise, right?
(In reply to Boris Zbarsky [:bz] from comment #2)
> This is about Promise, not MozPromise, right?
Yes.
Oops, this is embarrassing. Sorry for the confusion.
(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.
Summary: Create an API to access Promise's state {pending, resolved or rejected} → Create an API to access DOM Promise's state {pending, resolved or rejected}
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?
Flags: needinfo?(bzbarsky)
Or maybe just take advantage of the JS::GetPromiseState() utility?

http://searchfox.org/mozilla-central/source/js/src/jsapi.cpp#4680
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.
Flags: needinfo?(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.  ;)
This should be in the DOM bugzilla component.
Component: Audio/Video: Playback → DOM: Core & HTML
Assignee: nobody → kaku
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.
Attachment #8788390 - Flags: review?(bzbarsky) → review-
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.
> I think what I need to do is just returning this mState member if SPIDERMONKEY_PROMISE is not defined.

Correct.
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
Attachment #8788390 - Flags: review?(bzbarsky) → review+
Thanks for the review!

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=30972bb59a42
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad2e3708134a
Create an API to access DOM Promise's state. r=bz
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ad2e3708134a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.