Closed Bug 1494518 Opened 2 years ago Closed 2 years ago

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


(Core :: JavaScript Engine, enhancement)

Not set



Tracking Status
firefox64 --- fixed


(Reporter: arai, Assigned: arai)




(1 file, 1 obsolete file)
>     // 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.
till, can I have your opinion?
Flags: needinfo?(till)
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: nobody → arai.unmht
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+
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.
Bug 1494518 - Do not pass non-PromiseObject to JSRuntime::enqueuePromiseJob. r=till
Backout by
Backed out changeset 2188bef55157 for mochitest failures in dom/bindings/test/test_promise_rejections_from_jsimplemented.html
I overlooked the case that reaction->promise() is already-wrapped PromiseObject at that point.
I'll add another branch.
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]:

Attachment #9012681 - Flags: review?(till) → review+
Blocks: 1491403
Bug 1494518 - Do not pass non-PromiseObject to JSRuntime::enqueuePromiseJob. r=till
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.