The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla17

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: anant, Assigned: anant)

Tracking

({addon-compat, dev-doc-needed})

Trunk
mozilla17
addon-compat, dev-doc-needed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
I understand you're working on this.
Assignee: nobody → anant
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 2

5 years ago
Created attachment 648503 [details] [diff] [review]
Pass the calling as the last argument to the constructor
Attachment #648503 - Flags: review?(mrbkap)
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|.
Attachment #648503 - Flags: review?(mrbkap)
(Assignee)

Comment 4

5 years ago
(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?
(Assignee)

Comment 5

5 years ago
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.
(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.
(Assignee)

Comment 7

5 years ago
Created attachment 648610 [details] [diff] [review]
Pass the calling as the first argument to the constructor
Attachment #648503 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #648610 - Flags: review?(mrbkap)

Updated

5 years ago
Attachment #648610 - Attachment description: Pass the calling as the last argument to the constructor → Pass the calling as the first argument to the constructor
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].

Updated

5 years ago
Keywords: addon-compat, dev-doc-needed
(Assignee)

Comment 9

5 years ago
Created attachment 648719 [details] [diff] [review]
Pass the calling window as the first argument to the constructor
Attachment #648610 - Attachment is obsolete: true
Attachment #648610 - Flags: review?(mrbkap)
Attachment #648719 - Flags: review?(mrbkap)
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!
Attachment #648719 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8f98c855ecc
https://hg.mozilla.org/mozilla-central/rev/f8f98c855ecc
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.