Bug 26382

Summary: [ES5] Implement Function.prototype.bind
Product: WebKit Reporter: tav <tav>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, arv, barraclough, cmarcelo, erights, ggaren, gsnedders, jesus, john.david.dalton, kangax, kent.hansen, marcus, mathias, me, mike, mjs, oliver, pmuellr, simon_kaegi, skyul, webkit.review.bot, webmaster
Priority: P2 Keywords: ES5
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Attempt to implement Function.prototype.bind()
none
Attempt 2 to implement Function.prototype.bind()
none
Native implementation of Function.prototype.bind()
none
Optimization for bind() used just for binding 'this' (non JIT case)
none
Native implementation of Function.prototype.bind(), v4
none
Optimization for bind() used just for binding 'this' (non JIT case)
none
Optimization for bind() used just for binding 'this' (JIT case)
none
Optimization for bind() used just for binding 'this' (non JIT case)
none
Native implementation of Function.prototype.bind(), v5
none
Optimization for bind() used just for binding 'this' (JIT case), v2
none
Native implementation of Function.prototype.bind(), v6
none
New patch, reentrant implementation.
sam: review+, webkit-ews: commit-queue-
Build fixes, GC fixes, added more test cases, fix for recursive hasInstance sam: review+

Description tav 2009-06-13 23:17:56 PDT
ES5 adds a bind() method to Function.prototype as pioneered by the Prototype Library.

  Function.prototype.bind (thisArg [, arg1 [, arg2, …]]) 

It is defined in §15.3.4.5 of http://www.ecma-international.org/publications/files/drafts/tc39-2009-025.pdf

--------------------------------------------------

The bind method takes one or more arguments, thisArg and (optionally) arg1, arg2, etc, and returns a new function object by performing the following steps: 

1. Let Target be the this value. 

2. If IsCallable(Target) is false, throw a TypeError exception. 

3. Let A be a new (possibly empty) internal list of all of the argument values provided after thisArg (arg1, arg2 etc), in order. 

4. Let F be a new native ECMAScript object . 

5. Set the [[TargetFunction]] internal property of F to Target. 

6. Set the [[BoundThis]] internal property of F to the value of thisArg. 

7. Set the [[BoundArgs]] internal property of F to A. 

8. Set the [[Class]] internal property of F to "Function". 

9. Set the [[Prototype]] internal property of F to the standard built-in Function prototype object as  specified in 15.3.3.1. 

10. Set the [[Call]] internal property of F as described in 15.3.4.5.1. 

11. Set the [[Construct]] internal property of F as described in 15.3.4.5.2. 

12. Set the [[HasInstance]] internal property of F as described in 15.3.4.5.3. 

13. The [[Scope]] internal property of F is unused and need not exist. 

14. If the [[Class]] internal property of Target is "Function", then 

a. Let L be the length property of Target minus the length of A. 
b. Set the length own property of F to either 0 or L, whichever is larger.  

15. Else set the length own property of F to 0. 

16. The length own property of F is given attributes as specified in 15.3.5.1. 

17. Set the [[Extensible]] internal property of F to true. 

18. Call the [[DefineOwnProperty]] internal method of F with arguments "caller", PropertyDescriptor {[[Value: null, [[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: false}, and false. 

19. Call the [[DefineOwnProperty]] internal method of F with arguments "arguments", PropertyDescriptor {[[Value: null, [[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: false}, and false. 

20. Return F. 

Note: The length property of the bind method is 1. 

Note: Function objects created using Function.prototype.bind do not have a prototype property. 

[[Call]] 

When the [[Call]] internal method of a function object, F, which was created using the bind function is called with a this value and a list of arguments ExtraArgs the following steps are taken: 

1. Let boundArgs be the value of F‘s [[BoundArgs]] internal property. 

2. Let boundThis be the value of F‘s [[BoundThis]] internal property. 

3. Let target be the value of F‘s [[TargetFunction]] internal property. 

4. Let args be a new list containing the same values as the list boundArgs in the same order followed by the same values as the list ExtraArgs in the same order. 

5. Return the result of  calling the [[Call]] internal method of target providing boundThis as the this value and providing args as the arguments. 

[[Construct]] 

When the [[Construct]] internal method of a function object, F that was created using the bind function is called with a list of arguments ExtraArgs the following steps are taken: 

1. Let target be the value of F‘s [[TargetFunction]] internal property. 

2. If target has no [[Construct]] internal method, a TypeError exception is thrown. 

3. Let boundArgs be the value of F‘s [[BoundArgs]] internal property. 

4. Let args be a new list containing the same values as the list boundArgs in the same order followed by the same values as the list ExtraArgs in the same order. 

5. Return the result of  calling the [[Construct]] internal method of  target providing Args as the arguments. 

[[HasInstance]] (V) 

When the [[HasInstance]] internal method of a function object F, that was created using the bind function is called with argument V the following steps are taken: 

1. Let target be the value of F‘s [[TargetFunction]] internal property. 

2. If target has no [[HasInstance]] internal method, a TypeError exception is thrown. 

3. Return the result of  calling the [[HasInstance]] internal method of target providing V as the argument. 

--------------------------------------------------

In order to facilitate the implementation of .bind(), Object now has a number of internal properties. These are defined in §8.6.2 of the spec.

Property: [[TargetFunction]]
Value Type Domain: Object

The target function of a function object created using the standard built-in Function.prototype.bind method. Only ECMAScript objects created using Function.prototype.bind have a [[TargetFunction]] internal property. 

Property: [[BoundThis]]
Value Type Domain: any

The pre-bound this value of a function Object created using the standard built-in Function.prototype.bind method. Only ECMAScript objects created using Function.prototype.bind have a [[BoundThis]] internal property. 

Property: [[BoundArguments]]
Value Type Domain: List of any

The pre-bound argument values of a function Object created using the standard built-in Function.prototype.bind method. Only ECMAScript objects created using Function.prototype.bind have a [[BoundArguments]] internal property. 

See http://www.prototypejs.org/api/function/bind for the inspiration which led to .bind() becoming part of the spec.
Comment 1 Patrick Mueller 2009-12-07 13:33:51 PST
*** Bug 32243 has been marked as a duplicate of this bug. ***
Comment 2 kangax 2010-01-18 20:39:27 PST
Would be great to have this. And here's a similar Mozilla ticket for reference — https://bugzilla.mozilla.org/show_bug.cgi?id=429507
Comment 3 Caio Marcelo de Oliveira Filho 2010-04-21 14:35:29 PDT
Created attachment 53992 [details]
Attempt to implement Function.prototype.bind()
Comment 4 Caio Marcelo de Oliveira Filho 2010-04-23 07:57:15 PDT
Created attachment 54158 [details]
Attempt 2 to implement Function.prototype.bind()

This clean up some minor issues, adds tests and more build systems support. The implementation for a very simple case (just binding 'this' in a small function without parameters), is around 4 times slower than using JS directly. When there's complexity, this difference is smaller: taking around 50% more time in more complex cases (with curried parameter and a larger function). Talking to Oliver Hunt, if I understood correctly this overhead might be due the re-entering the VM.
Comment 5 Maciej Stachowiak 2010-04-24 19:06:10 PDT
I don't think we should land a version of Function.prototype.bind() if it is slower than a JS-implemented version. We should look into ways to make this faster, or find a way to implement fully or partially in JS but still look native in the required ways.
Comment 6 kangax 2010-04-24 19:31:05 PDT
The fastest implementation of `bind` I was able to come up with back in the days was the one with load-time + run-time forking (to avoid unnecessary arguments concatenation). Does native `bind` perform better than this "optimized" version?

Function.prototype.bind = (function(){
  
  var _slice = Array.prototype.slice;
  
  return function(context) {
    var fn = this,
        args = _slice.call(arguments, 1);

    if (args.length) { 
      return function() {
        return arguments.length
          ? fn.apply(context, args.concat(_slice.call(arguments)))
          : fn.apply(context, args);
      }
    } 
    return function() {
      return arguments.length
        ? fn.apply(context, arguments)
        : fn.call(context);
    }; 
  }
})();


There are also some tests in this thread: https://prototype.lighthouseapp.com/projects/8886/tickets/215-optimize-bind-bindaseventlistener
Comment 7 Caio Marcelo de Oliveira Filho 2010-04-24 20:30:23 PDT
Thanks for looking into it.

For the record, the simplest case (and the one with worst performance) I was testing was comparing 'g' and 'h'

    g = function() { return obj.f(); }
    h = obj.f.bind(obj);  // using the native implementation

Agreed that we should land a solution only if it is not slower. So, I'll look into Maciej's suggestions to improve the performance and report back here.
Comment 8 Caio Marcelo de Oliveira Filho 2010-04-25 11:00:04 PDT
There's a benchmark for the bind() function, which includes both Prototype.js trunk implementation and the one pointed out by kangax:

http://www.broofa.com/Tools/JSLitmus/tests/PrototypeBind.html

I've changed my patch to rename the native bind to 'bindNative' (so no collisions with any other implementation), and added a case for it.

My conclusion is that the situation isn't as bad as we thought. The difference between my previous tests is that real world 'bind()' implementations perform worse than special tuned functions that I was using.

Results in the following chart -> 
http://chart.apis.google.com/chart?chtt=Prototype%20bind()%20Performance|Ops/sec%20(Safari%20533.7%20on%20Linux)&chts=000000,10&cht=bhg&chd=t:8258481,7556237,6140297,1255825,873981,641343,1546185,981289,788777,20532293,966813,778470,20099368,2898464,2094766,4424939,3588929,509668,4555667,2904887,2065063&chds=0,20532293&chxt=x&chxl=0:|0|20.5M&chsp=0,1&chm=tnative%20bind(obj)(8.3M),000000,0,0,10|tnative%20bind(obj\,1\,2)(7.6M),000000,0,1,10|tnative%20bind(obj\,1\,2[\,3\,4])(6.1M),000000,0,2,10|t1.6.0.3%20bind(obj)(1.3M),000000,0,3,10|t1.6.0.3%20bind(obj\,1\,2)(874K),000000,0,4,10|t1.6.0.3%20bind(obj\,1\,2[\,3\,4])(641.3K),000000,0,5,10|ttrunk%20bind(obj)(1.5M),000000,0,6,10|ttrunk%20bind(obj\,1\,2)(981.3K),000000,0,7,10|ttrunk%20bind(obj\,1\,2[\,3\,4])(788.8K),000000,0,8,10|tTobie's%20bind(obj)(20.5M),000000,0,9,10|tTobie's%20bind(obj\,1\,2)(966.8K),000000,0,10,10|tTobie's%20bind(obj\,1\,2[\,3\,4])(778.5K),000000,0,11,10|tBroofa's%20bind(obj)(20.1M),000000,0,12,10|tBroofa's%20bind(obj\,1\,2)(2.9M),000000,0,13,10|tBroofa's%20bind(obj\,1\,2[\,3\,4])(2.1M),000000,0,14,10|tKangax's%20bind(obj)(4.4M),000000,0,15,10|tKangax's%20bind(obj\,1\,2)(3.6M),000000,0,16,10|tKangax's%20bind(obj\,1\,2[\,3\,4])(509.7K),000000,0,17,10|tImproved%20bind(obj)(4.6M),000000,0,18,10|tImproved%20bind(obj\,1\,2)(2.9M),000000,0,19,10|tImproved%20bind(obj\,1\,2[\,3\,4])(2.1M),000000,0,20,10&chbh=15,0,5&chs=250x490
Comment 9 Oliver Hunt 2010-04-25 11:21:38 PDT
Okay, here's my thought for getting a non-reentrant version of bind, it only works for non-partially applied bound functions (but according to Caio's numbers the native impl of partially applied bind is faster than the js version).  Basically we do this:
    DEFINE_OPCODE(op_call) {
...
    call_start:
        JSValue v = callFrame->r(func).jsValue();

        CallData callData;
        CallType callType = v.getCallData(callData);

        if (callType == CallTypeJS) {
...
        
        if (callType == CallTypeHost) {
             if (callData.boundFunction) {
                  callFrame->r(func) = callData.boundFunction;
                  // we actually need to work out what thisRegister is :D
                  callFrame->r(thisRegister) = static_cast<JSBoundFunction*>(v.asCell())->thisArgument();
                  goto call_start;
             }
             ...

That should save the reentry cost -- in the jit we'd probably just want to do the handling on return from host call stub.
Comment 10 Caio Marcelo de Oliveira Filho 2010-04-25 13:05:49 PDT
For reference, the same chart as before but with JSC compiled without JIT:

http://chart.apis.google.com/chart?chtt=Prototype%20bind()%20Performance|Ops/sec%20(Safari%20533.7%20on%20Linux)&chts=000000,10&cht=bhg&chd=t:7791053,7395421,5339330,1009812,752429,533684,1062186,766510,586194,8581717,767408,585147,8457131,2138261,1198116,3382643,2843309,474214,3431348,2124399,1180846&chds=0,8581717&chxt=x&chxl=0:|0|8.6M&chsp=0,1&chm=tnative%20bind(obj)(7.8M),000000,0,0,10|tnative%20bind(obj\,1\,2)(7.4M),000000,0,1,10|tnative%20bind(obj\,1\,2[\,3\,4])(5.3M),000000,0,2,10|t1.6.0.3%20bind(obj)(1M),000000,0,3,10|t1.6.0.3%20bind(obj\,1\,2)(752.4K),000000,0,4,10|t1.6.0.3%20bind(obj\,1\,2[\,3\,4])(533.7K),000000,0,5,10|ttrunk%20bind(obj)(1.1M),000000,0,6,10|ttrunk%20bind(obj\,1\,2)(766.5K),000000,0,7,10|ttrunk%20bind(obj\,1\,2[\,3\,4])(586.2K),000000,0,8,10|tTobie's%20bind(obj)(8.6M),000000,0,9,10|tTobie's%20bind(obj\,1\,2)(767.4K),000000,0,10,10|tTobie's%20bind(obj\,1\,2[\,3\,4])(585.1K),000000,0,11,10|tBroofa's%20bind(obj)(8.5M),000000,0,12,10|tBroofa's%20bind(obj\,1\,2)(2.1M),000000,0,13,10|tBroofa's%20bind(obj\,1\,2[\,3\,4])(1.2M),000000,0,14,10|tKangax's%20bind(obj)(3.4M),000000,0,15,10|tKangax's%20bind(obj\,1\,2)(2.8M),000000,0,16,10|tKangax's%20bind(obj\,1\,2[\,3\,4])(474.2K),000000,0,17,10|tImproved%20bind(obj)(3.4M),000000,0,18,10|tImproved%20bind(obj\,1\,2)(2.1M),000000,0,19,10|tImproved%20bind(obj\,1\,2[\,3\,4])(1.2M),000000,0,20,10&chbh=15,0,5&chs=250x490
Comment 11 Caio Marcelo de Oliveira Filho 2010-04-25 15:47:53 PDT
Created attachment 54243 [details]
Native implementation of Function.prototype.bind()

Same as before, just fixes order of functions.
Comment 12 Caio Marcelo de Oliveira Filho 2010-04-25 15:53:47 PDT
Created attachment 54245 [details]
Optimization for bind() used just for binding 'this' (non JIT case)

This patch follows the suggestion of Oliver Hunt in the bug comments. For the non-JIT case, this will make the native implementation win all the other JavaScript-only implementations we found out. In my local tests it even gets a better result than the explicit 'function() { obj.method(); }' call.

We are using http://www.broofa.com/Tools/JSLitmus/tests/PrototypeBind.html as the benchmark.
Comment 13 Caio Marcelo de Oliveira Filho 2010-05-02 18:28:27 PDT
Created attachment 54896 [details]
Native implementation of Function.prototype.bind(), v4

Use Cell space available, and avoid allocating space for holding arguments if they are not used.
Comment 14 Caio Marcelo de Oliveira Filho 2010-05-02 18:31:12 PDT
Created attachment 54897 [details]
Optimization for bind() used just for binding 'this' (non JIT case)

Updated because of changes in previous patch.
Comment 15 Caio Marcelo de Oliveira Filho 2010-05-02 18:33:05 PDT
Created attachment 54898 [details]
Optimization for bind() used just for binding 'this' (JIT case)

Uses same idea from previous optimization in the JIT context. With this patch, in the linked benchmark, the native is faster all the three scenarios.
Comment 16 Caio Marcelo de Oliveira Filho 2010-05-02 19:24:38 PDT
Created attachment 54900 [details]
Optimization for bind() used just for binding 'this' (non JIT case)

After some discussion with Oliver and testing, it isn't always safe to rewrite the 'callee' register, since it might be reused by the bytecode. For the non-JIT case the fix is simple, just change the local variable used instead of the call frame.

The JIT version is already correct in this case, but suffer other problem: that slow cases can look at the callee register again (ex.: compileOpCallSlowCase() when OPTIMIZED_CALL is disabled).
Comment 17 Caio Marcelo de Oliveira Filho 2010-05-05 15:09:56 PDT
Created attachment 55158 [details]
Native implementation of Function.prototype.bind(), v5

Adds a missing 'OverridesMarkChildren' to the StructureFlags of JSBoundFunction. This is needed because JSBoundFunction implements markChildren method.

Also added new test cases to guarantee that CallFrame rewriting done by possible optimizations will not break the code.
Comment 18 Caio Marcelo de Oliveira Filho 2010-05-05 15:32:43 PDT
Created attachment 55161 [details]
Optimization for bind() used just for binding 'this' (JIT case), v2

This an improvement of earlier implementation of the JIT rewriting optimization.

Functionally it should be complete, still have a FIXME to check and possibly removing a redundant information passed to the stub. Note that I'm still working only in one case of Call until we settle in a solution.

After some tests and discussion with Oliver, we understood that it was not always safe to rewrite the register containing the 'callee' since it could be a non-temporary register. The solution in this patch is to force every call emitted to use a temporary for 'callee'. This causes impact on the size of bytecode generated, but guarantee that it's safe to rewrite the call frame, making the optimization correct.

I still want to give a different take on optimizing this case, and try to save/restore the 'callee' instead of guarantee that it is always a temporary. Then compare both solutions.
Comment 19 Caio Marcelo de Oliveira Filho 2010-05-12 08:37:45 PDT
Created attachment 55852 [details]
Native implementation of Function.prototype.bind(), v6

Fixes building in 64bits: JSBoundFunction was too large to fit in the cell, so moved other members inside the private data. Tested that on MacOSX.

Also cached the CallData information of target function, this gave a small boost in performance.
Comment 20 Geoffrey Garen 2010-05-19 17:06:10 PDT
I'd really prefer that we didn't add overhead to normal calls in order to make bind faster. We're working really hard right now to remove overhead from normal calls, and it's paying off in terms of performance.
Comment 21 Oliver Hunt 2010-05-19 17:12:53 PDT
(In reply to comment #20)
> I'd really prefer that we didn't add overhead to normal calls in order to make bind faster. We're working really hard right now to remove overhead from normal calls, and it's paying off in terms of performance.

Geoff actually pointed out that the new specialised thunk stuff i added would potentially make the jit code much easier.
Comment 22 Geoffrey Garen 2010-05-19 17:13:41 PDT
I think a better solution would be for the binding function to fix up callframe[callee] and callframe[this], and then jump to the bound function.
Comment 23 Erik Arvidsson 2011-03-31 10:38:33 PDT
ping

The lack of this is starting to cause compat issue.
Comment 24 Oliver Hunt 2011-03-31 10:45:13 PDT
(In reply to comment #23)
> ping
> 
> The lack of this is starting to cause compat issue.

Given that JS can be used to make a reasonably performant  implementation and most sites that use Function.prototype.bind  do actually start of with an existence check and then add the js impl this is not high priority.
Comment 25 Mark S. Miller 2011-04-01 07:06:20 PDT
(In reply to comment #24)
> (In reply to comment #23)
> > ping
> > 
> > The lack of this is starting to cause compat issue.
> 
> Given that JS can be used to make a reasonably performant  implementation and most sites that use Function.prototype.bind  do actually start of with an existence check and then add the js impl this is not high priority.

JS can reasonably emulate the [[Call]] behavior of .bind() but not its [[Construct]] behavior. This is important because the [[Construct]] behavior of .bind() is the only way to implement a correct var-args construction (new) operation in ES5.
Comment 26 Simon Kaegi 2011-04-27 11:51:58 PDT
another ping

The lack of this is starting to cause compatibility issues in some of the browser-based tooling being done at Eclipse.
Comment 27 Simon Kaegi 2011-04-28 07:43:16 PDT
(In reply to comment #26)
> The lack of this is starting to cause compatibility issues in some of the browser-based tooling being done at Eclipse.

To be clear the shim is fine for our case. Just comes as a surprise that we need it for JSC given that all other current VMs provide support. Despite our best effort to inform we get folk using Function.bind and testing in other browsers and then have failing unit tests during our nightly builds.
Comment 28 Caio Marcelo de Oliveira Filho 2011-04-28 08:32:31 PDT
Hello Geoffrey,

(In reply to comment #22)
> I think a better solution would be for the binding function to fix up callframe[callee] and callframe[this], and then jump to the bound function.

Is this still valid? Can you elaborate a bit on that?
Comment 29 Mark S. Miller 2011-04-28 09:35:16 PDT
(In reply to comment #25)
> 
> JS can reasonably emulate the [[Call]] behavior of .bind() but not 
> its [[Construct]] behavior. This is important because the [[Construct]] 
> behavior of .bind() is the only way to implement a correct var-args 
> construction (new) operation in ES5.

To expand on that, given the ES5 specified Function.prototype.bind, one can implement a "construct(f, args)" operation as follows:

    > function construct(f, args) {
    >   var bound = Function.prototype.bind.apply(f, [null].concat(args));
    >   return new bound();
    > }

    > construct(Date, [1957, 5, 27])
    Thu Jun 27 1957 00:00:00 GMT-0700 (PDT)

Without the specified primitive implementation of bind, I know of no way for a pure JavaScript shim to correctly emulate this behavior, and therefore no way for an ES-next to ES5 compiler to correctly compile "new f(...args)" to ES5.
Comment 30 Gavin Barraclough 2011-09-22 12:20:57 PDT
Created attachment 108382 [details]
New patch, reentrant implementation.

A fresh implementation.  This has three interesting features:
* Based on ToT!
* Subclasses JSFunction (this allows the JIT to make fast calls to bound functions)
* Cell size is no longer restricted (doesn't need a data object).
Comment 31 WebKit Review Bot 2011-09-22 12:24:25 PDT
Attachment 108382 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/JavaScriptCore/runtime/ConstructData.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/runtime/JSFunction.h:47:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSFunction.h:47:  The parameter name "descriptor" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSFunction.h:57:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSFunction.h:57:  The parameter name "globalObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSFunction.h:57:  The parameter name "nativeFunction" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSFunction.h:58:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSFunction.h:58:  The parameter name "globalObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSFunction.h:58:  The parameter name "nativeExecutable" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSFunction.h:134:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSBoundFunction.h:45:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSBoundFunction.h:45:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 12 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 32 Sam Weinig 2011-09-22 12:33:50 PDT
Comment on attachment 108382 [details]
New patch, reentrant implementation.

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

> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:92
> +    // FIXME: our instanceof implementation will have already (incorrectly) performed
> +    // a [[Get]] of .prototype from the bound function object, which is incorrect!
> +    proto = m_targetFunction->get(exec, exec->propertyNames().prototype);

You should add a test showing this is incorrect.

> Source/JavaScriptCore/runtime/JSBoundFunction.h:66
> +    WriteBarrier<JSObject> m_targetFunction;
> +    WriteBarrier<Unknown> m_boundThis;
> +    WriteBarrier<Unknown> m_boundArgs;

How are these marked?
Comment 33 Early Warning System Bot 2011-09-22 12:38:37 PDT
Comment on attachment 108382 [details]
New patch, reentrant implementation.

Attachment 108382 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9798358
Comment 34 Gyuyoung Kim 2011-09-22 12:50:02 PDT
Comment on attachment 108382 [details]
New patch, reentrant implementation.

Attachment 108382 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9792455
Comment 35 Gavin Barraclough 2011-09-22 14:04:56 PDT
Created attachment 108399 [details]
Build fixes, GC fixes, added more test cases, fix for recursive hasInstance
Comment 36 WebKit Review Bot 2011-09-22 14:09:05 PDT
Attachment 108399 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/JavaScriptCore/runtime/ConstructData.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/runtime/JSFunction.h:47:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSFunction.h:47:  The parameter name "descriptor" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSFunction.h:57:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSFunction.h:57:  The parameter name "globalObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSFunction.h:57:  The parameter name "nativeFunction" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSFunction.h:58:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSFunction.h:58:  The parameter name "globalObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSFunction.h:58:  The parameter name "nativeExecutable" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSFunction.h:135:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSBoundFunction.h:45:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/JSBoundFunction.h:45:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 12 in 63 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 37 Gavin Barraclough 2011-09-22 14:21:29 PDT
Fixed in r95751.
Comment 38 Adam Roben (:aroben) 2011-09-23 03:21:30 PDT
Comment on attachment 108399 [details]
Build fixes, GC fixes, added more test cases, fix for recursive hasInstance

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

> LayoutTests/fast/js/Object-getOwnPropertyNames-expected.txt:46
> -PASS getSortedOwnPropertyNames(Function.prototype) is ['apply', 'call', 'constructor', 'length', 'name', 'toString']
> +FAIL getSortedOwnPropertyNames(Function.prototype) should be apply,call,constructor,length,name,toString. Was apply,bind,call,constructor,length,name,toString.

Shouldn't we update the test to expect the bind property?
Comment 39 Gavin Barraclough 2011-09-23 12:14:42 PDT
(In reply to comment #38)
> (From update of attachment 108399 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108399&action=review
> 
> > LayoutTests/fast/js/Object-getOwnPropertyNames-expected.txt:46
> > -PASS getSortedOwnPropertyNames(Function.prototype) is ['apply', 'call', 'constructor', 'length', 'name', 'toString']
> > +FAIL getSortedOwnPropertyNames(Function.prototype) should be apply,call,constructor,length,name,toString. Was apply,bind,call,constructor,length,name,toString.
> 
> Shouldn't we update the test to expect the bind property?

Ooops, yes, meant to do so, clearly forgot to.  Will do.