Properly handle resolve/reject callbacks on xray'd promises

RESOLVED FIXED in Firefox 52

Status

()

Core
JavaScript: Standard Library
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: till, Assigned: till)

Tracking

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox51 affected, firefox52 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

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

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
mozreview-review
Comment on attachment 8785353 [details]
Bug 1298414 - Properly handle resolve/reject callbacks on xray'd promises. ,f?bz

https://reviewboard.mozilla.org/r/74588/#review73826

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

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();
  obj.whateverYourNewFunctionIs(whateverArgs);
Flags: needinfo?(bzbarsky)
Comment hidden (mozreview-request)
(Assignee)

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

https://reviewboard.mozilla.org/r/74588/#review74592

::: 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?

r=me
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 tschneidereit@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cf0ccb1a85c
Properly handle resolve/reject callbacks on xray'd promises. r=bz,efaust
(Assignee)

Updated

a year ago
Flags: needinfo?(till)

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2cf0ccb1a85c
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Updated

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