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

RESOLVED FIXED in Firefox 44

Status

()

Firefox
Developer Tools
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: shu, Assigned: past)

Tracking

unspecified
Firefox 44
Points:
---

Firefox Tracking Flags

(firefox44 fixed, firefox45 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
NIing Panos as a heads up.
Flags: needinfo?(past)
(Assignee)

Comment 2

3 years ago
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.
Flags: needinfo?(past)
(Reporter)

Comment 3

3 years ago
(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.
(Reporter)

Comment 4

3 years ago
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
(Reporter)

Comment 5

3 years ago
Created attachment 8675195 [details] [diff] [review]
Implement Debugger.Object.asEnvironment.
Attachment #8675195 - Flags: review?(jimb)

Updated

3 years ago
Assignee: nobody → shu

Comment 6

3 years ago
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!
Attachment #8675195 - Flags: review?(jimb) → review+
(Reporter)

Comment 8

3 years ago
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?
(Reporter)

Updated

3 years ago
Flags: needinfo?(past)
https://hg.mozilla.org/mozilla-central/rev/4634f3828a89
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
(Reporter)

Comment 11

3 years ago
Reopening since only platform support is fixed, still need devtools to use the new API.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 12

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

Comment 13

3 years ago
Created attachment 8688472 [details] [diff] [review]
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?
Attachment #8688472 - Flags: feedback?(shu)
(Assignee)

Updated

3 years ago
Status: REOPENED → ASSIGNED
Flags: needinfo?(past)
(Reporter)

Comment 14

3 years ago
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?
Attachment #8688472 - Flags: feedback?(shu)
(Reporter)

Comment 15

3 years ago
See comment 14
Flags: needinfo?(jimb)
(Reporter)

Updated

3 years ago
Flags: needinfo?(past)
(Assignee)

Comment 16

3 years ago
(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.
Flags: needinfo?(past)

Comment 17

3 years ago
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

3 years ago
(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...)
Flags: needinfo?(jimb) → needinfo?(past)
(Reporter)

Comment 19

3 years ago
(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.
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.
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.
See Also: → bug 1132501
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.
(Assignee)

Comment 23

3 years ago
There are quite a few old bugs discussing console performance with comparisons between various browsers. I found bug 722685, bug 746869 and bug 761257.
Flags: needinfo?(past)
(Assignee)

Comment 24

3 years ago
Created attachment 8690162 [details] [diff] [review]
Patch v2

This works but needs a test.
(Assignee)

Updated

3 years ago
Attachment #8688472 - Attachment is obsolete: true
(Assignee)

Comment 25

3 years ago
(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.
Sounds good to me!
(Assignee)

Comment 27

3 years ago
Created attachment 8693610 [details] [diff] [review]
Debugger server changes v3

Added tests and improved the fix based on the results of running said tests. The system worked!
Attachment #8693610 - Flags: review?(bgrinstead)
(Assignee)

Updated

3 years ago
Attachment #8690162 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Assignee: shu → past
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.
Depends on: 1218455
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.
Attachment #8693610 - Flags: review?(bgrinstead) → review-
Created attachment 8695112 [details]
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
Attachment #8695112 - Flags: review?(past)
(Assignee)

Comment 31

3 years ago
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?
Attachment #8695112 - Flags: review?(past) → review+
(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.
(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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ca30d9070e53
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.