Closed Bug 1394495 Opened 2 years ago Closed 2 years ago

Debugger creates a an unnecessary breakpoint on reload

Categories

(DevTools :: Debugger, defect, P1)

defect

Tracking

(firefox56 fixed, firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: jlast, Assigned: jlast)

Details

Attachments

(2 files, 3 obsolete files)

This should address the issue outlined in this bug:
https://github.com/devtools-html/debugger.html/issues/3682
Attached patch fix-bps.patch (obsolete) — Splinter Review
Attachment #8901898 - Flags: review?(jdescottes)
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.
Attached patch fix-bps2.patch (obsolete) — Splinter Review
Attachment #8901898 - Attachment is obsolete: true
Attachment #8901898 - Flags: review?(jdescottes)
Attachment #8901901 - Flags: review?(jdescottes)
Assignee: nobody → jlaster
Status: NEW → ASSIGNED
Priority: -- → P1
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
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.
Attached patch fix-bps3.patch (obsolete) — Splinter Review
Attachment #8901901 - Attachment is obsolete: true
Attachment #8901901 - Flags: review?(jdescottes)
Attachment #8901921 - Flags: review?(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.
Attachment #8901921 - Flags: review?(jdescottes) → review+
Attached patch fix-bps4.patchSplinter Review
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]:
Attachment #8901928 - Flags: review?(jdescottes)
Attachment #8901928 - Flags: approval-mozilla-beta?
Attached patch fix-bps5.patchSplinter Review
fixes the commit message
Attachment #8901921 - Attachment is obsolete: true
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
This bug was introduced in the july 19th update (https://bugzilla.mozilla.org/show_bug.cgi?id=1382432), but just discovered now.
Comment on attachment 8901928 [details] [diff] [review]
fix-bps4.patch

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

Looks good, let's wait for try
Attachment #8901928 - Flags: review?(jdescottes) → review+
Keywords: checkin-needed
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c0b800709f1
Debugger creates a an unnecessary breakpoint on reload. r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3c0b800709f1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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?
Flags: needinfo?(jlaster)
Sorry, I attached the wrong link. The bug was introduced in July for the new debugger UI. Both 56 and 57 are affected.
Flags: needinfo?(jlaster)
Comment on attachment 8901928 [details] [diff] [review]
fix-bps4.patch

Fix for new feature, let's uplift this for beta 8.
Attachment #8901928 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.