Closed
Bug 1394495
Opened 7 years ago
Closed 7 years ago
Debugger creates a an unnecessary breakpoint on reload
Categories
(DevTools :: Debugger, defect, P1)
DevTools
Debugger
Tracking
(firefox56 fixed, firefox57 fixed)
RESOLVED
FIXED
Firefox 57
People
(Reporter: jlast, Assigned: jlast)
Details
Attachments
(2 files, 3 obsolete files)
8.98 KB,
patch
|
jdescottes
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
8.96 KB,
patch
|
jdescottes
:
review+
|
Details | Diff | Splinter Review |
This should address the issue outlined in this bug: https://github.com/devtools-html/debugger.html/issues/3682
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8901898 -
Flags: review?(jdescottes)
Assignee | ||
Comment 2•7 years ago
|
||
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.
Assignee | ||
Comment 3•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=00a52015ceaa2118d3f99bd3866674b0ef0299bb
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8901898 -
Attachment is obsolete: true
Attachment #8901898 -
Flags: review?(jdescottes)
Attachment #8901901 -
Flags: review?(jdescottes)
Updated•7 years ago
|
Assignee: nobody → jlaster
Status: NEW → ASSIGNED
Priority: -- → P1
Comment 5•7 years ago
|
||
try with fixed patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbc2262daffeb9847b4d827b46c6c6675e918d3c
Comment 6•7 years ago
|
||
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•7 years ago
|
||
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.
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8901901 -
Attachment is obsolete: true
Attachment #8901901 -
Flags: review?(jdescottes)
Attachment #8901921 -
Flags: review?(jdescottes)
Assignee | ||
Comment 9•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc227ca9a30d954f5667a4289dfa7fc5391de417
Comment 10•7 years ago
|
||
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+
Assignee | ||
Comment 11•7 years ago
|
||
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?
Assignee | ||
Comment 12•7 years ago
|
||
fixes the commit message
Attachment #8901921 -
Attachment is obsolete: true
Assignee | ||
Comment 13•7 years ago
|
||
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
Assignee | ||
Comment 14•7 years ago
|
||
This bug was introduced in the july 19th update (https://bugzilla.mozilla.org/show_bug.cgi?id=1382432), but just discovered now.
Comment 15•7 years ago
|
||
try push for beta patch (attachment 8901928 [details] [diff] [review]) : https://treeherder.mozilla.org/#/jobs?repo=try&revision=756df70de119e4ae16133bc9c40951c1b3f0f6e0
Updated•7 years ago
|
Attachment #8901942 -
Flags: review+
Comment 16•7 years ago
|
||
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+
Assignee | ||
Comment 17•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc227ca9a30d954f5667a4289dfa7fc5391de417 ^ mozilla-central try run
Updated•7 years ago
|
status-firefox56:
--- → affected
status-firefox57:
--- → affected
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 18•7 years ago
|
||
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
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3c0b800709f1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 20•7 years ago
|
||
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)
Assignee | ||
Comment 21•7 years ago
|
||
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 22•7 years ago
|
||
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+
Comment 23•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/9796d42a13d4
Comment 24•7 years ago
|
||
(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-
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•