1135435 - debugger should explain the exception when conditional breakpoint throws

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Zimon Dai [:zimonD]]

this is a following up bug of bug983469. We should add some UI changes to notify the user of conditional breakpoints throwing errors.

Comment 1

[Zimon Dai [:zimonD]]

Attachment: add UI for breakpoint condition throwing

Comment 2

[Zimon Dai [:zimonD]]

Comment on attachment 8573692 [details] [diff] [review]
add UI for breakpoint condition throwing

Here's a WIP patch.

As there's no response from bug762979, I couldn't add this UI change to the new breakpoint mockup. In this patch, I just display the message thrown to the breakpoint item view.

Anyway, it's a start. When we are done with the UI changes and behaviours, I will write some tests.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8573692 [details] [diff] [review]
add UI for breakpoint condition throwing

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

Good start!

Needs test(s), of course.

::: browser/themes/shared/devtools/debugger.inc.css
@@ +46,5 @@
>  }
>  
> +.dbg-breakpoint-condition-thrown-message {
> +  display: none;
> +  color: #CC1B1B;

I think we should be using css variables from our theme. ni? bgrins for more info and references.

::: toolkit/devtools/server/actors/script.js
@@ +4722,5 @@
>     *
>     * @param aFrame Debugger.Frame
>     *        The frame to evaluate the condition in
> +   * @returns Object
> +   *          In form of { result: boolean|undefined, message}

Would be good to document the difference between `result == false` and `result == undefined`, because that isn't clear to me from reading these docs.

@@ +4792,5 @@
> +          reason.message = message;
> +        }
> +        reason.actors = [ this.actorID ];
> +      }
> +      else return undefined;

Style nit: devtools js always uses braces and new lines for if/else clauses.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

:bgrins, can you give us some info about the devtools theme css variables and what we should be using here?

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #3)
> > +.dbg-breakpoint-condition-thrown-message {
> > +  display: none;
> > +  color: #CC1B1B;
> 
> I think we should be using css variables from our theme. ni? bgrins for more
> info and references.

Yep, you can convert this to a CSS variable.  Here are the relevant text color variables for light-theme [0] and dark-theme [1].  Here's an example similar to what you are doing [2].  You can do something like this (note that I haven't checked to make sure that this color works in your case, please check in both light and dark themes to make sure it looks good and try the other options if not):

  color: var(--theme-body-color-alt)

[0]: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/light-theme.css#22
[1]: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/dark-theme.css#22.
[2]: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/toolbars.inc.css#61

Comment 6

[Zimon Dai [:zimonD]]

Attachment: bug1135435

Sorry for the delay on uploading this patch.... too busy during the last several weeks...

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8579157 [details] [diff] [review]
bug1135435

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

This looks great!

A few comments below.

Can you also send a PR to update the remote debugging protocol documentation to reflect this change? https://github.com/jimblandy/DebuggerDocs

Thanks!

::: browser/devtools/debugger/debugger-panes.js
@@ +455,5 @@
> +   */
> +  showBreakpointConditionThrownMessage: function(aLocation, aMessage = "") {
> +    let breakpointItem = this.getBreakpoint(aLocation);
> +    if (!breakpointItem) {
> +      return promise.reject(new Error("No breakpoint found."));

Since we don't otherwise return any value, I don't think that returning a rejected promise is the right thing to do here.

If this code should always be able to access the breakpoint, then we should do this:

  DevToolsUtils.dbg_assert(!!breakpointItem,
                           "sowBreakpointConditionThrownMessage: Should always get a breakpoint");

However, that is not quite our situation: it is possible that the user deleted the BP on the frontend at the same time as the server hit the pause, and therefore before the "paused" message reached the frontend and before the "remove breakpoint" message reached the server and prevented the BP from getting hit.

I think the best thing we can do here (unfortunately) is this:

  if (!breakpointItem) {
    return;
  }

@@ +706,5 @@
>     *         An object containing the breakpoint container, checkbox,
>     *         line number and line text nodes.
>     */
>    _createBreakpointView: function(aOptions) {
> +    let { location, disabled, text, message } = aOptions;

Please document the "message" option in the doc comment above, like the other options are.

::: browser/devtools/debugger/test/browser.ini
@@ +139,5 @@
>  [browser_dbg_breakpoints-break-on-last-line-of-script-on-reload.js]
>  skip-if = e10s # Bug 1093535
>  [browser_dbg_breakpoints-button-01.js]
>  [browser_dbg_breakpoints-button-02.js]
> +skip-if = e10s && debug

Is this supposed to be for the new test? If so, it should be below the new test, not above it.

Additionally, I think you forgot to include the new test in this patch.

::: toolkit/devtools/server/actors/script.js
@@ +4724,5 @@
>     * @param aFrame Debugger.Frame
>     *        The frame to evaluate the condition in
> +   * @returns Object
> +   *          - result: boolean|undefined
> +   *            Indicating whether condition evaluates to true or not.

Nit: "True when the conditional breakpoint should trigger a pause, false otherwise."

Callers of this function just want to know whether to pause or not -- we can paper over the cause of the pause a bit and simultaneously simplify the code a little.

@@ +4797,5 @@
> +          reason.message = message;
> +        }
> +        reason.actors = [ this.actorID ];
> +      }
> +      else {

Style nit: Brace and if on same line.

Comment 8

[Zimon Dai [:zimonD]]

Attachment: add UI for breakpoint condition throwing

Comment 9

[Zimon Dai [:zimonD]]

Comment on attachment 8579742 [details] [diff] [review]
add UI for breakpoint condition throwing

Updated. I will send a PR to the docs once the patch is good enough XD

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8579742 [details] [diff] [review]
add UI for breakpoint condition throwing

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

Looks great, thanks!

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=33cc8d5b6485

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Also, I suggest you fill out the forms needed to become a level 1 commiter, that way you can push to try yourself in the future: https://www.mozilla.org/en-US/about/governance/policies/commit/

Comment 12

[Zimon Dai [:zimonD]]

Attachment: try: -b do -p all -u reftest,reftest-e10s,reftest-no-accel,crashtest,crashtest-e10s,xpcshell,jsreftest,marionette,marionette-e10s,mozmill,cppunit,jittests,jittest-1,jittest-2,mochitests,web-platform-tests -t none

Comment 13

[Zimon Dai [:zimonD]]

Attachment: add UI for breakpoint condition throws

Comment 14

[Zimon Dai [:zimonD]]

Comment on attachment 8580526 [details] [diff] [review]
add UI for breakpoint condition throws

modified a unit test on breakpoint condition.

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71b0e3e03168

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8580526 [details] [diff] [review]
add UI for breakpoint condition throws

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

Great!

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/698791e62f58

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/698791e62f58