Add support for stepping to the debugger

RESOLVED FIXED in Firefox 14

Status

()

Firefox
Developer Tools: Debugger
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: past, Assigned: past)

Tracking

Trunk
Firefox 14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 10 obsolete attachments)

41.12 KB, patch
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.
Created attachment 583524 [details] [diff] [review]
WIP

Still work in progress.
Assignee: nobody → past
Status: NEW → ASSIGNED
Created attachment 583915 [details] [diff] [review]
WIP 2

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
Created attachment 584060 [details] [diff] [review]
(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.
Attachment #583915 - Attachment is obsolete: true
Priority: -- → P2
Created attachment 591835 [details] [diff] [review]
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.
Attachment #584060 - Attachment is obsolete: true
Created attachment 596642 [details] [diff] [review]
Improved, rebased (but still incomplete) patch

Rebased and removed the frame.where addition, since it landed in bug 687093.
Attachment #591835 - Attachment is obsolete: true
Created attachment 600402 [details] [diff] [review]
Updated patch

Updated to incorporate changes in bug 729576.
Attachment #596642 - Attachment is obsolete: true
Depends on: 729576
Created attachment 601554 [details] [diff] [review]
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.
Attachment #600402 - Attachment is obsolete: true

Updated

5 years ago
Blocks: 731721
Created attachment 604122 [details] [diff] [review]
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.
Attachment #601554 - Attachment is obsolete: true
Created attachment 605444 [details] [diff] [review]
Working patch

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
Created attachment 605747 [details] [diff] [review]
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.
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)
Created attachment 606872 [details] [diff] [review]
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.
Attachment #605747 - Attachment is obsolete: true
Attachment #606872 - Flags: review?(rcampbell)
Created attachment 606873 [details] [diff] [review]
Changes between v2 and v3
(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/integration/fx-team/rev/6e1983c0efbb
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6e1983c0efbb
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 14
You need to log in before you can comment on or make changes to this bug.