Last Comment Bug 724862 - Implement protocol support for modifying the values of a debuggee object's properties
: Implement protocol support for modifying the values of a debuggee object's pr...
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: Firefox 15
Assigned To: Panos Astithas [:past]
:
:
Mentors:
Depends on: 692984 749697
Blocks: minotaur 723062
  Show dependency treegraph
 
Reported: 2012-02-07 06:13 PST by Panos Astithas [:past]
Modified: 2012-05-26 10:20 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (15.26 KB, patch)
2012-04-27 10:33 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
WIP v2 (16.31 KB, patch)
2012-05-21 09:43 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
WIP v3 (12.27 KB, patch)
2012-05-22 11:28 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Patch v4 (39.59 KB, patch)
2012-05-23 04:23 PDT, Panos Astithas [:past]
rcampbell: review+
Details | Diff | Splinter Review

Description Panos Astithas [:past] 2012-02-07 06:13:53 PST
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 [:past] 2012-04-27 10:33:27 PDT
Created attachment 619093 [details] [diff] [review]
WIP

In progress, frame.eval causes a crash.
Comment 2 Panos Astithas [:past] 2012-05-21 09:43:46 PDT
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.
Comment 3 Victor Porof [:vporof][:vp] 2012-05-21 22:51:00 PDT
(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] 2012-05-22 01:26:43 PDT
(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 [:past] 2012-05-22 11:24:00 PDT
(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 [:past] 2012-05-22 11:28:41 PDT
Created attachment 626098 [details] [diff] [review]
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 [:past] 2012-05-23 04:23:39 PDT
Created attachment 626389 [details] [diff] [review]
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) 2012-05-23 14:14:30 PDT
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 [:past] 2012-05-23 23:26:45 PDT
(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 [:past] 2012-05-24 06:09:00 PDT
https://hg.mozilla.org/integration/fx-team/rev/3585db1e2441
Comment 11 Rob Campbell [:rc] (:robcee) 2012-05-26 10:20:42 PDT
https://hg.mozilla.org/mozilla-central/rev/3585db1e2441

Note You need to log in before you can comment on or make changes to this bug.