Closed Bug 1073816 Opened 5 years ago Closed 5 years ago

Function.prototype.bind creates bound function with wrong length (with default/rest arguments or proxy)

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: jorendorff, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

ziyunfei mentioned this bug on IRC:

    js> [].contains.length
    1
    js> [].contains.bind().length
    2

It's really a bug in Function.prototype.bind, not Array.prototype.contains.

ES6 says that Function.prototype.bind actually works by getting the "length" property of the this-value.
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-function.prototype.bind

We don't do this. It's observable in the case of proxies and in this case. Here's a test using a proxy:

----

var log = "";
var p = new Proxy(function () {}, {
    get(target, name) {
        log += `get ${name}\n`;
        return name === "length" ? 17 : target[name];
    },
});

var bp = p.bind(null, 1, 2, 3);
assertEq(log, "get bind\n" + "get length\n");
assertEq(bp.length, 14);
mjrosenb pointed out that this affects all functions with defaults. Here's a test:

function f(a, b=1, c=2){}
assertEq(f.length, 1);  // pass
assertEq(f.bind(null).length, 1);  // FAIL
Summary: Function.prototype.bind creates bound function with wrong length (when applied to Array.prototype.contains) → Function.prototype.bind creates bound function with wrong length (with default arguments or proxy)
Functions with rest arguments:

function f(a, ...b){}
assertEq(f.length, 1);  // pass
assertEq(f.bind(null).length, 1);  // FAIL
Summary: Function.prototype.bind creates bound function with wrong length (with default arguments or proxy) → Function.prototype.bind creates bound function with wrong length (with default/rest arguments or proxy)
(In reply to Jason Orendorff [:jorendorff] from comment #0)
> var log = "";
> var p = new Proxy(function () {}, {
>     get(target, name) {
>         log += `get ${name}\n`;
>         return name === "length" ? 17 : target[name];
>     },
> });
> 
> var bp = p.bind(null, 1, 2, 3);
> assertEq(log, "get bind\n" + "get length\n");
> assertEq(bp.length, 14);

js>p.length
typein:22:0 TypeError: proxy must report the same value for a non-writable, non-configurable property

Do we have a bug to make function.length configurable?
> Do we have a bug to make function.length configurable?

Yes, bug 911142. It's not trivial to implement, unfortunately.

That said, I don't think it's required to fix this bug. The way we derive the source function's .length value in js_fun_bind[1] should be changed to match what we do in js::fun_resolve[2].

Also note bug 1000780.


[1] http://mxr.mozilla.org/mozilla-central/source/js/src/jsfun.cpp?rev=c477d2a7ab65#1626
[2] http://mxr.mozilla.org/mozilla-central/source/js/src/jsfun.cpp?rev=c477d2a7ab65#490
Blocks: es6
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
Attachment #8561368 - Flags: review?(till)
Just realized that we should probably avoid the DefineProperty call when length isn't truncated.
Comment on attachment 8561368 [details] [diff] [review]
Implement ES6 Function.prototype.bind

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

Excellent!

r=me with the resolve hooks avoided and nits addressed.

::: js/src/jsfun.cpp
@@ +1667,5 @@
>  {
> +    // Steps 5-6.
> +    RootedId id(cx, NameToId(cx->names().length));
> +    bool hasLength;
> +    if (!HasOwnProperty(cx, target, id, &hasLength))

I'm not quite sure, but this probably triggers the resolve hook on JSFunctions and causes the `length` property to be defined on the target, too. We should avoid that by checking if target is a JSFunction and then whether it has `hasResolvedLength()` set. If not, use `nargs` as before.

@@ +1670,5 @@
> +    bool hasLength;
> +    if (!HasOwnProperty(cx, target, id, &hasLength))
> +        return false;
> +
> +    // Step 7.

Steps 7-8.

@@ +1687,5 @@
>  
> +    // Steps 11-12.
> +    id = NameToId(cx->names().name);
> +    RootedValue targetName(cx);
> +    if (!GetProperty(cx, target, target, id, &targetName))

Same as for `length`: check for `hasResolvedName()` and use the atom if it's not set.

@@ +1689,5 @@
> +    id = NameToId(cx->names().name);
> +    RootedValue targetName(cx);
> +    if (!GetProperty(cx, target, target, id, &targetName))
> +        return false;
> +    // Step 13.

Nit: new-line above comment.

@@ +1697,4 @@
>  
> +    // Step 14. Relevant bits from SetFunctionName.
> +    StringBuffer sb(cx);
> +    if (!sb.append("bound") || !sb.append(' ') || !sb.append(name))

I would combine the first two appends into sb.append("bound ").

@@ +1719,3 @@
>          return nullptr;
>  
> +    // Steps 9-10. Set length again, because NewFunction truncates.

As you already mentioned, we should only do this if the length has changed.
Attachment #8561368 - Flags: review?(till) → review+
Attachment #8561368 - Attachment is obsolete: true
Attachment #8561415 - Flags: review?(till)
Comment on attachment 8561415 [details] [diff] [review]
v2 Implement ES6 Function.prototype.bind

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

\o/

::: js/src/jsfun.cpp
@@ +1670,5 @@
> +    if (target->is<JSFunction>() && !target->as<JSFunction>().hasResolvedLength()) {
> +        uint16_t len;
> +        if (!target->as<JSFunction>().getLength(cx, &len))
> +            return false;
> +

Nit: no new-line required.

@@ +1688,5 @@
> +                return false;
> +            // d.
> +            if (targetLen.isNumber()) {
> +                length = JS::ToInteger(targetLen.toNumber());
> +                length = Max(0.0, length - argslen);

I would probably move the `Max` call to below the if/else to have it only once. Up to you, though.

@@ +1733,3 @@
>          return nullptr;
>  
> +    // Steps 9-10. Set length again, because can NewFunction truncate it.

s/can NewFunction truncate/NewFunction sometimes truncates/
Attachment #8561415 - Flags: review?(till) → review+
Meh, Gaia failures are not fun.
Depends on: 1132219
I am going to split the function name changes to a different bug, so that we can coordinate this easier.
Blocks: 1132630
Attachment #8564073 - Flags: review?(bzbarsky)
Comment on attachment 8564073 [details] [diff] [review]
Function.prototype.bind now throws for some wrapper

Going to punt this to Bobby, because I don't understand the implications of this behavior change.
Attachment #8564073 - Flags: review?(bzbarsky) → review?(bobbyholley)
Comment on attachment 8564073 [details] [diff] [review]
Function.prototype.bind now throws for some wrapper

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

So the issue is that bind now tries to get the length property of the callable when it's invoked, which causes an exception?

::: js/xpconnect/tests/unit/test_bug930091.js
@@ +17,5 @@
>    sb.cow = { foopy: 2, __exposedProps__: { foopy: 'rw' } };
>    sb.payload = Cu.evalInSandbox('new Object()', xosb);
>    Cu.evalInSandbox(checkThrows.toSource(), sb);
> +  Cu.evalInSandbox('checkThrows(function() { Function.bind.call(fun, null, payload); });', sb);
> +  Cu.evalInSandbox('checkThrows(function() { Function.bind.call(fun, payload); });', sb);

This isn't going to be testing what we want anymore - it'll just be testing that bind throws (which is a good thing to test if that's the new behavior, but not sufficient). The thing we're interested in testing is that invoking |fun| with payload as an argument (including as |this|) fails a security check.

If you need to get rid of the .bind, this should be:

checkThrows(() => fun(payload));
and
checkThrows(() => Function.call.call(fun, payload));

Or somesuch.
Attachment #8564073 - Flags: review?(bobbyholley) → review-
Like this? I left the bind test in, don't think it hurts.
Attachment #8564073 - Attachment is obsolete: true
Attachment #8564402 - Flags: review?(bobbyholley)
Comment on attachment 8564402 [details] [diff] [review]
v2 Function.prototype.bind now throws for some wrapper

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

r=me with that.

::: js/xpconnect/tests/unit/test_bug930091.js
@@ +17,5 @@
>    sb.cow = { foopy: 2, __exposedProps__: { foopy: 'rw' } };
>    sb.payload = Cu.evalInSandbox('new Object()', xosb);
>    Cu.evalInSandbox(checkThrows.toSource(), sb);
> +  Cu.evalInSandbox('checkThrows(function() { Function.bind.call(fun, null, payload); });', sb);
> +  Cu.evalInSandbox('checkThrows(function() { Function.bind.call(fun, payload); });', sb);

Move these to the bottom and explain why they fail.
Attachment #8564402 - Flags: review?(bobbyholley) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/7344fdc1d969
I opened bug 1132630 to change the name of bound functions now.
https://hg.mozilla.org/mozilla-central/rev/7344fdc1d969
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.