Debugger does not remove some breakpoints
Categories
(DevTools :: Debugger, defect, P3)
Tracking
(firefox56 fixed, firefox57 fixed)
People
(Reporter: jlast, Assigned: jlast)
References
Details
Attachments
(2 files)
1.37 KB,
patch
|
jdescottes
:
review+
|
Details | Diff | Splinter Review |
1.37 KB,
patch
|
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Comment 2•7 years ago
|
||
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 [1]. 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. [1]: https://github.com/devtools-html/debugger.html/blob/master/src/utils/prefs.js#L48-L53
Comment 3•7 years ago
|
||
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.
Updated•7 years ago
|
Comment 4•7 years ago
|
||
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.
Assignee | ||
Comment 5•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=196c78e924442d10431c75d1eec3388717b45b5c
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad7459b71ea0 Debugger does not remove some breakpoints. r=jdescottes
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ad7459b71ea0
Assignee | ||
Comment 8•7 years ago
|
||
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]:
Updated•7 years ago
|
Comment 9•7 years ago
|
||
Comment on attachment 8896354 [details] [diff] [review] fix-bps.patch Fix an issue related to new debugger UI. Beta56+. Should be in 56.0b3.
Comment 10•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/d4b77eb2881c
Comment 11•7 years ago
|
||
(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.
Updated•6 years ago
|
Comment 12•5 years ago
|
||
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.
I would recommend against doing this. The preferences system largely assumes that preferences set on the user branch also have a default value. This isn't universally true, and isn't necessarily required. When it is not true however, there a some edge cases and problems that pop up. I don't think any of the known problems would affect this particular preference, but as a general rule it is better to provide a default value.
In this case, it is very easy to have a default value for the preference, so I would strongly recommend doing that.
Comment 13•5 years ago
|
||
Apologies for posting on such an old bug. I got some wires crossed and didn't notice how old this bug was.
Description
•