1494518 - The comment about promise doesn't match to the code in EnqueuePromiseReactionJob.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Tooru Fujisawa [:arai]]

https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/js/src/builtin/Promise.cpp#1008-1025
>     // When using JS::AddPromiseReactions, no actual promise is created, so we
>     // might not have one here.
>     // Additionally, we might have an object here that isn't an instance of
>     // Promise. This can happen if content overrides the value of
>     // Promise[@@species] (or invokes Promise#then on a Promise subclass
>     // instance with a non-default @@species value on the constructor) with a
>     // function that returns objects that're not Promise (subclass) instances.
>     // In that case, we just pretend we didn't have an object in the first
>     // place.
>     // If after all this we do have an object, wrap it in case we entered the
>     // handler's compartment above, because we should pass objects from a
>     // single compartment to the enqueuePromiseJob callback.
>     RootedObject promise(cx, reaction->promise());
>     if (promise && promise->is<PromiseObject>()) {
>         if (!cx->compartment()->wrap(cx, &promise)) {
>             return false;
>         }
>     }
> ...
>     // Note: the global we pass here might be from a different compartment
>     // than job and promise. While it's somewhat unusual to pass objects
>     // from multiple compartments, in this case we specifically need the
>     // global to be unwrapped because wrapping and unwrapping aren't
>     // necessarily symmetric for globals.
>     return cx->runtime()->enqueuePromiseJob(cx, job, promise, global);
> }

The comment says:
  * `promise` passed to `enqueuePromiseJob` should be from the same compartment as `job`
  * we might have non-PromiseObject
  * if we have non-PromiseObject, we pretend we didn't have an object

The code does:
  * get the promise we have
  * wrap it if it's PromiseObject
  * pass the maybe-wrapped object to enqueuePromiseJob
  * if we have non-PromiseObject, we pass it without wrapping

So the behavior for non-PromiseObject is different between them.
we should fix either the comment or the code (I guess we should fix the code?)

bug 1491403 is going to pass the `promise` to the embedder's callback, so we should be more careful what we pass.
I think we should add some assertion in JSRuntime::enqueuePromiseJob.

Comment 1

[Tooru Fujisawa [:arai]]

till, can I have your opinion?

Comment 2

[Tooru Fujisawa [:arai]]

for current JSRuntime::enqueuePromiseJob and bug 1491403 use case, we don't need non-PromiseObject.
(we'll need to think about other cases anyway)
I'll post a patch that just stop passing non-PromiseObject.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Do not pass non-PromiseObject to JSRuntime::enqueuePromiseJob.

Comment 4

[Till Schneidereit [:till]]

Comment on attachment 9012518 [details] [diff] [review]
Do not pass non-PromiseObject to JSRuntime::enqueuePromiseJob.

Review of attachment 9012518 [details] [diff] [review]:
-----------------------------------------------------------------

Oh huh. Is this something we could have a test case for? If so, would be great to add. If it's hard, then don't bother.

Comment 5

[Tooru Fujisawa [:arai]]

Thank you for reviewing :D

currently the non-PromiseObject is not used.
we could add testcase after bug 1491403, but it would depend on how it's used there.

Comment 6

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2188bef5515739adde624aa79b998ce72e4c90b7
Bug 1494518 - Do not pass non-PromiseObject to JSRuntime::enqueuePromiseJob. r=till

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/902ed3a61434
Backed out changeset 2188bef55157 for mochitest failures in dom/bindings/test/test_promise_rejections_from_jsimplemented.html

Comment 8

[Tooru Fujisawa [:arai]]

I overlooked the case that reaction->promise() is already-wrapped PromiseObject at that point.
I'll add another branch.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Do not pass non-PromiseObject to JSRuntime::enqueuePromiseJob. r=till

dom/bindings/test/test_promise_rejections_from_jsimplemented.html passes with this change.

Comment 10

[Till Schneidereit [:till]]

Comment on attachment 9012681 [details] [diff] [review]
Do not pass non-PromiseObject to JSRuntime::enqueuePromiseJob. r=till

Review of attachment 9012681 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 11

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7d5eb0d44a7ee8b6d53d91d96986af873fa16af
Bug 1494518 - Do not pass non-PromiseObject to JSRuntime::enqueuePromiseJob. r=till

Comment 12

[Andrei Ciure[:aciure]]

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