Closed Bug 808791 Opened 7 years ago Closed 7 years ago

Reuse existing scopes in the variables view when faced with name duplicates

Categories

(DevTools :: Debugger, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 21

People

(Reporter: vporof, Assigned: jimb)

Details

(Keywords: dev-doc-needed)

Attachments

(6 files)

Attached image because this
No description provided.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
I'm curious, can you attach the code or protocol log that led to this?
STR:

1. Open/close script debugger
2. Open browser debugger
3. Add breakpoint in DebuggerUI.jsm:291 (removeEventListener("Debugger:Loaded"))
4. Open script debugger
5. Profit

I have never seen this happening in the wild though.
This is the log from the |frames| response and it looks like the frontend correctly displays all the parent scopes that the server sent. If there is any bug here, it's not in the frontend.
(In reply to Panos Astithas [:past] from comment #3)
> Created attachment 678669 [details]
> Log from the |frames| protocol response
> 
> This is the log from the |frames| response and it looks like the frontend
> correctly displays all the parent scopes that the server sent. If there is
> any bug here, it's not in the frontend.

If this isn't a bug, I'd at least assert it's a bit confusing.
So my theory is this:
- 1st (inner) dbgLoaded function scope: the lexical scope containing this, bkp, etc.
- 2nd function scope: a closure over the function expression for dbgLoaded
- 3rd scope: a closure over the lexical scope inside DP__create
- 4th scope: a closure over the function expression for DP__create
- 5th scope: ?
- 6th scope: the global scope

It would be nice if we could find someone more familiar with the intricacies of scopes in JavaScript to clarify this.
(In reply to Victor Porof [:vp] from comment #4)
> (In reply to Panos Astithas [:past] from comment #3)
> > Created attachment 678669 [details]
> > Log from the |frames| protocol response
> > 
> > This is the log from the |frames| response and it looks like the frontend
> > correctly displays all the parent scopes that the server sent. If there is
> > any bug here, it's not in the frontend.
> 
> If this isn't a bug, I'd at least assert it's a bit confusing.

Indeed and it would be great if we could find a way to better explain to the user what is going on.
Unassigning myself for now.
Assignee: vporof → nobody
Status: ASSIGNED → NEW
Priority: -- → P3
Jim, does my explanation in comment 5 make sense, or is there a bug in the debugger? If that's correct, then we could consider adding an option to collapse scopes with the same name and have it enabled by default.
Flags: needinfo?(jimb)
Since the toolbox landed, the codepath in the STR from comment 2 is harder to exercise. Alternative:

1. Open/close script debugger
2. Open browser debugger
3. Add breakpoint in DebuggerPanel.jsm:35 (onDebuggerLoaded)
4. Open script debugger
Attached image STR 2
This can happen in the wild as well. See gist: https://gist.github.com/4288239
I think there are several things going on here:

First: there will often be more elements on the scope chain than one might expect. In code like this:

   foo(function bar() { ... })

when we evaluate 'function bar...', the function object we create captures the enclosing environment. But actually, we first extend the enclosing environment with a new scope chain element binding 'bar' to the function object. This ensures that in code like this:

   var bar = "global"; foo(function bar() { return bar; });

the reference to 'bar' within bar's body refers to the function, not to the global variable. This scope chain object, which SpiderMonkey calls a DeclEnv, holds only a single, read-only binding for 'bar'. If we do eventually call bar, we'll create a second scope chain object for bar's local variables, with the DeclEnv as its parent.

In the diagram, Function scopes number 2, 4, and 5 look like DeclEnvs to me. (That's a little weird, because there shouldn't be two in a row, but let's leave that aside...)

I think it would make sense for the UI to not show DeclEnvs. They only ever contain the one binding, for the function's name. User declarations can't affect them. They clutter up the display. People might find it less confusing overall if they were left implicit.


Second: The [labels] on the "Function scope" forms are almost certainly wrong. The third one, for example, is almost certainly the locals for DP__create, not dbgLoaded. From the logs Panos posted, it does seem to me that the UI is correctly displaying what it received from the server, so the bug is probably in the server.

The way EnvironmentActor.prototype.form is trying to get the callee looks really delicate. Debugger.Environment.prototype.callee has been implemented for some time (bug 747514, mentioned in many comments), so there's no need to pass the stack frame in any more. I'm suspicious of this code.
Flags: needinfo?(jimb)
(In reply to Victor Porof [:vp] from comment #11)
> This can happen in the wild as well. See gist:
> https://gist.github.com/4288239

Ahh, that's wonderful. (And: great bug report to begin with). Thanks, Victor!
I think this code is just wrong:

    // Deduce the frame that created the parent scope in order to pass it to
    // parent.form(). TODO: this can be removed after bug 747514 is done.
    let parentFrame = aObject;
    if (this.obj.type == "declarative" && aObject.older) {
      parentFrame = aObject.older;
    }

There's simply no connection between the parent of an environment and the older frame. Consider this:

function makeAdder(x) { function Adder(y) { return x+y; } }
add3 = makeAdder(3);
add3(4)

When we call add3(4), the calling frame is the top-level frame, but the enclosing environment is that created for the call to makeAdder.

We should actually do the cleanups the comments suggest now that bug 747514 is landed.
I think I have a fix for this.
Assignee: nobody → jimb
Attached patch xpcshell testsSplinter Review
That last patch seems to correct the worst of it: things get the right lables. But I'm still seeing one more environment than I expected; I don't understand why it's there. I suspect a Debugger bug there; looking into it.

If you're curious, the xpcshell tests patch has a pretty legible representation of what we're getting with the patch applied (i.e. with the original bug fixed, but with the mysterious extra environment present). The test passes with all patches attached thus far applied, but I suspect the output should be different.
The excess environment chain element does seem to be a bug; filed as bug 822566.
Comment on attachment 693208 [details] [diff] [review]
xpcshell tests

Since I've confirmed that Debugger.Environment is responsible for the weird extra environment chain element, I think the fix for the protocol side is ready for review.
Attachment #693208 - Flags: review?(past)
Attachment #693210 - Flags: review?(past)
Comment on attachment 693210 [details] [diff] [review]
JS debug server: construct environment forms correctly, using Debugger.Environment.prototype.callee.

Review of attachment 693210 [details] [diff] [review]:
-----------------------------------------------------------------

I'm still surprised that I forgot to update this code when bug 747514 landed. Filing a bug for every TODO comment is such a smart idea.
Attachment #693210 - Flags: review?(past) → review+
Comment on attachment 693208 [details] [diff] [review]
xpcshell tests

Review of attachment 693208 [details] [diff] [review]:
-----------------------------------------------------------------

You could also call the test test_framebindings-06.js, but I don't really care. I am sooo going to use the new JSON matching function though!
Attachment #693208 - Flags: review?(past) → review+
Attachment #693207 - Flags: review?(khuey) → review?(jmaher)
(In reply to Panos Astithas [:past] from comment #23)
> I am sooo going to use the new JSON matching function though!

Yeah, I like it too! I hope that will be helpful matching packets.
Comment on attachment 693207 [details] [diff] [review]
Add some xpcshell testing functions handy for matching JSON packets.

Review of attachment 693207 [details] [diff] [review]:
-----------------------------------------------------------------

quite a patch, thanks for including tests with it.

::: testing/xpcshell/head.js
@@ +545,5 @@
> +      _dump("TEST-PASS | " + stack.filename + " | [" + stack.name + " : " +
> +            stack.lineNumber + "] " + text + "\n");
> +    }
> +  } else {
> +    if (todo) {

why not '} else if (todo) {' ?

@@ +560,5 @@
>    if (!stack)
>      stack = Components.stack.caller;
>  
>    var text = left + " == " + right;
> +  do_report_result(left == right, left + " == " + right, stack, todo);

why not do:
do_report_result(left == right, text, stack, todo);
Attachment #693207 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #25)
> Comment on attachment 693207 [details] [diff] [review]
> Add some xpcshell testing functions handy for matching JSON packets.
> 
> Review of attachment 693207 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> quite a patch, thanks for including tests with it.
> 
> ::: testing/xpcshell/head.js
> @@ +545,5 @@
> > +      _dump("TEST-PASS | " + stack.filename + " | [" + stack.name + " : " +
> > +            stack.lineNumber + "] " + text + "\n");
> > +    }
> > +  } else {
> > +    if (todo) {
> 
> why not '} else if (todo) {' ?

Because there's a symmetry there: we have four combinations: passed/failed, and expected/unexpected. The 'if' nesting is supposed to highlight that. (I don't know if it's effective.)

> @@ +560,5 @@
> >    if (!stack)
> >      stack = Components.stack.caller;
> >  
> >    var text = left + " == " + right;
> > +  do_report_result(left == right, left + " == " + right, stack, todo);
> 
> why not do:
> do_report_result(left == right, text, stack, todo);

Ah, I missed that --- thanks!
The do_check_matches / todo_check_matches functions are really cool!

They should be documented at:
https://developer.mozilla.org/en/docs/Writing_xpcshell-based_unit_tests

I've added the dev-doc-needed keyword to that effect.
Keywords: dev-doc-needed
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.