1478936 - JS_GetFunctionArity's comment doesn't match to the body

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

JSFunction::nargs() is not `function.length`, and JS_GetFunctionArity comment is wrong, and the consumer expects `function.length`.

https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/js/src/vm/JSFunction.h#123-124
> class JSFunction : public js::NativeObject
> {
> ...
>     uint16_t        nargs_;       /* number of formal arguments
>                                      (including defaults and the rest parameter unlike f.length) */

https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/js/src/vm/JSFunction.h#221-223
> class JSFunction : public js::NativeObject
> {
> ...
>     size_t nargs() const {
>         return nargs_;
>     }

https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/js/src/jsapi.h#3090-3094
> /*
>  * Return the arity (length) of fun.
>  */
> extern JS_PUBLIC_API(uint16_t)
> JS_GetFunctionArity(JSFunction* fun);

https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/js/src/jsapi.cpp#3666-3670
> JS_PUBLIC_API(uint16_t)
> JS_GetFunctionArity(JSFunction* fun)
> {
>     return fun->nargs();
> }

https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/js/xpconnect/wrappers/XrayWrapper.cpp#533-538
>         } else if (key == JSProto_Function) {
>             if (id == GetJSIDByIndex(cx, XPCJSContext::IDX_LENGTH)) {
>                 FillPropertyDescriptor(desc, wrapper, JSPROP_PERMANENT | JSPROP_READONLY,
>                                        NumberValue(JS_GetFunctionArity(JS_GetObjectFunction(target))));
>                 return true;
>             }

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Fix the comment for JS_GetFunctionArity, and add JS_GetFunctionLength which matches to the original comment, and fixed consumer.

JS_GetFunctionArity returns nargs, that is the number of comma-separated components in parameter, including default parameter and rest parameter.
JS_GetFunctionLength returns .length property of the function object.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8998405 [details] [diff] [review]
Fix the comment for JS_GetFunctionArity, and add JS_GetFunctionLength which matches to the original comment, and fixed consumer.

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

Nice :)

Comment 3

[Jan de Mooij [:jandem]]

Sorry I forgot this before: could you add assertSameCompartment(cx, fun); in JS_GetFunctionLength? Thanks!

Comment 4

[Tooru Fujisawa [:arai]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> Sorry I forgot this before: could you add assertSameCompartment(cx, fun); in
> JS_GetFunctionLength? Thanks!

unfortunately it fails
https://treeherder.mozilla.org/logviewer.html#?job_id=192933763&repo=try&lineNumber=8446

I'll investigate it.
maybe we should wrap it either in caller or in JS_GetFunctionLength.

Comment 5

[Jan de Mooij [:jandem]]

Yeah I think the caller should just use JSAutoRealm around the call.

It's not strictly necessary because when we delazify, we also enter the function's realm. However I think it's good if all APIs that take a cx + object assertSameCompartment so it's easier to reason about the code.

Comment 6

[Tooru Fujisawa [:arai]]

okay, I'll land with the fix.

Comment 7

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60a38b319b323f9b8c44040eedecb49715a129b0
Bug 1478936 - Fix the comment for JS_GetFunctionArity, and add JS_GetFunctionLength which matches to the original comment, and fixed consumer. r=jandem

Comment 8

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/60a38b319b32