Closed Bug 1081385 Opened 5 years ago Closed 2 years ago

exportFunction produces wrong .length attribute

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: bugzilla, Assigned: bzbarsky)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140923175406

Steps to reproduce:

I used exportFunction() to create a callable function in the unprivileged (unsafeWindow) context. In the loaded document I accessed the .length attribute of this function


Actual results:

I got a value of 0.


Expected results:

The real number of arguments of the exported function should be returned.
Gabor, can you look at this and put it in the right component?
Flags: needinfo?(gkrizsanits)
Component: General → XPConnect
Flags: needinfo?(gkrizsanits)
Product: Add-on SDK → Core
I think the fix is to get the length of the |callable| here http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/ExportHelpers.cpp#330 and then pass it to NewFunctionByIdWithReserved. I think this would be a good first bug candidate.
Do we want to just do .length, or JS_GetFunctionArity combined with JS_GetObjectFunction (and bail on non-function callables, but get "saner" results for functions if someone mucked with .length on them)?
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to Boris Zbarsky [:bz] from comment #3)
> Do we want to just do .length, or JS_GetFunctionArity combined with
> JS_GetObjectFunction (and bail on non-function callables, but get "saner"
> results for functions if someone mucked with .length on them)?

I would do JS_GetFunctionArity. I don't see any reason to port any mockery on the .length property to the exported version.
It basically depends on what we want the behavior to be when exporting function proxies or other non-function callables.
Are there any news on this bug?
A bit of background why I think is is important. It allows fingerprinting the browser for addons that use exportFunction() by checking if the length attribute matches the expected value.
Not only the value of the length property is wrong but also the property descriptor: ECMA says that [[Configurable]] should be true but after exportFunction it's false.

This prevents me from fixing this bug on the addon side.
Is this really that hard to fix? Or just not interesting enough?
The hard part is that it's not clear what the right fix is for various edge cases.

That said I'm happy to just make a call on it.  Let's do comment 4.
Mentor: bzbarsky
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Mentor: bzbarsky
Thanks for taking the responsibility.
Comment on attachment 8936711 [details] [diff] [review]
Give functions created via exportFunction the same .length as the function being exported

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

Thanks for fixing this.

::: js/xpconnect/src/ExportHelpers.cpp
@@ +334,5 @@
> +    unsigned nargs = 0;
> +    RootedObject unwrapped(cx, js::UncheckedUnwrap(callable));
> +    if (unwrapped) {
> +        if (JSFunction* fun = JS_GetObjectFunction(unwrapped)) {
> +            nargs = JS_GetFunctionArity(fun);

Nit: I think in js/xpconnect the {} should be avoided for one liner if branches (that used to be the case at least, I haven't been following any coding style discussions lately). Also it might make sense to merge the two if's.
Attachment #8936711 - Flags: review?(gkrizsanits) → review+
> the {} should be avoided for one liner if branches 

Will do!

> Also it might make sense to merge the two if's.

Can't do that, because the second "if" is declaring "fun".
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74c036bf1c66
Give functions created via exportFunction the same .length as the function being exported.  r=gabor
https://hg.mozilla.org/mozilla-central/rev/74c036bf1c66
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.