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

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Victor Porof [:vporof][:vp]]

Attachment: because this

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

I'm curious, can you attach the code or protocol log that led to this?

Comment 2

[Victor Porof [:vporof][:vp]]

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.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: 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.

Comment 4

[Victor Porof [:vporof][:vp]]

(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.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

(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.

Comment 7

[Victor Porof [:vporof][:vp]]

Unassigning myself for now.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 9

[Victor Porof [:vporof][:vp]]

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

Comment 10

[Victor Porof [:vporof][:vp]]

Attachment: STR 2

Comment 11

[Victor Porof [:vporof][:vp]]

This can happen in the wild as well. See gist: https://gist.github.com/4288239

Comment 12

[Jim Blandy :jimb]

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.

Comment 13

[Jim Blandy :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!

Comment 14

[Jim Blandy :jimb]

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.

Comment 15

[Jim Blandy :jimb]

I think I have a fix for this.

Comment 16

[Jim Blandy :jimb]

Attachment: Add some xpcshell testing functions handy for matching JSON packets.

Comment 17

[Jim Blandy :jimb]

Attachment: xpcshell tests

Comment 18

[Jim Blandy :jimb]

Attachment: JS debug server: construct environment forms correctly, using Debugger.Environment.prototype.callee.

Comment 19

[Jim Blandy :jimb]

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.

Comment 20

[Jim Blandy :jimb]

The excess environment chain element does seem to be a bug; filed as bug 822566.

Comment 21

[Jim Blandy :jimb]

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.

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

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!

Comment 24

[Jim Blandy :jimb]

(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 25

[Joel Maher ( :jmaher ) (UTC -8)]

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);

Comment 26

[Jim Blandy :jimb]

(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!

Comment 27

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2505111c4cee
https://hg.mozilla.org/integration/mozilla-inbound/rev/49e48938f65c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae9b3bb6b75a

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2505111c4cee
https://hg.mozilla.org/mozilla-central/rev/49e48938f65c
https://hg.mozilla.org/mozilla-central/rev/ae9b3bb6b75a

Comment 29

[:Paolo Amadini]

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.