Closed Bug 290488 Opened 17 years ago Closed 17 years ago

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

Categories

(Core :: JavaScript Engine, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla1.8beta2

People

(Reporter: bc, Assigned: brendan)

References

Details

(Keywords: js1.5, verified1.7.13)

Attachments

(3 files, 2 obsolete files)

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.
Attached file js/tests/js1_5/Function/15.3.4.4.js (obsolete) —
/cvsroot/mozilla/js/tests/js1_5/Function/15.3.4.4.js,v	<--  15.3.4.4.js
initial revision: 1.1
Summary: 13.3.4.4. - Function.prototype.call() should use global object as scope when no thisArg is passed → 15.3.4.4. - Function.prototype.call() should use global object as scope when no thisArg is passed
>    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!
Attached patch fixed testcase (obsolete) — Splinter Review
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
Attachment #180813 - Attachment is obsolete: true
Attached patch proposed fixSplinter Review
Passes the patched testcase.

/be
Attachment #180840 - Flags: review?(shaver)
Attachment #180840 - Flags: approval1.8b2+
Assignee: general → brendan
Flags: blocking1.8b2+
Keywords: js1.5
Priority: -- → P3
Target Milestone: --- → mozilla1.8beta2
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
Status: NEW → ASSIGNED
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
Attachment #180839 - Attachment is obsolete: true
Attachment #180970 - Flags: review?(moz)
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?
Attachment #180840 - Flags: review?(shaver) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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
Attachment #180970 - Flags: review?(moz) → review+
Flags: testcase+
Blocks: 327194
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
Comment on attachment 180840 [details] [diff] [review]
proposed fix

approved for aviary101/moz17 branches, a=dveditz for drivers
Attachment #180840 - Flags: approval1.7.13+
Attachment #180840 - Flags: approval-aviary1.0.8+
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.