Closed Bug 740825 Opened 12 years ago Closed 12 years ago

Implement conditional breakpoints

Categories

(DevTools :: Debugger, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 19

People

(Reporter: past, Assigned: vporof)

References

Details

Attachments

(1 file, 9 obsolete files)

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.
No longer blocks: minotaur
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
Wip, doesn't do any UI yet, but knows how to evaluate conditionals when needed.
Depends on: 707302
Blocks: 727429
Depends on: 794886
Attached patch v2 (obsolete) — Splinter Review
Preliminary UI support added. All breakpoints are conditional (which is wrong).
Attachment #673789 - Attachment is obsolete: true
Attached image panel (obsolete) —
Attached patch v3 (obsolete) — Splinter Review
Done.
Needs a test.
Attachment #674186 - Attachment is obsolete: true
Attachment #674265 - Flags: feedback?(past)
(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.)
Attached patch v3.1 (obsolete) — Splinter Review
Aargh.. forgot to qref a fix for the expression panel not showing up at the correct location in some cases. Sorry for the spam.
Attachment #674265 - Attachment is obsolete: true
Attachment #674265 - Flags: feedback?(past)
Attachment #674281 - Flags: feedback?(past)
Attached patch v3.2 (obsolete) — Splinter Review
Fixed a failing test and rebased on top of bug 796148 since that one's probably going to land sooner.
Attachment #674281 - Attachment is obsolete: true
Attachment #674281 - Flags: feedback?(past)
Attachment #674596 - Flags: feedback?(past)
Depends on: 796148
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.
Attachment #674281 - Attachment is obsolete: false
Attachment #674281 - Attachment is obsolete: true
(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 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.
Attachment #674596 - Flags: feedback?(past) → feedback+
(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.
(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.
(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.
Attached patch v3.3 (obsolete) — Splinter Review
Rebased.
Depends on: 725392
Attached patch v4 (obsolete) — Splinter Review
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
Attachment #674187 - Attachment is obsolete: true
Attachment #674596 - Attachment is obsolete: true
Attachment #678090 - Attachment is obsolete: true
Attached patch v4.1 (obsolete) — Splinter Review
Fixed some dumb stuff I was doing.
Attachment #680113 - Attachment is obsolete: true
Attached patch v5Splinter Review
Added tests.
I'd really like to get this in before the merge.
Attachment #680382 - Attachment is obsolete: true
Attachment #680705 - Flags: review?(past)
Depends on: 808372
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.
Attachment #680705 - Flags: review?(past) → review+
(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.
> @@ +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.
Filed bugs 812172 and 812173.
Addressed comments and landed:
https://hg.mozilla.org/integration/fx-team/rev/43303dfa12a5
Whiteboard: [fixed-in-fx-team]
Orange fix for browser_dbg_bug740825_conditional-breakpoints-01.js:
https://hg.mozilla.org/integration/fx-team/rev/984b4d761ec8
And again for browser_dbg_bug740825_conditional-breakpoints-02.js:
https://hg.mozilla.org/integration/fx-team/rev/d0ece9d955a4
https://hg.mozilla.org/mozilla-central/rev/43303dfa12a5
https://hg.mozilla.org/mozilla-central/rev/984b4d761ec8
https://hg.mozilla.org/mozilla-central/rev/d0ece9d955a4
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 19
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: