Closed Bug 1388128 Opened 2 years ago Closed 2 years ago
Debugger does not remove some breakpoints
No description provided.
We currently have a bug where users can't delete some breakpoints. https://github.com/devtools-html/debugger.html/issues/3549 This happens when the user had breakpoints prior to July 20th when we updated the saved breakpoint schema and didn't remove the saved breakpoints. This change does just that. The debugger schema flag is used to update the save fields when the schema changes . This was actually bumped when we made the change, but I accidentally bumped the pref on the last release, which defeats the purpose of the field. : https://github.com/devtools-html/debugger.html/blob/master/src/utils/prefs.js#L48-L53
Oh! I totally misunderstood the behavior of the preferences files, which is why I r+'d the change that introduced the regression. I assumed the following: - in preferences/debugger.js, set debugger.prefs-schema-version to 1.0.0 - start FF with a clean/new ProfileA - in preferences/debugger.js, set debugger.prefs-schema-version to 1.0.2 - start FF with ProfileA again => Here I assumed the pref would remain on 1.0.0 and that the migration code of the debugger would kick in, clear the existing breakpoints. But since 1.0.0 was not a user-value, Firefox automatically updates it to 1.0.2, and the debugger ignores the migration. This brings the question: why do have a default value in preferences/debugger.js? Now that I understand this a bit better I think this pref should only be driven by JS here. On debugger startup you check the current value of the pref: - if it's not here, it's a clean profile => set the pref - if it's different from the expected version => cleanup breakpoints + set the pref - if it's the same, you know that it means that the user already ran this version of the debugger => nothing to do I think the default value in preferences/debugger.js only brings confusion here. I'm ok with landing this as it should cleanup breakpoints for most users and I guess it's kind of urgent to address.
Assignee: nobody → jlaster
Status: NEW → ASSIGNED
Priority: -- → P3
Comment on attachment 8894610 [details] [diff] [review] fix-bps.patch Review of attachment 8894610 [details] [diff] [review]: ----------------------------------------------------------------- R+ based on the assumption that it is urgent to fix. Have a look at my comment and let me know what you think though. I can PR a change doing that.
Attachment #8894610 - Flags: review?(jdescottes) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad7459b71ea0 Debugger does not remove some breakpoints. r=jdescottes
Approval Request Comment [Feature/Bug causing the regression]: this is related to the new debugger UI [User impact if declined]: some breakpoints with an old profile won't be deleted [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [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?]: it is only used the clear the breakpoints pref [String changes made/needed]:
Attachment #8896354 - Flags: approval-mozilla-beta?
Comment on attachment 8896354 [details] [diff] [review] fix-bps.patch Fix an issue related to new debugger UI. Beta56+. Should be in 56.0b3.
Attachment #8896354 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Jason Laster [:jlast] from comment #8) > [Is this code covered by automated tests?]: yes > [Has the fix been verified in Nightly?]: yes > [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.
You need to log in before you can comment on or make changes to this bug.