724862 - Implement protocol support for modifying the values of a debuggee object's properties

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P1)

Description

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

This is the protocol-specific part for getting the new API bits in bug 692984 (setVariable basically) into the debugger client API, in order to be used by bug 723062. The "assign" protocol operation is already implemented (although not tested until bug 692984 lands), so what is actually missing is a dbg-client.jsm wrapper for making the call.

Comment 1

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

Attachment: WIP

In progress, frame.eval causes a crash.

Comment 2

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

Attachment: WIP v2

The crasher was fixed, so I could move forward with this. Changed values are now actually reflected on the debuggee.

Comment 3

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

(In reply to Panos Astithas [:past] from comment #2)
> Created attachment 625669 [details] [diff] [review]
> WIP v2
> 
> The crasher was fixed, so I could move forward with this. Changed values are
> now actually reflected on the debuggee.

This makes me very, very happy.

Comment 4

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

(In reply to Panos Astithas [:past] from comment #2)
> Created attachment 625669 [details] [diff] [review]
> WIP v2
> 
> The crasher was fixed, so I could move forward with this. Changed values are
> now actually reflected on the debuggee.

+      // Maintain the symbolic name of the property.
+      Object.defineProperty(element, "token", {
+        value: aVar.token + "." + pKey, // XXX: this doesn't work for arrays
+        writable: false,
+        enumerable: true,
+        configurable: true
+      });

Can we aVar.token + "[" + pKey + "]" the bastard?

Comment 5

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

(In reply to Victor Porof from comment #4)
> (In reply to Panos Astithas [:past] from comment #2)
> > Created attachment 625669 [details] [diff] [review]
> > WIP v2
> > 
> > The crasher was fixed, so I could move forward with this. Changed values are
> > now actually reflected on the debuggee.
> 
> +      // Maintain the symbolic name of the property.
> +      Object.defineProperty(element, "token", {
> +        value: aVar.token + "." + pKey, // XXX: this doesn't work for arrays
> +        writable: false,
> +        enumerable: true,
> +        configurable: true
> +      });
> 
> Can we aVar.token + "[" + pKey + "]" the bastard?

Yep, good point, that's pretty much what it needed.

Comment 6

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

Attachment: WIP v3

It was much simpler than what I initially thought, so this version is quite simpler than previous ones. No need for an "assign" request, just a "clientEvaluate" will do. This patch works as intended, but it breaks the new tests from bug 723062. I'll get to those tomorrow, but in the meantime Victor, see if you can spot anything else that I could remove from the additions of bug 723062 to debugger-view.js, now that we don't need to do anything after sending the "clientEvaluate" request.

Comment 7

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

Attachment: Patch v4

I had to remove the previous tests, since they rely on artificially generating another scope and modifying its values. Since this scope does not exist on the server, such tests can no longer work. I think the new tests should be enough for this part. Also fixed variable modification in other frames, besides the newest.

Comment 8

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

Comment on attachment 626389 [details] [diff] [review]
Patch v4

   /**
    * Handler for the debugger client's unsolicited newScript notification.
    */
   _onNewScript: function SS__onNewScript(aNotification, aPacket) {
+    // Ignore scripts generated from 'clientEvaluate' packets.
+    if (aPacket.url == "debugger eval code") {
+      return;
+    }
+

do we really want to ignore these? I suppose we do since they're being sent from the debugger itself.

+      // Maintain the symbolic name of the property.
+      Object.defineProperty(element, "token", {
+        value: aVar.token + "['" + pKey + "']",

... the bastard.

-            break;
-          case "undefined":
-            grip = { type: "undefined" };
-        }
-
-        self._applyGrip(value, grip);
+        let expr = "(" + element.token + "=" + textbox.value + ")";
+        DebuggerController.StackFrames.evaluate(expr);
       }

wow, that is a lot simpler.

** test code **

loving those lines of deletion. R+!

dbg-script-actors.js

    */
   onAssign: function EA_onAssign(aRequest) {
-    let desc = this.obj.getVariableDescriptor(aRequest.name);
+    // TODO: enable the commented-out part when getVariableDescriptor lands
+    // (bug 725815).
+    /*let desc = this.obj.getVariableDescriptor(aRequest.name);
 
what's the impact of this not being available for this patch? Are we going to be able to change the contents of any object?

Comment 9

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #8)
> Comment on attachment 626389 [details] [diff] [review]
> Patch v4
> 
>    /**
>     * Handler for the debugger client's unsolicited newScript notification.
>     */
>    _onNewScript: function SS__onNewScript(aNotification, aPacket) {
> +    // Ignore scripts generated from 'clientEvaluate' packets.
> +    if (aPacket.url == "debugger eval code") {
> +      return;
> +    }
> +
> 
> do we really want to ignore these? I suppose we do since they're being sent
> from the debugger itself.

Exactly, they are debugger-generated and furthermore, we don't have a script to display if the user chooses to select this entry from the script list.

> dbg-script-actors.js
> 
>     */
>    onAssign: function EA_onAssign(aRequest) {
> -    let desc = this.obj.getVariableDescriptor(aRequest.name);
> +    // TODO: enable the commented-out part when getVariableDescriptor lands
> +    // (bug 725815).
> +    /*let desc = this.obj.getVariableDescriptor(aRequest.name);
>  
> what's the impact of this not being available for this patch? Are we going
> to be able to change the contents of any object?

No impact at all. When I filed this bug I thought that we would have to use the "assign" protocol request to modify the debuggee's value, but since we want to evaluate the user input in the context of the debuggee, "assign" is not sufficient. "clientEvaluate" proved to be all that was necessary.

I'm only leaving these changes in as a precursor of future work. onAssign is still unused and therefore untested, but I plan to fix the latter when bug 725815 is done.

Comment 10

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

https://hg.mozilla.org/integration/fx-team/rev/3585db1e2441

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/3585db1e2441