BrowserSetting prefs are not cleared after restarting Firefox in safe mode
Categories
(WebExtensions :: General, defect)
Tracking
(Not tracked)
People
(Reporter: adw, Unassigned)
References
(Blocks 1 open bug)
Details
If you have an extension that sets a pref via a BrowserSetting and then restart Firefox in safe mode, the pref retains the value set by the extension. Safe mode disables the extension, so ideally the pref would be cleared the same way it would be if the extension were disabled normally while Firefox is running.
I haven't debugged and don't know how hard it would be to fix this. I don't know whether there are any notifications/callbacks when extensions are being or have been disabled due to safe mode.
|
Reporter | |
Comment 1•5 years ago
|
||
And if you disable or uninstall the extension while in safe mode, the pref remains set. Not great, especially for add-on experiments, where the pref may be flipped by the add-on and recognized in the product. The pref should absolutely be cleared after the extension is disabled/uninstalled. (The "..." button on the extension in about:addons lets you disable it in safe mode even though it's already disabled due to safe mode. After doing that, the extension remains disabled after restarting Firefox in normal mode.)
|
||
Comment 2•5 years ago
|
||
My irc response:
6:41 PM it's not about browsersetting
6:41 PM it's the underlying ExtensionSettingsStore/ExtensionPreferencesManager code
6:42 PM to do this we'd have to load that very early in startup
6:43 PM and that will have an impact on every startup
6:43 PM it would be much easier, and possibly much better, to not load user prefs when starting in safe mode
6:43 PM and the startup impact would be negligible.
And if you disable or uninstall the extension while in safe mode, the pref remains set.
I believe any user set pref remains.
The pref should absolutely be cleared after the extension is disabled/uninstalled.
Safe mode is not a normal disable process.
The "..." button on the extension in about:addons lets you disable it in safe mode even though it's already disabled due to safe mode. After doing that, the extension remains disabled after restarting Firefox in normal mode.
There's a reason for that, to disable misbehaving extensions that you may not have been able to otherwise disable (in normal mode).
|
||
Comment 3•5 years ago
|
||
This seems to me like an addon manager bug -- any extension APIs that rely on onDisable won't get that signal in safe mode though they really should. The ideal fix would be to call addStartupChange() for each active addon during startup when switching in or out of safe mode (that is, starting in safe mode when the previous session was not in safe mode or vice versa). The tricky bit is that I don't believe there is (currently) a way to know whether the previous session in a given profile was in safe mode or not. If there's way to know this (or if something is added), the startup bits described above would be pretty simple and would address this bug, as well as bug 1578513 presumably.
|
|
||
Comment 4•5 years ago
|
||
Extensions are not disabled in safe mode, they are just not loaded. You can then go into about:addons and disable/uninstall extensions, but settings remain (bug 1578513).
If we were to disable settings in safe mode, we'd have to have startup code that will re-enable them, and it would need to happen very early in startup (e.g. proxy.settings must be early).
Safe mode is not a normal case, and any user-set pref will also remain set, so I'm inclined to wontfix this, and focus on bug 1578513 which is more problematic.
IMO a better safe-mode fix might be to no load user set prefs at all, but then we'd likely be unable to remove them (ie. the fix for bug 1578513)
|
|
||
Comment 5•5 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #4)
Extensions are not disabled in safe mode, they are just not loaded. You can then go into about:addons and disable/uninstall extensions, but settings remain (bug 1578513).
I think its worth making a distinction between how it would ideally work and how it happens to work right now. I agree that the current behavior is not great, but what I outlined in comment 3 is a practical way to get to the most desirable behavior (ie safe mode actually disables any user-installed addons). Comment 4 seems to claim that either the current behavior is intentional (which I don't think is true) or that changing the current behavior would be too difficult (which I also don't think is true)
|
|
||
Comment 6•5 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #5)
(In reply to Shane Caraveo (:mixedpuppy) from comment #4)
Extensions are not disabled in safe mode, they are just not loaded. You can then go into about:addons and disable/uninstall extensions, but settings remain (bug 1578513).
I think its worth making a distinction between how it would ideally work and how it happens to work right now. I agree that the current behavior is not great, but what I outlined in comment 3 is a practical way to get to the most desirable behavior (ie safe mode actually disables any user-installed addons).
Given my comments below (regarding newtab/etc), I'm not certain it produces the most desirable behavior. I'm not saying it isn't, but that I think this requires more thought on the side effects. Getting the disable/enable/uninstall signals working when in safe mode avoids having to worry about other side effects, even though it means we leave the user-set pref issue as it is.
Comment 4 seems to claim that either the current behavior is intentional (which I don't think is true)
I think it was intentional. It might not be necessary now with all the new stuff like webextensions, refresh firefox, etc., but safe mode provided (probably in addition to other things) a way to get into about:addons and disable an extension that was causing serious problems.
or that changing the current behavior would be too difficult (which I also don't think is true)
I think it's a larger project (than just fixing bug 1578513) unless we don't care about prefs on startup. Actually disabling extensions would undo newtab, homepage, search defaults, proxy settings, probably other items, that are all used earlier than extension startup.
I'm sure there's a way forward with that, but I don't think it's something that can be quickly addressed.
|
|
||
Comment 7•5 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
I think it's a larger project (than just fixing bug 1578513) unless we don't care about prefs on startup. Actually disabling extensions would undo newtab, homepage, search defaults, proxy settings, probably other items, that are all used earlier than extension startup.
I'm sure there's a way forward with that, but I don't think it's something that can be quickly addressed.
What exactly do you mean by "extension startup"? When a background page starts running would certainly be too late, but we're talking about this
https://searchfox.org/mozilla-central/rev/9bb55ae4d808fc48afcf93f99da6a685265b86c6/toolkit/components/extensions/ExtensionParent.jsm#134-140
which happens quite early in startup. It does kick off async work (loading APIs and calling their onDisable methods) though it should probably wait for that work to finish (that sort of waiting is undesirable in general but the presence of startup changes should be infrequent). That code should probably also be called explicitly from AddonManager.startup() rather than implicitly when ExtensionParent.jsm happens to be loaded, but the main point is, I think actually disabling everything in safe mode is both desirable and achievable. If you really feel differently, okay, I just want to make sure we're working from the same understanding of the tradeoffs.
|
|
||
Comment 8•5 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #7)
(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
I think it's a larger project (than just fixing bug 1578513) unless we don't care about prefs on startup. Actually disabling extensions would undo newtab, homepage, search defaults, proxy settings, probably other items, that are all used earlier than extension startup.
I'm sure there's a way forward with that, but I don't think it's something that can be quickly addressed.
What exactly do you mean by "extension startup"? When a background page starts running would certainly be too late, but we're talking about this
https://searchfox.org/mozilla-central/rev/9bb55ae4d808fc48afcf93f99da6a685265b86c6/toolkit/components/extensions/ExtensionParent.jsm#134-140
Which is kind of dead code, it is accessed before startup changes has been added to.
If you really feel differently, okay, I just want to make sure we're working from the same understanding of the tradeoffs.
I don't understand what you think the tradeoffs are. I do think you have an opinion about what the desirable behavior is, which is fine, I don't have a big opinion on that right now outside the scope of work.
Regardless whether we did that approach, bug 1578513 is still necessary first for that to even work (i.e. real disabling in safe mode resulting in settings also being disabled). Given that, I feel your proposal is a larger change. Note that I also want to uplift the fixes so we can get the settings problem cleaned up.
|
|
||
Comment 9•5 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #8)
What exactly do you mean by "extension startup"? When a background page starts running would certainly be too late, but we're talking about this
https://searchfox.org/mozilla-central/rev/9bb55ae4d808fc48afcf93f99da6a685265b86c6/toolkit/components/extensions/ExtensionParent.jsm#134-140Which is kind of dead code, it is accessed before startup changes has been added to.
Can you elaborate? addStartupChange() is called from (descendants of) checkForChanges(). That is called quite early in startup, ExtensionParent.jsm should not be imported before then. (Regardless, as I said in comment 7 I think that code should be moved to a function that is called explicitly to ensure that it happens at the right time rather than relying on exactly when ExtensionParent.jsm is loaded)
I don't understand what you think the tradeoffs are. I do think you have an opinion about what the desirable behavior is, which is fine, I don't have a big opinion on that right now outside the scope of work.
Regardless whether we did that approach, bug 1578513 is still necessary first for that to even work (i.e. real disabling in safe mode resulting in settings also being disabled). Given that, I feel your proposal is a larger change. Note that I also want to uplift the fixes so we can get the settings problem cleaned up.
It looks to me like fixing this bug would also fix bug 1578513 and would do it in a much more direct way -- bug 1578513 is about the effective disable that happens when you enter safe mode not being handled properly. You could fix it with ad hoc code, but it would be much better to just handle that effective disable correctly.
Description
•