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

RESOLVED FIXED in Firefox 64

Status

()

enhancement
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: arai, Assigned: arai)

Tracking

Trunk
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

9 months ago
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.
Assignee

Comment 1

9 months ago
till, can I have your opinion?
Flags: needinfo?(till)
Assignee

Comment 2

9 months ago
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.
Assignee

Comment 3

9 months ago
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Flags: needinfo?(till)
Attachment #9012518 - Flags: review?(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.
Attachment #9012518 - Flags: review?(till) → review+
Assignee

Comment 5

9 months ago
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.
Assignee

Comment 6

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

Comment 7

9 months ago
Backout by dluca@mozilla.com:
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
Assignee

Comment 8

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

Comment 9

9 months ago
dom/bindings/test/test_promise_rejections_from_jsimplemented.html passes with this change.
Attachment #9012518 - Attachment is obsolete: true
Attachment #9012681 - Flags: review?(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
Attachment #9012681 - Flags: review?(till) → review+

Updated

9 months ago
Blocks: 1491403
Assignee

Comment 11

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

Comment 12

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e7d5eb0d44a7
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.