Closed Bug 1298414 Opened 3 years ago Closed 3 years ago

Properly handle resolve/reject callbacks on xray'd promises

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- affected
firefox52 --- fixed

People

(Reporter: till, Assigned: till)

References

Details

Attachments

(1 file)

No description provided.
@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 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+
(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)
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)
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
Flags: needinfo?(till)
https://hg.mozilla.org/mozilla-central/rev/2cf0ccb1a85c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1319005
You need to log in before you can comment on or make changes to this bug.