837023 - (simple-prefs) changing the default type of a simple-pref causes an error while loading addons

Status: RESOLVED INCOMPLETE

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Erik Vold [:erikvold] (please needinfo? me)]

Paul brought this up in the mailing list https://groups.google.com/forum/?hl=en&fromgroups=#!topic/mozilla-labs-jetpack/ivRxVwBnU2E

It looks like the error occurs because the setting must have already existed in your profile as a different type.  So say a previous version of your add-on used pref `x` with default of true, which is type boolean, then in a new version of your add-on you declare `x` to have a default of `"A"`, this is what caused the error below..

Timestamp: 12/24/12 10:53:42 PM
Error: mind-the-time: An exception occurred.
Traceback (most recent call last):
  File "resource://gre/modules/NetUtil.jsm", line 140, in null
    aCallback(pipe.inputStream, aStatusCode, aRequest);
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/net/url.js", line 42, in null
    resolve(data);
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 142, in resolve
    while (pending.length) result.then.apply(result, pending.shift())
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 36, in then
    return { then: function then(resolve) { resolve(value) } }
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 116, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 142, in resolve
    while (pending.length) result.then.apply(result, pending.shift())
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 36, in then
    return { then: function then(resolve) { resolve(value) } }
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 116, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 142, in resolve
    while (pending.length) result.then.apply(result, pending.shift())
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 36, in then
    return { then: function then(resolve) { resolve(value) } }
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 116, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 142, in resolve
    while (pending.length) result.then.apply(result, pending.shift())
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 36, in then
    return { then: function then(resolve) { resolve(value) } }
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 116, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 142, in resolve
    while (pending.length) result.then.apply(result, pending.shift())
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 36, in then
    return { then: function then(resolve) { resolve(value) } }
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 116, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 142, in resolve
    while (pending.length) result.then.apply(result, pending.shift())
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 122, in then
    else result.then(resolved, rejected)
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 36, in then
    return { then: function then(resolve) { resolve(value) } }
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 116, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 54, in effort
    try { return f(options) }
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 116, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 142, in resolve
    while (pending.length) result.then.apply(result, pending.shift())
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 36, in then
    return { then: function then(resolve) { resolve(value) } }
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 116, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 54, in effort
    try { return f(options) }
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/addon/runner.js", line 91, in onLocalizationReady
    run(options);
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/addon/runner.js", line 120, in run
    setDefaultPrefs(options.prefsURI);
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/addon/runner.js", line 58, in setDefaultPrefs
    evaluate(sandbox, prefsURI);
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/toolkit/loader.js", line 174, in evaluate
    : loadSubScript(uri, sandbox, encoding);
  File "jar:file:///Users/paul/Library/Application%20Support/Firefox/Profiles/5p425v2u.Developer/extensions/[e-mail]!/defaults/preferences/prefs.js", line 1, in null
    pref("[e-mail]", "A");
  File "resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/addon/runner.js", line 51, in null
    branch.setCharPref(key, val);
[Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.setCharPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: resource://gre/modules/XPIProvider.jsm -> jar:file:///Users/paul/Library/Application%20Support/Firefox/Profiles/5p425v2u.Developer/extensions/[e-mail]!/bootstrap.js -> resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/toolkit/loader.js -> resource://jid0-hynmqxa9zqgfjadreri4n2ahksi-at-jetpack/addon-sdk/lib/sdk/addon/runner.js :: <TOP_LEVEL> :: line 51"  data: no]

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

One possibility here is to first check if the pref exists, and that it is of the same type, then if it does exist and is not of the same type, delete the old pref, and recreate it..

Comment 2

[Erik Vold [:erikvold] (please needinfo? me)]

We should match w/e the behavior is for old school add-ons though, and I'm not sure what that is atm.

Comment 3

[Paul Morris [:pmorris]]

While deleting the old pref and recreating it (as Erik mentions in Comment 1) nicely prevents the error... it would mean that users would lose their settings.  With automatic background updates of add-ons, they would also probably lose them without knowing that they had lost them or why.  

So for better UX, add-on devs would still need to deal with converting old prefs to new ones.

(Fortunately, it's probably not that often that prefs need to change type.  I only need to do it because radio buttons weren't an option when I first added the prefs to my add-on.)

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Paul Morris from comment #3)
> While deleting the old pref and recreating it (as Erik mentions in Comment
> 1) nicely prevents the error... it would mean that users would lose their
> settings.  With automatic background updates of add-ons, they would also
> probably lose them without knowing that they had lost them or why.  
> 
> So for better UX, add-on devs would still need to deal with converting old
> prefs to new ones.

So this is currently possible, the use case at issue here is when the add-on dev has not managed their
upgrade.  This is something that should be caught by AMO reviewers, but cold slip by notice is some cases.  With the suggestion I made, it would less likely that AMO reviewers will catch the issue, which is a problem with it.

Irakli what do you think should happen here?

Comment 5

[Tomislav Jovanovic :zombie]

i don't think there is much that we could do that would work for all (or even most) cases. the addon author is in the best position to migrate the the preference by reading the user's old value, resetting (clearing) it, and storing a new one.

the best we can do to enable this is to ignore/not throw when we come up on such a case, and let the addon author deal with it using the sdk/preferences/service module as described above.


btw, note that there are two distinct branches of preferences that cause this problem, the default one (which we use to set default preferences on addon startup) and the user one (stored only when user makes a change). because of that, i believe bug 1012231 is related to this one, and we should also fix it (somehow).

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I think what we could do is at the bootstrap if it's for install / upgrade reason check if default prefs match existing prefs (if they do exist) & if they don't erase existing prefs, set back a new prefs.

Erik do you by a chance know how such conflicts are resolved with bootstrapped or old-school addons ?

Comment 7

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #6)
> I think what we could do is at the bootstrap if it's for install / upgrade
> reason check if default prefs match existing prefs (if they do exist) & if
> they don't erase existing prefs, set back a new prefs.
> 
> Erik do you by a chance know how such conflicts are resolved with
> bootstrapped or old-school addons ?

bootstrap add-ons don't handle default preferences, it has to be manually done, which is why we have our own implementation in the sdk.  I just checked old school add-ons and they do either:

1. old default value/type is replaced with new default value/type
2. if a user setting is made, then an error is thrown, the old user set value is kept, and the new default value/type is ignored.  So if the old type was a string, and the new type is an int, the new type is ignored.

This latter piece, when trying to set a new default type for an old pref that has a user setting, seems less than ideal, because it would leave the add-on in a hairy situation to deal with.

I think it would be better do wipe away the old user set value/type and replace it with the new default value/type.

This may be better implemented as a platform fix than in the sdk repo, since there currently isn't a way to delete a single pref, and one can only delete a pref branch.  So in order to delete the pref we'd have to record all of the values in the branch, delete the branch, and reset the values that we didn't want to delete.

Comment 8

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562