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

VERIFIED FIXED in mozilla1.8beta2

Status

()

Core
JavaScript Engine
P3
normal
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: bc, Assigned: brendan)

Tracking

({js1.5, verified1.7.13})

Trunk
mozilla1.8beta2
js1.5, verified1.7.13
Points:
---
Bug Flags:
blocking1.7.13 +
blocking-aviary1.0.8 +
blocking1.8b2 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 180813 [details]
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
(Reporter)

Updated

12 years ago
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

Comment 2

12 years ago
>    return arguments.callee.__parent__ == global;

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

(Reporter)

Comment 3

12 years ago
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!
(Assignee)

Comment 4

12 years ago
Created attachment 180839 [details] [diff] [review]
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
Attachment #180813 - Attachment is obsolete: true
(Assignee)

Comment 5

12 years ago
Created attachment 180840 [details] [diff] [review]
proposed fix

Passes the patched testcase.

/be
Attachment #180840 - Flags: review?(shaver)
Attachment #180840 - Flags: approval1.8b2+
(Assignee)

Updated

12 years ago
Assignee: general → brendan
Flags: blocking1.8b2+
Keywords: js1.5
Priority: -- → P3
Target Milestone: --- → mozilla1.8beta2
(Assignee)

Comment 6

12 years ago
Comment on attachment 180839 [details] [diff] [review]
fixed testcase

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

/be
(Reporter)

Comment 7

12 years ago
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. ?
(Assignee)

Comment 8

12 years ago
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
(Reporter)

Comment 9

12 years ago
Ok, but the patched test passes both sections before your patch. It fails to
detect the bug.
(Assignee)

Comment 10

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

/be 
(Assignee)

Comment 11

12 years ago
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
(Assignee)

Comment 12

12 years ago
Created attachment 180970 [details] [diff] [review]
better-fixed testcase
Attachment #180839 - Attachment is obsolete: true
Attachment #180970 - Flags: review?(moz)
(Assignee)

Comment 13

12 years ago
Created attachment 180971 [details] [diff] [review]
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
(Assignee)

Comment 14

12 years ago
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+
(Assignee)

Comment 16

12 years ago
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
(Assignee)

Comment 17

12 years ago
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
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Reporter)

Comment 18

12 years ago
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+
(Reporter)

Updated

12 years ago
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
Keywords: fixed-aviary1.0.8, fixed1.7.13
(Reporter)

Comment 21

11 years ago
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
Keywords: fixed-aviary1.0.8, fixed1.7.13 → verified-aviary1.0.8, verified1.7.13
You need to log in before you can comment on or make changes to this bug.