The default bug view has changed. See this FAQ.

Implement conditional breakpoints

RESOLVED FIXED in Firefox 19

Status

()

Firefox
Developer Tools: Debugger
P3
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: past, Assigned: vporof)

Tracking

Trunk
Firefox 19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

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: 676586
(Assignee)

Updated

5 years ago
Assignee: nobody → vporof
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Created attachment 673789 [details] [diff] [review]
v1

Wip, doesn't do any UI yet, but knows how to evaluate conditionals when needed.
(Assignee)

Updated

5 years ago
Depends on: 707302
(Assignee)

Updated

5 years ago
Blocks: 727429
(Assignee)

Updated

5 years ago
Depends on: 794886
(Assignee)

Comment 2

5 years ago
Created attachment 674186 [details] [diff] [review]
v2

Preliminary UI support added. All breakpoints are conditional (which is wrong).
Attachment #673789 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
Created attachment 674187 [details]
panel
(Assignee)

Comment 4

5 years ago
Created attachment 674265 [details] [diff] [review]
v3

Done.
Needs a test.
Attachment #674186 - Attachment is obsolete: true
Attachment #674265 - Flags: feedback?(past)
(Assignee)

Comment 5

5 years ago
(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.)
(Assignee)

Comment 6

5 years ago
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.
Attachment #674265 - Attachment is obsolete: true
Attachment #674265 - Flags: feedback?(past)
Attachment #674281 - Flags: feedback?(past)
(Assignee)

Comment 7

5 years ago
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.
Attachment #674281 - Attachment is obsolete: true
Attachment #674281 - Flags: feedback?(past)
Attachment #674596 - Flags: feedback?(past)
(Assignee)

Updated

5 years ago
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
(Assignee)

Comment 9

5 years ago
(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+
(Assignee)

Comment 11

5 years ago
(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.
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

4 years ago
Created attachment 678090 [details] [diff] [review]
v3.3

Rebased.
(Assignee)

Updated

4 years ago
Depends on: 725392
(Assignee)

Comment 15

4 years ago
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
Attachment #674187 - Attachment is obsolete: true
Attachment #674596 - Attachment is obsolete: true
Attachment #678090 - Attachment is obsolete: true
(Assignee)

Comment 16

4 years ago
Created attachment 680382 [details] [diff] [review]
v4.1

Fixed some dumb stuff I was doing.
Attachment #680113 - Attachment is obsolete: true
(Assignee)

Comment 17

4 years ago
Created attachment 680705 [details] [diff] [review]
v5

Added tests.
I'd really like to get this in before the merge.
Attachment #680382 - Attachment is obsolete: true
Attachment #680705 - Flags: review?(past)
(Assignee)

Updated

4 years ago
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+
(Assignee)

Comment 19

4 years ago
(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.
(Assignee)

Comment 20

4 years ago
> @@ +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.
(Assignee)

Comment 21

4 years ago
Filed bugs 812172 and 812173.
(Assignee)

Comment 22

4 years ago
Addressed comments and landed:
https://hg.mozilla.org/integration/fx-team/rev/43303dfa12a5
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 23

4 years ago
Orange fix for browser_dbg_bug740825_conditional-breakpoints-01.js:
https://hg.mozilla.org/integration/fx-team/rev/984b4d761ec8
(Assignee)

Comment 24

4 years ago
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
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 19
You need to log in before you can comment on or make changes to this bug.