Closed
Bug 1201008
Opened 9 years ago
Closed 9 years ago
Toolbox can't be closed after setting a breakpoint
Categories
(DevTools :: Debugger, defect)
Tracking
(firefox44 fixed)
RESOLVED
FIXED
Firefox 44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: phorea, Assigned: jlong)
Details
(Whiteboard: [polish-backlog][difficulty=easy])
Attachments
(1 file, 2 obsolete files)
2.89 KB,
patch
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
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)
Comment 3•9 years ago
|
||
Alright great, should we block this on bug 1200798 then?
Assignee | ||
Comment 4•9 years ago
|
||
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.
Assignee | ||
Comment 5•9 years ago
|
||
(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)
Assignee | ||
Comment 6•9 years ago
|
||
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)
Assignee | ||
Comment 7•9 years ago
|
||
I went ahead and did a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=de193fe9a41b
Updated•9 years ago
|
Attachment #8656066 -
Attachment is obsolete: true
Comment 8•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
Whiteboard: [polish-backlog][difficulty=easy]
Assignee | ||
Comment 9•9 years ago
|
||
New patch w/test
Attachment #8660803 -
Attachment is obsolete: true
Assignee | ||
Comment 10•9 years ago
|
||
Assignee | ||
Comment 11•9 years ago
|
||
Saw the same intermittents on other bugs too.
Keywords: checkin-needed
Comment 12•9 years ago
|
||
Keywords: checkin-needed
Comment 13•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•