983469 - Conditional breakpoints should pause with error when condition throws

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

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

Really annoying trying to track down why your breakpoint isn't getting hit and realizing that you had a typo or whatever. Seems more sane to just always pause.

Comment 1

[Eddy Bruel [:ejpbruel]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #0)
> Really annoying trying to track down why your breakpoint isn't getting hit
> and realizing that you had a typo or whatever. Seems more sane to just
> always pause.

I'm all for it, as long as we display some kind of error message. Your conditional breakpoint hitting because the expression you wrote for it contains a typo could be equally confusing if you don't realize it.

Comment 2

[Zimon Dai [:zimonD]]

I am working on this bug and could pause on breakpoint exception, but I've found that the breakpoint condition is executed with `frame.eval` and that do not return any error infomation (https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#4701)

The ideal behaviour of this feature is that, when the debugger runs to a breakpoint containing condition errors:

1) the debugger pauses at the breakpoint
2) the condition popup appears with an error message

Or we could tell user that the condition contains a typo or something before submitting the breakpoint

Comment 3

[Zimon Dai [:zimonD]]

Never mind. I figured that out. Will provide a patch soon

Comment 4

[Zimon Dai [:zimonD]]

Nick, any ideas on how to inform the user of an exception? Or maybe we could give different breakpoints different colors (blue: normal, yellow: conditional, red: conditional with exception) ?

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Zimon Dai from comment #4)
> Nick, any ideas on how to inform the user of an exception? Or maybe we could
> give different breakpoints different colors (blue: normal, yellow:
> conditional, red: conditional with exception) ?

I am actually working on new breakpoints in bug 762979. There will actually be different colours for active, conditional, and normal breakpoints. There isn't one for conditional with exception, but open for suggestions.

Comment 6

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

(In reply to Zimon Dai [:zimonD] from comment #4)
> Nick, any ideas on how to inform the user of an exception? Or maybe we could
> give different breakpoints different colors (blue: normal, yellow:
> conditional, red: conditional with exception) ?

Let's hold off on colors, because as :gl mentioned, we have other plans for that.

In fact, I think we can just land the puase-on-conditional-bp-exception initial behavior and file follow ups for UI that explains that we paused because the conditional expression threw an error.

Comment 7

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

(In reply to Zimon Dai [:zimonD] from comment #2)
> I am working on this bug and could pause on breakpoint exception, but I've
> found that the breakpoint condition is executed with `frame.eval` and that
> do not return any error infomation
> (https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/
> actors/script.js#4701)

Actually, it does return error information, via completion values:

https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Debugger.Frame#Function_Properties_of_the_Debugger.Frame_Prototype_Object

https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Conventions#Completion_Values

Comment 8

[Zimon Dai [:zimonD]]

Attachment: WIP patch

Comment 9

[Zimon Dai [:zimonD]]

here's a patch, any suggestions?

Comment 10

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

Comment on attachment 8561749 [details] [diff] [review]
WIP patch

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

Hi Zimon, in the future when you attach the patch to a bug you can use the "feedback" flag to request feedback when you upload it, if you don't then it tends to get looked over :)

This patch looks pretty good overall, thanks for submitting it!

There are a few changes I've suggested below, and we will also need to add a devtools-mochitest[0][1] test for this behavior. A good test to crib from would be [2]. The debugger test helpers are defined in [3].

[0] https://wiki.mozilla.org/DevTools/Hacking#DevTools_Mochitests
[1] https://wiki.mozilla.org/DevTools/mochitests_coding_standards
[2] https://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/browser_dbg_breakpoints-reload.js
[3] https://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/head.js

::: toolkit/devtools/server/actors/script.js
@@ +4707,5 @@
> +      "try{" +
> +        this.condition +
> +      "} catch (e) {" +
> +        "{ return: e }" +
> +      "}");

Instead of manipulating the source that is evaluated, you can just check the completion result:

    let completion = aFrame.eval(this.condition);
    if (!completion) {
      // The evaluation was killed (possibly by the slow script dialog)
    if (completion.return) {
      // The evaluation returned a value
    } else if (completion.throw) {
      // The evaluation failed and threw an error
    } else {
      dbg_assert(false, 
                 "Shouldn't ever get yield completions from an eval");
    }

https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Conventions#Completion_Values

@@ +4731,4 @@
>        return undefined;
>      }
>  
> +    let evalResult = this.isValidCondition(aFrame);

I would modify `isValidCondition` to return the completion value and check against that here. Probably rename `isValidCondition` to `checkCondition`, too. It should probably assert that this is a conditional breakpoint, as well.

Then, below you would have:

    if (!this.condition) {
      reason.type = "breakpoint";
    } else {
      let completion = this.checkCondition();
      // Do the completion type checking I showed in the comment above and decide
      // whether to pause or continue execution (by returning undefined).
    }

@@ +4741,5 @@
>        reason.type = "breakpoint";
>        // TODO: add the rest of the breakpoints on that line (bug 676602).
>        reason.actors = [ this.actorID ];
> +    } else {
> +      // the conditional breakpoint failed with an exception (bug 983469).

Nitpick: Capitalize the first word in sentences in comments.

Additionally, we don't need to add the bug number here: it won't be an outstanding bug we need to fix in the future once this patch lands, and if people are interested in the history, they can use version control blame.

@@ +4743,5 @@
>        reason.actors = [ this.actorID ];
> +    } else {
> +      // the conditional breakpoint failed with an exception (bug 983469).
> +      reason.type = "conditionException";
> +      reason.actors= [ this.actorID ];

Nitpick: space after "actors" and before "=".

Comment 11

[Zimon Dai [:zimonD]]

Hey Nick, thanks for the detailed review...

I did not directly use aFrame.eval(this.condition) with a reason. I will attach an HTML page for testing. Let's say adding a conditional breakpoint at line 5 with the condition "1a", expecting  an syntax error.

Now build and run, aFrame.eval result is an object, its 'throw' property holds an Error instance. BUT, the error is empty and do not contain anything, message or stack.

Maybe I am missing something?

Comment 12

[Zimon Dai [:zimonD]]

Attachment: to test aFrame.eval

Comment 13

[Zimon Dai [:zimonD]]

Ah, I realized that frame.eval returns a debuggee object when error occurred (not an instance of Error as I previously thought). But the error object seems not containing information?

Comment 14

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

(In reply to Zimon Dai [:zimonD] from comment #13)
> Ah, I realized that frame.eval returns a debuggee object when error occurred
> (not an instance of Error as I previously thought). But the error object
> seems not containing information?

See https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Debugger.Object to get property descriptors and if the descriptors have either (a) a "value" or (b) a "get" with a _native_ function, then it is safe to use that value or run that getter without otherwise messing with the debuggee's state.

See https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js?from=actors/script.js#3481-3482 for code that checks the same things.

However, we don't currently display the condition value or exception, and I think we can save this for a follow up bug. All you should need to do here is determine if you should pause or not.

Comment 15

[Zimon Dai [:zimonD]]

Attachment: Pause on breakpoint condition exception

Comment 16

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

Comment on attachment 8563871 [details] [diff] [review]
Pause on breakpoint condition exception

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

Getting very close!

::: toolkit/devtools/server/actors/script.js
@@ +4691,5 @@
>     *
>     * @param aFrame Debugger.Frame
>     *        The frame to evaluate the condition in
>     */
> +  checkCondition: function(aFrame) {

Can you document the return value for checkCondition and its meaning in the above comment?

@@ +4739,5 @@
>        reason.actors = [ this.actorID ];
> +    } else {
> +      let completion = this.checkCondition(aFrame);
> +
> +      if (completion.class === "Error") {

As written now, checkCondition does not return a completion, it returns the value that was thrown or returned OR undefined if the script was killed.

In addition, you can't just check if class is "Error" because that doesn't get "ReferenceError" or "TypeError" etc. Furthermore, the value thrown doesn't need to be an error! This is valid (although not particularly idomatic) JS:

    const FOOBAR_ERR_CODE = 1;
    // ...
    if (foobar) throw FOOBAR_ERR_CODE;

I think we should:

* Abandon the `reason.throw` and `reason.type = "..."` stuff below and leave the protocol as-is -- we can add more metadata to the protocol in a follow up bug (which also requires documenting the protocol changes).

* Make `checkCondition()` return `true` if we should pause and `false` if we should continue. This skips the faulty "Error" checks and simplifies its interface.

Comment 17

[Zimon Dai [:zimonD]]

Attachment: Pause on breakpoint condition exception

Comment 18

[Zimon Dai [:zimonD]]

Comment on attachment 8564548 [details] [diff] [review]
Pause on breakpoint condition exception

OK, I uploaded a new patch.

Comment 19

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

Comment on attachment 8564548 [details] [diff] [review]
Pause on breakpoint condition exception

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

Looks great!

I tried pushing it to our test integration server, "try server", but it looks like the patch needs to be rebased on the latest fx-team tip:

applying bug983469-conditional-throw.patch
patching file toolkit/devtools/server/actors/script.js
Hunk #2 FAILED at 4723
1 out of 2 hunks FAILED -- saving rejects to file toolkit/devtools/server/actors/script.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh bug983469-conditional-throw.patch

Once you upload the rebased patch, I can push it to try, and assuming that the patch didn't break anything else and the tests come back green, we can push this to fx-team where it will incubate for a day or so before getting merged into mozilla-central and starting its journey on Nightly -> Dev edition -> Beta -> Release!

Comment 20

[Zimon Dai [:zimonD]]

Attachment: Pause on breakpoint condition exception

Comment 21

[Zimon Dai [:zimonD]]

Comment on attachment 8565805 [details] [diff] [review]
Pause on breakpoint condition exception

rebased, thx Nick.

Comment 22

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

Here is a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a55b01904450

Comment 23

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

ni? me to follow up on try push

Comment 24

[Zimon Dai [:zimonD]]

Attachment: Pause on breakpoint condition exception

Comment 25

[Zimon Dai [:zimonD]]

Comment on attachment 8566405 [details] [diff] [review]
Pause on breakpoint condition exception

Ah, there's a unit test of debugger server unmodified and caused the failure. I updated the patch.

Comment 26

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

Comment on attachment 8566405 [details] [diff] [review]
Pause on breakpoint condition exception

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

::: toolkit/devtools/server/tests/unit/test_conditional_breakpoint-03.js
@@ +38,4 @@
>  
>          // Remove the breakpoint.
>          bpClient.remove(function (aResponse) {
>            gThreadClient.resume(function () {

When you resume here, you're going to hit the debugger statement and pause again. This could cause intermittent test failures. I think you should remove the debugger statement from the code that's being evaluated.

Comment 27

[Zimon Dai [:zimonD]]

Attachment: Pause on breakpoint condition exception

Comment 28

[Zimon Dai [:zimonD]]

Comment on attachment 8566602 [details] [diff] [review]
Pause on breakpoint condition exception

I am too careless...

Comment 29

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

Comment on attachment 8566602 [details] [diff] [review]
Pause on breakpoint condition exception

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

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

Comment 30

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

Zimon, looks like more test failures on try. Are you running the tests locally? Please make sure the tests pass locally before uploading the next version of the patch.

https://wiki.mozilla.org/DevTools/Hacking#Running_the_Developer_Tools_Tests

    ./mach test toolkit/devtools/server
    ./mach test browser/devtools/debugger

Comment 31

[Zimon Dai [:zimonD]]

Attachment: Pause on breakpoint condition exception

Comment 32

[Zimon Dai [:zimonD]]

Comment on attachment 8566930 [details] [diff] [review]
Pause on breakpoint condition exception

browser/devtools/debugger:

Browser Chrome Test Summary
	Passed: 9901
	Failed: 0
	Todo: 0

toolkit/devtools/server(latest log):

Ran 169 tests
Expected results: 167
Unexpected results: 0
Skipped: 2

Hoping this time everything is OK... Thx for your patience, Nick.

Comment 33

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

Comment on attachment 8566930 [details] [diff] [review]
Pause on breakpoint condition exception

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

Thanks, Zimon!

Here is a new try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=90d8e44a509e

Fingers crossed!

Comment 34

[Zimon Dai [:zimonD]]

Tests all pasted!

Comment 35

[Zimon Dai [:zimonD]]

(In reply to Zimon Dai [:zimonD] from comment #34)
> Tests all pasted!

... passed

Comment 36

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

(In reply to Zimon Dai [:zimonD] from comment #35)
> (In reply to Zimon Dai [:zimonD] from comment #34)
> > Tests all pasted!
> 
> ... passed

Awesome, congrats!

Can you file a follow-up bug for adding some kind of UI to explain why the pause occurred? We can open discussion on what exactly that UI should be there.

Thanks!

Comment 37

[Zimon Dai [:zimonD]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #36)
> (In reply to Zimon Dai [:zimonD] from comment #35)
> > (In reply to Zimon Dai [:zimonD] from comment #34)
> > > Tests all pasted!
> > 
> > ... passed
> 
> Awesome, congrats!
> 
> Can you file a follow-up bug for adding some kind of UI to explain why the
> pause occurred? We can open discussion on what exactly that UI should be
> there.
> 
> Thanks!

see bug1135435

Comment 38

[Ryan VanderMeulen [:RyanVM]]

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

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Backed out for timeouts in browser_dbg_server-conditional-bp-01.js on mochitest-e10s.
https://hg.mozilla.org/integration/fx-team/rev/4e52d97f1816

https://treeherder.mozilla.org/logviewer.html#?job_id=2064022&repo=fx-team

Comment 40

[Zimon Dai [:zimonD]]

Attachment: Pause on breakpoint condition exception

Comment 41

[Zimon Dai [:zimonD]]

Comment on attachment 8567788 [details] [diff] [review]
Pause on breakpoint condition exception

Updated the patch and rebased to the latest commit.

Comment 42

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

Zimon, sorry, I forgot to show you how to run the devtools mochitests under e10s. You can do that with

    ./mach mochitest-devtools browser/devtools/debugger --e10s

Can you verify that you're latest patch passes these tests locally? Then I will do another try push and we can re-land this patch.

Comment 43

[Zimon Dai [:zimonD]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #42)
> Zimon, sorry, I forgot to show you how to run the devtools mochitests under
> e10s. You can do that with
> 
>     ./mach mochitest-devtools browser/devtools/debugger --e10s
> 
> Can you verify that you're latest patch passes these tests locally? Then I
> will do another try push and we can re-land this patch.

Latest fx-team build with ./mach run --e10s keeps crashing on every tab. I cannot really finish any test...but I believe that the latest patch fixes the timeout problem...


Anyway, I would paste the stderr of running with e10s below:

 0:00.42 /project/mozilla/fx-team/obj-x86_64-apple-darwin14.1.0/dist/Nightly.app/Contents/MacOS/firefox --e10s -no-remote -foreground -profile /project/mozilla/fx-team/obj-x86_64-apple-darwin14.1.0/tmp/scratch_user
Shumway is registered
SystemMessageCache: init[Child 64279] ###!!! ABORT: LoadSheetSync failed with error 80520015 loading built-in stylesheet 'resource://gre-resources/counterstyles.css': file /project/mozilla/fx-team/layout/style/nsLayoutStylesheetCache.cpp, line 378
[Child 64279] ###!!! ABORT: LoadSheetSync failed with error 80520015 loading built-in stylesheet 'resource://gre-resources/counterstyles.css': file /project/mozilla/fx-team/layout/style/nsLayoutStylesheetCache.cpp, line 378

###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv

Comment 44

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

Eddy, you are more familiar with e10s than I am. Can you help out Zimon?

Comment 45

[Eddy Bruel [:ejpbruel]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #44)
> Eddy, you are more familiar with e10s than I am. Can you help out Zimon?

I have no idea what is causing this, so I asked the e10s team. They told me that they recently landed some code that had unintended side-effects. This code has since been disabled, so the problem may have resolved itself.

zimonD, could you try to run the tests for e10s again with your patch applied? If the problem persists, please let me know. In that case, I'll ask the e10s people again.

Comment 46

[Zimon Dai [:zimonD]]

Browser Chrome Test Summary
	Passed: 9414
	Failed: 0
	Todo: 0

I will upload a rebased patch.

Comment 47

[Zimon Dai [:zimonD]]

Attachment: Pause on breakpoint condition exception

Comment 48

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

Comment on attachment 8570284 [details] [diff] [review]
Pause on breakpoint condition exception

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

Awesome, thanks for the patient work!

Here is another (hopefully, final) try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6dabc1db62e

Comment 49

[Zimon Dai [:zimonD]]

Seems allright except for a build fail...

Comment 50

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

Yup, looks good to me!

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/20661f32e7e7

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/20661f32e7e7