293782 - Local variables can cause predefined function object properties to be undefined

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jonathan Watt [:jwatt]]

If a function has a local variable with the same name as a predefined function
object property (e.g. apply, call and name) then those properties will be
undefined for that function object. Test using the following:

javascript:function f(){var name=1;};alert(f.name);

Comment 1

[Brendan Eich [:brendan]]

Attachment: Fix SpiderMonkey to hide arg/var properties of function objects

Yay, this patch removes the XXX'd hacks in jsinterp.c (SetFunctionSlot) and
jsobj.c (with_GetProperty) that try with too much code to work around the
pre-ECMA pollution of function objects with properties, used by the compiler
and by Call object reflection, describing their args and local vars.

/be

Comment 2

[Mike Shaver (:shaver emeritus)]

Comment on attachment 183623 [details] [diff] [review]
Fix SpiderMonkey to hide arg/var properties of function objects

I like -- perhaps love -- everything here but "FUNNY".	I appreciate a good
function pun, but wonder if _ACTIVATION or some such might be clearer. 
(Especially given the now-dead, but long-lasting, CHECK_FOR_FUNNY_ID macro,
though that might only matter to you and me.)

Not enough to keep this from hitting the tree, though, as it is indeed truly
righteous.

r+a=shaver

Comment 3

[Jonathan Watt [:jwatt]]

(In reply to comment #2)
> (From update of attachment 183623 [details] [diff] [review] [edit])
> I like -- perhaps love -- everything here but "FUNNY".	I appreciate a good
> function pun, but wonder if _ACTIVATION or some such might be clearer. 

huh, yeah, that would have been clearer to me at least.

Comment 4

[Bob Clary [:bc] (inactive)]

Checking in 10.1.6.js;
/cvsroot/mozilla/js/tests/js1_5/Function/10.1.6.js,v  <--  10.1.6.js
initial revision: 1.1

Comment 5

[Brendan Eich [:brendan]]

Attachment: patch I checked in

- Renamed _FUNNY => _HIDDEN.
- Fixed a leftover js_LookupProperty in Function that was looking for duplicate
formal parameters -- of course it must now be js_LookupPropertyWithFlags(...,
JSLOOKUP_HIDDEN, ...).
- Robusticated call_resolve to insist that any hidden property found in the
callee or a prototype of the callee came from the same function as the callee's
fp->fun.  Necessary since (mutable) __proto__ was added.

/be

Comment 6

[Brendan Eich [:brendan]]

Fixed.  Marking deps fixed too.  Hooray!

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: followup fix

There's no way around it, given id uniquely identifying a property in an
object: to hide function formarl parameter and local variable properties in a
disjoint set of properities requires a disjoint set of ids.  Hence this patch,
which undoes the regression of bug 137000.  Checking in shortly.

/be

Comment 8

[Mike Shaver (:shaver emeritus)]

Comment on attachment 183781 [details] [diff] [review]
followup fix

r+a=shaver.

Comment 9

[Brendan Eich [:brendan]]

I checked in the followup fix, with one slight tweak to jsemit.c (to test !ok
and break if so after the call to js_LookupHiddenProperty for the native obj
case in js_LookupCompileTimeConstant).

/be

Comment 10

[Darin Fisher]

latest patch looks to be causing tinderbox orange!

Comment 11

[Brendan Eich [:brendan]]

Sorry about that -- over-reliance on the JS testsuite, and neither shaver nor I
remembered the JS_CompileUCFunctionForPrincipals API impl, which defines formal
parameters.

/be

Comment 12

[Jonathan Watt [:jwatt]]

The inner variables of functions still show up in for-in statements. Try

function f(){var a, b;}
for (prop in f) alert('f['+prop+'] == '+f[prop])

Should I file a separate bug, or does this need to be reopened?

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> The inner variables of functions still show up in for-in statements. Try
> 
> function f(){var a, b;}
> for (prop in f) alert('f['+prop+'] == '+f[prop])
> 
> Should I file a separate bug, or does this need to be reopened?

Fixed.  This was just a matter of removing JSPROP_ENUMERATE attributes passed to
js_AddHiddenProperty for args and vars, per ECMA-262 Ed. 3, 10.1.3 and 10.2.3.

/be

Comment 14

[Jonathan Watt [:jwatt]]

Brendan, the problem seems to still exist in a build I made in the last hour or so.

Comment 15

[Brendan Eich [:brendan]]

jwatt: thanks for catching this -- one JSPROP_ENUMERATE was left behind.  Fixed.

/be

Comment 16

[Bob Clary [:bc] (inactive)]

testcase for enumerating function local variables.
Checking in 10.1.6-01.js;
/cvsroot/mozilla/js/tests/js1_5/Function/10.1.6-01.js,v  <--  10.1.6-01.js
initial revision: 1.1
done