Closed Bug 1398893 Opened 2 years ago Closed 2 years ago

Breakpoints cannot be removed

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)

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.
Attached patch clear-bps.patch (obsolete) — Splinter Review
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?
[wip] I plan on testing this for the next couple hours.
The tests are passing for dt, but are failing for bc.
Attached patch clear-bps-2.patch (obsolete) — Splinter Review
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)
Attachment #8906849 - Attachment is obsolete: true
Attachment #8906849 - Flags: review?(jdescottes)
Attachment #8906850 - Flags: review?(jdescottes)
Attachment #8906850 - Flags: approval-mozilla-beta?
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.
Attachment #8906850 - Flags: review?(jdescottes)
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)
Attached patch bug1398893.central.patch (obsolete) — Splinter Review
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
Flags: needinfo?(jlaster)
: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 jdescottes@mozilla.com:
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.
Attachment #8907037 - Attachment is obsolete: true
Attachment #8907217 - Flags: review+
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+
https://hg.mozilla.org/mozilla-central/rev/adc30ee91284
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.