1201008 - Toolbox can't be closed after setting a breakpoint

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Petruta Horea [:phorea], Desktop QA]

Reproduced with Nightly 43.0a1, Aurora 42.0a2 2015-09-01 and Firefox 41 beta 6 under Win 7 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.10.

Steps to reproduce:
1. Open: data:text/html,hi
2. Open Developer Tools -> Debugger or Ctrl/Cmd+Shift+S
3. Add a Breakpoint
4. Close the toolbox

Expected results:
The toolbox closes

Actual results:
The toolbox can't be closed

Notes:
1. Browser console error when adding the breakpoint: https://www.pastebin.mozilla.org/8844822
2. This issues reproduces back to Firefox 37RC
3. In versions previous to 37, eg 34.0.5 the Browser console could be closed but it won't reopen again (same as bug 1191916)

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: debugger-breakpoint-empty.patch

James, this is a "breaks the toolbox" bug in the debugger.  It happens when setting a breakpoint when there isn't a source.

Seems like a frontend fix is needed to stop from trying to add a breakpoint in this case.  I started by tracking the exception down inside debugger-controller, but maybe this would be better handled from the editor component instead.  Because even with this patch, the breakpoint still shows up as visible in the editor.  Can you help make sure this is being tracked and prioritized?

Comment 2

[James Long (:jlongster)]

Yep, I can fix this. It is a bit annoying right now because breakpoints are stored in several places (the editor internally tracks them and displays them, and the Breakpoints controller also does that). It might not be a one-line fix, but it shouldn't be too bad.

FWIW, I'm really close to having breakpoints and sources all cleaned up in bug 1200798. It's going to be a lot easier, with data coming from a single source of truth and asynchronous flow more clearly laid out. It will also be a good example of how I'm thinking we could clean up some of our tools (I'm going to talk to more people about this very soon)

Comment 3

[Brian Grinstead [:bgrins]]

Alright great, should we block this on bug 1200798 then?

Comment 4

[James Long (:jlongster)]

No, this should be a small patch so let's go ahead and get it in. I'll probably have a rough draft for that bug soon, but it's going to take a while to convert many tests (how you wait for events has changed). Hopefully only a few weeks, but I don't want to block stuff on it except big things.

Comment 5

[James Long (:jlongster)]

(I'm also going to feedback from others on the team before actually landing it, so it'll probably be a little while before it actually lands)

Comment 6

[James Long (:jlongster)]

Attachment: 1201008.patch

I'd rather do it this way. I think your patch still adds the breakpoint to the editor, but just doesn't do anything else. My patch adds the check a level higher, so the breakpoint isn't even added to the editor. I think it's clearer that nothing is going on. I think there would be an error when you try the remove the breakpoint with you patch (I could be wrong).

Also, instead of try/catch, I'd like to explicitly check if we are actually looking at a source or not.

My refactor gets rid of the code I'm patching, but we can go ahead and land this.

Comment 7

[James Long (:jlongster)]

I went ahead and did a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=de193fe9a41b

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8660803 [details] [diff] [review]
1201008.patch

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

This looks like a much better solution, r=me on the code change.  But can you please add a regression test for this before landing?  Should be relatively small given the STR in the description and will also confirm that the issue stays fixed during your refactor.

::: browser/devtools/debugger/debugger-view.js
@@ +296,5 @@
>        }
> +      // Bug 1201008: Only add the breakpoint to the editor if we're currently
> +      // looking at a source. Even if no source is loaded, you can
> +      // interact with line 1 of the editor.
> +      else if(DebuggerView.Sources.selectedValue) {

nit: extra space between if and (

Comment 9

[James Long (:jlongster)]

Attachment: 1201008.patch

New patch w/test

Comment 10

[James Long (:jlongster)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bbcf0d4f74e

Comment 11

[James Long (:jlongster)]

Saw the same intermittents on other bugs too.

Comment 12

[Pulsebot]

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

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/0dae594de5ac