1394495 - Debugger creates a an unnecessary breakpoint on reload

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P1)

Description

[Jason Laster [:jlast]]

This should address the issue outlined in this bug:
https://github.com/devtools-html/debugger.html/issues/3682

Comment 1

[Jason Laster [:jlast]]

Attachment: fix-bps.patch

Comment 2

[Jason Laster [:jlast]]

STR:

1. `git clone https://github.com/devtools-html/debugger-examples`
2. `yarn start`
3. go to localhost:7999 and increment example
4. add a breakpoint in math.js on line 6
5. in your editor go to increment/example.js and add ten lines of comments to the top of the file
6. in the terminal CD into debugger-examples and run `yarn` then CD into examples/increment and run `webpack`
7. go back to firefox reload the page

You should see the breakpoint on math.js line 6. You should also see the breakpoint in bundle.js on the correct line.

---

it is good to repeat the last couple steps a couple times

1. add or remove comments in increment/example.js
2. run webpack
3. reload firefox

You should check:
1. the breakpoint is in the right spot in the bundle
2. there is one breakpoint in the list

Occasionally, you might see the debugger pause on another line. This is okay. It is a race condition where the debugger has not yet removed the old breakpoint. We cannot fix that at the moment.

Comment 3

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=00a52015ceaa2118d3f99bd3866674b0ef0299bb

Comment 4

[Jason Laster [:jlast]]

Attachment: fix-bps2.patch

Comment 5

[Julian Descottes [:jdescottes]]

try with fixed patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbc2262daffeb9847b4d827b46c6c6675e918d3c

Comment 6

[Julian Descottes [:jdescottes]]

Locally I have test failures on browser_dbg-breakpoints.js:

140 INFO TEST-UNEXPECTED-FAIL | devtools/client/debugger/new/test/mochitest/browser_dbg-breakpoints.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/debugger/new/test/mochitest/browser_dbg-breakpoints.js:76 - TypeError: bp2 is undefined
Stack trace:
    @chrome://mochitests/content/browser/devtools/client/debugger/new/test/mochitest/browser_dbg-breakpoints.js:76:3
    Tester_execTest@chrome://mochikit/content/browser-test.js:787:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:687:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
    Tester_execTest@chrome://mochikit/content/browser-test.js:787:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:687:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
141 INFO TEST-UNEXPECTED-FAIL | devtools/client/debugger/new/test/mochitest/browser_dbg-breakpoints.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/debugger/new/test/mochitest/browser_dbg-breakpoints.js:99 - TypeError: bp1 is undefined
Stack trace:
    @chrome://mochitests/content/browser/devtools/client/debugger/new/test/mochitest/browser_dbg-breakpoints.js:99:3
    Tester_execTest@chrome://mochikit/content/browser-test.js:787:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:687:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
    Tester_execTest@chrome://mochikit/content/browser-test.js:787:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:687:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59

Comment 7

[Julian Descottes [:jdescottes]]

I initially thought this was a test issue, but the patch seems to be breaking enabling/disabling breakpoints pretty badly. 

STRs: 
- go to http://firefox-dev.tools/debugger-examples/examples/pythagorean/
- open debugger
- add a breakpoint
- disable the breakpoint
- enable the breakpoint 

=> the breakpoint is sliding down.

Comment 8

[Jason Laster [:jlast]]

Attachment: fix-bps3.patch

Comment 9

[Jason Laster [:jlast]]

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

Comment 10

[Julian Descottes [:jdescottes]]

Comment on attachment 8901921 [details] [diff] [review]
fix-bps3.patch

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

Fixes the bug from the summary as well as the STRs provided on GitHub and tests are green locally for the debugger. 
Let's wait for try.

Comment 11

[Jason Laster [:jlast]]

Attachment: fix-bps4.patch

Approval Request Comment

[Feature/Bug causing the regression]: 1341305

[User impact if declined]: When a user adds a breakpoint in an application with sourcemaps, changes their code and reload will see a second breakpoint that they can't remove. We will disable the new debugger UI pref. This means the new debugger UI will hopefully be released in 57.

[Is this code covered by automated tests?]: We have integration tests that guarantee that we don't introduce new regression

[Has the fix been verified in Nightly?]: We have a second patch for nightly.

[Needs manual test from QE? If yes, steps to reproduce]:  no. 

[List of other uplifts needed for the feature/fix]:

[Is the change risky?]: no

[Why is the change risky/not risky?]:  the risk is contained to the new debugger UI. We are confident the patch will fix the bug, but if for some reason it does not work we will disable the new UI. 

[String changes made/needed]:

Comment 12

[Jason Laster [:jlast]]

Attachment: fix-bps5.patch

fixes the commit message

Comment 13

[Jason Laster [:jlast]]

re Feature/Bug causing the regression, I copied the wrong URL. I meant to link to this bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1394495). The original report was on github, https://github.com/devtools-html/debugger.html/issues/3682

Comment 14

[Jason Laster [:jlast]]

This bug was introduced in the july 19th update (https://bugzilla.mozilla.org/show_bug.cgi?id=1382432), but just discovered now.

Comment 15

[Julian Descottes [:jdescottes]]

try push for beta patch (attachment 8901928 [details] [diff] [review]) : https://treeherder.mozilla.org/#/jobs?repo=try&revision=756df70de119e4ae16133bc9c40951c1b3f0f6e0

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8901928 [details] [diff] [review]
fix-bps4.patch

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

Looks good, let's wait for try

Comment 17

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc227ca9a30d954f5667a4289dfa7fc5391de417
^ mozilla-central try run

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c0b800709f1
Debugger creates a an unnecessary breakpoint on reload. r=jdescottes

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3c0b800709f1

Comment 20

[Gerry Chang [:gchang]]

Hi Jason,
From you uplift request, the feature is bug 1341305 and it's fixed only in 57. Why we need this patch in 56?

Comment 21

[Jason Laster [:jlast]]

Sorry, I attached the wrong link. The bug was introduced in July for the new debugger UI. Both 56 and 57 are affected.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8901928 [details] [diff] [review]
fix-bps4.patch

Fix for new feature, let's uplift this for beta 8.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9796d42a13d4

Comment 24

[Andrei Vaida [:avaida]]

(In reply to Jason Laster [:jlast] from comment #11)
> [Is this code covered by automated tests?]: We have integration tests that
> guarantee that we don't introduce new regression
> 
> [Has the fix been verified in Nightly?]: We have a second patch for nightly.
> 
> [Needs manual test from QE? If yes, steps to reproduce]:  no. 

Setting qe-verify- based on Jason's assessment on manual testing needs and the fact that this fix has automated coverage.