725815 - [jsdbg2] Implement Environment.prototype.getVariableDescriptor

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

https://wiki.mozilla.org/Debugger
and search for: getVariableDescriptor

Comment 1

[:Gavin Sharp [email: [e-mail]]]

(I'm assuming the component move was unintentional?)

Comment 2

[Sebastian Zartner [:sebo]]

What happened to this?

Sebastian

Comment 3

[Eddy Bruel [:ejpbruel]]

Attachment: Implement DebuggerEnvironment_getVariableDescriptor

I whipped up a patch that I think should work. It's been a while since I last worked on Spidermonkey code, however, and my C++ is a bit rusty, so I have a couple of questions before putting the patch up for proper review.

The questions are included as comments in the patch (look for NOTE).

Comment 4

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

Comment on attachment 8666619 [details] [diff] [review]
Implement DebuggerEnvironment_getVariableDescriptor

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

I thought maybe I could help out, but it turns out I wasn't able to help out much. Sorry! I would flag shu for feedback if I were you (jorendorff tends to have a very deep review queue).

::: js/src/vm/Debugger.cpp
@@ +8109,5 @@
> +
> +    // NOTE: In DebuggerObject_getOwnPropertyDescriptor, it is claimed that this
> +    // can cause the debuggee to run, presumably due to proxy effects. To my
> +    // knowledge, scope objects are never proxified, so this should be a non-
> +    // issue here. Is this correct?

(a) Sounds like a question for shu.

(b) Seems like the kind of thing that we should MOZ_ASSERT here

@@ +8121,5 @@
> +        // because the latter may throw for optimized out slots and arguments.
> +        // To my knowledge, GetOwnPropertyDescriptor never throws, so this
> +        // should not be necessary here. Is this correct?
> +        if (!GetOwnPropertyDescriptor(cx, env, id, &desc)) {
> + 	    return false;

Nit: no {} on single line if statements

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8666619 [details] [diff] [review]
Implement DebuggerEnvironment_getVariableDescriptor

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

Clearing f?.

::: js/src/vm/Debugger.cpp
@@ +8109,5 @@
> +
> +    // NOTE: In DebuggerObject_getOwnPropertyDescriptor, it is claimed that this
> +    // can cause the debuggee to run, presumably due to proxy effects. To my
> +    // knowledge, scope objects are never proxified, so this should be a non-
> +    // issue here. Is this correct?

Scope objects can be proxies, or in the case of WithObject, essentially proxies but not implemented quite that way at the C++ level.

While none of these weird scope objects are scripted proxies, I bet they can run JS code anyway. Try something like

    var myProxy = new Proxy({}, {
        getOwnPropertyDescriptor(t, id) {
            if (id === "foo")
                throw new Error("too dizzy");
            return Reflect.getOwnPropertyDescriptor(t, id);
        }
    });
    with (proxy) debugger;  // and examine `foo`

Also test the case where the debuggee global's prototype chain has been modified to include a proxy:

    Object.setPrototypeOf(this, myProxy);

Also test the case where the binding is an uninitialized let/const. I guess for consistency with env.getVariable() it could return a descriptor with `value: {uninitialized: true}`.

    {
        debugger;  // and examine `x` and `y`
        let x;
        const y = 2;
    }

...But if you have time to read a table and revisit the API design around getVariableDescriptor(), consult <http://www.ecma-international.org/ecma-262/6.0/index.html#table-15>. I think it's misleading to present variables as properties, and the Debugger's API might be clearer if we avoid that.

(But that's just my 2 cents, do what's useful for your team.)

@@ +8119,5 @@
> +        // NOTE: In DebuggerEnv_getVariable, when env is a DebugScopeObject, we
> +        // use DebugScopeObject::getMaybeSentinelValue instead of GetProperty,
> +        // because the latter may throw for optimized out slots and arguments.
> +        // To my knowledge, GetOwnPropertyDescriptor never throws, so this
> +        // should not be necessary here. Is this correct?

GetOwnPropertyDescriptor can certainly throw if a scripted proxy is involved. I don't know about the debug proxies for lexical and call scopes. (My answer for that kind of thing is to write some tests, sorry.)

@@ +8135,5 @@
> +        if (obj->is<JSFunction>() &&
> +            IsInternalFunctionObject(obj->as<JSFunction>())) {
> +            // NOTE: Do we want to return a property descriptor with value set
> +            // to JS_OPTIMIZED_OUT for optimized out values? Or would it be
> +            // better to return undefined?

Definitely don't let magic values escape to JS code. Undefined would be fine, and a separate optimizedOut property I suppose.

Or... if this bothers users, we could probably just create and return a new Function object. But it wouldn't be the same Function object each time you look at the variable, which could have weird UI consequences (e.g. if the UI highlights variables that change when stepping).

@@ +8143,5 @@
> +    }
> +
> +    // NOTE: Why do we need to check for desc.object() here? What does desc.
> +    // object() represent? Should we put this code in its own function and
> +    // share it with DebuggerObject_getOwnPropertyDescriptor.

GetOwnPropertyDescriptor sets desc.object() to null if env has no own property env[id].

The above code computing `v` should go inside this if-block, since desc.value() is meaningless if no property was found.

@@ +8181,5 @@
>      JS_FN("names", DebuggerEnv_names, 0, 0),
>      JS_FN("find", DebuggerEnv_find, 1, 0),
>      JS_FN("getVariable", DebuggerEnv_getVariable, 1, 0),
>      JS_FN("setVariable", DebuggerEnv_setVariable, 2, 0),
> +    JS_FN("setVariableDescriptor", DebuggerEnv_getVariableDescriptor, 1, 0),

typo, "set" should be "get".

Comment 6

[Eddy Bruel [:ejpbruel]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #4)
> ...But if you have time to read a table and revisit the API design around
> getVariableDescriptor(), consult
> <http://www.ecma-international.org/ecma-262/6.0/index.html#table-15>. I
> think it's misleading to present variables as properties, and the Debugger's
> API might be clearer if we avoid that.
> 
> (But that's just my 2 cents, do what's useful for your team.)

After reading up on the spec, I tend to agree with jorendorff. Representing variables as properties only make sense when the underlying Environment Record is an object Environment Record. Since few JavaScript code tends to use with-statements these days, I'd argue representing variables as properties does not make sense most of the time.

Are there any good reasons why we *should* represent variables as properties on DebuggerEnvironment? If not, I'd like to vote not to implement this API, and to refactor EnvironmentActor to reflect that this API will never be implemented (it currently uses DebuggerEnvironment.getVariable, but still returns a list of (fake) properties to the client).

Jim, since you're (one of?) the primary designers of the debugger API, I feel like we should get your opinion on this before making a decision. Do you think that representing variables as properties makes sense?

Comment 7

[Jim Blandy :jimb]

This function was designed a long time ago, I think when ES5 was just being finalized. So it was an attempt to cover the cases I knew of at the time. If it doesn't fit the way the language behaves now, we definitely have time to read a table and revisit the API design.

Ideally, the Debugger API should provide enough information to allow a client to pick up evaluation at the program's current point (assuming you're stopped between statements, or at a call point). So Debugger.Environment just needs to give clients whatever information they need to understand how variable references are going to work in that scope.

One possibility is to dismiss the 'with' case entirely by saying that, when D.E.p.type is "with", the client just has to take `D.E.p.object` and use that to figure out what the bindings are. Then DEp.getVariableDescriptor can be defined to work only on non-'with' environments, and can just describe whatever characteristics ES6 gives to variables.

Looking at the table Jason referenced, it seems like the only characteristic a non-with binding might have other than its value is its mutability. So would it make sense for the descriptor to be this?

  { value: any, mutable: bool }

People would have to use "'value' in desc" to distinguish between initialized-to-undefined and not-initialized. I guess this is okay? (At least it's a friendlier detection technique than throwing an exception!)

Comment 8

[Jeff Walden [:Waldo]]

(In reply to Jim Blandy :jimb from comment #7)
> Looking at the table Jason referenced, it seems like the only characteristic
> a non-with binding might have other than its value is its mutability. So
> would it make sense for the descriptor to be this?
> 
>   { value: any, mutable: bool }

The configurability of the binding seems like it's also relevant, as eval of non-strict code can introduce |var| declarations that are deletable -- see DeleteBinding().

> People would have to use "'value' in desc" to distinguish between
> initialized-to-undefined and not-initialized. I guess this is okay?

Tolerable.  That |in| looks along the entire prototype chain, however, seems unideal in making this query not quite exactly what it should be.  It seems to me that a variable's value is somewhat similar to the value returned by a function call, or evaluating a script, or whatever.  "not initialized" is qualitatively different from "has this value" in the same way that a normal completion producing a value, is different from abrupt completion that throws.  So I would probably try to distinguish these two states in a similar manner.

> (At least it's a friendlier detection technique than throwing an exception!)

:-)  Yeah, we're exposing something slightly lower-level than JS itself has ever needed to expose.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.