Closed Bug 941287 Opened 6 years ago Closed 5 years ago

Debugger needs UI for optimized-out variables

Categories

(DevTools :: Debugger, defect, P3)

defect

Tracking

(relnote-firefox 38+)

RESOLVED FIXED
Firefox 38
Tracking Status
relnote-firefox --- 38+

People

(Reporter: jimb, Assigned: jlong)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 2 obsolete files)

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.
Blocks: 716647
Jim, is this just spec'd out, or is it implemented in Debugger.Object right now?
Flags: needinfo?(jimb)
Priority: -- → P3
(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
Flags: needinfo?(jimb)
Depends on: 963879
CC'ing ejpbruel who was talking about this today in Toronto.
assigning to jlongster for great justice!
Assignee: nobody → jlong
Status: NEW → ASSIGNED
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?
(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>".
Summary: JS Debugger needs UI for optimized-out variables → Debugger needs UI for optimized-out variables
Attached patch 941287.patch (obsolete) — Splinter Review
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.
Attachment #8546874 - Flags: review?(nfitzgerald)
Attached image screenshot.png
Oh, I totally forgot about localization. Will fix that soon.
(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 on attachment 8546874 [details] [diff] [review]
941287.patch

I think Victor would be a better reviewer for this.
Attachment #8546874 - Flags: review?(nfitzgerald) → review?(vporof)
'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.
(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 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?
Attachment #8546874 - Flags: review?(vporof) → review-
(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?
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).
(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!
Attached patch 941287.patch (obsolete) — Splinter Review
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.
Attachment #8546874 - Attachment is obsolete: true
Attachment #8547839 - Flags: review?(vporof)
(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 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.
Attachment #8547839 - Flags: review?(vporof) → review-
(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.
(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.
(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.
(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?
(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.
(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 :)
(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?
(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.
(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.
Attached patch 941287.patchSplinter Review
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.
Attachment #8547839 - Attachment is obsolete: true
Attachment #8549169 - Flags: review?(vporof)
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.
Attachment #8549169 - Flags: review?(vporof) → review+
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).
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/34c2733995e8
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/34c2733995e8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
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]
relnote-firefox: --- → ?
Keywords: dev-doc-needed
Have added this to the 38 Dev Edition release notes as follows: "Optimized-out variables are now visible in Debugger UI"
Looks good!
Flags: needinfo?(jlong)
Product: Firefox → DevTools
Blocks: 1565711
Blocks: 1565713
No longer blocks: 1565711
No longer blocks: 1565713
You need to log in before you can comment on or make changes to this bug.