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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

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);

Comment 1

[Jason Orendorff [:jorendorff]]

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

Comment 2

[ziyunfei]

Functions with rest arguments:

function f(a, ...b){}
assertEq(f.length, 1);  // pass
assertEq(f.bind(null).length, 1);  // FAIL

Comment 3

[ziyunfei]

(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?

Comment 4

[Till Schneidereit [:till]]

> 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

Comment 5

[Tom S [:evilpie]]

Attachment: Implement ES6 Function.prototype.bind

Comment 6

[Tom S [:evilpie]]

Just realized that we should probably avoid the DefineProperty call when length isn't truncated.

Comment 7

[Till Schneidereit [:till]]

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.

Comment 8

[Tom S [:evilpie]]

Attachment: v2 Implement ES6 Function.prototype.bind

Comment 9

[Till Schneidereit [: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/

Comment 10

[Tom S [:evilpie]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing the same xpcshell failures that the Try pushes for this bug were showing.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b6aa9b4039b

https://treeherder.mozilla.org/logviewer.html#?job_id=6417363&repo=mozilla-inbound

Comment 12

[Ryan VanderMeulen [:RyanVM]]

And Gaia unit test failures:
https://treeherder.mozilla.org/logviewer.html#?job_id=6418506&repo=mozilla-inbound

Comment 13

[Tom S [:evilpie]]

Meh, Gaia failures are not fun.

Comment 14

[Tom S [:evilpie]]

I am going to split the function name changes to a different bug, so that we can coordinate this easier.

Comment 15

[Tom S [:evilpie]]

Attachment: Function.prototype.bind now throws for some wrapper

Comment 16

[Boris Zbarsky [: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.

Comment 17

[Bobby Holley (:bholley)]

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.

Comment 18

[Tom S [:evilpie]]

Attachment: v2 Function.prototype.bind now throws for some wrapper

Like this? I left the bind test in, don't think it hurts.

Comment 19

[Bobby Holley (:bholley)]

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.

Comment 20

[Tom S [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7344fdc1d969
I opened bug 1132630 to change the name of bound functions now.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7344fdc1d969