Function.prototype.bind: Bound functions must use the [[Prototype]] of their target function instead of Function.prototype

• Component: JavaScriptCore
• Status: RESOLVED DUPLICATE

[Claude Pache]

A function object constructed using Function#bind must retain the [[Prototype]] of its target function.
Currently, it gets the default one, namely Function.prototype.

This is a breaking change introduced in ES6.

For the reference, see steps 2 and 8 of BoundFunctionCreate() in the spec:
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-boundfunctioncreate

For the justification, see https://bugs.ecmascript.org/show_bug.cgi?id=3819#c0

Here is a test case:

```
function getProtoBound(proto) {
    var f = Object.setPrototypeOf(function() { }, proto)
    var boundF = Function.prototype.bind.call(f, null)
    return Object.getPrototypeOf(boundF)
}

for (var proto of [ Function.prototype, function(){}, [], null ]) {
    console.log(Object.is(getProtoBound(proto), proto)) // should always log true
}
```

[Matthew Hill]

I'd like to do this. Seems like it should be easy enough for a first contrib.

[Geoffrey Garen]

Sounds good. Indeed, it should be a simple patch.

[Matthew Hill]

So I'm trying to run the tests, and there seem to already be (failing) tests cases for this bug. After making some changes, when I run `Tools/Scripts/run-javascriptcore-tests`, I get the following output:


````
........cat: test_fail_24491: No such file or directory
cat: test_fail_24492: No such file or directory
cat: test_fail_24493: No such file or directory
es6.yaml/es6/prototype_of_bound_functions_arrow_functions.js.default: ERROR: Unexpected exit code:
es6.yaml/es6/prototype_of_bound_functions_basic_functions.js.default: ERROR: Unexpected exit code:
es6.yaml/es6/prototype_of_bound_functions_classes.js.default: ERROR: Unexpected exit code:
24493/24976 (failed 3) .....cat: test_fail_24495: No such file or directory
es6.yaml/es6/prototype_of_bound_functions_subclasses.js.default: ERROR: Unexpected exit code:
24976/24976 (failed 4)

** The following JSC stress test failures have been introduced:
	es6.yaml/es6/prototype_of_bound_functions_arrow_functions.js.default
	es6.yaml/es6/prototype_of_bound_functions_basic_functions.js.default
	es6.yaml/es6/prototype_of_bound_functions_classes.js.default
	es6.yaml/es6/prototype_of_bound_functions_subclasses.js.default

Results for JSC stress tests:
    4 failures found.
````

Note the `cat` errors. I presume those files (test_fail_xxx) should contain the test output. How do I stop `cat` from err'ing out? The files it's referring to are empty

[Matthew Hill]

Or do the empty test output files indicate that the exit code was 0?

[Geoffrey Garen]

I'm not sure why your test output looks like that.

Still, you should be able to fix it by changing those tests from expected failures to expected passes, by editing es6.yaml.

[Matthew Hill]

Created attachment 260799 [details]
Patch

[Matthew Hill]

Thank you -- that did the trick.

Please see the attached patch. One thing I'm unusure of is how I should implement #3 of the `BoundFunctionCreate` spec, wrt `ReturnIfAbrupt`. How is this handled in JSC?

````
...
#2 Let proto be targetFunction.[[GetPrototypeOf]]().
#3 ReturnIfAbrupt(proto).
...
````

[Geoffrey Garen]

> Please see the attached patch. One thing I'm unusure of is how I should
> implement #3 of the `BoundFunctionCreate` spec, wrt `ReturnIfAbrupt`. How is
> this handled in JSC?
> 
> ````
> ...
> #2 Let proto be targetFunction.[[GetPrototypeOf]]().
> #3 ReturnIfAbrupt(proto).
> ...

I think this just means that you should check for an exception when accessing the prototype. That isn't relevant until we implement proxies, since only proxies can throw when accessing their prototypes.

[Geoffrey Garen]

Comment on attachment 260799 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260799&action=review

> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:83
>      JSBoundFunction* function = new (NotNull, allocateCell<JSBoundFunction>(vm.heap)) JSBoundFunction(vm, globalObject, globalObject->boundFunctionStructure(), targetFunction, boundThis, boundArgs);

You can see that this code believed it could share a structure between all bound functions in a global object. Your patch makes that fact no longer true. It would be preferable to remove the old code, which attempted to share a structure, and simply allocate a new structure explicitly here.

[Matthew Hill]

Created attachment 260806 [details]
Patch

[Matthew Hill]

Thanks, I think I've done that now. Are structures used for IC, like v8 hidden classes?

[Geoffrey Garen]

> Are structures used for IC, like v8 hidden classes?

Yes.

[Geoffrey Garen]

Comment on attachment 260806 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 260806 [details]
Patch

Clearing flags on attachment: 260806

Committed r189522: <http://trac.webkit.org/changeset/189522>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

It looks like this may have caused a 4.3% regression on speedometer benchmark.

[Chris Dumez]

(In reply to comment #16)
> It looks like this may have caused a 4.3% regression on speedometer
> benchmark.

Tracked by <rdar://problem/22629950>.

[Geoffrey Garen]

Let's roll out this change until we can fix the regression.

It seems like we need a way to avoid allocating all those structures. A simple option is to use the default structure in cases where the target function has not yet allocated its .prototype property.

Come to think of it, we should also take care not to force the allocation of the .prototype property just to figure out if it exists. Most functions do not have a .prototype property because they are not called as constructors.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 149017

[Ryosuke Niwa]

Created attachment 260899 [details]
A/B testing results

It looks like ReactJS got 15% slower with this patch.

[Matthew Hill]

@Geoffrey:

Isn't the structure system designed to create one empty object structure per prototype? If so, how would the number of structures being allocated cause this regression?

And I don't see the relevance of the `prototype` property to a `bind` call? If my above assumption re. structures is incorrect, would it not make more sense to check whether [[Prototype]] is not Function.prototype, and only then create a new structure.

[Matthew Hill]

@Ryosuke 

How can I run these tests locally?

[Ryosuke Niwa]

(In reply to comment #22)
> @Ryosuke 
> 
> How can I run these tests locally?

Yes, run-perf-tests PerformanceTests/Speedometer/

[Chris Dumez]

Speedometer recovered after the roll out.

[Geoffrey Garen]

> @Geoffrey:
> 
> Isn't the structure system designed to create one empty object structure per
> prototype? If so, how would the number of structures being allocated cause
> this regression?

When you call Structure::create (via JSBoundFunction::createStucture), you allocate a structure. There is no default mechanism to avoid or de-duplicate that allocation. Allocation takes time and increases GC pressure.

> And I don't see the relevance of the `prototype` property to a `bind` call?

You're right: I misspoke when I mentioned .prototype.

> If my above assumption re. structures is incorrect, would it not make more
> sense to check whether [[Prototype]] is not Function.prototype, and only
> then create a new structure.

Yes, I think that's the right short-term solution: Maintain the per-global-object bound function structure, and only allocate a new structure if the per-global-object structure will not do.

[Matthew Hill]

Created attachment 260962 [details]
Patch

[Geoffrey Garen]

Comment on attachment 260962 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260962&action=review

> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:81
> +    JSValue targetPrototype = targetFunction->prototype();
> +    bool hasDefaultPrototype = &targetPrototype == (JSValue *)globalObject->functionPrototype();

This is wrong. You're taking the address of a value on the stack, which will never be equal to the address of a value on the heap.

You should notice in testing that the hasDefaultPrototype condition never holds true.

[Matthew Hill]

You're right. What should I do?

I originally felt I should use `JSValue::strictEqual` (which takes JSValue operands) but that requires an ExecState.

[Matthew Hill]

Created attachment 260999 [details]
Patch

[Matthew Hill]

The code *should* be correct now.

The reason I thought Structures for empty objects with a given prototype were cached was because I saw `PrototypeMap::emptyObjectStructureForPrototype`... any reason we don't use that here? It seems a waste to re-create the same Structure more than once. I'm sure this code will still perform poorly if someone has an array of functions with the same non-Function.prototype [[Prototype]] when they come to bind them.

[Geoffrey Garen]

(In reply to comment #30)
> The code *should* be correct now.
> 
> The reason I thought Structures for empty objects with a given prototype
> were cached was because I saw
> `PrototypeMap::emptyObjectStructureForPrototype`... any reason we don't use
> that here? It seems a waste to re-create the same Structure more than once.
> I'm sure this code will still perform poorly if someone has an array of
> functions with the same non-Function.prototype [[Prototype]] when they come
> to bind them.

"Empty object" means an instance of JSFinalObject. We can't use emptyObjectStructureForPrototype because we're creating an instance of JSBoundFunction.

[Geoffrey Garen]

Comment on attachment 260999 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260999&action=review

> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:81
> +    bool hasDefaultPrototype = targetPrototype.isObject() && asObject(targetPrototype) == (JSObject *)globalObject->functionPrototype();

There's no need to cast functionPrototype() to JSObject*: FunctionPrototype inherits from JSObject.

WebKit style uses C++ cast syntax rather than C syntax.

WebKit style puts the * next to the type name, as in "JSObject*".

The clearest and most efficient way to write this is "targetPrototype == globalObject->functionPrototype()". That's only one compare-and-branch instead of two.

[Matthew Hill]

Thanks Geoffrey. The reason for the cast was that it seemed to be necessary (see below).  

````
bool hasDefaultPrototype = targetPrototype == globalObject->functionPrototype();
````

doesn't compile. 

````
run-javascriptcore-tests

...

error: invalid operands to binary expression ('JSC::JSValue' and 'JSC::FunctionPrototype *')
    bool hasDefaultPrototype = targetPrototype == globalObject->functionPrototype();
                               ~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
````

Even 

````
asObject(targetPrototype) == globalObject->functionPrototype();
````

doesn't compile with

````
error: comparison of distinct pointer types ('JSC::JSObject *' and 'JSC::FunctionPrototype *')
    bool hasDefaultPrototype = asObject(targetPrototype) == globalObject->functionPrototype();
                               ~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
````

which I don't understand because as you rightly say, FunctionPrototype inherits from JSObject

[Geoffrey Garen]

> error: invalid operands to binary expression ('JSC::JSValue' and
> 'JSC::FunctionPrototype *')
>     bool hasDefaultPrototype = targetPrototype ==
> globalObject->functionPrototype();

To provide the right type, use JSValue(globalObject->functionPrototype()).

> error: comparison of distinct pointer types ('JSC::JSObject *' and
> 'JSC::FunctionPrototype *')
>     bool hasDefaultPrototype = asObject(targetPrototype) ==
> globalObject->functionPrototype();
>                                ~~~~~~~~~~~~~~~~~~~~~~~~~ ^ 
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ````
> 
> which I don't understand because as you rightly say, FunctionPrototype
> inherits from JSObject

In this formulation we have the right type but the compiler doesn't know it. So, you need to #include FunctionPrototype.h.

We shouldn't use this formulation, though, unless we can guarantee that the target callee in fact has a [[Prototype]]. I don't think we can guarantee that.

[Alexey Shvayka]

Thank you for the effort, Matthew.
The issue was fixed in https://trac.webkit.org/changeset/196956.

*** This bug has been marked as a duplicate of bug 154195 ***