Closed Bug 724862 Opened 12 years ago Closed 12 years ago

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

Categories

(DevTools :: Debugger, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 15

People

(Reporter: past, Assigned: past)

References

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 file, 3 obsolete files)

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.
Attached patch WIP (obsolete) — Splinter Review
In progress, frame.eval causes a crash.
Assignee: nobody → past
Status: NEW → ASSIGNED
Depends on: 749697
Priority: P3 → P1
Attached patch WIP v2 (obsolete) — Splinter Review
The crasher was fixed, so I could move forward with this. Changed values are now actually reflected on the debuggee.
Attachment #619093 - Attachment is obsolete: true
(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.
(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?
(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.
Attached patch WIP v3 (obsolete) — Splinter Review
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.
Attachment #625669 - Attachment is obsolete: true
Attachment #626098 - Flags: feedback?(vporof)
Attached patch Patch v4Splinter Review
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.
Attachment #626098 - Attachment is obsolete: true
Attachment #626098 - Flags: feedback?(vporof)
Attachment #626389 - Flags: review?(rcampbell)
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?
(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.
Attachment #626389 - Flags: review?(rcampbell) → review+
https://hg.mozilla.org/mozilla-central/rev/3585db1e2441
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.