Closed Bug 1398893 Opened 2 years ago Closed 2 years ago
Breakpoints cannot be removed
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.
From irc conversation this is a bad issue but rare. We hope to fix it in late beta 56.
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]:
[wip] I plan on testing this for the next couple hours.
The tests are passing for dt, but are failing for bc.
The initial patch was in the method for removing breakpoints and not pending breakpoints.
Assignee: nobody → jlaster
Status: NEW → ASSIGNED
Priority: -- → P1
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.
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.
In case we need it, here is a version of the patch for central.
jlaster, can you land the patch for m-c? Thanks.
Flags: needinfo?(lhenry) → needinfo?(jlaster)
Thanks for the info Liz. Here is a try push for the m-c patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6588c206efd3db6e79058a4f1136336a935e8045
: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.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/adc30ee91284 clean pending breakpoints if debugger in corrupted state;r=jdescottes
Try green and r+'d on the GitHub PR.
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+
You need to log in before you can comment on or make changes to this bug.