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

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Victor Porof [:vporof][:vp]]

Attachment: 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!

Comment 1

[Shu-yu Guo [:shu]]

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.

Comment 2

[James Long (:jlongster)]

Looking into it, will fix ASAP. I guess it's too easy to break stuff with changing only a few lines of code :(

Comment 3

[James Long (:jlongster)]

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.

Comment 4

[James Long (:jlongster)]

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.

Comment 5

[James Long (:jlongster)]

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.

Comment 6

[James Long (:jlongster)]

Attachment: 1008372.patch

This fixes the original bug that Victor found

Comment 7

[Victor Porof [:vporof][:vp]]

Comment on attachment 8422746 [details] [diff] [review]
1008372.patch

Nick is most likely a better reviewer for this.

Comment 8

[James Long (:jlongster)]

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 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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?

Comment 10

[James Long (:jlongster)]

(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.

Comment 11

[James Long (:jlongster)]

Attachment: 1008372.patch (rebased)

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Yeah we need a test (or modify an existing one if you prefer that), I should have originally caught that.

Comment 13

[James Long (:jlongster)]

Attachment: 1008372.patch

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.

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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/

Comment 15

[James Long (:jlongster)]

Attachment: 1008372.patch (w/test)

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.

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8435331 [details] [diff] [review]
1008372.patch (w/test)

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

Nice.

Comment 17

[James Long (:jlongster)]

https://tbpl.mozilla.org/?tree=Try&rev=aa9a58c031ef

Comment 18

[Till Schneidereit [:till]]

What's the status on this? I'm hitting it constantly and have to restart the browser just to be able to continue debugging.

Comment 19

[James Long (:jlongster)]

I thought this had landed, sorry. I will push this through asap.

Comment 20

[James Long (:jlongster)]

Attachment: 1008372.patch (rebased)

Comment 21

[James Long (:jlongster)]

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

Comment 22

[James Long (:jlongster)]

Attachment: 1008372.patch

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.

Comment 23

[James Long (:jlongster)]

https://tbpl.mozilla.org/?tree=Try&rev=3ab4430ce1d8

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/838aa70f96c0

Comment 25

[Panos Astithas (he/him) [:past] (please ni?)]

This probably needs an uplift?

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/838aa70f96c0

Comment 27

[James Long (:jlongster)]

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:

Comment 28

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8444654 [details] [diff] [review]
1008372.patch

Aurora approval granted. Does this affect beta?

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8bc2222d4b0d

Comment 30

[James Long (:jlongster)]

(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?

Comment 31

[Panos Astithas (he/him) [:past] (please ni?)]

(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 32

[James Long (:jlongster)]

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]:

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c5808372764c