Closed
Bug 1398893
Opened 7 years ago
Closed 7 years ago
Breakpoints cannot be removed
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)
7.67 KB,
patch
|
jdescottes
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
2.47 KB,
patch
|
jdescottes
:
review+
|
Details | Diff | Splinter Review |
It's possible to get the new debugger into a state where breakpoints cannot be removed. We would like to clear the breakpoints when we get into this situation.
Comment 1•7 years ago
|
||
From irc conversation this is a bad issue but rare. We hope to fix it in late beta 56.
status-firefox56:
--- → affected
status-firefox57:
--- → affected
tracking-firefox56:
--- → +
tracking-firefox57:
--- → +
Assignee | ||
Comment 2•7 years ago
|
||
Approval Request Comment [Feature/Bug causing the regression]: 1294139 [User impact if declined]: we will release the new debugger in 57 [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: No [Needs manual test from QE? No, this only affects the case where breakpoint data is corrupted. I am not sure how to create a small STR. [List of other uplifts needed for the feature/fix]: [Is the change risky?]: No, [Why is the change risky/not risky?]: this will only affect people who have corrupted breakpoint data. Very few people should get into this state. If they are, it just clears the data. [String changes made/needed]:
Attachment #8906753 -
Flags: review?(jdescottes)
Attachment #8906753 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 3•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5318799c93511a47f5edc8539fd6d04a37470bf
Assignee | ||
Comment 4•7 years ago
|
||
[wip] I plan on testing this for the next couple hours.
Assignee | ||
Comment 5•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef4b2ed7fd543206946b2323a09679c74dcb71d1
Assignee | ||
Comment 6•7 years ago
|
||
The tests are passing for dt, but are failing for bc.
Assignee | ||
Comment 7•7 years ago
|
||
The initial patch was in the method for removing breakpoints and not pending breakpoints.
Attachment #8906753 -
Attachment is obsolete: true
Attachment #8906753 -
Flags: review?(jdescottes)
Attachment #8906753 -
Flags: approval-mozilla-beta?
Attachment #8906849 -
Flags: review?(jdescottes)
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8906849 -
Attachment is obsolete: true
Attachment #8906849 -
Flags: review?(jdescottes)
Attachment #8906850 -
Flags: review?(jdescottes)
Attachment #8906850 -
Flags: approval-mozilla-beta?
Updated•7 years ago
|
Assignee: nobody → jlaster
Status: NEW → ASSIGNED
Priority: -- → P1
Comment 9•7 years ago
|
||
Comment on attachment 8906850 [details] [diff] [review] clear-bps-2.patch Review of attachment 8906850 [details] [diff] [review]: ----------------------------------------------------------------- The code change seems sane to me but it should really be looked at by someone who is familiar with this area. For instance I have no idea why we need to do this in the pending breakpoints reducer, but not in the "regular" breakpoints reducer. I guess the matching issue on GH is https://github.com/devtools-html/debugger.html/issues/3971 ? We should try to put enough information on bugzilla so that we can track back to GitHub. An issue number is the bare minimum.
Attachment #8906850 -
Flags: review?(jdescottes)
Comment 10•7 years ago
|
||
This patch is based on a beta changeset. It does not apply on central. I think we have to land this first on central and then on beta, so we need two separate patches here. Liz: can you confirm if we need to land a patch on central first, before asking for a beta uplift? I might be wrong here.
Flags: needinfo?(lhenry)
Comment 11•7 years ago
|
||
In case we need it, here is a version of the patch for central.
Comment 12•7 years ago
|
||
jlaster, can you land the patch for m-c? Thanks.
Flags: needinfo?(lhenry) → needinfo?(jlaster)
Comment 13•7 years ago
|
||
Thanks for the info Liz. Here is a try push for the m-c patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6588c206efd3db6e79058a4f1136336a935e8045
Flags: needinfo?(jlaster)
Comment 14•7 years ago
|
||
:jlaster created a proper PR for this patch on GitHub: https://github.com/devtools-html/debugger.html/pull/3981 I am waiting for a green try + approval of the PR on GitHub by a debugger peer before moving on with landing this.
Comment 15•7 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/adc30ee91284 clean pending breakpoints if debugger in corrupted state;r=jdescottes
Comment 16•7 years ago
|
||
Try green and r+'d on the GitHub PR.
Attachment #8907037 -
Attachment is obsolete: true
Attachment #8907217 -
Flags: review+
Updated•7 years ago
|
Attachment #8906850 -
Flags: review+
Comment 17•7 years ago
|
||
Comment on attachment 8906850 [details] [diff] [review] clear-bps-2.patch Fix for a bad but rare debugger issue, OK to uplift for beta 12.
Attachment #8906850 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 18•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/cdfda62c6eff
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/adc30ee91284
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•