Last Comment Bug 290488 - - should use global object as scope when no thisArg is passed
: - should use global object as scope when ...
: js1.5, verified1.7.13
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: mozilla1.8beta2
Assigned To: Brendan Eich [:brendan]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 327194
  Show dependency treegraph
Reported: 2005-04-15 10:18 PDT by Bob Clary [:bc:]
Modified: 2006-02-22 02:34 PST (History)
2 users (show)
dveditz: blocking1.7.13+
dveditz: blocking‑aviary1.0.8+
brendan: blocking1.8b2+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

js/tests/js1_5/Function/ (2.59 KB, text/plain)
2005-04-15 10:23 PDT, Bob Clary [:bc:]
no flags Details
fixed testcase (1.25 KB, patch)
2005-04-15 15:08 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
proposed fix (1.61 KB, patch)
2005-04-15 15:09 PDT, Brendan Eich [:brendan]
shaver: review+
dveditz: approval‑aviary1.0.8+
dveditz: approval1.7.13+
brendan: approval1.8b2+
Details | Diff | Splinter Review
better-fixed testcase (1.40 KB, patch)
2005-04-17 12:32 PDT, Brendan Eich [:brendan]
bob: review+
Details | Diff | Splinter Review
js.c patch needed by the better-fixed testcase (1.74 KB, patch)
2005-04-17 12:34 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review

Description Bob Clary [:bc:] 2005-04-15 10:18:46 PDT
From Brendan: (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:] 2005-04-15 10:23:55 PDT
Created attachment 180813 [details]

/cvsroot/mozilla/js/tests/js1_5/Function/,v	<--
initial revision: 1.1
Comment 2 Erik Fabert 2005-04-15 10:47:46 PDT
>    return arguments.callee.__parent__ == global;

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

Comment 3 Bob Clary [:bc:] 2005-04-15 10:57:12 PDT

/cvsroot/mozilla/js/tests/js1_5/Function/,v  <--
new revision: 1.2; previous revision: 1.1

Thanks Erik!
Comment 4 Brendan Eich [:brendan] 2005-04-15 15:08:49 PDT
Created attachment 180839 [details] [diff] [review]
fixed testcase

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

Comment 5 Brendan Eich [:brendan] 2005-04-15 15:09:49 PDT
Created attachment 180840 [details] [diff] [review]
proposed fix

Passes the patched testcase.

Comment 6 Brendan Eich [:brendan] 2005-04-15 15:12:34 PDT
Comment on attachment 180839 [details] [diff] [review]
fixed testcase

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

Comment 7 Bob Clary [:bc:] 2005-04-15 15:30:25 PDT
Brendan, when developing the testcase in venkman, it seemed that both
func()/ and closure()/ 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] 2005-04-15 16:45:01 PDT
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, 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__.

Comment 9 Bob Clary [:bc:] 2005-04-15 16:51:22 PDT
Ok, but the patched test passes both sections before your patch. It fails to
detect the bug.
Comment 10 Brendan Eich [:brendan] 2005-04-15 17:11:39 PDT
Oh, sorry -- I didn't actually test that!  Ok, I'll try patching the test again.
 Feel free to beat me to it.

Comment 11 Brendan Eich [:brendan] 2005-04-15 20:58:54 PDT
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

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.

Comment 12 Brendan Eich [:brendan] 2005-04-17 12:32:01 PDT
Created attachment 180970 [details] [diff] [review]
better-fixed testcase
Comment 13 Brendan Eich [:brendan] 2005-04-17 12:34:16 PDT
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:


STATUS: - 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]'


The "proposed fix" patch makes this testcase pass.

Comment 14 Brendan Eich [:brendan] 2005-04-17 12:59:46 PDT
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.

Comment 15 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-04-17 13:10:51 PDT
Comment on attachment 180840 [details] [diff] [review]
proposed fix

r=shaver.  What did we decide about similar code in str_replace for the lambda
Comment 16 Brendan Eich [:brendan] 2005-04-17 13:17:12 PDT

What we really want for |this| binding, which comes up with, 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.

Comment 17 Brendan Eich [:brendan] 2005-04-17 13:19:44 PDT
So the lambda-replace case awaits a change, and I would rather not change it at
this point.  ECMA-262 Edition 3 does not specify how |this| binds, and
we've shipped the parent-binding lambda-replace since 1997 or so.

Comment 18 Bob Clary [:bc:] 2005-04-18 13:48:20 PDT
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;
/cvsroot/mozilla/js/tests/js1_5/Function/,v	<--
new revision: 1.3; previous revision: 1.2
Comment 19 Daniel Veditz [:dveditz] 2006-02-16 15:41:32 PST
Comment on attachment 180840 [details] [diff] [review]
proposed fix

approved for aviary101/moz17 branches, a=dveditz for drivers
Comment 20 Daniel Veditz [:dveditz] 2006-02-16 15:46:26 PST
Fix checked into aviary101/moz17 branches
Comment 21 Bob Clary [:bc:] 2006-02-22 02:34:09 PST
passes js1_5/Function/ for 
ff on 1.7.13,, 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, as well so I'm not entirely sure the test is adequate.

Note You need to log in before you can comment on or make changes to this bug.