290488 - 15.3.4.4. - Function.prototype.call() should use global object as scope when no thisArg is passed

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Bob Clary [:bc] (inactive)]

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.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: js/tests/js1_5/Function/15.3.4.4.js

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

Comment 2

[Erik Fabert]

>    return arguments.callee.__parent__ == global;

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

Comment 3

[Bob Clary [:bc] (inactive)]

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!

Comment 4

[Brendan Eich [:brendan]]

Attachment: fixed testcase

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

Comment 5

[Brendan Eich [:brendan]]

Attachment: proposed fix

Passes the patched testcase.

/be

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 180839 [details] [diff] [review]
fixed testcase

The description should probably say "function invoked via .call with{, no}
closure...".

/be

Comment 7

[Bob Clary [:bc] (inactive)]

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

Comment 8

[Brendan Eich [:brendan]]

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

Comment 9

[Bob Clary [:bc] (inactive)]

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

Comment 10

[Brendan Eich [:brendan]]

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

/be 

Comment 11

[Brendan Eich [:brendan]]

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

Comment 12

[Brendan Eich [:brendan]]

Attachment: better-fixed testcase

Comment 13

[Brendan Eich [:brendan]]

Attachment: js.c patch needed by the better-fixed testcase

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 14

[Brendan Eich [:brendan]]

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 15

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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?

Comment 16

[Brendan Eich [:brendan]]

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

Comment 17

[Brendan Eich [:brendan]]

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 18

[Bob Clary [:bc] (inactive)]

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 19

[Daniel Veditz [:dveditz]]

Comment on attachment 180840 [details] [diff] [review]
proposed fix

approved for aviary101/moz17 branches, a=dveditz for drivers

Comment 20

[Daniel Veditz [:dveditz]]

Fix checked into aviary101/moz17 branches

Comment 21

[Bob Clary [:bc] (inactive)]

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.