1207868 - Global lexical scope will break webconsole autocomplete for global 'let' and 'const'

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Shu-yu Guo [:shu]]

Once bug 589199 lands, autocomplete for global 'let' and 'const' bindings will break, because the current autocomplete code only considers properties on the global window. We need to teach it to also look at the global lexical scope somehow. I'm not sure how to expose it yet -- maybe a new property on Debugger.Object for global objects.

Comment 1

[Shu-yu Guo [:shu]]

NIing Panos as a heads up.

Comment 2

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

Thanks for the ping. Will these properties be present in the Debugger.Environment of the window Debugger.Object? If so, we could just make sure the console code examines that as well.

Comment 3

[Shu-yu Guo [:shu]]

(In reply to Panos Astithas [:past] from comment #2)
> Thanks for the ping. Will these properties be present in the
> Debugger.Environment of the window Debugger.Object? If so, we could just
> make sure the console code examines that as well.

No, the global lexical scope is a different scope. The scope chain for a normal content window, from outermost to innermost, is

Window -> Global lexical scope -> ...

So the D.E of the window's D.O will just have properties on the window: those explicitly assigned as properties and those declared with 'var'. 'let' and 'const' bindings are going to in the child scope of the Window.

I think we would need to expose a 'lexicalScope' property on D.O of globals that returns a D.E corresponding to the global lexical scope.

Comment 4

[Shu-yu Guo [:shu]]

After talking with jimb on IRC:

- Implement the documented but unimplemented D.O.p.asEnvironment
- Have it return the global lexical environment whose parent is the global
- Use it in devtools

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Implement Debugger.Object.asEnvironment.

Comment 6

[Jim Blandy :jimb]

Comment on attachment 8675195 [details] [diff] [review]
Implement Debugger.Object.asEnvironment.

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

I'm glad to see RequireGlobalObject getting used!

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4634f3828a89

Comment 8

[Shu-yu Guo [:shu]]

I've pushed the implementation for D.O.p.asEnvironment in SpiderMonkey. Panos, could you whip up a patch to use it in the autocomplete code?

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4634f3828a89

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4634f3828a89

Comment 11

[Shu-yu Guo [:shu]]

Reopening since only platform support is fixed, still need devtools to use the new API.

Comment 12

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

I'm keeping the NI until I can find the time to look into this. Anyone else is more than welcome to fix it however!

Comment 13

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

Attachment: Use asEnvironment in the debugger server v1

I took a look at this today and whipped up a patch that fails mysteriously in DebuggerEnv_checkThis with:

Handler function JSPropertyProvider threw an exception: Error: Debugger.Environment is not a debuggee environment
Stack: DebuggerEnvironmentSupport.getProperties@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/js-property-provider.js:523:12
getMatchedProps_impl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/js-property-provider.js:406:17
getMatchedPropsInEnvironment@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/js-property-provider.js:365:10
JSPropertyProvider@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/js-property-provider.js:277:24
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:87:14
WCA_onAutocomplete@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webconsole.js:908:18
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1606:15
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:569:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:87:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:87:14
Line: 523, column: 12

I can't see why |obj.asEnvironment()| wouldn't be a debuggee environment when |obj| has the debuggee global as its referent. Shu, any ideas?

Comment 14

[Shu-yu Guo [:shu]]

Comment on attachment 8688472 [details] [diff] [review]
Use asEnvironment in the debugger server v1

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

That error message is saying that the Debugger.Environment for the global lexical scope is in a global (window) that isn't a debuggee. Did some quick debugging -- the global object is gotten via |this.dbg.makeGlobalObjectReference(this.evalWindow)|, which explicitly does *not* add the this.evalWindow as a debuggee. So, when the global lexical scope is gotten via asEnvironment, you get a Debugger.Environment on a global that isn't a debuggee.

So I see 2 paths:

1. Try to lift the restriction on Debugger.Environments for the global lexical scope. Not sure what this restriction is actually needed for -- I'll NI Jim.
2. Add this.evalWindow as a debuggee. Is this feasible / why is it not already added as a debuggee?

Comment 15

[Shu-yu Guo [:shu]]

See comment 14

Comment 16

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

(In reply to Shu-yu Guo [:shu] from comment #14)
> 2. Add this.evalWindow as a debuggee. Is this feasible / why is it not
> already added as a debuggee?

The console avoided adding the window as a debuggee for performance reasons. One of the test workloads we've used was a tight loop that logged results continuously and IIRC we were better than Chrome on it. 3D games developers wanted to be able to do this.

If the other option is impossible, then perhaps we should bite the bullet and add the window as a debuggee for the duration of the expression evaluation. Even then I bet we will find sites that stutter while evaluating stuff in the console, but the tradeoff may be worth it.

Comment 17

[Jim Blandy :jimb]

You're generally not supposed to be able to mess with things that aren't debuggees. That check isn't there because of some weird architectural requirement, it's supposed to help catch bugs in users of the API. Perhaps it's over-sensitive here, but I think it's functioning as designed.

The whole reason we're trying to get the environment for an autocompletion is because we're about to evaluate an expression in that environment. So it's weird that the global isn't a debuggee. Since there shouldn't be any performance impact to making the window a debuggee all the time, I think Shu's "path 2" is the right way to go.

I'm going to bet that the console, for historical reasons, carefully avoids making the window a debuggee except at those points where we evaluate an expression: it probably adds it as a debuggee, does the eval, and then removes it. We should be able to just delete that whole dance: add the debuggee when we create the console, and then remove all debuggees from the Debugger when we close the console.

Comment 18

[Jim Blandy :jimb]

(In reply to Panos Astithas [:past] from comment #16)
> The console avoided adding the window as a debuggee for performance reasons.
> One of the test workloads we've used was a tight loop that logged results
> continuously and IIRC we were better than Chrome on it. 3D games developers
> wanted to be able to do this.

There oughtn't any longer be performance reasons for not making the window a debuggee. Can you find any of the original bugs, so we can make sure that making the window a debuggee all the time doesn't regress? (I really don't think it should, but...)

Comment 19

[Shu-yu Guo [:shu]]

(In reply to Jim Blandy :jimb from comment #18)
> (In reply to Panos Astithas [:past] from comment #16)
> > The console avoided adding the window as a debuggee for performance reasons.
> > One of the test workloads we've used was a tight loop that logged results
> > continuously and IIRC we were better than Chrome on it. 3D games developers
> > wanted to be able to do this.
> 
> There oughtn't any longer be performance reasons for not making the window a
> debuggee. Can you find any of the original bugs, so we can make sure that
> making the window a debuggee all the time doesn't regress? (I really don't
> think it should, but...)

It still is not free -- there are still extra branches taken here and there, but it certainly shouldn't be noticeable for hot code.

Comment 20

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

We do have some Talos tests for DevTools now to catch perf regressions, although I believe it currently only measures opening and closing tools, not actually e.g. using the console to run some input.

Comment 21

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

See also bug 1132501, which plans to make the window a debuggee of another Debugger instance, so shouldn't hurt to also make it a debuggee of this Debugger instance.

Comment 22

[James Long (:jlongster)]

After that bug, a `threadClient` instance will always exist on the tab. Why not just use that if you need to interact with debuggable objects?

I'm still not sure why you'd want multiple Debugger instances except for the (important) use case of multiple clients debugging the same page.

Are we sure that there is no perf hit for have 2 instances over 1? We already did extensive research and were very careful to enable the 1 automatically.

Comment 23

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

There are quite a few old bugs discussing console performance with comparisons between various browsers. I found bug 722685, bug 746869 and bug 761257.

Comment 24

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

Attachment: Patch v2

This works but needs a test.

Comment 25

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

(In reply to James Long (:jlongster) from comment #22)
> After that bug, a `threadClient` instance will always exist on the tab. Why
> not just use that if you need to interact with debuggable objects?
> 
> I'm still not sure why you'd want multiple Debugger instances except for the
> (important) use case of multiple clients debugging the same page.

If the debugger is going to be always on, then this sounds like a worthy optimization. However, given that I won't have time to do this work, I would prefer that this were a followup.

Comment 26

[James Long (:jlongster)]

Sounds good to me!

Comment 27

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

Attachment: Debugger server changes v3

Added tests and improved the fix based on the results of running said tests. The system worked!

Comment 28

[Brian Grinstead [:bgrins]]

Looks like Bug 1218455 (adding 'this' to the autocomplete results list) will interact with this a bit.  I've already pushed that (sorry) so marking as dependent.

Comment 29

[Brian Grinstead [:bgrins]]

Comment on attachment 8693610 [details] [diff] [review]
Debugger server changes v3

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

I've been working on top of this patch to get some the property provider tests working (and rebasing on top of 1218455), so I'll attach an updated patch.

::: devtools/server/actors/webconsole.js
@@ +919,5 @@
>        }
>      }
>      // This is the general case (non-paused debugger)
>      else {
> +      dbgObject = this.dbg.addDebuggee(this.evalWindow);

I think we might want to be a bit more defensive here and only remove the debugee if it wasn't already added.  What do you think about this?

Up above `let hadDebuggee = false` then in here:

hadDebugee = this.dbg.hasDebuggee(this.evalWindow);
dbgObject = this.dbg.addDebuggee(this.evalWindow);

@@ +924,5 @@
>      }
>  
>      let result = JSPropertyProvider(dbgObject, environment, aRequest.text,
>                                      aRequest.cursor, frameActorId) || {};
> +    if (!frameActorId) {

Then instead of !frameActorId

if (dbgObject && !hadDebugee) {}

::: devtools/shared/webconsole/js-property-provider.js
@@ +231,2 @@
>    }
> +  obj = getVariableInEnvironment(env, properties.shift());

This is going to break array indexing on a single prop, i.e. `testArray[0].` (see devtools/shared/webconsole/test/unit/test_js_property_provider.js).  We need to reuse some of the array member property functionality.

Comment 30

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1207868 - Fix lexical scope autocomplete for global 'let' and 'const';r=past

Bug 1207868 - Fix lexical scope autocomplete for global 'let' and 'const';r=past

Comment 31

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

Comment on attachment 8695112 [details]
MozReview Request: Bug 1207868 - Fix lexical scope autocomplete for global 'let' and 'const';r=past

https://reviewboard.mozilla.org/r/27015/#review24417

::: devtools/shared/webconsole/js-property-provider.js:548
(Diff revision 1)
> +    // it's not a valid ECMAScript identifier name

Do we have a test for this?

Comment 32

[Brian Grinstead [:bgrins]]

(In reply to Panos Astithas [:past] from comment #31)
> Comment on attachment 8695112 [details]
> MozReview Request: Bug 1207868 - Fix lexical scope autocomplete for global
> 'let' and 'const';r=past
> 
> https://reviewboard.mozilla.org/r/27015/#review24417
> 
> ::: devtools/shared/webconsole/js-property-provider.js:548
> (Diff revision 1)
> > +    // it's not a valid ECMAScript identifier name
> 
> Do we have a test for this?

The text was taken from https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Debugger.Environment (in getVariable), and I noticed it due to log spam in the unit test (although it was still passing).  I will see if I can add a failing case for that.

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ca30d9070e53

Comment 34

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #32)
> (In reply to Panos Astithas [:past] from comment #31)
> > Comment on attachment 8695112 [details]
> > MozReview Request: Bug 1207868 - Fix lexical scope autocomplete for global
> > 'let' and 'const';r=past
> > 
> > https://reviewboard.mozilla.org/r/27015/#review24417
> > 
> > ::: devtools/shared/webconsole/js-property-provider.js:548
> > (Diff revision 1)
> > > +    // it's not a valid ECMAScript identifier name
> > 
> > Do we have a test for this?
> 
> The text was taken from
> https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Debugger.
> Environment (in getVariable), and I noticed it due to log spam in the unit
> test (although it was still passing).  I will see if I can add a failing
> case for that.

Ah, just needed a fallible version of the js-property-provider and the test will now fail without the try/catch https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/js-property-provider.js#546

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ca30d9070e53