Properly handle resolve/reject callbacks on xray'd promises

RESOLVED FIXED in Firefox 52



JavaScript: Standard Library
a year ago
a year ago


(Reporter: till, Assigned: till)



Firefox Tracking Flags

(firefox51 affected, firefox52 fixed)


MozReview Requests


Submitter Diff Changes Open Issues Last Updated
Error loading review requests:


(1 attachment)

Comment hidden (empty)
Comment hidden (mozreview-request)

Comment 2

a year ago
@bz: I tried to find a way to test this, but, because I know little about how to create tests that would be able to invoke the JSAPI functions *and* create Xray'd objects, I wasn't able to. Maybe you can give me a hint for how to go about this?
Assignee: nobody → till
Flags: needinfo?(bzbarsky)
Which JSAPI functions?
Flags: needinfo?(bzbarsky) → needinfo?(till)

Comment 4

a year ago
Comment on attachment 8785353 [details]
Bug 1298414 - Properly handle resolve/reject callbacks on xray'd promises. ,f?bz

Looks sane to me.
Attachment #8785353 - Flags: review?(efaustbmo) → review+

Comment 5

a year ago
(In reply to Boris Zbarsky [:bz] from comment #3)
> Which JSAPI functions?

Sorry, that was pretty unclear. I mean JS::ResolvePromise and JS::RejectPromise, which call PromiseObject::resolve/reject.
Flags: needinfo?(till) → needinfo?(bzbarsky)
Ah, ok.

I think your best bet is to add something to TestingFunctions that will do the ResolvePromise/RejectPromise call.  Then in a chrome mochitest where you can get access to Promise Xrays (e.g. dom/promise/tests/test_promise_xrays.html is a perfectly reasonable place for this) you can do:

  var obj = Components.utils.getJSTestingFunctions();
Flags: needinfo?(bzbarsky)
Comment hidden (mozreview-request)

Comment 8

a year ago
Testing this turned out to be useful: rejecting xray'd promises didn't actually worked.

I failed to figure out how to re-request a review in mozreview, so setting needinfo instead.
Flags: needinfo?(efaustbmo)
Flags: needinfo?(bzbarsky)
Comment on attachment 8785353 [details]
Bug 1298414 - Properly handle resolve/reject callbacks on xray'd promises. ,f?bz

::: dom/promise/tests/test_promise_xrays.html:237
(Diff revision 2)
> +function testResolve4() {
> +  var p = new win.Promise((res, rej) => {});
> +  Components.utils.getJSTestingFunctions().resolvePromise(p, 42);
> +  p.then(
> +    function(arg) {
> +      is(arg, 42, "TestingFunctions resolvePromise with chrome Promise should work");

There is no chrome Promise here.  For testResolve3 the Promise used as the resolution value came from chrome, which is why the strings were what they were.

Over here, it's more like "Resolving an Xray to a promise with TestingFunctions resolvePromise should work" and similar for the "should not fail" case.  And similar in testReject2.

::: js/src/builtin/Promise.cpp:964
(Diff revision 2)
>  {
>      if (state() != JS::PromiseState::Pending)
>          return true;
>      RootedValue funVal(cx, this->getReservedSlot(PROMISE_RESOLVE_FUNCTION_SLOT));
> -    // TODO: ensure that this holds for xray'd promises. (It probably doesn't)
> +    MOZ_ASSERT(IsCallable(funVal));

So funVal can't end up being a dead object wrapper?

Attachment #8785353 - Flags: review+
Flags: needinfo?(bzbarsky)
I don't know how to re-review either, but r=me on the update.

Just adding ni? so this makes more noise in bugmail ;)
Flags: needinfo?(efaustbmo) → needinfo?(till)

Comment 11

a year ago
Pushed by
Properly handle resolve/reject callbacks on xray'd promises. r=bz,efaust


a year ago
Flags: needinfo?(till)

Comment 12

a year ago
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52


a year ago
Depends on: 1319005
You need to log in before you can comment on or make changes to this bug.