779626 - Javascript-global-constructor objects should be passed a window reference

Status: RESOLVED FIXED

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

Description

[Anant Narayanan [:anant]]

In bug 723206, we added the ability to receive arguments for global constructors implemented in IDL. Javascript initializers are passed a reference to the calling window, but constructors aren't, and is required in many cases.

The proposal is to append a window reference to the end of the argument list passed to the constructor in order to remain compatible with existing objects registered as Javascript-global-constructors.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I understand you're working on this.

Comment 2

[Anant Narayanan [:anant]]

Attachment: Pass the calling as the last argument to the constructor

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 648503 [details] [diff] [review]
Pass the calling as the last argument to the constructor

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

This looks right, but I think we should pass the window as the first argument instead of the last. That way code can be written that knows where the window will be passed.

::: dom/base/nsDOMClassInfo.cpp
@@ +5738,5 @@
>            rooter.changeLength(i + 1);
>          }
>  
> +        // Append window to the end of arguments
> +        JS::Value win;

Why not wrap directly into args, i.e. why use the temporary?

@@ +5742,5 @@
> +        JS::Value win;
> +        nsCOMPtr<nsIXPConnectJSObjectHolder> holder;
> +        nsCOMPtr<nsIDOMWindow> currentWin(do_GetInterface(currentInner));
> +        rv = WrapNative(cx, obj, currentWin, &NS_GET_IID(nsIDOMWindow),
> +          true, &win, getter_AddRefs(holder));

Nit: Please make |true| line up with |cx|.

Comment 4

[Anant Narayanan [:anant]]

(In reply to Blake Kaplan (:mrbkap) from comment #3)
> This looks right, but I think we should pass the window as the first
> argument instead of the last. That way code can be written that knows where
> the window will be passed.

I didn't want existing code that used Javascript-global-constructor to break. Should I look for existing instances and roll in the changes needed into this patch?

Comment 5

[Anant Narayanan [:anant]]

Ok, turns out it's not really a big deal since there's only one user of Javascript-global-constructor currently (https://mxr.mozilla.org/mozilla-central/search?string=Javascript-global-constructor) and it doesn't use any arguments. I'll post an updated patch.

Comment 6

[Alex Vincent [:WeirdAl]]

(In reply to Anant Narayanan [:anant] from comment #4)
> I didn't want existing code that used Javascript-global-constructor to
> break. Should I look for existing instances and roll in the changes needed
> into this patch?

Please keep add-ons and documentation for them in mind when you do this... I'm not using this currently in any add-on work, but in the past at other companies I might have.

Comment 7

[Anant Narayanan [:anant]]

Attachment: Pass the calling as the first argument to the constructor

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 648610 [details] [diff] [review]
Pass the calling as the first argument to the constructor

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

::: dom/base/nsDOMClassInfo.cpp
@@ +5726,5 @@
>          JSObject* thisObject = &thisValue.toObject();
>  
>          // wrap parameters in the target compartment
> +        // we also pass in the calling window as the first argument
> +        argc += 1;

++argc;

@@ +5738,5 @@
> +        rv = WrapNative(cx, obj, currentWin, &NS_GET_IID(nsIDOMWindow),
> +          true, &args[i], getter_AddRefs(holder));
> +        if (!JS_WrapValue(cx, &args[i]))
> +          return NS_ERROR_FAILURE;
> +        rooter.changeLength(i + 1);

I wouldn't use the i here; rather just use args[0].

Comment 9

[Anant Narayanan [:anant]]

Attachment: Pass the calling window as the first argument to the constructor

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 648719 [details] [diff] [review]
Pass the calling window as the first argument to the constructor

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

Thanks!

Comment 11

[Anant Narayanan [:anant]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8f98c855ecc

Comment 12

[Ed Morley [:emorley]]

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