Closed Bug 711164 Opened 8 years ago Closed 8 years ago

Add support for stepping to the debugger

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 14

People

(Reporter: past, Assigned: past)

References

Details

Attachments

(2 files, 10 obsolete files)

41.12 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review
3.90 KB, patch
Details | Diff | Splinter Review
The engine bits are ready, so work can begin on the protocol and UI fronts.
Attached patch WIP (obsolete) — Splinter Review
Still work in progress.
Assignee: nobody → past
Status: NEW → ASSIGNED
Attached patch WIP 2 (obsolete) — Splinter Review
Filled in the server-side parts, but not working yet.
Attachment #583524 - Attachment is obsolete: true
Getting the full functionality working requires Debugger.Frame.prototype.onPop.
Depends on: 674171
Attached patch (Almost) Working patch (obsolete) — Splinter Review
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.
Attachment #583915 - Attachment is obsolete: true
Priority: -- → P2
Rebased, fixed mochitest leaks and added the forgotten "where" property to frame grips, in order to help with writing tests.
Attachment #584060 - Attachment is obsolete: true
Rebased and removed the frame.where addition, since it landed in bug 687093.
Attachment #591835 - Attachment is obsolete: true
Attached patch Updated patch (obsolete) — Splinter Review
Updated to incorporate changes in bug 729576.
Attachment #596642 - Attachment is obsolete: true
Depends on: 729576
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.
Attachment #600402 - Attachment is obsolete: true
Blocks: 731721
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #601554 - Attachment is obsolete: true
Attached patch Working patch (obsolete) — Splinter Review
Fixed frame entering and now stepping seems to work as expected. I'll be adding a bunch of tests tomorrow.
Attachment #604122 - Attachment is obsolete: true
Attached patch Working patch with tests (obsolete) — Splinter Review
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.
Attachment #605444 - Attachment is obsolete: true
Attachment #605747 - Flags: review?(rcampbell)
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.
Attachment #605747 - Flags: review?(rcampbell)
Attached patch Working patch v3Splinter Review
(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.
Attachment #605747 - Attachment is obsolete: true
Attachment #606872 - Flags: review?(rcampbell)
(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!
Attachment #606872 - Flags: review?(rcampbell) → review+
https://hg.mozilla.org/mozilla-central/rev/6e1983c0efbb
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 14
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.