985998 - Exceptions updating preferences in AddonManager and XPIProvider

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[:Irving Reid (No longer working on Firefox)]

A second review of the AddonManager exception telemetry revealed two places where a small number of profiles are getting exceptions trying to update preferences. These prefs are internal record keeping within AddonManager/XPIProvider, not user visible. Reading the code, this happens because the data stored in prefs.js is a different type than the code expects; how that data came to be the wrong type is unknown.

Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.setIntPref]" at resource://gre/modules/AddonManager.jsm :: AMI_startup :: line 559

Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.setBoolPref]" at resource://gre/modules/addons/XPIProvider.jsm :: XPI_showUpgradeUI :: line 2248

I can wrap at try/catch around these to clear the bogus old value and force the new value in, but this does raise the question of whether the current behaviour is really what we want.

bsmedberg, you're the lone peer for prefs backend - any opinion?

Comment 1

[Benjamin Smedberg]

So you're saying that once a non-int pref is set, .setIntPref throws? That's unexpected to me: it ought to just set the int pref regardless. But also this is very under-specified, so maybe post to dev.platform explaining the change and see if anyone has more context for why it is the current way.

Comment 2

[:Irving Reid (No longer working on Firefox)]

Discussed on dev-platform at https://groups.google.com/forum/#!topic/mozilla.dev.platform/lB38FhUGH-s

The preferences affected by this are "extensions.blocklist.pingCountVersion" and "extensions.shownSelectionUI". shownSelectionUI is a minor annoyance, but when "extensions.blocklist.pingCountVersion" is broken we never do a blocklist query.

(at least) three options:

1) The quick fix to these is to put a default values for them into all.js; this will cause the invalid values from prefs.js to be discarded.

2) Fix the C++ preferences API to allow our code to override the type of a preference, perhaps only for preferences where we don't have a default in greprefs.js; this solves the problem for lots of API consumers.

3) If we want to find out more about what's behind the bustage, I could add code and telemetry to retrieve and report the broken value and then reset it, or I could re-analyze the existing telemetry data to see if the broken 'pingCountVersion' field correlates to anything interesting like presence of a common add-on.

Comment 3

[:Irving Reid (No longer working on Firefox)]

Attachment: Option 1: Set default values for some Add-on Manager prefs

This takes the minimal approach of just setting a default value of the correct type, for the two prefs I've seen with issues (and one other that would also cause problems with blocklist downloads). This doesn't protect any other JS code from unexpected exceptions.

Comment 4

[:Irving Reid (No longer working on Firefox)]

Attachment: Option 2: Change prefs API so that we can overwrite the type of a user_pref

By allowing set*Pref to override the type of an existing pref that doesn't have a default value, any JS code that isn't super-careful about handling preference exceptions will stop failing.

There's a subtle drive-by fix in this change; pref_SetValue() is only called from pref_HashPref(). pref_HashPref() is setting the global 'gDirty' flag if the pref has changed *and is not locked*, but pref_SetValue() has already unconditionally set 'gDirty' to true.

Removing the 'gDirty = true;' from pref_SetValue() means that in the almost non-existent circumstances that code updates a locked preference (and no others), we won't trigger a write of prefs.js.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8398545 [details] [diff] [review]
Option 1: Set default values for some Add-on Manager prefs

Review of attachment 8398545 [details] [diff] [review]:
-----------------------------------------------------------------

Seems like this would only prevent new cases of this, not fix existing cases. This is rather worrying for these prefs, as they've never been anything but int prefs - which means something outside of our code has been messing with them. Potentially, this could be *intentionally* to break the blocklist in a non-obvious way.

Option 2 would be a far more thorough and dependable approach, and my preferred solution.

Comment 6

[:Irving Reid (No longer working on Firefox)]

(In reply to Blair McBride [:Unfocused] from comment #5)
> Comment on attachment 8398545 [details] [diff] [review]
> Option 1: Set default values for some Add-on Manager prefs
> 
> Review of attachment 8398545 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems like this would only prevent new cases of this, not fix existing
> cases.

No, the wrong user_pref() value in prefs.js is discarded at load time if its type conflicts with the default we add to greprefs.js, so this will fix existing cases (I tested this manually).

> Option 2 would be a far more thorough and dependable approach, and my
> preferred solution.

I agree; I'd hate to have to audit our code for all the other places where we don't correctly handle failures in set*Pref().

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm not a particular expert on prefs, but my 2 cents:

(In reply to :Irving Reid from comment #2)
> 1) The quick fix to these is to put a default values for them into all.js;
> this will cause the invalid values from prefs.js to be discarded.

This seems like a good thing to do to me.

> 2) Fix the C++ preferences API to allow our code to override the type of a
> preference, perhaps only for preferences where we don't have a default in
> greprefs.js; this solves the problem for lots of API consumers.

This feels like a good idea with the "only for" (but not without it).  It's generally good practice for prefs that can to have defaults, and I think we rely on getIntPref/etc. not throwing in cases where the prefs have defaults.  So letting a pref with an int default be set to a bool leaves that in a sticky situation, since there's nothing reasonable for getIntPref to do when the value is a bool.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8398552 [details] [diff] [review]
Option 2: Change prefs API so that we can overwrite the type of a user_pref

I agree with dbaron.

Comment 9

[Benjamin Smedberg]

Comment on attachment 8398552 [details] [diff] [review]
Option 2: Change prefs API so that we can overwrite the type of a user_pref

This patch removes gDirty-setting? I'm ok with the approach here although I thoroughly hate this code.

Comment 10

[:Irving Reid (No longer working on Firefox)]

Attachment: Option 2 v2: Change prefs API so that we can overwrite the type of a user_pref

I started a patch to test how the 'gDirty' flag behaves in the preference service, and came across enough subtlety that I decided to back out the slight change I made to it in this patch (in particular, we *do* write the user values for locked default preferences to prefs.js, even though we ignore them).

I'll attach the WIP for the gDirty testing and behaviour changes; if anybody likes it, it should probably be forked into its own bug.

Kyle, bsmedberg suggested you as a possible second set of eyes on this patch.

Comment 11

[:Irving Reid (No longer working on Firefox)]

Attachment: WIP: Expose the prefs service 'dirty' flag, test it, fix a few cases where it is too dirty

I exposed the 'dirty' flag to XPCOM as a readonly attribute and started writing test cases; issues I noticed are:

We currently mark 'dirty' when setting default prefs, even though they don't get written out.

We write out user prefs that try to override locked prefs, even though the values don't get used in our code - though the user value would suddenly re-appear if the default value of the pref was later unlocked (so it's reasonable for overriding a locked pref to set 'dirty').

Comment 12

[:Irving Reid (No longer working on Firefox)]

Attachment: WIP: Expose the prefs service 'dirty' flag, test it, fix a few cases where it is too dirty

Forgot to 'hg add' the test file

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8401496 [details] [diff] [review]
Option 2 v2: Change prefs API so that we can overwrite the type of a user_pref

Review of attachment 8401496 [details] [diff] [review]:
-----------------------------------------------------------------

bsmedberg needs to sign off on the API change if he hasn't already.

::: modules/libpref/src/prefapi.cpp
@@ +710,5 @@
> + * Overwrite the type and value of an existing preference. Caller must
> + * ensure that they are not changing the type of a preference that has
> + * a default value.
> + */
> +static void pref_SetValue(PrefValue* oldValue, uint16_t *oldFlags,

This needs a better value than oldFlags, because it's also the outparam for the new flags ...

It should also be of type PrefType.  The value in prefHashEntry should probably be of type PrefType too.

@@ +770,2 @@
>      {
> +        NS_WARNING(nsPrintfCString("Trying to overwrite value of pref %s with the wrong type!", key).get());

value of default pref

Comment 14

[:Irving Reid (No longer working on Firefox)]

Attachment: Option 2 v3: Change prefs API so that we can overwrite the type of a user_pref

Based on an IRC conversation with khuey, leaving the parameter type as uint_16, because the field contains an or-ed together group of enum flag values, rather than a single value of the enumerated type.

Comment 15

[Benjamin Smedberg]

Comment on attachment 8407042 [details] [diff] [review]
Option 2 v3: Change prefs API so that we can overwrite the type of a user_pref

Per IRC, Irving's going to add docs to nsIPrefBranch.idl about the behavior here.

Comment 16

[:Irving Reid (No longer working on Firefox)]

Attachment: Allow set*Pref( ) to change the type of prefs with no default value, r=bsmedberg

Updated documentation comments in nsIPrefBranch.idl to reflect new behaviour; carrying forward r+.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/9fab567879bc

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9fab567879bc