Last Comment Bug 740825 - Implement conditional breakpoints
: Implement conditional breakpoints
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: Firefox 19
Assigned To: Victor Porof [:vporof][:vp]
:
Mentors:
Depends on: 707302 725392 794886 796148 808372
Blocks: 727429
  Show dependency treegraph
 
Reported: 2012-03-30 08:48 PDT by Panos Astithas [:past]
Modified: 2012-11-17 10:34 PST (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (5.06 KB, patch)
2012-10-22 01:28 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2 (15.50 KB, patch)
2012-10-23 04:08 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
panel (496.96 KB, image/png)
2012-10-23 04:08 PDT, Victor Porof [:vporof][:vp]
no flags Details
v3 (36.65 KB, patch)
2012-10-23 10:11 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v3.1 (36.95 KB, patch)
2012-10-23 10:34 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v3.2 (34.96 KB, patch)
2012-10-24 02:05 PDT, Victor Porof [:vporof][:vp]
past: feedback+
Details | Diff | Splinter Review
v3.3 (35.42 KB, patch)
2012-11-04 00:33 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v4 (48.64 KB, patch)
2012-11-09 09:56 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v4.1 (51.63 KB, patch)
2012-11-10 11:38 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v5 (84.24 KB, patch)
2012-11-12 10:52 PST, Victor Porof [:vporof][:vp]
past: review+
Details | Diff | Splinter Review

Description Panos Astithas [:past] 2012-03-30 08:48:23 PDT
Conditional breakpoints are essentially (breakpoint,expression) tuples. The UI needs to provide a different icon in the gutter for displaying a conditional breakpoint and the debugger server needs to set the breakpoint and also maintain a store of these tuples. When the breakpoint handler for a conditional breakpoint is called, it should evaluate the expression (using frame.eval) as a boolean and do the standard breakpoint thing if true, or resume if false.

If other breakpoints exist in that line (bug 676602) the handlers should be chained of course, but we should leave that as a followup.
Comment 1 Victor Porof [:vporof][:vp] 2012-10-22 01:28:42 PDT
Created attachment 673789 [details] [diff] [review]
v1

Wip, doesn't do any UI yet, but knows how to evaluate conditionals when needed.
Comment 2 Victor Porof [:vporof][:vp] 2012-10-23 04:08:04 PDT
Created attachment 674186 [details] [diff] [review]
v2

Preliminary UI support added. All breakpoints are conditional (which is wrong).
Comment 3 Victor Porof [:vporof][:vp] 2012-10-23 04:08:24 PDT
Created attachment 674187 [details]
panel
Comment 4 Victor Porof [:vporof][:vp] 2012-10-23 10:11:43 PDT
Created attachment 674265 [details] [diff] [review]
v3

Done.
Needs a test.
Comment 5 Victor Porof [:vporof][:vp] 2012-10-23 10:13:05 PDT
(To add a conditional breakpoint, you either click on a line, then press accel+shift+B, or click on the gutter normally then press accel+shift+B.)
Comment 6 Victor Porof [:vporof][:vp] 2012-10-23 10:34:23 PDT
Created attachment 674281 [details] [diff] [review]
v3.1

Aargh.. forgot to qref a fix for the expression panel not showing up at the correct location in some cases. Sorry for the spam.
Comment 7 Victor Porof [:vporof][:vp] 2012-10-24 02:05:14 PDT
Created attachment 674596 [details] [diff] [review]
v3.2

Fixed a failing test and rebased on top of bug 796148 since that one's probably going to land sooner.
Comment 8 Panos Astithas [:past] 2012-10-24 02:39:31 PDT
Comment on attachment 674281 [details] [diff] [review]
v3.1

Review of attachment 674281 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't tried it yet, but it looks like what we wanted, modulo the protocol thing.

::: browser/devtools/debugger/debugger-controller.js
@@ +422,5 @@
> +      if (expression) {
> +        // Evaluating the current breakpoint's conditional expression will
> +        // cause the stack frames to be cleared and active thread to pause,
> +        // sending a 'clientEvaluated' packed and adding the frames again.
> +        this.evaluate(expression, 0);

I'm glad we can get it working without protocol changes, but this is going to make conditional breakpoints in loops (which is a common use case) very slow. I'd like us to move this part to the server, adding a 'condition' property to the 'setBreakpoint' request.

I suppose we could do this in a followup, if you prefer.

::: browser/devtools/debugger/debugger-panes.js
@@ +217,3 @@
>     */
>    addBreakpoint:
> +  function DVB_addBreakpoint(aLineInfo, aLineText, aSourceLocation, aLineNumber, aActor,

Is this the former DebuggerController.Breakpoints.addBreakpoint? If not, we need to expose the condition parameter to whatever is the GCLI 'break' command calls now, in order to implement 'break add conditional'.

@@ +230,5 @@
>        attachment: {
>          enabled: true,
>          sourceLocation: aSourceLocation,
> +        lineNumber: aLineNumber,
> +        isConditionalBreakpoint: aConditionalExpression !== undefined,

This seems redundant since one can simply check !!conditionalExpression.
Comment 9 Victor Porof [:vporof][:vp] 2012-10-24 02:52:00 PDT
(In reply to Panos Astithas [:past] from comment #8)
> Comment on attachment 674281 [details] [diff] [review]
> v3.1
> 
> Review of attachment 674281 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I haven't tried it yet, but it looks like what we wanted, modulo the
> protocol thing.
> 
> ::: browser/devtools/debugger/debugger-controller.js
> @@ +422,5 @@
> > +      if (expression) {
> > +        // Evaluating the current breakpoint's conditional expression will
> > +        // cause the stack frames to be cleared and active thread to pause,
> > +        // sending a 'clientEvaluated' packed and adding the frames again.
> > +        this.evaluate(expression, 0);
> 
> I'm glad we can get it working without protocol changes, but this is going
> to make conditional breakpoints in loops (which is a common use case) very
> slow. I'd like us to move this part to the server, adding a 'condition'
> property to the 'setBreakpoint' request.
> 

Just curious, why do you say that? Sure, the protocol overhead may be a bit much, but is it too much?

> I suppose we could do this in a followup, if you prefer.
> 

I'll do some profiling with a giant stack depth and see what happens.

> ::: browser/devtools/debugger/debugger-panes.js
> @@ +217,3 @@
> >     */
> >    addBreakpoint:
> > +  function DVB_addBreakpoint(aLineInfo, aLineText, aSourceLocation, aLineNumber, aActor,
> 
> Is this the former DebuggerController.Breakpoints.addBreakpoint? If not, we
> need to expose the condition parameter to whatever is the GCLI 'break'
> command calls now, in order to implement 'break add conditional'.
> 

Nope, it's the debugger view part (DVB_addBreakpoint). But yeah, we need a flag in BP_addBreakpoint.

> @@ +230,5 @@
> >        attachment: {
> >          enabled: true,
> >          sourceLocation: aSourceLocation,
> > +        lineNumber: aLineNumber,
> > +        isConditionalBreakpoint: aConditionalExpression !== undefined,
> 
> This seems redundant since one can simply check !!conditionalExpression.

I don't like to trust param types. What happens in the conditionalExpression is "false". Or false. Or something weird we may get. That doesn't mean this shouldn't be considered a conditional breakpoint. Do you agree?
Comment 10 Panos Astithas [:past] 2012-10-24 03:31:04 PDT
Comment on attachment 674596 [details] [diff] [review]
v3.2

Review of attachment 674596 [details] [diff] [review]:
-----------------------------------------------------------------

More feedback after playing with the patch:
- Hitting ENTER in the condition textbox doesn't dismiss the popup.
- SourceEditor accepts a condition property in SE_addBreakpoint, which would presumably display a different icon in the gutter. If it looks ugly we can ask shorlander for a pretty one. It would be even better if it displayed the condition on hover.
Comment 11 Victor Porof [:vporof][:vp] 2012-10-24 03:38:51 PDT
(In reply to Panos Astithas [:past] from comment #10)
> Comment on attachment 674596 [details] [diff] [review]
> v3.2
> 
> Review of attachment 674596 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> More feedback after playing with the patch:
> - Hitting ENTER in the condition textbox doesn't dismiss the popup.

Good idea.

> - SourceEditor accepts a condition property in SE_addBreakpoint, which would
> presumably display a different icon in the gutter. If it looks ugly we can
> ask shorlander for a pretty one. It would be even better if it displayed the
> condition on hover.

Just checked, the 'condition' param doesn't change the visual aspect of the breakpoint. Should it? Do we need to supply a different icon? I only see a .annotationHTML.breakpoint selector in orion.css, no hint of a 'condition' attribute or different class.
Comment 12 Panos Astithas [:past] 2012-10-24 03:41:53 PDT
(In reply to Victor Porof [:vp] from comment #9)
> (In reply to Panos Astithas [:past] from comment #8)
> > ::: browser/devtools/debugger/debugger-controller.js
> > @@ +422,5 @@
> > > +      if (expression) {
> > > +        // Evaluating the current breakpoint's conditional expression will
> > > +        // cause the stack frames to be cleared and active thread to pause,
> > > +        // sending a 'clientEvaluated' packed and adding the frames again.
> > > +        this.evaluate(expression, 0);
> > 
> > I'm glad we can get it working without protocol changes, but this is going
> > to make conditional breakpoints in loops (which is a common use case) very
> > slow. I'd like us to move this part to the server, adding a 'condition'
> > property to the 'setBreakpoint' request.
> > 
> 
> Just curious, why do you say that? Sure, the protocol overhead may be a bit
> much, but is it too much?

Well, consider the B2G remote debugging case where you are debugging a tight loop that used to take a few msec to run, and now every iteration needs to send data over WiFi. It will be orders of magnitude slower now.

Furthermore, what if that loop is in code that runs as part of an animation? Suddenly you barely see any animation at all.

Without protocol support this will be fine for many cases, which is why I think it's OK to fix it in a followup, but will be problematic for others.

> > @@ +230,5 @@
> > >        attachment: {
> > >          enabled: true,
> > >          sourceLocation: aSourceLocation,
> > > +        lineNumber: aLineNumber,
> > > +        isConditionalBreakpoint: aConditionalExpression !== undefined,
> > 
> > This seems redundant since one can simply check !!conditionalExpression.
> 
> I don't like to trust param types. What happens in the conditionalExpression
> is "false". Or false. Or something weird we may get. That doesn't mean this
> shouldn't be considered a conditional breakpoint. Do you agree?

Agreed, good point.
Comment 13 Victor Porof [:vporof][:vp] 2012-10-24 06:49:00 PDT
(In reply to Panos Astithas [:past] from comment #12)
> Well, consider the B2G remote debugging case where you are debugging a tight
> loop that used to take a few msec to run, and now every iteration needs to
> send data over WiFi. It will be orders of magnitude slower now.
> 

You're right, I didn't think about that enough. I initially thought that this approach would avoid having one extra roundtrip of evaluations in case of the watch expressions (the breakpoint conditionals need to appear in there too imo, firebug does this and it's really helpful), but your argument supersedes mine. We'd have to do that anyway when stepping, not continuing.

> Without protocol support this will be fine for many cases, which is why I
> think it's OK to fix it in a followup, but will be problematic for others.

I'll see if I can quickly adapt the code and have the evaluation on the server. My plan now is to have conditional breakpoints and watch expressions in this release, or at least asap, as well as a few other fixes. I'll followup if it takes more than expected.
Comment 14 Victor Porof [:vporof][:vp] 2012-11-04 00:33:51 PDT
Created attachment 678090 [details] [diff] [review]
v3.3

Rebased.
Comment 15 Victor Porof [:vporof][:vp] 2012-11-09 09:56:39 PST
Created attachment 680113 [details] [diff] [review]
v4

Rebased, addressed comments and updated patch.
Right click + add breakpoint now takes the currently hovered line into account.
Popup knows how to hide on return/esc.
Browsing through or selecting code now automatically highlights the corresponding breakpoint in the pane if one is available.
I made sure the changes will be smooth, but I won't have time to move the conditional evaluation logic on the server for this release, will file a followup.

mq: http://pastebin.mozilla.org/1929668
Comment 16 Victor Porof [:vporof][:vp] 2012-11-10 11:38:16 PST
Created attachment 680382 [details] [diff] [review]
v4.1

Fixed some dumb stuff I was doing.
Comment 17 Victor Porof [:vporof][:vp] 2012-11-12 10:52:45 PST
Created attachment 680705 [details] [diff] [review]
v5

Added tests.
I'd really like to get this in before the merge.
Comment 18 Panos Astithas [:past] 2012-11-14 02:03:58 PST
Comment on attachment 680705 [details] [diff] [review]
v5

Review of attachment 680705 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just fix the removeBreakpoint call and please file followups for doing the work server-side and using a different icon for conditional breakpoints.

::: browser/devtools/debugger/debugger-controller.js
@@ +818,1 @@
>      let { url, line } = aFrame.where;

Let me recite Emerson: "A foolish consistency is the hobgoblin of little minds" :-)

@@ +1340,2 @@
>      if (!aFlags.noPaneUpdate) {
>        DebuggerView.Breakpoints.removeBreakpoint(url, line);

Not caused by this patch, but I got an error while removing breakpoints:

JavaScript error: chrome://browser/content/debugger-controller.js, line 1268: aCallback is not a function

I believe it is caused by this line that calls BP_removeBreakpoint with the parameters for SE_removeBreakpoint.

::: browser/devtools/debugger/debugger-panes.js
@@ +514,5 @@
>      createMenuItem.call(this, "enableOthers");
>      createMenuItem.call(this, "disableOthers");
>      createMenuItem.call(this, "deleteOthers");
>      createMenuSeparator();
> +    createMenuItem.call(this, "setConditionalSelf");

setConditionalSelf doesn't sound as intuitive as, say setCondition or configureCondition, but that's just nitpicking.

@@ +742,5 @@
>        // The container is empty or we didn't click on an actual item.
>        return;
>      }
>      let { sourceLocation: url, lineNumber: line, enabled } = breakpointItem.attachment;
> +    this[enabled ? "disableBreakpoint" : "enableBreakpoint"](url, line, { silent: true });

OK, I'm going to stop commenting on this kind of thing, since you are apparently quite fond of it.

@@ +819,5 @@
>      let { sourceLocation: url, lineNumber: line } = breakpointItem.attachment;
>      let breakpointClient = DebuggerController.Breakpoints.getBreakpoint(url, line);
>  
>      this.removeBreakpoint(url, line);
> +    DebuggerController.Breakpoints.removeBreakpoint(url, line);

Hm, maybe this is the cause of the error I am seeing. BP_removeBreakpoint takes  the client as the first argument.

::: browser/devtools/shared/VariablesView.jsm
@@ +1516,5 @@
> +  }
> +
> +  // For convenience, undefined and null are both considered types.
> +  let type = grip.type;
> +  if (["undefined", "null"].indexOf(type + "") != -1) {

What's wrong with if (type == "undefined" || type == "null")?

::: browser/locales/en-US/chrome/browser/devtools/debugger.dtd
@@ +65,5 @@
>  <!ENTITY debuggerUI.searchPanelTitle    "Operators">
> +
> +<!-- LOCALIZATION NOTE (debuggerUI.condBreakPanelTitle): This is the text that
> +  -  appears in the conditional breakpoint panel popup as a description. -->
> +<!ENTITY debuggerUI.condBreakPanelTitle "This breakpoint will stop only if the following expression is true">

"will stop execution" is more accurate I believe.
Comment 19 Victor Porof [:vporof][:vp] 2012-11-14 02:19:36 PST
(In reply to Panos Astithas [:past] from comment #18)
> Comment on attachment 680705 [details] [diff] [review]
> v5
> 
> Review of attachment 680705 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, just fix the removeBreakpoint call and please file followups for
> doing the work server-side and using a different icon for conditional
> breakpoints.

Will do.

> 
> ::: browser/devtools/debugger/debugger-controller.js
> @@ +818,1 @@
> >      let { url, line } = aFrame.where;
> 
> Let me recite Emerson: "A foolish consistency is the hobgoblin of little
> minds" :-)
> 

I appreciate the use of the full quote :)
If you're referring to 'let { depth } = aFrame;', I'm puzzled on how that sneaked in. I'll revert it to the more human let 'depth = aFrame.depth'.

> @@ +1340,2 @@
> >      if (!aFlags.noPaneUpdate) {
> >        DebuggerView.Breakpoints.removeBreakpoint(url, line);
> 
> Not caused by this patch, but I got an error while removing breakpoints:
> 
> JavaScript error: chrome://browser/content/debugger-controller.js, line
> 1268: aCallback is not a function
> 
> I believe it is caused by this line that calls BP_removeBreakpoint with the
> parameters for SE_removeBreakpoint.
> 
> ::: browser/devtools/debugger/debugger-panes.js
> @@ +514,5 @@
> >      createMenuItem.call(this, "enableOthers");
> >      createMenuItem.call(this, "disableOthers");
> >      createMenuItem.call(this, "deleteOthers");
> >      createMenuSeparator();
> > +    createMenuItem.call(this, "setConditionalSelf");
> 
> setConditionalSelf doesn't sound as intuitive as, say setCondition or
> configureCondition, but that's just nitpicking.
> 

Will change.

> ::: browser/devtools/shared/VariablesView.jsm
> @@ +1516,5 @@
> > +  }
> > +
> > +  // For convenience, undefined and null are both considered types.
> > +  let type = grip.type;
> > +  if (["undefined", "null"].indexOf(type + "") != -1) {
> 
> What's wrong with if (type == "undefined" || type == "null")?
> 

Nothing. It's even better. Will change.

> ::: browser/locales/en-US/chrome/browser/devtools/debugger.dtd
> @@ +65,5 @@
> >  <!ENTITY debuggerUI.searchPanelTitle    "Operators">
> > +
> > +<!-- LOCALIZATION NOTE (debuggerUI.condBreakPanelTitle): This is the text that
> > +  -  appears in the conditional breakpoint panel popup as a description. -->
> > +<!ENTITY debuggerUI.condBreakPanelTitle "This breakpoint will stop only if the following expression is true">
> 
> "will stop execution" is more accurate I believe.

Agreed.
Comment 20 Victor Porof [:vporof][:vp] 2012-11-14 02:28:38 PST
> @@ +819,5 @@
> >      let { sourceLocation: url, lineNumber: line } = breakpointItem.attachment;
> >      let breakpointClient = DebuggerController.Breakpoints.getBreakpoint(url, line);
> >  
> >      this.removeBreakpoint(url, line);
> > +    DebuggerController.Breakpoints.removeBreakpoint(url, line);
> 
> Hm, maybe this is the cause of the error I am seeing. BP_removeBreakpoint
> takes  the client as the first argument.
> 

Yes, this is it. It's this patch's fault, apparently. It should be breakpointClient as the only argument, not url and line.
Comment 21 Victor Porof [:vporof][:vp] 2012-11-15 07:17:10 PST
Filed bugs 812172 and 812173.
Comment 22 Victor Porof [:vporof][:vp] 2012-11-15 22:44:43 PST
Addressed comments and landed:
https://hg.mozilla.org/integration/fx-team/rev/43303dfa12a5
Comment 23 Victor Porof [:vporof][:vp] 2012-11-16 00:54:08 PST
Orange fix for browser_dbg_bug740825_conditional-breakpoints-01.js:
https://hg.mozilla.org/integration/fx-team/rev/984b4d761ec8
Comment 24 Victor Porof [:vporof][:vp] 2012-11-16 03:19:30 PST
And again for browser_dbg_bug740825_conditional-breakpoints-02.js:
https://hg.mozilla.org/integration/fx-team/rev/d0ece9d955a4

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