Open Bug 1414683 Opened 7 years ago Updated 2 years ago

[jsdbg2] Debugger.Environment.prototype.type should return more detailed information

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

Tracking Status
firefox58 --- fix-optional

People

(Reporter: jimb, Unassigned)

References

(Blocks 2 open bugs)

Details

At the moment, the Debugger.Environment.prototype.type accessor property distinguishes only between "declarative", "with", and "object" environments. This causes clients that want to help users recognize scopes to resort to other means to make finer discriminations, which is often inefficient and inaccurate. For example, here's some code implementing the Firefox devtools protocol's environment actor: // What is this environment's type? if (this.obj.type == "declarative") { form.type = this.obj.callee ? "function" : "block"; } else { form.type = this.obj.type; } http://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/devtools/server/actors/environment.js#45-50 On environments with callees, checking the callee property here causes the allocation of a Debugger.Object which is used as a boolean value and abandoned. Meanwhile, the implementation of D.E.p.type is throwing away exactly the details the environment actor wants: bool DebugEnvironmentProxy::isForDeclarative() const { EnvironmentObject& e = environment(); return e.is<CallObject>() || e.is<VarEnvironmentObject>() || e.is<ModuleEnvironmentObject>() || e.is<WasmInstanceEnvironmentObject>() || e.is<WasmFunctionCallObject>() || e.is<LexicalEnvironmentObject>(); } http://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/js/src/vm/EnvironmentObject.cpp#2371-2381 It's reasonable to be concerned about exposing implementation details via the Debugger API, but most of these distinctions look like they can be defined in terms of the source language.
Priority: -- → P3
Blocks: js-devtools
Here are some notes from a November call where we discussed this: https://docs.google.com/document/d/1QZyTzte5UvQsqN3ad38Q2RqIEJgTWsRqZgsYU4VEP5E/edit
Here is a quick patch based on my memory of our call in November. I did not try to compile this so, it is safe to say that it will likely contain lots of syntax errors. The main thing i'm interested in though is if this captures the primary goals: 1. make environment->type() more fine grained 2. simplify the environment actor https://github.com/jasonLaster/gecko-dev/compare/bookmarks/central...jasonLaster:js-env-type Jim, let me know how close this was to what you remember...
I'm not going to do an implementation of this, but I did look it over to see how complex it would be, and I wanted to share what I could see. I think that patch is a step in the right direct. My main concern was that it only covers a few of the cases that the debugger's own AST visitor marks, so I was curious to see where things diverged. As you might guess, the main thing is that, from a user standpoint, it would be nice to be able to have more specific information about where a lexical scope is used. Some places in the engine have custom scopes to handle spec edge cases, but there a lots of places where things are generic lexical scopes. While that's fine for the engine, it's not ideal if the goal is for users to easily see what scopes map to. Here's a list of the scopes we handle in the devtools AST visitor, compared to what I could see in the engine. * Global * ScopeKind::Global * Lexical Global * Emitted in a standard lexical scope * Class Inner * Emitted in a standard lexical scope * For-loop lexical capture * Emitted in a standard lexical scope * Switch * Emitted in a standard lexical scope * Block * Emitted in a standard lexical scope * Module * ScopeKind::Module * Named function expression * ScopeKind::NamedLambda * ScopeKind::StrictNamedLambda * Function * ScopeKind::Function * ScopeKind::ParameterExpressionVar * ScopeKind::FunctionBodyVar * Lexical Function Body * ScopeKind::Lexical * Catch * ScopeKind::SimpleCatch * ScopeKind::Catch * With * ScopeKind::With * Class Field Inititializer * Doesn't exist in engine The other issue is that not all of these ScopeKind types are exposed in their respective environment types, so that might also need to be passed through somewhere.
Blocks: dbg-scopes
Keywords: helpwanted
Blocks: dbg-68
No longer blocks: dbg-68
Whiteboard: [debugger-mvp]
Whiteboard: [debugger-mvp]

I'd love to help with this, but it's unclear from the bug exactly what is the goal of this change. Is the goal to simply give additional information on scope by including "function" and "block" to "with" and "object"? Or do something more comprehensive like what Mr. Smyth recommended? Or simply clean up the patch that Mr. Laster drafted? If you could be a little more specific, I'd love to help!

Thanks!
Will

Dropping the good-first-bug and helpwanted tags, given this bug isn't currently mentored.

Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.