Closed Bug 983469 Opened 6 years ago Closed 5 years ago

Conditional breakpoints should pause with error when condition throws

Categories

(DevTools :: Debugger, defect)

x86
macOS
defect
Not set

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: fitzgen, Assigned: zimonD)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 8 obsolete files)

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.
(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.
Summary: conditional breakpoint expressions that throw should cause us to stop at the breakpoint → Conditional breakpoints should pause with error when condition throws
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
Never mind. I figured that out. Will provide a patch soon
Flags: needinfo?(nfitzgerald)
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) ?
(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.
(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.
Flags: needinfo?(nfitzgerald)
(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
Attached patch WIP patch (obsolete) — Splinter Review
here's a patch, any suggestions?
Flags: needinfo?(nfitzgerald)
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 "=".
Flags: needinfo?(nfitzgerald)
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?
Flags: needinfo?(nfitzgerald)
Attached file to test aFrame.eval
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?
(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.
Flags: needinfo?(nfitzgerald)
Attachment #8561749 - Attachment is obsolete: true
Attachment #8563871 - Flags: feedback+
Attachment #8563871 - Flags: feedback+ → feedback?(nfitzgerald)
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.
Attachment #8563871 - Flags: feedback?(nfitzgerald) → feedback+
Attachment #8563871 - Attachment is obsolete: true
Comment on attachment 8564548 [details] [diff] [review]
Pause on breakpoint condition exception

OK, I uploaded a new patch.
Attachment #8564548 - Flags: feedback?(nfitzgerald)
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!
Attachment #8564548 - Flags: review+
Attachment #8564548 - Flags: feedback?(nfitzgerald)
Attachment #8564548 - Flags: feedback+
Assignee: nobody → daizhuoxian
Status: NEW → ASSIGNED
Attachment #8564548 - Attachment is obsolete: true
Comment on attachment 8565805 [details] [diff] [review]
Pause on breakpoint condition exception

rebased, thx Nick.
Flags: needinfo?(nfitzgerald)
Attachment #8565805 - Flags: review+
Attachment #8565805 - Flags: review+ → review?(nfitzgerald)
ni? me to follow up on try push
Flags: needinfo?(nfitzgerald)
Attachment #8565805 - Attachment is obsolete: true
Attachment #8565805 - Flags: review?(nfitzgerald)
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.
Attachment #8566405 - Flags: review?(nfitzgerald)
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.
Attachment #8566405 - Flags: review?(nfitzgerald) → review+
Attachment #8566405 - Attachment is obsolete: true
Comment on attachment 8566602 [details] [diff] [review]
Pause on breakpoint condition exception

I am too careless...
Attachment #8566602 - Flags: review?(nfitzgerald)
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
Attachment #8566602 - Flags: review?(nfitzgerald) → review+
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
Flags: needinfo?(nfitzgerald) → needinfo?(daizhuoxian)
Attachment #8566602 - Attachment is obsolete: true
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.
Flags: needinfo?(daizhuoxian)
Attachment #8566930 - Flags: review?(nfitzgerald)
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!
Attachment #8566930 - Flags: review?(nfitzgerald) → review+
Tests all pasted!
(In reply to Zimon Dai [:zimonD] from comment #34)
> Tests all pasted!

... passed
(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!
Keywords: checkin-needed
(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
https://hg.mozilla.org/integration/fx-team/rev/b413c3707a12
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
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
Whiteboard: [fixed-in-fx-team]
Attachment #8566930 - Attachment is obsolete: true
Comment on attachment 8567788 [details] [diff] [review]
Pause on breakpoint condition exception

Updated the patch and rebased to the latest commit.
Attachment #8567788 - Flags: review?(nfitzgerald)
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.
Flags: needinfo?(daizhuoxian)
(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
Flags: needinfo?(daizhuoxian) → needinfo?(nfitzgerald)
Eddy, you are more familiar with e10s than I am. Can you help out Zimon?
Flags: needinfo?(nfitzgerald) → needinfo?(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.
Flags: needinfo?(ejpbruel)
Browser Chrome Test Summary
	Passed: 9414
	Failed: 0
	Todo: 0

I will upload a rebased patch.
Attachment #8567788 - Attachment is obsolete: true
Attachment #8567788 - Flags: review?(nfitzgerald)
Attachment #8570284 - Flags: review?(nfitzgerald)
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
Attachment #8570284 - Flags: review?(nfitzgerald) → review+
Seems allright except for a build fail...
Yup, looks good to me!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/20661f32e7e7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.