Crash report: https://crash-stats.mozilla.org/report/index/6e2399fc-80bb-4ad6-9221-196920201016

MOZ_CRASH Reason: MOZ_DIAGNOSTIC_ASSERT(false) (NS_SUCCEEDED(GetPrefValue(aPref, &value, PrefValueKind::User)))

Top 10 frames of crashing thread:

0 XUL void mozilla::Internals::UpdateMirror<std::__1::atomic<float> > modules/libpref/Preferences.cpp:4338
1 XUL NotifyCallbacks modules/libpref/Preferences.cpp:1682
2 XUL pref_SetPref modules/libpref/Preferences.cpp:1640
3 XUL mozilla::Preferences::SetCString modules/libpref/Preferences.cpp:4673
4 XUL nsPrefBranch::SetCharPrefNoLengthCheck modules/libpref/Preferences.cpp:2146
5 XUL NS_InvokeByIndex 
6 XUL XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:1142
7 XUL XPC_WN_CallMethod js/xpconnect/src/XPCWrappedNativeJSOps.cpp:925
8 XUL js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:598
9 XUL Interpret js/src/vm/Interpreter.cpp:3335

Note

• 83.0a1 seems to be affected, 82.0 is not affected

Steps to Reproduce

1. Launch Firefox
2. Access about:config
3. Search for "layout.css.devPixelsPerPx" and edit the value to 1,5

Please feel free to modify the Component, as I'm not 100% sure where this needs to go

Seems like either the back-end or the front-end should handle this somehow.

Hello! Attaching regression range made on Windows10x64:
Last good revision: 1aeb7707569fc8c0f1357c9818a3352180dfa50d
First bad revision: 2adbab502a1e690fccd61fb604be820e60d61723
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=1aeb7707569fc8c0f1357c9818a3352180dfa50d&tochange=2adbab502a1e690fccd61fb604be820e60d61723

Kris can you please have a look? Thank you!

This assertion should, in theory, be impossible because we disallow the deletion of mirrored prefs. But that doesn't seem to be what's happening here at all. I experimented with a few float and atomicFloat prefs and they all crash if you enter an invalid (non-float) value. This can't happen to, say, a uint because when I try there are checks to bind the value. This tells me that if we do have checks for float value prefs, then they aren't working, and if we don't then we probably should because they are causing this call to fail.

I can bring back the fallibility of this method, but I don't think that really fixes the problem. There was a plain MOZ_ASSERT here in the past so it wouldn't have failed on release and we wouldn't have caught it. This way, we could just attempt to update a float pref to anything, like "abcd" or "1,0". I tried this and while it won't cause any immediate crashes if the method is fallible because it doesn't update the mirror value, it will cause issues the next time you start Firefox with the pref set. So a reversion doesn't actually fix anything, it just moves the problem around. I'm curious how long this has quietly happened or if something else has broken recently.

Njn, do you know where in preferences we verify the value input by the user is correct? Is that a part of the back-end or the front-end?

Njn, do you know where in preferences we verify the value input by the user is correct? Is that a part of the back-end or the front-end?

It's a good question. The root cause of this problem is probably the fact that floats prefs aren't really a thing, and that we just use strings to store them, even though there is some API surface (such as Preferences::GetFloat, Preferences::SetFloat, and nsIPrefBranch.getFloatPref) that papers over this, and float mirror values are a thing. But I can't tell exactly what the problem would be from a cursory inspection.

Alright, I've got more information now about what's happening to the prefs and the mirror values. Here's what I worked out:

• The user sets the float pref (which is really just stored as a string) to some invalid value
• The attempt to call UpdateMirror ultimately results in a call to GetValue from its call to GetPrefValue (where we are asserting), which fails because of this cast. This is the only place where we safeguard this kind of failure. Because we can't update the mirror, we used to silently fail, but now we crash.

Ultimately, we need to identify the prefs that have float mirror values and forbid strings that cannot be formatted into floats. Otherwise, all we can do is silently fail which prevents crashing on preferences specifically but may cause strange behavior with mirror values where the pref is used.

I also filed bug 1672265 about the underlying issue ("float" mirrors failing to cast)

In bug 1642727 we assumed this method was now essentially infallible because we do not delete mirror prefs. However, users can input some pref values that cause the value casts in GetValue to fail, which causes a crash. This commit reverts this behavior back to what it originally does (fail silently) to prevent crashing. We need to fix the underlying issue (bug 1672265) but we also need to ensure this doesn't crash, and any unexpected behavior caused by incorrect pref entry will return to its original behavior before this change, which relies on the individual components' handling of bad pref values rather than UpdateMirror.

https://hg.mozilla.org/mozilla-central/rev/5bf85f5f74c1

The patch landed in nightly and beta is affected.
:KrisWright, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment on attachment 9182755 [details]
Bug 1671586 - Let GetPrefValue() fail when updating a mirror.

Beta/Release Uplift Approval Request

• User impact if declined: Users will crash in about:config if they enter an invalid mirrored float pref value, such as "1,5" or "foo".
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This just reverts UpdateMirror to its old behavior, so we don't crash and we retain the old mirrored value.
• String changes made/needed:

Comment on attachment 9182755 [details]
Bug 1671586 - Let GetPrefValue() fail when updating a mirror.

looks like this is a diagnostic assert (thus won't crash release builds) and we don't see many crashes in the wild, so I think this can ride to 85.

Verified that the crash is no more reproducible on Firefox 85.0b2 and 86.0a1. Tests were made on macOS 10.15.7, Ubuntu 20.04 and Windows 10.