Closed Bug 1201008 Opened 4 years ago Closed 4 years ago

Toolbox can't be closed after setting a breakpoint

Categories

(DevTools :: Debugger, defect)

42 Branch
defect
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: petruta.rasa, Assigned: jlong)

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(1 file, 2 obsolete files)

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)
Attached patch debugger-breakpoint-empty.patch (obsolete) — Splinter Review
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?
Flags: needinfo?(jlong)
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)
Assignee: nobody → jlong
Flags: needinfo?(jlong)
Alright great, should we block this on bug 1200798 then?
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.
(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)
Attached patch 1201008.patch (obsolete) — Splinter Review
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.
Attachment #8660803 - Flags: review?(bgrinstead)
Attachment #8656066 - Attachment is obsolete: true
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 (
Attachment #8660803 - Flags: review?(bgrinstead) → review+
Whiteboard: [polish-backlog][difficulty=easy]
Attached patch 1201008.patchSplinter Review
New patch w/test
Attachment #8660803 - Attachment is obsolete: true
Saw the same intermittents on other bugs too.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0dae594de5ac
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.