723062 - Allow the user to edit the value of a debuggee object's property in the debugger

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

The Debugger API allows modifying a debuggee object's property values. The debugger UI should allow in-place property editing that takes advantage of that, as in the case of the HTML panel or the Rule View.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Note that this should only be allowed for mutable variables. For immutable ones it might be a good idea to show a tooltip or some decoration (a tiny lock?) to help the user understand why clicking on the value has no effect.

On second thought, maybe a descriptive tooltip/doorhanger should appear when the user tries to click and modify an immutable variable, so that we don't pollute the display all the time.

Comment 2

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

I feel like this should be two bugs:
1. allow the UI to change variable values (with all the amendments from comment #1)
2. implement the actual functionality in the debugger

Does this make sense? (is there a bug filed for 2.?)

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Victor Porof from comment #2)
> I feel like this should be two bugs:
> 1. allow the UI to change variable values (with all the amendments from
> comment #1)
> 2. implement the actual functionality in the debugger
> 
> Does this make sense? (is there a bug filed for 2.?)

Fine by me. Care to file one?

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Panos Astithas [:past] from comment #3)
> (In reply to Victor Porof from comment #2)
> > I feel like this should be two bugs:
> > 1. allow the UI to change variable values (with all the amendments from
> > comment #1)
> > 2. implement the actual functionality in the debugger
> > 
> > Does this make sense? (is there a bug filed for 2.?)
> 
> Fine by me. Care to file one?

I went ahead and filed bug 724862 for the client API bits, so this bug can only track the UI changes.

Comment 5

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

Cool, thanks!

Comment 6

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

Attachment: v1

A couple of additional minor changes (not worth a new bug):
* replaced querySelector with getElementsByClassName because faster
* fixed documentation typo in DVP__setGrip
* 'info' and 'value' was used inconsistently, decided on 'value'

Apart from that, my only real question is about setting the new value. Should we eval the new contents or regex manually (like implemented in this patch)?

Comment 7

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

While working on this, I found a few things that may be bugs. Tested with an empty patch queue and got the same results:
* not all params are taken into consideration when calling a function; for example, visit http://htmlpad.org/debugger/ and when calling test, the fArg is undefined; however, it is not properly colored in the panel entry, which makes me suspect that the grip isn't set correctly (in other cases, undefined vars are shown correctly)
* in the arguments array, if an argument is false, it is not displayed; on the same address, after 'click me', for the second frame, the third element in the arguments pseudo-array is not shown.

Known bugs?

Comment 8

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

Attachment: v2

Attached the version which uses eval instead of manual regexes. This handles much many more cases than the previous version, but eval is ugly. Is there a simpler version?

I used eval("(function() { return (" + value + "); })();") instead of eval(value) because, for example, eval("{}") is undefined :)

Comment 9

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

Attachment: v2.1

Fixed a few cases in which the eval approach didn't work as it should.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Victor Porof from comment #6)
> Apart from that, my only real question is about setting the new value.
> Should we eval the new contents or regex manually (like implemented in this
> patch)?

I think eval is better, because it allows for expressions like "foo"+"bar" or 2*Math.PI. But since this is user-entered text, we need to eval in a sandbox, like we do in the web console, for security reasons. See JST_evalInSandbox in HUDService.jsm.

This won't allow entering things like 2*this.foo of course, which we could enable if we sent a clientEvaluate request to the server instead of evalInSandbox in the client. Not sure how prevalent this use case is though. On the other hand it doesn't look like the competition supports this either, although Web Inspector does support something like "a"+document.title, which is something that we could get with evalInSandbox (assuming that bug 726949 is worked around somehow).

Dave and Rob might want to chime in regarding how far we want to go with this, and maybe Kevin knows whether WebKit has plans to support the complex value setting scenario?

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Victor Porof from comment #7)
> While working on this, I found a few things that may be bugs. Tested with an
> empty patch queue and got the same results:
> * not all params are taken into consideration when calling a function; for
> example, visit http://htmlpad.org/debugger/ and when calling test, the fArg
> is undefined; however, it is not properly colored in the panel entry, which
> makes me suspect that the grip isn't set correctly (in other cases,
> undefined vars are shown correctly)
> * in the arguments array, if an argument is false, it is not displayed; on
> the same address, after 'click me', for the second frame, the third element
> in the arguments pseudo-array is not shown.
> 
> Known bugs?

Hadn't noticed them so far, but definitely bugs!

Comment 12

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

(In reply to Panos Astithas [:past] from comment #10)
Suppose:
1) I have a variable which is an [object Something]; can I change it's value to a number for example, or am I able to change only it's properties? After I setVariable(), will the entire property panel need to be refreshed or just the changed field(s)?
2) I have a variable which is a number. I change it to be a {}. I should now be able to expand it and add properties. Will I be able to do that?
3) back to 1)

Comment 13

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

(In reply to Panos Astithas [:past] from comment #11)
> > Known bugs?
> 
> Hadn't noticed them so far, but definitely bugs!

Let's file 'em! ^^

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 601201 [details] [diff] [review]
v2.1

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

Very nice. One thing worth copying from the rule view implementation is the border and shadow styling for the textbox.

::: browser/themes/gnomestripe/devtools/debugger.css
@@ +289,5 @@
>  }
> +
> +.element-input {
> +  padding-top: 2px!important;
> +  -moz-margin-start: 5px!important;

Can we get rid of the !important declarations? We don't use them almost anywhere in devtools IIRC.

Comment 15

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

(In reply to Panos Astithas [:past] from comment #14)
> Comment on attachment 601201 [details] [diff] [review]
> v2.1
> 
> Review of attachment 601201 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Very nice. One thing worth copying from the rule view implementation is the
> border and shadow styling for the textbox.
> 
> ::: browser/themes/gnomestripe/devtools/debugger.css
> @@ +289,5 @@
> >  }
> > +
> > +.element-input {
> > +  padding-top: 2px!important;
> > +  -moz-margin-start: 5px!important;
> 
> Can we get rid of the !important declarations? We don't use them almost
> anywhere in devtools IIRC.

I had to add !important because of the "plain" class attached to the textbox, which will ignore these paddings and margins if not set. But since we'll add some obvious text input styling anyhow, we can get rid of these alltogether.

(In reply to Panos Astithas [:past] from comment #11)
> Hadn't noticed them so far, but definitely bugs!

Filed bug 731281.

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Victor Porof from comment #12)
> (In reply to Panos Astithas [:past] from comment #10)
> Suppose:
> 1) I have a variable which is an [object Something]; can I change it's value
> to a number for example, or am I able to change only it's properties? After
> I setVariable(), will the entire property panel need to be refreshed or just
> the changed field(s)?
> 2) I have a variable which is a number. I change it to be a {}. I should now
> be able to expand it and add properties. Will I be able to do that?
> 3) back to 1)

I think setVariable should be sufficient for these use cases. A more interesting case is wanting to change the property descriptor of a variable, say for defining getter and setter. This would require the defineVariable call instead, and I don't think the remote protocol spec specifies such an operation. But perhaps I'm reading the spec wrong, maybe defineVariable is what we should already be using for the "assign" request. CCing Jim for consultation.

Comment 17

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

(In reply to Panos Astithas [:past] from comment #10)
> On the other hand it doesn't look like the competition supports this either,
> although Web Inspector does support something like "a"+document.title, which
> is something that we could get with evalInSandbox (assuming that bug 726949
> is worked around somehow).

I may be doing something wrong, but document.title works for me. Attaching a WIP.

Comment 18

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

Attachment: v3

Now you can edit anything, be it an object or something more primitive. And the input looks like an input.

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 601960 [details] [diff] [review]
v3

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

This is definitely better UX. One snag I ran into: setting a value to null doesn't remove the textbox afterwards. Lots of nits and suggestions for improved UX below.

One fundamental issue however is that by using a sandbox to eval code, we allow debuggee code to run in the nested event loop. Try using our beloved test page to get into a paused state, and then change bArg to content.wrappedJSObject.load(). You should see in the console that the function executes and an onNewScript notification fires. This sounds scary, so I'd like to hear what the others think first.

::: browser/devtools/debugger/DebuggerUI.jsm
@@ +349,5 @@
> +   *        String to evaluate in the sandbox.
> +   * @returns something
> +   *          The result of the evaluation.
> +   */
> +  _evalInSandbox: function DebuggerUI__evalInSandbox(aString) {

This is not private, since it's referenced from the view.

@@ +350,5 @@
> +   * @returns something
> +   *          The result of the evaluation.
> +   */
> +  _evalInSandbox: function DebuggerUI__evalInSandbox(aString) {
> +    let sandbox = new Cu.Sandbox(this.aWindow, {

I'm ambivalent about storing the sandbox for later reuse or getting a new one each time, like you do here. It's fast enough in my testing though, and I don't expect this to be used very often, so it's probably OK.

::: browser/devtools/debugger/debugger-view.js
@@ +696,5 @@
>    },
>  
>    /**
> +   * Makes an element's (variable or priperty) value editable.
> +   * Make sure 'this' is bounds to an object containing the properties:

Typo (bounds)

@@ +699,5 @@
> +   * Makes an element's (variable or priperty) value editable.
> +   * Make sure 'this' is bounds to an object containing the properties:
> +   * {
> +   *   "scope": the original scope to be used, probably DebuggerView.Properties,
> +   *   "element": the element whom's value needs to be made editable,

and another one.

@@ +704,5 @@
> +   *   "value": the node displaying the value
> +   * }
> +   *
> +   * @param event e
> +   *        Optional, the event requesting this action.

Nit: @param event aEvent [optional]
Also, more occurrences of e->aEvent in this function.

@@ +706,5 @@
> +   *
> +   * @param event e
> +   *        Optional, the event requesting this action.
> +   */
> +  _activateElementInputMode: function DVP__activateElementInputMode(e) {

When editing an object we need to collapse it first, in order to avoid displaying an inconsistent state while the user is editing. Even removing the arrow would be nice.

@@ +729,5 @@
> +    }
> +    function DVP_element_textbox_keydown(e) {
> +      if (e.keyCode === e.DOM_VK_RETURN ||
> +          e.keyCode === e.DOM_VK_ENTER) {
> +        DVP_element_textbox_save();

One thing worth copying from the HTML panel is using ESC to cancel any changes made.

@@ +748,5 @@
> +
> +        try {
> +          result = window.parent.DebuggerUI._evalInSandbox(evalStr);
> +        } catch(e) {
> +          dump("Could not eval in sandbox: " + e + "\n");

s/dump/Cu.reportError/ and move this check to _evalInSandbox and let that return undefined. No need to burden callers with an internal detail.

@@ +768,5 @@
> +            } else {
> +              grip = {
> +                "type": "object",
> +                "class": result.constructor.name || "Object"
> +              };

We should be storing the object value as well, shouldn't we? Unless you intend to invoke the debugger client here and not in _applyGrip.

@@ +791,5 @@
> +    // to change it to another string in the textbox, so to avoid typing the ""
> +    // again, tackle with the selection bounds just a bit
> +    if (value.textContent.match(/^"[^"]*"$/)) {
> +      textbox.selectionEnd--;
> +      textbox.selectionStart++;

Like!

@@ +903,4 @@
>  
>      // the title element, containing the arrow and the name
>      title.className = "title";
> +    title.addEventListener("click", function() { element.toggle(); }, false);

This should also remove any open textbox.

::: browser/devtools/debugger/debugger.css
@@ +127,5 @@
>  }
>  
>  .variable > .title {
> +  display: inline-block;
> +  vertical-align: middle;

With these changes the arrow is a bit lower than the variable for me.

::: browser/themes/pinstripe/devtools/debugger.css
@@ +248,5 @@
>  }
>  
>  .arrow[open] {
> +  display: inline-block;
> +  vertical-align: middle;

...or it could be these changes, dunno.

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Victor Porof from comment #17)
> (In reply to Panos Astithas [:past] from comment #10)
> > On the other hand it doesn't look like the competition supports this either,
> > although Web Inspector does support something like "a"+document.title, which
> > is something that we could get with evalInSandbox (assuming that bug 726949
> > is worked around somehow).
> 
> I may be doing something wrong, but document.title works for me. Attaching a
> WIP.

It does now, it didn't work with a plain eval.

Comment 21

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

(In reply to Panos Astithas [:past] from comment #19)
> Comment on attachment 601960 [details] [diff] [review]
> v3
> 
> ::: browser/devtools/debugger/debugger.css
> @@ +127,5 @@
> >  }
> >  
> >  .variable > .title {
> > +  display: inline-block;
> > +  vertical-align: middle;
> 
> With these changes the arrow is a bit lower than the variable for me.
> 
> ::: browser/themes/pinstripe/devtools/debugger.css
> @@ +248,5 @@
> >  }
> >  
> >  .arrow[open] {
> > +  display: inline-block;
> > +  vertical-align: middle;
> 
> ...or it could be these changes, dunno.

Yeah, the css is a mess right now, don't look at it :)(In reply to Panos Astithas [:past] from comment #20)
> > 
> > I may be doing something wrong, but document.title works for me. Attaching a
> > WIP.
> 
> It does now, it didn't work with a plain eval.

Yes, ofc. I thought it shouldn't work with evalInSandbox because comment #10. Anyway, it's great that it does.

Comment 22

[Kevin Dangoor]

(In reply to Panos Astithas [:past] from comment #10)
> Dave and Rob might want to chime in regarding how far we want to go with
> this, and maybe Kevin knows whether WebKit has plans to support the complex
> value setting scenario?


If by "complex value setting scenario" you mean being able to set the value of a variable in the debugger to {foo: "bar", baz: 10}, I was unable to find anything that looked like this was something WebKit or Chromium are working on.

Comment 23

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

(In reply to Kevin Dangoor from comment #22)
> (In reply to Panos Astithas [:past] from comment #10)
> > Dave and Rob might want to chime in regarding how far we want to go with
> > this, and maybe Kevin knows whether WebKit has plans to support the complex
> > value setting scenario?
> 
> 
> If by "complex value setting scenario" you mean being able to set the value
> of a variable in the debugger to {foo: "bar", baz: 10}, I was unable to find
> anything that looked like this was something WebKit or Chromium are working
> on.

Is there a reason why we couldn't do this?

Comment 24

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Kevin Dangoor from comment #22)
> (In reply to Panos Astithas [:past] from comment #10)
> > Dave and Rob might want to chime in regarding how far we want to go with
> > this, and maybe Kevin knows whether WebKit has plans to support the complex
> > value setting scenario?
> 
> 
> If by "complex value setting scenario" you mean being able to set the value
> of a variable in the debugger to {foo: "bar", baz: 10}, I was unable to find
> anything that looked like this was something WebKit or Chromium are working
> on.

I was thinking about setting a variable to 2*this.foo, or something else that depends on other variables in scope.

Comment 25

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

Attachment: v4

Now with textbox autoresizing capabilities :)
Also, entering 'input-mode' for an element collapses it and hides the twisty. If previously expanded, when exiting 'input-mode' it will expand again.

I think we're ready to write a test for this and start working on bug 724862. Panos?

Comment 26

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

Attachment: v4.1

ESC didn't work very nicely.

Comment 27

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Victor Porof from comment #25)
> Created attachment 602068 [details] [diff] [review]
> v4
> 
> Now with textbox autoresizing capabilities :)
> Also, entering 'input-mode' for an element collapses it and hides the
> twisty. If previously expanded, when exiting 'input-mode' it will expand
> again.
> 
> I think we're ready to write a test for this and start working on bug
> 724862. Panos?

Sure, no need to hold up the UI bits for protocol-related issues. Go on while the discussion on which kinds of expressions we want to support continues, and we can change that part anyway in bug 724862.

Comment 28

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

Attachment: v5

Has tests and everything. Any progress/thoughts/concerns on what exactly we should allow when it comes to actually changing values?

Comment 29

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

(In reply to Victor Porof from comment #28)
> Created attachment 605867 [details] [diff] [review]
> v5
> 

Interestingly enough, in try it seems that:
browser_dbg_propertyview-eval.js | More complex evals like document.title should work
..fails, even though it's fine on my machine.

https://tbpl.mozilla.org/?tree=Try&rev=da8c853a6e3c

Comment 30

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

Attachment: v5.1

Fixed evalInSandbox and added a few more test scenarios.

Comment 31

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 606153 [details] [diff] [review]
v5.1

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

The editing behavior is very good now. I've got a few nits and three issues that are important enough to warrant broader discussion. I expect we'll do that in person, next week.

::: browser/devtools/debugger/DebuggerUI.jsm
@@ +578,5 @@
>      script.contentType = aContentType;
>      elt.setUserData("sourceScript", script, null);
>      dbg._updateEditorBreakpoints();
> +
> +    this._sandbox = null;

DebuggerUI is supposed to be the global debugger object, whereas DebuggerPane corresponds to the tab-related part. We don't support debugging multiple tabs simultaneously at the moment, and I'm not clear if we will at some point (mainly because of the nested event loops), but if we ever do, we will want to maintain a separate sandbox for each tab.

@@ +595,5 @@
> +      let contentWindow = this.aWindow.gBrowser.selectedBrowser.contentWindow;
> +
> +      this._sandbox = new Cu.Sandbox(contentWindow, {
> +        sandboxName: "debugger sandbox",
> +        sandboxPrototype: contentWindow,

I'll just note that evaluating user-entered code, in the scope of the content window, in a nested event loop, with the debuggee paused, has not been discussed extensively yet, but I expect us to do so in person next week.

::: browser/devtools/debugger/debugger-view.js
@@ +739,5 @@
> +              };
> +            } else {
> +              grip = {
> +                "type": "object",
> +                "class": result.constructor.name || "Object"

I wonder if this has security implications that we should be concerned with. See the notes in:
https://developer.mozilla.org/en/Components.utils.evalInSandbox#Security

Making sure that result.constructor is a function and result.constructor.name is a string seem prudent.

::: browser/devtools/debugger/test/Makefile.in
@@ +63,5 @@
>  	browser_dbg_propertyview-07.js \
>  	browser_dbg_propertyview-08.js \
> +	browser_dbg_propertyview-edit.js \
> +	browser_dbg_propertyview-edit2.js \
> +	browser_dbg_propertyview-eval.js \

Nits: I'd rather maintain a consistent naming style in our tests, if you don't mind. Also, adding a short comment in each test describing its purpose would be nice.

::: browser/devtools/debugger/test/browser_dbg_propertyview-edit.js
@@ +115,5 @@
> +  });
> +}
> +
> +function testVar1(aVar, aCallback) {
> +  var changeTo = "\"nasu\"";

We can always use single quotes, you know :-P

::: browser/devtools/debugger/test/browser_dbg_propertyview-eval.js
@@ +18,5 @@
> +    testSimpleCall();
> +  });
> +}
> +
> +function testSimpleCall() {

Most of the tests here seem to only exercise Cu.evalInSandbox, which seems redundant. Oh, well.

@@ +23,5 @@
> +  gPane.activeThread.addOneTimeListener("framesadded", function() {
> +    Services.tm.currentThread.dispatch({ run: function() {
> +
> +      ok(DebuggerUI.evalInSandbox("1.618") === 1.618,
> +        "Eval for a number didn't work properly.");

Using is() instead of ok() in these tests would be better, because if a test fails you get to see the evalInSanbox result.

Comment 32

[Panos Astithas (he/him) [:past] (please ni?)]

CC'ing Mark for his opinion on the security implications of evalInSandbox. Of course this is the same thing we do in the web console, but I figured an extra pair of eyes can't hurt.

Comment 33

[Dave Camp (:dcamp)]

Shouldn't that eval go over the wire to be evaluated in the current debuggee frame?

Comment 34

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Dave Camp (:dcamp) from comment #33)
> Shouldn't that eval go over the wire to be evaluated in the current debuggee
> frame?

Yes, I forgot to post a summary last week with my discussions with Jason and Mark. A clientEvaluate request is the way to go, and that renders the issues with nested event loops moot. It is interesting to note that Webkit doesn't seem to do this, perhaps due to a protocol limitation.

The security implications of not sandboxing the evaluation of user-supplied input are not deemed important enough to worry about. Or, to put it another way, we would have more important things to worry about from a malicious party gaining control of a debugger, than the ability to edit debuggee properties.

This means that this bug can just keep the user input as is and bug 724862 shall provide the necessary glue to send it to the server.

Comment 35

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v5.2

Rebased and ripped out the evalInSandbox bits as mentioned in comment 34.

Comment 36

[David Bruant]

A Bugzilla search on 'setUserData' led me to this bug. bug 749981 aims at removing node.setUserData since it's retired from DOMCore.
In case you still have code depending on node.setUserData, I recommand using WeakMaps instead.

Comment 37

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

Attachment: v5.2.1

Rebase left two extra functions in debugger-view. Fixed.

Comment 38

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to David Bruant from comment #36)
> A Bugzilla search on 'setUserData' led me to this bug. bug 749981 aims at
> removing node.setUserData since it's retired from DOMCore.
> In case you still have code depending on node.setUserData, I recommand using
> WeakMaps instead.

Filed bug 751204 for this work.

Comment 39

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

Attachment: v5.2.2

Rebased.
Also fixed a small issue with a twisty not showing up in certain circumstances. Added test.

Comment 40

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

Attachment: v5.2.3

Found another smallish glitch: clicking on a property name wouldn't expand the property, only the twisty worked. Fixed and added a test to make sure this works.
Panos, maybe you should take a look again at this patch, just to be sure.

Comment 41

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 621937 [details] [diff] [review]
v5.2.3

Looks good.

Comment 42

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/3b8e3fae1b31

Comment 43

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

https://hg.mozilla.org/mozilla-central/rev/3b8e3fae1b31