Closed Bug 1008372 Opened 6 years ago Closed 6 years ago

Some breakpoints can never be removed after transitioning to a new line, even after closing the toolbox

Categories

(DevTools :: Debugger, defect)

defect
Not set

Tracking

(firefox31 fixed, firefox32 fixed, firefox33 fixed)

RESOLVED FIXED
Firefox 33
Tracking Status
firefox31 --- fixed
firefox32 --- fixed
firefox33 --- fixed

People

(Reporter: vporof, Assigned: jlong)

Details

(Whiteboard: in-testsuite+)

Attachments

(2 files, 5 obsolete files)

Attached image screencap.gif
STR:

1. Go to http://todomvc.com/architecture-examples/backbone/
2. Open debugger
3. Add a breakpoint on line 7 of app-view.js
4. Breakpoint slides to line 11
5. Reload the page
6. Execution pauses on line 11 (yay!)
7. Add breakpoints on lines 5, 4, 3 and 2 respectively
8. Reload the page.
9. Execution pauses on line 2 (yay!)
10. Remove all breakpoints
11. Reload the page

Execution pauses on a breakpoint that's not actually there, every time. Even after closing the toolbox, and reopening the debugger, execution will still pause!
Similar bug happens to me.

STR:

1. Make some page with some JS and open it.
2. Open debugger, set a breakpoint somewhere.
3. Reload. Breakpoint hits.
4. Close the tab.
5. Open a new tab and navigate to the same page from step 1.
6. Open debugger, verify no breakpoints are visually set.
7. Reload. Invisible breakpoint hits.
Looking into it, will fix ASAP. I guess it's too easy to break stuff with changing only a few lines of code :(
Assignee: nobody → jlong
Actually, I just recreated it in a build without bug 799077, and shu said it happened to him when no breakpoints moved. I don't think it's related to that. Not sure what it's related to yet, will keep digging.
I think shu and victor's bugs are actually different; I can reproduce both, but in shu's case there is a noSuchActor reported when trying to remove the breakpoint. Looks like the server shuts down too fast or something? The original bug is looking like something else, and I think I'm pretty close to fixing it.
Blocks: 1010552
No longer blocks: 1010552
The bug shu mentioned is a more serious one and I filed a bug for it here: https://bugzilla.mozilla.org/show_bug.cgi?id=1010552

I tried to fix it, but I don't really know how to. I see where the problem is, but I don't know how to make the server keep the actors around so it can handle the breakpoint removal requests.
Attached patch 1008372.patch (obsolete) — Splinter Review
This fixes the original bug that Victor found
Attachment #8422746 - Flags: review?(vporof)
Comment on attachment 8422746 [details] [diff] [review]
1008372.patch

Nick is most likely a better reviewer for this.
Attachment #8422746 - Flags: review?(vporof)
The issue is that `findBreakpoints` in the BreakpointStore is not checking `wholeLineBreakpoints` because a column exists, so it needs to be null and it will figure out that a breakpoint already exists.
Comment on attachment 8422746 [details] [diff] [review]
1008372.patch

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

Holy moly, I think it is time to split up _setBreakpoint into smaller pieces.

::: toolkit/devtools/server/actors/script.js
@@ +1570,5 @@
>            if (!actualLocation) {
>              actualLocation = {
>                url: aLocation.url,
>                line: line,
> +              column: null

Can we just drop the column property in this case?
Attachment #8422746 - Flags: review+
(In reply to Nick Fitzgerald [:fitzgen] from comment #9)
> 
> Holy moly, I think it is time to split up _setBreakpoint into smaller pieces.

I think I'll feel confident pretty soon to be aggressive about cleaning up current code. It definitely needs to be done.
> 
> ::: toolkit/devtools/server/actors/script.js
> @@ +1570,5 @@
> >            if (!actualLocation) {
> >              actualLocation = {
> >                url: aLocation.url,
> >                line: line,
> > +              column: null
> 
> Can we just drop the column property in this case?

Yep! I will do that. Wasn't sure if it was better to be explicit.
Attached patch 1008372.patch (rebased) (obsolete) — Splinter Review
Attachment #8422746 - Attachment is obsolete: true
Yeah we need a test (or modify an existing one if you prefer that), I should have originally caught that.
Attached patch 1008372.patch (obsolete) — Splinter Review
New patch with tests. Fitzgen mind looking over the test real quick? It throws the appropriate error without the fix, and passes with the fix.
Attachment #8434415 - Attachment is obsolete: true
Attachment #8435195 - Flags: review?(nfitzgerald)
Comment on attachment 8435195 [details] [diff] [review]
1008372.patch

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

Cancelling review based on irc discussion about adding a third pause.

::: browser/devtools/debugger/test/browser_dbg_breakpoints-actual-location2.js
@@ +40,5 @@
> +      var bpClient = yield gPanel.addBreakpoint({
> +        url: gSources.selectedValue,
> +        line: 19
> +      });
> +      

trailing whitespace

(setq-default show-trailing-whitespace t)

::: browser/devtools/debugger/test/doc_breakpoint-move.html
@@ +15,5 @@
> +      function ermahgerd() {
> +      debugger;
> +      // This is just a line
> +      // and here we are
> +      return 5;

C'mon man, we can indent this properly ;)

http://web-mode.org/
Attachment #8435195 - Flags: review?(nfitzgerald)
Attached patch 1008372.patch (w/test) (obsolete) — Splinter Review
This test is better now. Not much different, but it makes sure that it hits the final breakpoint correctly at the end of the test.
Attachment #8435195 - Attachment is obsolete: true
Attachment #8435331 - Flags: review?(nfitzgerald)
Comment on attachment 8435331 [details] [diff] [review]
1008372.patch (w/test)

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

Nice.
Attachment #8435331 - Flags: review?(nfitzgerald) → review+
What's the status on this? I'm hitting it constantly and have to restart the browser just to be able to continue debugging.
I thought this had landed, sorry. I will push this through asap.
Attached patch 1008372.patch (rebased) (obsolete) — Splinter Review
Attachment #8435331 - Attachment is obsolete: true
Not sure why the tests failed last time. The error looks to be like an error in our testing framework. Pushed again: https://tbpl.mozilla.org/?tree=Try&rev=43c8acb2689c
Attached patch 1008372.patchSplinter Review
Looks like there was a test failing (test_sourcemaps-03.js). I had to force a column to exist in `getOriginalLocation`. We probably need to clean up how we deal with columns, but I think I'll do that in bug 905700.
Attachment #8444441 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/838aa70f96c0
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
This probably needs an uplift?
https://hg.mozilla.org/mozilla-central/rev/838aa70f96c0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Comment on attachment 8444654 [details] [diff] [review]
1008372.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: In a not-so-rare case, the user cannot remove breakpoints in the script and Firefox must be completely restarted to get the debugger working again.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): not much risk. It only changes 1 line of code.
String or IDL/UUID changes made by this patch:
Attachment #8444654 - Flags: approval-mozilla-aurora?
Comment on attachment 8444654 [details] [diff] [review]
1008372.patch

Aurora approval granted. Does this affect beta?
Attachment #8444654 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Lawrence Mandel [:lmandel] from comment #28)
> Comment on attachment 8444654 [details] [diff] [review]
> 1008372.patch
> 
> Aurora approval granted. Does this affect beta?

Just tested it and yes, this affects beta. Should I request uplift there too?
(In reply to James Long (:jlongster) from comment #30)
> Just tested it and yes, this affects beta. Should I request uplift there too?

Since it's such a simple fix, I'd say yes.
Comment on attachment 8444654 [details] [diff] [review]
1008372.patch

Approval Request Comment
[Feature/regressing bug #]: unknown
[User impact if declined]: In a not-so-rare case, the user cannot remove breakpoints in the script and Firefox must be completely restarted to get the debugger working again.
[Describe test coverage new/current, TBPL]: has test, has been on m-c for several weeks, has been on aurora for about a week
[Risks and why]: There's not much risk here. It's a one-line change with a full test that comes with it.
[String/UUID change made/needed]:
Attachment #8444654 - Flags: approval-mozilla-beta?
Attachment #8444654 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: in-testsuite+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.