941287 - Debugger needs UI for optimized-out variables

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Jim Blandy :jimb]

In the commit below, we added a way for Debugger.Environment.prototype.getVariable to indicate that the given variable's value is not available, because it was optimized out:

https://github.com/jimblandy/DebuggerDocs/commit/9343e30df3b50cb7cae8d05504788329c6a6e7ec

As we add support for debugging optimized code, these values will start to show up. The server and client need to be updated to handle them.

Comment 1

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

Jim, is this just spec'd out, or is it implemented in Debugger.Object right now?

Comment 2

[Jim Blandy :jimb]

(In reply to Nick Fitzgerald [:fitzgen] from comment #1)
> Jim, is this just spec'd out, or is it implemented in Debugger.Object right
> now?

This is specified:
https://wiki.mozilla.org/Debugger#Function_Properties_of_the_Debugger.Environment_Prototype_Object

... but not implemented:
http://dxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.cpp#5692

Comment 3

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

CC'ing ejpbruel who was talking about this today in Toronto.

Comment 4

[Rob Campbell [:rc] (:robcee)]

assigning to jlongster for great justice!

Comment 5

[James Long (:jlongster)]

A few questions:

* Is this going into the VariablesView? By "needs UI" you mean we just need to add the flag in there?
* Since it's optimized out, we don't have access to the value anymore, right? Can we just have the text "<optimized out>" or something in the VariablesView where the value normally is?

Comment 6

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

(In reply to James Long (:jlongster) from comment #5)
> A few questions:
> 
> * Is this going into the VariablesView? By "needs UI" you mean we just need
> to add the flag in there?

Yes.

It would be cool if we did something smart about evals through the console, watch expressions, and conditional breakpoints that attempted to use optimized away variables. I think we would need a new debugger api completion variant or a known error type that would be thrown.

Filed bug 1000317

> * Since it's optimized out, we don't have access to the value anymore,
> right? Can we just have the text "<optimized out>" or something in the
> VariablesView where the value normally is?

Yes but the specific text should be localizable and ideally differentiable from the string value "<optimized out>".

Comment 7

[James Long (:jlongster)]

Attachment: 941287.patch

This wasn't too bad now that I'm more familiar with the code. Here's a quick attempt.

It's a little hacky in that it sends a grip of type `null` across the wire, with an additional property `optimizedOut`. Perhaps it should be `undefined`. But it seems like a reasonable approach, and on the frontend it checks the special properties first.

Note that strings appear differently in the UI, so it is differentiated from a string with the value "(optimized away)". Attached is a screenshot of how it looks.

In the code I noticed two other conditions where we don't have the values:

      if (value && (value.missingArguments || value.uninitialized)) {
        continue;
      }

Should I do the same for this, showing messages `(missing arguments)` and `(uninitialized)` respectively? "missing arguments" isn't very clear, what message should I show for that? I don't even know what that means.

Comment 8

[James Long (:jlongster)]

Attachment: screenshot.png

Comment 9

[James Long (:jlongster)]

Oh, I totally forgot about localization. Will fix that soon.

Comment 10

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

(In reply to James Long (:jlongster) from comment #7)
> Created attachment 8546874 [details] [diff] [review]
> 941287.patch
> 
> This wasn't too bad now that I'm more familiar with the code. Here's a quick
> attempt.
> 
> It's a little hacky in that it sends a grip of type `null` across the wire,
> with an additional property `optimizedOut`. Perhaps it should be
> `undefined`. But it seems like a reasonable approach, and on the frontend it
> checks the special properties first.
> 
> Note that strings appear differently in the UI, so it is differentiated from
> a string with the value "(optimized away)". Attached is a screenshot of how
> it looks.
> 
> In the code I noticed two other conditions where we don't have the values:
> 
>       if (value && (value.missingArguments || value.uninitialized)) {
>         continue;
>       }
> 
> Should I do the same for this, showing messages `(missing arguments)` and
> `(uninitialized)` respectively?

Is `uninitialized` for when a let variable is created later in this block? If it was a var, we should just have undefined...

> "missing arguments" isn't very clear, what
> message should I show for that? I don't even know what that means.

I don't know either.

Comment 11

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

Comment on attachment 8546874 [details] [diff] [review]
941287.patch

I think Victor would be a better reviewer for this.

Comment 12

[Shu-yu Guo [:shu]]

'uninitialized' is the initial value of all lexical bindings, for TDZ checks.

'missing arguments' is when the Debugger requests an arguments object on a debug scope, and for a variety of reasons, there isn't one. Could be that there is no arguments binding inside the function, and so the frontend decided to optimize away arguments. Could be that after analyzing the function we decided we can save on creating arguments objects. I'm not sure how to best explain this either.

Comment 13

[Jim Blandy :jimb]

(In reply to James Long (:jlongster) from comment #7)
> In the code I noticed two other conditions where we don't have the values:
> 
>       if (value && (value.missingArguments || value.uninitialized)) {
>         continue;
>       }
> 
> Should I do the same for this, showing messages `(missing arguments)` and
> `(uninitialized)` respectively? "missing arguments" isn't very clear, what
> message should I show for that? I don't even know what that means.

Without weighing in on which exact message is best, showing these in a way that indicates that their values are unavailable is definitely preferable to just omitting them. Users see the latter behavior as a bug in the tools.

How about "(arguments object unavailable)" and "(no value set yet)"?

Comment 14

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

Comment on attachment 8546874 [details] [diff] [review]
941287.patch

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

Needs test and localization.

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +3352,5 @@
>        case "Infinity":
>        case "-Infinity":
>        case "-0":
> +        if(aGrip.optimizedOut) {
> +          return "(optimized away)";

Automatic r- because this isn't localized :)
Are these the only kinds of values that may be optimized out?

Comment 15

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

(In reply to James Long (:jlongster) from comment #8)
> Created attachment 8546875 [details]
> screenshot.png

The styling shouldn't change between variables. If one was optimized out, we can't presumably know its type, right?

Comment 16

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

Another question I'd like us to debate is: should users be able to change the values of optimized out variables? It's debatable, and I'm inclined to say no. In this case, though, editing should be disabled in the variables view (clicking on the value shouldn't create an input).

Comment 17

[James Long (:jlongster)]

(In reply to Victor Porof [:vporof][:vp] from comment #14)
> Comment on attachment 8546874 [details] [diff] [review]
> 941287.patch
> 
> Review of attachment 8546874 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Needs test and localization.
> 
> ::: browser/devtools/shared/widgets/VariablesView.jsm
> @@ +3352,5 @@
> >        case "Infinity":
> >        case "-Infinity":
> >        case "-0":
> > +        if(aGrip.optimizedOut) {
> > +          return "(optimized away)";
> 
> Automatic r- because this isn't localized :)
> Are these the only kinds of values that may be optimized out?

Yeah, I mentioned above that I forgot about that until after I posted the patch. Will fix!

It looks like there's 2 other types, mentioned in the above comments. Just need to figure out the right UI message for those too.

(In reply to Victor Porof [:vporof][:vp] from comment #15)
> (In reply to James Long (:jlongster) from comment #8)
> > Created attachment 8546875 [details]
> > screenshot.png
> 
> The styling shouldn't change between variables. If one was optimized out, we
> can't presumably know its type, right?

Now sure what you mean? We don't know the types at all. The screenshot shows that if you have a string with the value "(optimized away)" in your code, the UI will display our internal message differently. I just wanted to make sure you couldn't accidentally see a value there that *looked* like our internal one.

(In reply to Victor Porof [:vporof][:vp] from comment #16)
> Another question I'd like us to debate is: should users be able to change
> the values of optimized out variables? It's debatable, and I'm inclined to
> say no. In this case, though, editing should be disabled in the variables
> view (clicking on the value shouldn't create an input).

Good point, I think we should just disable it too. Will do that. Thanks!

Comment 18

[James Long (:jlongster)]

Attachment: 941287.patch

This is a more complete patch. The fields are now uneditable and it shows all 3 cases: optimized out, uninitialized, and missing arguments.

I show "(uninitialized)" for a `let` binding in the TDZ, which I think makes sense. Right now I show "(unavailable)" for missing args but I don't really think that's optimal... I don't think we need to have a detailed message, but something short and sweet that hints at what is going on.

Comment 19

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

(In reply to James Long (:jlongster) from comment #17)

> Now sure what you mean? We don't know the types at all. The screenshot shows
> that if you have a string with the value "(optimized away)" in your code,
> the UI will display our internal message differently. I just wanted to make
> sure you couldn't accidentally see a value there that *looked* like our
> internal one.
> 

Aaaah, that was intentional. Excellent.

Comment 20

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

Comment on attachment 8547839 [details] [diff] [review]
941287.patch

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

Needs a test.

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +2587,5 @@
>    _customizeVariable: function() {
>      let ownerView = this.ownerView;
>      let descriptor = this._initialDescriptor;
>  
> +    if (ownerView.eval && this.eval && this.getter || this.setter) {

Doesn't this break getters and setters? Can you still edit them? How about the actual getter/setter variable, can it still be turned into a regular value?

@@ +2824,5 @@
>      let descriptor = this._initialDescriptor;
>      let target = this._target;
>      let name = this._nameString;
>  
> +    if (this.eval && ownerView.eval) {

Ditto.

@@ +4094,5 @@
>      return this._variable._valueLabel;
>    },
>  
>    get shouldActivate() {
> +    return this._variable.eval && this._variable.ownerView.eval;

Ditto.

Comment 21

[Jim Blandy :jimb]

(In reply to Victor Porof [:vporof][:vp] from comment #16)
> Another question I'd like us to debate is: should users be able to change
> the values of optimized out variables? It's debatable, and I'm inclined to
> say no. In this case, though, editing should be disabled in the variables
> view (clicking on the value shouldn't create an input).

The reasons a variable might be optimized out at present all imply that its value either won't be used, or that the copy that will be used can't be found (or else we'd give it to you). So we should definitely not allow modification in the UI, since we can't implement it in the back end.

Comment 22

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

(In reply to Jim Blandy :jimb from comment #21)
> (In reply to Victor Porof [:vporof][:vp] from comment #16)
> > Another question I'd like us to debate is: should users be able to change
> > the values of optimized out variables? It's debatable, and I'm inclined to
> > say no. In this case, though, editing should be disabled in the variables
> > view (clicking on the value shouldn't create an input).
> 
> The reasons a variable might be optimized out at present all imply that its
> value either won't be used, or that the copy that will be used can't be
> found (or else we'd give it to you). So we should definitely not allow
> modification in the UI, since we can't implement it in the back end.

I see "editing" a value in this case as creating a variable with the given value in the relevant scope. Why should we really care that a variable was optimized away? If it has a name, it exists as far as the developer's concerned.

Just playing devil's advocate; I still don't think we should allow this.

Comment 23

[Jim Blandy :jimb]

(In reply to Victor Porof [:vporof][:vp] from comment #22)
> I see "editing" a value in this case as creating a variable with the given
> value in the relevant scope. Why should we really care that a variable was
> optimized away? If it has a name, it exists as far as the developer's
> concerned.

I agree with you about the intended effect. My point wasn't that it was somehow ill-defined, but rather that it's futile, because of restrictions of the Debugger API.

Comment 24

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

(In reply to Jim Blandy :jimb from comment #23)
> 
> I agree with you about the intended effect. My point wasn't that it was
> somehow ill-defined, but rather that it's futile, because of restrictions of
> the Debugger API.

Maybe I'm missing something, but won't eval("let varname = newvalue") work?

Comment 25

[Shu-yu Guo [:shu]]

(In reply to Victor Porof [:vporof][:vp] from comment #24)
> (In reply to Jim Blandy :jimb from comment #23)
> > 
> > I agree with you about the intended effect. My point wasn't that it was
> > somehow ill-defined, but rather that it's futile, because of restrictions of
> > the Debugger API.
> 
> Maybe I'm missing something, but won't eval("let varname = newvalue") work?

If by that 'eval' you mean an 'eval' in JS, the presence of 'eval' would prevent anything from being optimized out.

If by that 'eval' you mean Debugger.Frame.eval, and 'varname' is an extant binding that's been optimized out, D.F.eval would throw.

Comment 26

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

(In reply to Shu-yu Guo [:shu] from comment #25)
> 
> If by that 'eval' you mean Debugger.Frame.eval, and 'varname' is an extant
> binding that's been optimized out, D.F.eval would throw.

Sad. Okay, I'm convinced :)

Comment 27

[James Long (:jlongster)]

(In reply to Victor Porof [:vporof][:vp] from comment #20)
> Comment on attachment 8547839 [details] [diff] [review]
> 941287.patch
> 
> Review of attachment 8547839 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Needs a test.
> 
> ::: browser/devtools/shared/widgets/VariablesView.jsm
> @@ +2587,5 @@
> >    _customizeVariable: function() {
> >      let ownerView = this.ownerView;
> >      let descriptor = this._initialDescriptor;
> >  
> > +    if (ownerView.eval && this.eval && this.getter || this.setter) {
> 
> Doesn't this break getters and setters? Can you still edit them? How about
> the actual getter/setter variable, can it still be turned into a regular
> value?

All of the tests pass, so I don't think so. `Variable` is `Scope`, so it inherits `eval` from the owner view like `Scope` does, so it's the same as its parent scope. The tests passing confirmed this for me - there are two tests called "variables-view-edit-getset-*" that look like they test this.

Should I run a specific test case manually? Do I create an object with a getter & setter and inspect it in the VariablesView?

Comment 28

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

(In reply to James Long (:jlongster) from comment #27)

> Should I run a specific test case manually? Do I create an object with a
> getter & setter and inspect it in the VariablesView?

Try testing it manually too, just to be sure. We want getters and setters to be editable.

Comment 29

[James Long (:jlongster)]

(In reply to Victor Porof [:vporof][:vp] from comment #28)
> (In reply to James Long (:jlongster) from comment #27)
> 
> > Should I run a specific test case manually? Do I create an object with a
> > getter & setter and inspect it in the VariablesView?
> 
> Try testing it manually too, just to be sure. We want getters and setters to
> be editable.

It works. However, if it makes you feel better, I think I found a different approach where I don't have to touch VariablesView as much. Patch coming.

Comment 30

[James Long (:jlongster)]

Attachment: 941287.patch

This patch makes the fields non-editable by simply forcing the "writable" property on them to be `false`. This is probably more straight-forward than the previous approach. All tests are passing.

Comment 31

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

Comment on attachment 8549169 [details] [diff] [review]
941287.patch

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

Oh, this is much more elegant! I love it.

Comment 32

[James Long (:jlongster)]

Woo! https://treeherder.mozilla.org/#/jobs?repo=try&revision=07eb23d2c153

Comment 33

[James Long (:jlongster)]

try is green, except there's a build bust for B2G Desktop Linux but this patch can't possibly be the cause (it's just a few JS changes in the devtools).

Comment 34

[Ryan VanderMeulen [:RyanVM]]

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

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/34c2733995e8

Comment 36

[Florian Bender]

Release Note Request (optional, but appreciated)
[Why is this notable]: Interesting information for Devs
[Suggested wording]: [Unsure] Mark optimized-out variables.
[Links (documentation, blog post, etc)]: [waiting on dev-doc-needed]

Comment 37

[Lukas Blakk [:lsblakk] use ?needinfo]

Have added this to the 38 Dev Edition release notes as follows: "Optimized-out variables are now visible in Debugger UI"

Comment 38

[Will Bamberg [:wbamberg]]

I've updated https://developer.mozilla.org/en-US/docs/Tools/Debugger/How_to/Examine,_modify,_and_watch_variables#Examine_variables to note this. Let me know if it's OK.

Comment 39

[James Long (:jlongster)]

Looks good!