1323930 - Align promise handling for callback return values with spec

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Per spec at https://heycam.github.io/webidl/#call-a-user-objects-operation step 19 or https://heycam.github.io/webidl/#get-a-user-objects-attribute-value step 10 or https://heycam.github.io/webidl/#invoke-a-callback-function step 17, we do the JS to IDL conversion before we do "clean up after running script", which means the current Realm at that point is the Realm that was pushed in the corresponding "prepare to run script" steps.  That's the Realm defined by https://heycam.github.io/webidl/#dfn-associated-realm for the object in question.

In terms of our impl, it's the entry global's compartment, which is the compartment of the UncheckedUnwrap of the object we're working with.

Right now we end up doing the to-JS conversion while in the compartment of whatever our callback object is, which isn't right.

Now the good news is that I'm not sure this is really observable _except_ in the case of Promises returned by callbacks.  For those, we want to use the Promise.resolve from the right global.

Patch coming up.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Align the handling of Promise return values from callbacks with the current wording of the WebIDL spec

Bobby, I think you have the best chance of knowing something about my XXX comments.  Put another way, can I just do |globalObj = GetEntryGlobal()->GetGlobalJSObject()| with no null-checks?

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8819199 [details] [diff] [review]
Align the handling of Promise return values from callbacks with the current wording of the WebIDL spec

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

Not really sure about the spec parts here, but I'll trust you on it. rs=me, but would be good to have tests.

::: dom/bindings/Codegen.py
@@ +5300,5 @@
> +                    // principal-clamping it ends up doing.
> +                    { // Scope for entry global
> +                      nsIGlobalObject* entryGlobal = GetEntryGlobal();
> +                      // XXXbz can entryGlobal ever be null here, given that
> +                      // we're under an AutoEntryScript?

Only if somebody put an AutoNoJSAPI on the stack in the mean time. Given that this is binding code, I think we should assert against it.

@@ +5303,5 @@
> +                      // XXXbz can entryGlobal ever be null here, given that
> +                      // we're under an AutoEntryScript?
> +                      if (entryGlobal) {
> +                        JSObject* global = entryGlobal->GetGlobalJSObject();
> +                        // XXXbz can global be null here?  I wouldn't think so....

No, the AutoEntryScript/AutoJSAPI initialization routines either fail or assert against it failing, depending on the callpath.

Anyway, this can all tetris down to GetEntryGlobal->GetGlobalJSObject(), with the scoping removed.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1aa4dd218e13
Align the handling of Promise return values from callbacks with the current wording of the WebIDL spec.  r=bholley

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1aa4dd218e13