Closed Bug 1388128 Opened 7 years ago Closed 7 years ago

Debugger does not remove some breakpoints

Categories

(DevTools :: Debugger, defect, P3)

defect

Tracking

(firefox56 fixed, firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: jlast, Assigned: jlast)

References

Details

Attachments

(2 files)

      No description provided.
Attached patch fix-bps.patchSplinter Review
Attachment #8894610 - Flags: review?(jdescottes)
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
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
Blocks: 1384391
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 jdescottes@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad7459b71ea0
Debugger does not remove some breakpoints. r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/ad7459b71ea0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Attached patch fix-bps.patchSplinter Review
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.
Flags: qe-verify-
Product: Firefox → DevTools

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.

Apologies for posting on such an old bug. I got some wires crossed and didn't notice how old this bug was.

You need to log in before you can comment on or make changes to this bug.