488842 - Local Scope Variables are not displayed by default

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Michael Kraft [:morac]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090416 Firefox/3.5b4pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090416 Firefox/3.5b4pre

Starting with the Mozilla 1.9.1 branch (Firefox 3.5), most local scope variables are not displayed by default in either the Javascript Debugger add-on window or ChromeBug which seems to indicate a problem with the underlying venkman code.

In the Mozilla 1.9.1 branch any "missing" variables can be added as a watch and they will display correctly.  In the nightly trunk loads, watched scope variables appear to display data associated with other variables (basically garbage data).  I'm going to file the 2nd problem as a separate bug.


Reproducible: Always

Steps to Reproduce:
1. Open Javascript Debugger Window (Venkman)
2. Breakpoint some function
3. Cause breakpoint to hit
Actual Results:  
Not all local scope variables are displayed.

Expected Results:  
All local scope variables should be displayed like they were in Firefox 3.0.x.

I've noticed this for a long time with Firefox 3.5 builds going back to the early alphas so it's been a problem for a while now.

Comment 1

[Michael Kraft [:morac]]

Attachment: Debugger window at breakpoint in Firefox 3.0.8

I'm attaching two screen shots of a break point being hit in my add-on.  This one is from Firefox 3.0.8.  As you can see all the local scope variables are displayed correctly.

Comment 2

[Michael Kraft [:morac]]

Attachment: Debugger window at breakpoint in Firefox 3.5b4pre

Here's the same break point in Firefox 3.5b4pre (20090416).  As you can see only 5 of the local scope variables are now displayed.

Comment 3

[Michael Kraft [:morac]]

I tracked down the version this originally broke in.  It was the 20080701 trunk load, which then was that 1.9.1a1pre loads.

I'll also mention that in the 20080630 and earlier loads, the "scope" branch in the local variables listing defaults to being collapsed (closed).  On the 20080701 loads, "scope" defaults to be open.  Since this is also when the problem started I don't think it's a coincidence.

Here's what was changed around that time period:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=june+29%2C+2008&enddate=July+1%2C+2008

Of the bugs listed only the following are JS related:
Bug 411575, Bug 442242, Bug 440558

Of those my guess would be bug 411575 just based on it's complexity, but I'm not sure about that.

Comment 4

[Karsten Düsterloh]

I can see this in comm-central SeaMonkey builds (Gecko 1.9.1) as well now. :-(
I wonder how closely related to bug 488843 this is...

Comment 5

[Michael Kraft [:morac]]

(In reply to comment #4)
> I can see this in comm-central SeaMonkey builds (Gecko 1.9.1) as well now. :-(
> I wonder how closely related to bug 488843 this is...

They probably are not related.  

This bug popped up in July of 2008, while bug 488843 showed up April of 2009.  I probably should have filed this bug when I first noticed the problem last year, but for whatever reason I failed to do so.  I was using watches as a work around for this bug which is how I noticed that watches on local scope variables weren't working.

Comment 6

[Karsten Düsterloh]

After some digging in these unknown waters it seems that this is definitely not Venkman-specific, because jsdIValue.getProperties is just outright lying at me:
- I always get at maximum five local variables in jsdIValue.scope, regardless of how many I define or whether I use var or let
- I always get to see only those five local variable which were defined _last_

function f(aParam)
{
  let x=43;
  var z="";
  let y={some: "object"};
  // => 4 properties: aParam, x, z, y
}

function f(aParam)
{
  let x=43;
  var z="";
  let y={some: "object"};
  var a=x+z, q=[22], p;
  // => 5 properties: z, y, a, q, p
}

Michael, can you confirm this? Your screenshot shows 5 properties, but I can't tell if those are the last five defined...

Comment 7

[John J. Barton]

Firebug sees the same five locals, and does not see 'x'.

Comment 8

[John J. Barton]

Some numerology:
#define SCOPE_HASH_THRESHOLD 6 
http://mxr.mozilla.org/mozilla1.9.1/source/js/src/jsscope.cpp#99

if (scope->entryCount > SCOPE_HASH_THRESHOLD) {
...
http://mxr.mozilla.org/mozilla1.9.1/source/js/src/jsscope.cpp#123

Then in the getProperties() defn:
    n = STOBJ_NSLOTS(obj);
    if (n > scope->entryCount)
        n = scope->entryCount;

So the 5 could come from the limiter in getProperties() and perhaps the scope->entryCount is somehow stuck at 5 < 6? Hard to imagine how anything could work, but maybe some hints.

Comment 9

[Brendan Eich [:brendan]]

Sherlock Holmes (in The Sign of the Four): "I never guess: it is an appalling habit, destructive to the logical faculty."

Please, no numerology!

Fallout from bug 411575. Igor, can you please take a look?

/be

Comment 10

[Michael Kraft [:morac]]

(In reply to comment #6)
> After some digging in these unknown waters it seems that this is definitely not
> Venkman-specific, because jsdIValue.getProperties is just outright lying at 
> Michael, can you confirm this? Your screenshot shows 5 properties, but I can't
> tell if those are the last five defined...

It isn't the last 5 variables. It's just the same 5 variables every time.  I'm not sure how those 5 were "chosen".  See the first attached screen shot for what it looks like under Firefox 3.0.8.  You can see the variable "canUndo" is in the middle of the list of variables.

Comment 11

[Karsten Düsterloh]

(In reply to comment #10)
> It isn't the last 5 variables. It's just the same 5 variables every time.  I'm
> not sure how those 5 were "chosen".  See the first attached screen shot for
> what it looks like under Firefox 3.0.8.  You can see the variable "canUndo" is
> in the middle of the list of variables.

Erm, the list of variables shown in Venkman is sorted lexically. Please just look at the order of _declaration_, regardless of bracing depth.

Comment 12

[Michael Kraft [:morac]]

(In reply to comment #11)
> Erm, the list of variables shown in Venkman is sorted lexically. Please just
> look at the order of _declaration_, regardless of bracing depth.

Ah okay.  In that case, yes they are the last 5 declared variables.  So only the last 5 declared variables are displayed.

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #9)
> Sherlock Holmes (in The Sign of the Four): "I never guess: it is an appalling
> habit, destructive to the logical faculty."
> 
> Please, no numerology!
> 
> Fallout from bug 411575. Igor, can you please take a look?

I was reviewing other code and re-read jsfun.cpp -- the problem is that bug 411575 optimized call objects so their properties use JSPROP_SHARED, which means no slots are allocated.

The magic 5 comes from the number of fixed and reserved slots for the functions you are testing: proto, parent, private, and two reserved by js_FunctionClass's flags. Nothing to do with SCOPE_HASH_THRESHOLD.

This all seems fixable using JS_GET_LOCAL_NAME_COUNT.

/be

Comment 14

[Michael Kraft [:morac]]

Isn't that what the added call_reserveSlots function is supposed to do?   It's
part of js_CallClass.  As I never see reserveSlots being called, I'm not sure
where or how it's supposed to be initialized though.

Comment 15

[Igor Bukanov]

(In reply to comment #14)
> Isn't that what the added call_reserveSlots function is supposed to do?

The reserve slots are allocated after the function returns and its frame no longer exists. Before that the call object directly access frame's arguments and variables, the same place where the interpreter accesses them.

Comment 16

[Igor Bukanov]

It seems that the bug is that the debugging API has not been updated to deal with the optimized Call object. What code exactly Venkman uses to access arguments and variables?

Comment 17

[Brendan Eich [:brendan]]

Igor: it's all layered on JS_GetPropertyDescArray. If you wander into jsd you will be in a maze of overlayered/lazily-cons'ed passages, but it bottoms out in jsdbgapi.cpp.

/be

Comment 18

[Karsten Düsterloh]

(In reply to comment #16)
> What code exactly Venkman uses to access arguments and variables?

On hitting a breakpoint, Venkman calls its console.views.locals.changeFrame function <http://mxr.mozilla.org/comm-central/source/mozilla/extensions/venkman/resources/content/venkman-views.js#597>, which passes the frame's jsdFrame.scope into a ValueRecord <http://mxr.mozilla.org/comm-central/source/mozilla/extensions/venkman/resources/content/venkman-records.js#571>. 
Later down the road, the jsdIValue's .getProperties method is used: <http://mxr.mozilla.org/comm-central/source/mozilla/extensions/venkman/resources/content/venkman-records.js#985>.

Comment 19

[John J. Barton]

(In reply to comment #17)
> Igor: it's all layered on JS_GetPropertyDescArray. If you wander into jsd you
> will be in a maze of overlayered/lazily-cons'ed passages, but it bottoms out in
> jsdbgapi.cpp.

better take Holmes with you ;-)

Comment 20

[Brendan Eich [:brendan]]

John: should the status whiteboard have [firebug-p2] or higher?

/be

Comment 21

[John J. Barton]

yes, thanks and I listed on the 1.4 for FF3.5 bug.

Comment 22

[Brendan Eich [:brendan]]

Please don't remove the regressing bug 411575, which this bug should block.

/be

Comment 23

[John J. Barton]

hmm, don't know how that happened, sorry.

Comment 24

[Igor Bukanov]

(In reply to comment #17)
> Igor: it's all layered on JS_GetPropertyDescArray.

HM, that function does the right job. First, it enumerates over properties to make sure that all are resolved. Second, for each property it calls js_GetProperty. That takes care about calling the proper argument or variable getter. So the PutCallObject optimization must be transparent for this and should not affect the results.

Comment 25

[Brendan Eich [:brendan]]

The problem can be deduced from the code shown in comment 8:

    n = STOBJ_NSLOTS(obj);
    if (n > scope->entryCount)
        n = scope->entryCount;

and the observations in comment 13. Indeed the above is not correct even ignoring the change for bug 411575 since STOBJ_NSLOTS(obj) counts JS_INITIAL_NSLOTS, which for function objects are not among those mapped by the scope->entryCount property entries in the object's map (scope).

So that's a latent bug. It happened not to bite until the fix for bug 411575 went in. But then it became a bound of 5 (JS_INITIAL_NSLOTS happens to be 5, but the value comes from proto+parent+private + 2 slots reserved by js_FunctionClass's flags) on the properties iterated from scope->lastProp up the ancestor line in the property tree.

This suggests the simplest fix:

    n = scope->entryCount;

/be

Comment 26

[Brendan Eich [:brendan]]

Attachment: fix

Comment 27

[Brendan Eich [:brendan]]

Going for b4, give the Venkman and Firebug people some long-overdue relief!

/be

Comment 28

[Brendan Eich [:brendan]]

Attachment: patch to jsfun.cpp for testing purposes only (use with next attachment)

Comment 29

[Brendan Eich [:brendan]]

Attachment: testcase extracted from this bug, for use with previous patch applied

The printfs should show all the vars and args, then the return value of the function f, the toString representation of the value of the 'q' local variable.

/be

Comment 30

[Brendan Eich [:brendan]]

Fixed in tm and m-c:

http://hg.mozilla.org/tracemonkey/rev/3bef1cf566cb
http://hg.mozilla.org/mozilla-central/rev/ca32db711816

/be

Comment 31

[Justin Dolske [:Dolske]]

Pushed to 191 per beltzner: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9145aa3b4492

Comment 32

[Brendan Eich [:brendan]]

I dug around and found the original code in CVS:

3.33         (brendan% 23-Feb-02):     n = scope->entryCount;
3.33         (brendan% 23-Feb-02):     if (n > scope->map.nslots)
3.33         (brendan% 23-Feb-02):         n = scope->map.nslots;

This came in with the property tree. I think I was concerned about not over-allocating pda->array by too much, but it still doesn't make sense. This code changed again for bug 363603 much later:

3.84         (igor.buk 06-Jan-07):     n = STOBJ_NSLOTS(obj);
3.84         (igor.buk 06-Jan-07):     if (n > scope->entryCount)
3.84         (igor.buk 06-Jan-07):         n = scope->entryCount;

which reversed the minimization, but the latent bug was there all along, waiting for bug 411575's patch to make it bite.

/be

Comment 33

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/3bef1cf566cb

Comment 34

[Michael Kraft [:morac]]

I can verify that this is fixed in the trunk and in Firefox 3.5b4.

Comment 35

[Michael Kraft [:morac]]

Verified fixed:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090527 Firefox/3.5pre (.NET CLR 3.5.30729)

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090527 Firefox/3.5pre (.NET CLR 3.5.30729)