Closed Bug 1398893 Opened 2 years ago Closed 2 years ago

Breakpoints cannot be removed


(DevTools :: Debugger, defect, P1)



(firefox56+ fixed, firefox57+ fixed)

Firefox 57
Tracking Status
firefox56 + fixed
firefox57 + fixed


(Reporter: jlast, Assigned: jlast)



(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
Priority: -- → P1
Comment on attachment 8906850 [details] [diff] [review]

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 ?
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:
Flags: needinfo?(jlaster)
:jlaster created a proper PR for this patch on GitHub:

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
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]

Fix for a bad but rare debugger issue, OK to uplift for beta 12.
Attachment #8906850 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.