222029 - Make our f.caller property match IE's w.r.t. f.apply and f.call

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

See the URL.  The question is, does

function f(o) { return f.caller.p; }
function g(o) { var p = 'local'; return f.call(o); }

var o = {p:'object'};
var p = 'global';
g.p = 'correct';
print(g());

print 'correct' or 'undefined' (it should never print 'local', 'global', or
'object', of course).  SpiderMonkey links f.caller to Function.prototype.call,
so f.caller.p is undefined.  JScript skips one activation from the invocation
done via call (or apply), and so would print 'correct'.

I don't see any gain from sticking to our guns and not matching the JScript
engine's behavior for this unspecified property and special case. The added code
is tiny, and I'm taking this opportunity to clean up a few other things.

Phil, do we need a Rhino bug too?

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: proposed change

Comment 2

[Brendan Eich [:brendan]]

Contrast the call/apply case with this one:

var s = "a:bb:ccc:dd:e";
var t = s.replace(/:/g, function f(a,b,c){print(a,b,c,f.caller.name);return '@';});
print(t);

which should print

: 1 a:bb:ccc:dd:e replace
: 4 a:bb:ccc:dd:e replace
: 8 a:bb:ccc:dd:e replace
: 11 a:bb:ccc:dd:e replace
a@bb@ccc@dd@e

I.e., String.prototype.replace should not invoke the funarg in a way that causes
f.caller to skip replace.  The special JSINVOKE_SKIP_CALLER cases are call&apply
only, and no other indirect (native, hence JSINVOKE_INTERNAL) ways of invoking
functions that might be user-defined (therefore, that might use caller).

/be

Comment 3

[Phil Schwartau]

Testcase added to JS testsuite:

      mozilla/js/tests/js1_5/Function/regress-222029-001.js

Comment 4

[Phil Schwartau]

Without this patch, the testcase fails as follows in the SpiderMonkey shell:

*-* Testcase js1_5/Function/regress-222029-001.js failed:
Bug Number 222029
STATUS: Make our f.caller property match IE's wrt f.apply and f.call
Failure messages were:
FAILED!: [reported from test()] Section 2 of test -
FAILED!: [reported from test()] Expected value 'hello', Actual value 'goodbye'
FAILED!: [reported from test()]
FAILED!: [reported from test()] Section 3 of test -
FAILED!: [reported from test()] Expected value 'hello', Actual value 'goodbye'


and actually crashes in the Rhino shell:

*-* Testcase js1_5/Function/regress-222029-001.js failed:
Expected exit code 0, got 3
Testcase terminated with signal 0
Complete testcase output was:
uncaught JavaScript exception: TypeError: Can't read property "p" from undefined


Rhino seems to give me |undefined| for |f.caller| no matter how I call f.
cc'ing Igor to see if that is correct -

Comment 5

[Brendan Eich [:brendan]]

Looks like Rhino simply doesn't implement caller -- good for it! ;-)  ECMA-262
does not specify caller, it's a hold-over from the old Netscape days.

/be

Comment 6

[Phil Schwartau]

We now have two testcases:

      mozilla/js/tests/js1_5/Function/regress-222029-001.js
      mozilla/js/tests/js1_5/Function/regress-222029-002.js

Comment 7

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

Comment on attachment 133207 [details] [diff] [review]
proposed change

Sure.

Comment 8

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 9

[Phil Schwartau]

Verified FIXED.

The above two tests now pass in the SpiderMonkey shell. I have added both
of them to the skip list for Rhino, mozilla/js/tests/rhino-n.tests:

[//d/JS_TRUNK/mozilla/js/tests] cvs ci rhino-n.tests
Checking in rhino-n.tests;
/cvsroot/mozilla/js/tests/rhino-n.tests,v  <--  rhino-n.tests
new revision: 1.55; previous revision: 1.54
done

with the following comment:

"Skip new tests that use SpiderMonkey's f.caller property (non-ECMA)."