From Brendan: 15.3.4.4 Function.prototype.call (thisArg [ , arg1 [ , arg2, & ] ] ) The call method takes one or more arguments, thisArg and (optionally) arg1, arg2 etc, and performs a function call using the [[Call]] property of the object. If the object does not have a [[Call]] property, a TypeError exception is thrown. The called function is passed arg1, arg2, etc. as the arguments. If thisArg is null or undefined, the called function is passed the global object as the this value. Otherwise, the called function is passed ToObject(thisArg) as the this value. The length property of the call method is 1. In a normal function call (non object method), the arguments.callee.__parent__ object will point to the global object. However if you create a function closure, its arguments.callee.__parent__ object will point to its [[Call]] object rather than the global object.

/cvsroot/mozilla/js/tests/js1_5/Function/15.3.4.4.js,v <-- 15.3.4.4.js initial revision: 1.1

> return arguments.callee.__parent__ == global; Should be return arguments.callee.__parent__ == GLOBAL;

doh! /cvsroot/mozilla/js/tests/js1_5/Function/15.3.4.4.js,v <-- 15.3.4.4.js new revision: 1.2; previous revision: 1.1 Thanks Erik!

Bob, that test doesn't test Function.prototype.call. It actually tests __parent__, which must not be coerced to GLOBAL. Please use this patch to fix the test and check it in. /be

Passes the patched testcase. /be

Comment on attachment 180839 [details] [diff] [review] fixed testcase The description should probably say "function invoked via .call with{, no} closure...". /be

Brendan, when developing the testcase in venkman, it seemed that both func()/func.call() and closure()/closure.call() had the same result for scope [[Parent]] which is why I used (). I just tried your patched test and a current build passes both sections, while in the previous version of the test, section 1 passed and section 2 failed. ?

Section 2 was wrong. A closure's parent (scope chain link in ECMA-262 Edition 3, stored in the [[Scope]] internal property of the function object) should *not* be the global object. This bug is about Function.prototype.call, that's what I mailed you about. The __parent__ extension reflects [[Scope]], but for all objects. It's set in any new instance of constructor C from C.__parent__. /be

Ok, but the patched test passes both sections before your patch. It fails to detect the bug.

Oh, sorry -- I didn't actually test that! Ok, I'll try patching the test again. Feel free to beat me to it. /be

Duh, ECMA requires censoring the "activation object" (in SpiderMonkey class Call) by replacing it with null when it would otherwise bind to this, and replacing null with the global object when getting the value of the resulting Reference. So the test needs to use a property that has a non-global, non-Call parent. With the DOM, that's easy. Without it, not so easy. More in a bit. /be

The testcase now actually tests .call, and it uses a new method of the shell's "it" testing object, bindMethod, to define a function as a property of it with the function's parent set to it. I changed the testcase to check this binding directly, so failure gives a more useful diagnostic: BUGNUMBER: 290488 STATUS: 15.3.4.4 - Function.prototype.call() Scope STATUS: Section 1 of test - STATUS: Section 2 of test - FAILED!: bound method: this == GLOBAL FAILED!: Expected value '[object global]', Actual value '[object It]' FAILED!: The "proposed fix" patch makes this testcase pass. /be

Comment on attachment 180971 [details] [diff] [review] js.c patch needed by the better-fixed testcase I checked this in, with a somewhat better diagnostic for the case of a non-callable object passed as the second parameter to it.bindMethod. /be

Comment on attachment 180840 [details] [diff] [review] proposed fix r=shaver. What did we decide about similar code in str_replace for the lambda case?

Fixed. What we really want for |this| binding, which comes up with Function.prototype.call, String.prototype.replace when passed a function as the second parameter, and other places, is static binding to outer function's |this| for nested functions and lambda expressions. I'll push for this incompatible fix in Edition 4 of ECMA-262. /be

So the lambda-replace case awaits a change, and I would rather not change it at this point. ECMA-262 Edition 3 15.5.4.11 does not specify how |this| binds, and we've shipped the parent-binding lambda-replace since 1997 or so. /be

Comment on attachment 180970 [details] [diff] [review] better-fixed testcase Current builds pass this patched test. I really can't comment on what is being tested or if the test is correct however. r+ Checking in 15.3.4.4.js; /cvsroot/mozilla/js/tests/js1_5/Function/15.3.4.4.js,v <-- 15.3.4.4.js new revision: 1.3; previous revision: 1.2

Comment on attachment 180840 [details] [diff] [review] proposed fix approved for aviary101/moz17 branches, a=dveditz for drivers

Fix checked into aviary101/moz17 branches

passes js1_5/Function/15.3.4.4.js for ff on 1.7.13, 1.8.0.1, 1.8, 1.9a1 on windows/linux/mac and moz on 1.7.13 on windows, but it also passes ff on 1.7.5, 1.7.5.12 as well so I'm not entirely sure the test is adequate.