68825 - Nested function marked as JSPD_ARGUMENT

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[John Bandhauer]

JS_GetPropertyDesc is setting the JSPD_ARGUMENT flag for nested 
function properties.

We can see this using xpcshell (which hooks the 'debugger;' statement and uses 
jsdbgapi to dump the stack with args and vars).

Given foo.js...

function foo(a1,a2) {
    var v1 = "v1";        
    var v2 = "v2";
    function bar() {}
    print("arguments.length = "+arguments.length);
    debugger;        
}

foo(1,2);

...and running "xpcshell foo.js", I see...

arguments.length = 2
------------------------------------------------------------------------
Hit JavaScript "debugger" keyword. JS call stack...
0 foo(bar = [function], a1 = 1, a2 = 2) ["foo.js":6]
    v1 = "v1"
    v2 = "v2"
    this = [object global]
1 <TOP LEVEL> ["foo.js":9]
    this = [object global]
------------------------------------------------------------------------

I would expect 'bar' to have the JSPD_VARIABLE flag abd to appear in the locals 
list rather than the args list.

Stepping through JS_GetPropertyDesc shows that the JSPD_ARGUMENT flag is set in 
the code that checks for getter == js_CallClass.getProperty. I'm guessing that 
this case is getting lumped in with another case where that flag makes sense. I 
confess that I'm not clear on exactly what should be tested to make this work as 
expected. FWIW, in the test case above the 'bar' property is the first element 
dealt with in JS_GetPropertyDesc, so this is easy to step through in the 
debugger.

Comment 1

[Brendan Eich [:brendan]]

That #if JS_HAS_CALL_OBJECT paragraph is bizarre, and I didn't write it.  It
came from some JS1.4 development branch, some time in '98 (after I had "retired"
to mozilla.org -- HAH! ;-).

I'll give this some thought, but I'd love to hand it off to someone (jband this
means you).

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 3

[Brendan Eich [:brendan]]

Review this simple fix, please!

/be

Comment 4

[John Bandhauer]

But this makes all the args look like locals. The cure is worse than the 
disease.

If you look at the bottom of...
http://mozilla.org/scriptable/javascript-stack-dumper.html
... you'll see an example a JS call sequence and its 'debugger;' dump output.

If I run that example then xpcdebug.cpp formats it to show all the function args 
as both args (because they are found as properties of the arguments object - not 
marked as JSPD_ARGUMENT) and as locals.

Comment 5

[Brendan Eich [:brendan]]

(jband, can't you tell I'm trying to get you to take this bug? ;-)

Ok, try this: if the property of the call object that has the call class getter
as its getter is permanent, it's an arg; else it's a "variable" (nested function
created by JSOP_DEFFUN -- see jsinterp.c around line 3154).  Or do we really
need to go cross-check against the arguments object?

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: another whack

Comment 7

[John Bandhauer]

r/sr=jband This worksforme. Thanks.

Comment 8

[Brendan Eich [:brendan]]

Another way to tell would be to test JSVAL_IS_INT(sprop->id) -- that means the
call property is an argument, and its property id (as opposed to various aliases
in symbol ids) is its index in argv.

But soon, with JS_DOUBLE_HASHING, this tagged sprop->id scheme will change to
eliminate symbols, keeping the one true name in sprop->id and moving any
"tinyid" or "shortid" (can be up to 65535 args in SpiderMonkey) that must hold
an argv index elsewhere in the JSScopeProperty struct.

So I think the permanent test is best, for now.  Shaver, rogerl: how about an r=
for the second patch?  Thanks.

/be

Comment 9

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

r=shaver.

Comment 10

[Brendan Eich [:brendan]]

Fixed, jband verified.

/be

Comment 11

[Phil Schwartau]

Marking Verified -