Last Comment Bug 711164 - Add support for stepping to the debugger
: Add support for stepping to the debugger
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: Firefox 14
Assigned To: Panos Astithas [:past] (away until 7/21)
:
Mentors:
Depends on: 674171 729576
Blocks: minotaur 731721
  Show dependency treegraph
 
Reported: 2011-12-15 10:43 PST by Panos Astithas [:past] (away until 7/21)
Modified: 2012-03-20 03:28 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (10.07 KB, patch)
2011-12-21 08:58 PST, Panos Astithas [:past] (away until 7/21)
no flags Details | Diff | Splinter Review
WIP 2 (13.44 KB, patch)
2011-12-22 13:22 PST, Panos Astithas [:past] (away until 7/21)
no flags Details | Diff | Splinter Review
(Almost) Working patch (16.23 KB, patch)
2011-12-23 07:40 PST, Panos Astithas [:past] (away until 7/21)
no flags Details | Diff | Splinter Review
Improved (but still incomplete) patch (29.61 KB, patch)
2012-01-26 09:52 PST, Panos Astithas [:past] (away until 7/21)
no flags Details | Diff | Splinter Review
Improved, rebased (but still incomplete) patch (28.95 KB, patch)
2012-02-13 03:35 PST, Panos Astithas [:past] (away until 7/21)
no flags Details | Diff | Splinter Review
Updated patch (29.55 KB, patch)
2012-02-24 08:24 PST, Panos Astithas [:past] (away until 7/21)
no flags Details | Diff | Splinter Review
Updated patch with frame popping support (30.74 KB, patch)
2012-02-29 01:38 PST, Panos Astithas [:past] (away until 7/21)
no flags Details | Diff | Splinter Review
WIP (31.28 KB, patch)
2012-03-08 10:48 PST, Panos Astithas [:past] (away until 7/21)
no flags Details | Diff | Splinter Review
Working patch (30.92 KB, patch)
2012-03-13 10:26 PDT, Panos Astithas [:past] (away until 7/21)
no flags Details | Diff | Splinter Review
Working patch with tests (40.63 KB, patch)
2012-03-14 07:35 PDT, Panos Astithas [:past] (away until 7/21)
no flags Details | Diff | Splinter Review
Working patch v3 (41.12 KB, patch)
2012-03-17 09:06 PDT, Panos Astithas [:past] (away until 7/21)
rcampbell: review+
Details | Diff | Splinter Review
Changes between v2 and v3 (3.90 KB, patch)
2012-03-17 09:06 PDT, Panos Astithas [:past] (away until 7/21)
no flags Details | Diff | Splinter Review

Description Panos Astithas [:past] (away until 7/21) 2011-12-15 10:43:22 PST
The engine bits are ready, so work can begin on the protocol and UI fronts.
Comment 1 Panos Astithas [:past] (away until 7/21) 2011-12-21 08:58:52 PST
Created attachment 583524 [details] [diff] [review]
WIP

Still work in progress.
Comment 2 Panos Astithas [:past] (away until 7/21) 2011-12-22 13:22:01 PST
Created attachment 583915 [details] [diff] [review]
WIP 2

Filled in the server-side parts, but not working yet.
Comment 3 Panos Astithas [:past] (away until 7/21) 2011-12-22 13:22:58 PST
Getting the full functionality working requires Debugger.Frame.prototype.onPop.
Comment 4 Panos Astithas [:past] (away until 7/21) 2011-12-23 07:40:17 PST
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.
Comment 5 Panos Astithas [:past] (away until 7/21) 2012-01-26 09:52:31 PST
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.
Comment 6 Panos Astithas [:past] (away until 7/21) 2012-02-13 03:35:38 PST
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.
Comment 7 Panos Astithas [:past] (away until 7/21) 2012-02-24 08:24:30 PST
Created attachment 600402 [details] [diff] [review]
Updated patch

Updated to incorporate changes in bug 729576.
Comment 8 Panos Astithas [:past] (away until 7/21) 2012-02-29 01:38:22 PST
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.
Comment 9 Panos Astithas [:past] (away until 7/21) 2012-03-08 10:48:04 PST
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.
Comment 10 Panos Astithas [:past] (away until 7/21) 2012-03-13 10:26:22 PDT
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.
Comment 11 Panos Astithas [:past] (away until 7/21) 2012-03-14 07:35:57 PDT
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.
Comment 12 Rob Campbell [:rc] (:robcee) 2012-03-16 06:07:20 PDT
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 [:past] (away until 7/21) 2012-03-17 09:06:05 PDT
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.
Comment 14 Panos Astithas [:past] (away until 7/21) 2012-03-17 09:06:50 PDT
Created attachment 606873 [details] [diff] [review]
Changes between v2 and v3
Comment 15 Rob Campbell [:rc] (:robcee) 2012-03-17 13:46:05 PDT
(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 [:past] (away until 7/21) 2012-03-18 00:02:47 PDT
https://hg.mozilla.org/integration/fx-team/rev/6e1983c0efbb
Comment 17 Tim Taubert [:ttaubert] 2012-03-20 03:28:23 PDT
https://hg.mozilla.org/mozilla-central/rev/6e1983c0efbb

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