487059 - clearUserPref shouldn't throw if a pref doesn't have a user value

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

The current behavior isn't documented and requires others to
1) make a fragile assumption about the pref's default value,
2) check prefHasUserValue first, or
3) use try-catch, which means a useful exception will be missed if the pref doesn't exist at all.

1) and 3) are harmful; 2) is OK but not widely used, and it shouldn't be necessary either.

Comment 1

[Benjamin Smedberg]

Comment on attachment 371260 [details] [diff] [review]
patch

How is this better than never throwing at all? It seems odd to say that the presence or absence of a default pref would affect an API named "clearUserPref".

Comment 2

[Dão Gottwald [:dao]]

Attachment: never throw

Either way is fine by me. I covered only one case, because that was the one I cared about, and I thought it would be a less controversial change.

For prefs that are expected to have a default value, that exception seemed useful to me, as it means something went wrong (e.g. mistyped pref name). For prefs that don't have a default value, it's pretty much useless. But one could argue that you shouldn't unconditionally call clearUserPref in that case.

Comment 3

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/00e6498d0743

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Is there a reason this never landed on branch? I just got hit by this throwing on branch but not on trunk.

Comment 5

[Benjamin Smedberg]

Nobody asked for it to land there. I do not think it should.

Comment 6

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

Indeed, it's an API change and it came late in the cycle, so it's not really suitable for the branch.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on trunk with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090601 Minefield/3.6a1pre ID:20090601031227.

Comment 8

[Mike Kaply [:mkaply]]

The current behavior is documented:

https://developer.mozilla.org/en/Code_snippets/Preferences

Note that clearUserPref() will throw an exception unless the pref has a user value; you must test prefHasUserValue(pref) first, then clear the value with clearUserPref().

And this behavior has been around for many years.

Have you checked to verify this won't break any extensions? I know it broke mine.

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #8)
> The current behavior is documented:
> 
> https://developer.mozilla.org/en/Code_snippets/Preferences

You're citing a sentence hidden deep on a code snippets page. I wouldn't call that documentation.

Comment 10

[Dão Gottwald [:dao]]

Also, that sentence was added after this patch landed...

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

As per the discussion in dev-planning (http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/c083cf990ff2f03f#) this change should be backed out immediately on mozilla-central, mozilla-1.9.2 and the mozilla-1.9.2 beta relbranch.

Comment 12

[Arivald]

https://developer.mozilla.org/en/Code_snippets/Preferences page should be backed too, because it was modified 22:20, 27 Oct 2009.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We should do any work on bug 524995. Lets move it over there.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> The current behavior isn't documented

nsIPrefBranch.idl says:

  218 * @return Other The preference does not exist or have a user set value.

as the last sentence of the comment for clearUserPref, and has done so since revision 1.16 (October 2001).  How much more documented can you get?

Comment 15

[Dão Gottwald [:dao]]

That was half a year ago, but I think I was looking for "throws", so I was probably misreading the idl. Besides, while "The preference does not exist" is quite clear, I still find it questionable whether "have a user set value" here should mean the same as defined for prefHasUserValue, as this makes an explicitly set value matching the default value an exception case here. That's the most painful part that my first patch addressed specifically.

Comment 16

[Shawn Wilsher :sdwilsh]

On the relbranch:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3b49c063bb42

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #16)
> On the relbranch:
> http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3b49c063bb42

Wrong bug Shawn.

Comment 18

[Shawn Wilsher :sdwilsh]

(In reply to comment #17)
> Wrong bug Shawn.
Good catch.

Comment 19

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/56d482ecbbc6

Comment 20

[Shawn Wilsher :sdwilsh]

(In reply to comment #19)
> http://hg.mozilla.org/mozilla-central/rev/56d482ecbbc6
We decided to land this again?  Is anybody planning on posting to the add-on compatibility blog about this?

Comment 21

[Dão Gottwald [:dao]]

I did, as this was only backed out because it landed in a beta phase. I can communicate the change, but I don't expect significant add-on breakage.

Comment 22

[Mike Kaply [:mkaply]]

You have no way to predict add-on breakage.

This definitely needs to be communicated to the add-on team.

Comment 23

[Dão Gottwald [:dao]]

(In reply to comment #22)
> You have no way to predict add-on breakage.

I surely have. I searched through add-ons, now and in the past, and even without examining every occurrence (there are many of them), you get a good sense of how this is usually used in the wild. I know an add-on of yours was relying on the exception, but this is by no means the common case.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #23)
> I surely have. I searched through add-ons, now and in the past, and even
> without examining every occurrence (there are many of them), you get a good
> sense of how this is usually used in the wild. I know an add-on of yours was
> relying on the exception, but this is by no means the common case.

This only applies to add-ons hosted on AMO which are a fraction of all available add-ons. I will CC Jorge and Justin just to make sure we get the right message send out to all necessary channels.

Comment 25

[Jorge Villalobos [:jorgev] (he/him)]

Thank you for the ping. I'll make a note of this for the Firefox 6 upgrade cycle. I assume the milestone is correct and this won't make it to Firefox 5.

Comment 26

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIPrefBranch#clearUserPref()

And mentioned on Firefox 6 for developers.