711164 - Add support for stepping to the debugger

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

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

The engine bits are ready, so work can begin on the protocol and UI fronts.

Comment 1

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

Attachment: WIP

Still work in progress.

Comment 2

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

Attachment: WIP 2

Filled in the server-side parts, but not working yet.

Comment 3

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

Getting the full functionality working requires Debugger.Frame.prototype.onPop.

Comment 4

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

Attachment: (Almost) Working patch

This version works, for the limited case of "step in", until the active frame is changed, when we lose stepping again. Until onPop arrives there's not much I can do, so I think I'll get back to this patch then, to finish it.

Comment 5

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

Attachment: Improved (but still incomplete) patch

Rebased, fixed mochitest leaks and added the forgotten "where" property to frame grips, in order to help with writing tests.

Comment 6

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

Attachment: Improved, rebased (but still incomplete) patch

Rebased and removed the frame.where addition, since it landed in bug 687093.

Comment 7

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

Attachment: Updated patch

Updated to incorporate changes in bug 729576.

Comment 8

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

Attachment: Updated patch with frame popping support

I added the protocol bits for the new "forceCompletion" resumption packet and the frame "pop" request that is quite similar. This is not going to be used anywhere until we get Debugger.Frame.prototype.pop, but that's how it should work from my reading of the specs.

Comment 9

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

Attachment: WIP

Fixed stepping when leaving a stack frame, but still having issues with entering a frame. onEnterFrame doesn't get called for some reason. Fixing this and adding tests is what's still missing from this patch.

Comment 10

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

Attachment: Working patch

Fixed frame entering and now stepping seems to work as expected. I'll be adding a bunch of tests tomorrow.

Comment 11

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

Attachment: Working patch with tests

Added a bunch of tests and one of them pointed to a use case I missed yesterday (step out, without previous step in), which is now fixed.

Comment 12

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

Comment on attachment 605747 [details] [diff] [review]
Working patch with tests

 
+  _pauseAndRespond: function TA__pauseAndRespond(aFrame, aWhy) {
+    try {

even though it's private, it could still probably benefit from a little comment. Also, aWhy might be better named as aReason.

... though I guess packet has a .why property on it, so maybe it's ok as-is. If you don't feel like rewriting everything.

   onResume: function TA_onResume(aRequest) {
+    if (aRequest && aRequest.forceCompletion) {
+      // TODO: remove this when Debugger.Frame.prototype.pop is implemented

no comment here either. Bug on file for this TODO?

+    } else if (aRequest && aRequest.resumeLimit) {

Is this ever false?

     DebuggerServer.xpcInspector.exitNestedEventLoop();
+    if (aRequest && aRequest.forceCompletion) {
+      return { type: "resumeLimit", frameFinished: aRequest.forceCompletion };
+    }

seems like this could be combined with the initial if (aRequest && aRequest.forceCompletion) check and eliminate the else if() entirely.

+    // Clear stepping hooks.
+    this.dbg.onEnterFrame = undefined;
+    if (aFrame) {
+      aFrame.onStep = undefined;
+      aFrame.onPop = undefined;
+    }

can you just delete these instead or are you keeping them around intentionally?

+    // TODO: return the watches property when frame pop watch actors are
+    // implemented.
+    return { from: this.actorID };
   }

does this need a bug filed?

+    // TODO: add the rest of the breakpoints on that line.
+    let why = { type: "breakpoint", actors: [ this.actorID ] };
+    return this.threadActor._pauseAndRespond(aFrame, why);
   },

and this?

Close! Rerequest when ready.

Comment 13

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

Attachment: Working patch v3

(In reply to Rob Campbell [:rc] (robcee) from comment #12)
> Comment on attachment 605747 [details] [diff] [review]
> Working patch with tests
> 
>  
> +  _pauseAndRespond: function TA__pauseAndRespond(aFrame, aWhy) {
> +    try {
> 
> even though it's private, it could still probably benefit from a little
> comment. Also, aWhy might be better named as aReason.
> 
> ... though I guess packet has a .why property on it, so maybe it's ok as-is.
> If you don't feel like rewriting everything.

Nah, I don't mind. Changed.

>    onResume: function TA_onResume(aRequest) {
> +    if (aRequest && aRequest.forceCompletion) {
> +      // TODO: remove this when Debugger.Frame.prototype.pop is implemented
> 
> no comment here either. Bug on file for this TODO?

Fixed. Bug 736733.

> +    } else if (aRequest && aRequest.resumeLimit) {
> 
> Is this ever false?

Yes, when called from TA_disconnect (debugger teardown) and not from a client protocol request.

>      DebuggerServer.xpcInspector.exitNestedEventLoop();
> +    if (aRequest && aRequest.forceCompletion) {
> +      return { type: "resumeLimit", frameFinished: aRequest.forceCompletion
> };
> +    }
> 
> seems like this could be combined with the initial if (aRequest &&
> aRequest.forceCompletion) check and eliminate the else if() entirely.

You mean duplicating the two lines that resume and exit the nested event loop? OK, done.

> +    // Clear stepping hooks.
> +    this.dbg.onEnterFrame = undefined;
> +    if (aFrame) {
> +      aFrame.onStep = undefined;
> +      aFrame.onPop = undefined;
> +    }
> 
> can you just delete these instead or are you keeping them around
> intentionally?

No, delete doesn't work in this case for some reason (returns true, but hooks remain). But I expect them to be GCed, since they are no longer referenced from anywhere, right?

> +    // TODO: return the watches property when frame pop watch actors are
> +    // implemented.
> +    return { from: this.actorID };
>    }
> 
> does this need a bug filed?

I'd expect not, since this is even farther off than Debuger.Frame.prototype.pop; the protocol spec has just an empty heading:

https://wiki.mozilla.org/Remote_Debugging_Protocol#Frame_Pop_Watches

> +    // TODO: add the rest of the breakpoints on that line.
> +    let why = { type: "breakpoint", actors: [ this.actorID ] };
> +    return this.threadActor._pauseAndRespond(aFrame, why);
>    },
> 
> and this?

Bug 676602.

Comment 14

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

Attachment: Changes between v2 and v3

Comment 15

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

(In reply to Panos Astithas [:past] from comment #13)
> Created attachment 606872 [details] [diff] [review]
> Working patch v3
> 
> (In reply to Rob Campbell [:rc] (robcee) from comment #12)
[...]
> > seems like this could be combined with the initial if (aRequest &&
> > aRequest.forceCompletion) check and eliminate the else if() entirely.
> 
> You mean duplicating the two lines that resume and exit the nested event
> loop? OK, done.

ok! I find the extra nesting confusing.

> > can you just delete these instead or are you keeping them around
> > intentionally?
> 
> No, delete doesn't work in this case for some reason (returns true, but
> hooks remain). But I expect them to be GCed, since they are no longer
> referenced from anywhere, right?

Yeah they should be.

> > does this need a bug filed?
> 
> I'd expect not, since this is even farther off than
> Debuger.Frame.prototype.pop; the protocol spec has just an empty heading:
> 
> https://wiki.mozilla.org/Remote_Debugging_Protocol#Frame_Pop_Watches

ah ok.

> > +    // TODO: add the rest of the breakpoints on that line.
> > +    let why = { type: "breakpoint", actors: [ this.actorID ] };
> > +    return this.threadActor._pauseAndRespond(aFrame, why);
> >    },
> > 
> > and this?
> 
> Bug 676602.

awesome. Thanks!

Comment 16

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

https://hg.mozilla.org/integration/fx-team/rev/6e1983c0efbb

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/6e1983c0efbb