Nested function marked as JSPD_ARGUMENT

VERIFIED FIXED in mozilla0.9

Status

()

P1
normal
VERIFIED FIXED
18 years ago
17 years ago

People

(Reporter: jband_mozilla, Assigned: brendan)

Tracking

({js1.5})

Trunk
mozilla0.9
js1.5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

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

Comment 1

18 years ago
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
Status: NEW → ASSIGNED
Keywords: js1.5, mozilla0.9
Priority: -- → P1
Target Milestone: --- → mozilla0.9
(Assignee)

Comment 2

18 years ago
Created attachment 28436 [details] [diff] [review]
proposed fix
(Assignee)

Comment 3

18 years ago
Review this simple fix, please!

/be
(Reporter)

Comment 4

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

Comment 5

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

Comment 6

18 years ago
Created attachment 28466 [details] [diff] [review]
another whack
(Reporter)

Comment 7

18 years ago
r/sr=jband This worksforme. Thanks.
(Assignee)

Comment 8

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

Comment 10

18 years ago
Fixed, jband verified.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 11

18 years ago
Marking Verified - 
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.