740825 - Implement conditional breakpoints

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

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]]

Attachment: v1

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

Comment 2

[Victor Porof [:vporof][:vp]]

Attachment: v2

Preliminary UI support added. All breakpoints are conditional (which is wrong).

Comment 3

[Victor Porof [:vporof][:vp]]

Attachment: panel

Comment 4

[Victor Porof [:vporof][:vp]]

Attachment: v3

Done.
Needs a test.

Comment 5

[Victor Porof [:vporof][:vp]]

(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]]

Attachment: 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]]

Attachment: 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 (he/him) [:past] (please ni?)]

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]]

(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 (he/him) [:past] (please ni?)]

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]]

(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 (he/him) [:past] (please ni?)]

(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]]

(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]]

Attachment: v3.3

Rebased.

Comment 15

[Victor Porof [:vporof][:vp]]

Attachment: 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]]

Attachment: v4.1

Fixed some dumb stuff I was doing.

Comment 17

[Victor Porof [:vporof][:vp]]

Attachment: v5

Added tests.
I'd really like to get this in before the merge.

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

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]]

(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]]

> @@ +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]]

Filed bugs 812172 and 812173.

Comment 22

[Victor Porof [:vporof][:vp]]

Addressed comments and landed:
https://hg.mozilla.org/integration/fx-team/rev/43303dfa12a5

Comment 23

[Victor Porof [:vporof][:vp]]

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]]

And again for browser_dbg_bug740825_conditional-breakpoints-02.js:
https://hg.mozilla.org/integration/fx-team/rev/d0ece9d955a4

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/43303dfa12a5
https://hg.mozilla.org/mozilla-central/rev/984b4d761ec8
https://hg.mozilla.org/mozilla-central/rev/d0ece9d955a4