Last Comment Bug 487059 - clearUserPref shouldn't throw if a pref doesn't have a user value
: clearUserPref shouldn't throw if a pref doesn't have a user value
Status: RESOLVED FIXED
: dev-doc-complete, relnote
Product: Core
Classification: Components
Component: Preferences: Backend (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla6
Assigned To: Dão Gottwald [:dao]
:
: Benjamin Smedberg [:bsmedberg]
Mentors:
Depends on:
Blocks: 485088 524995
  Show dependency treegraph
 
Reported: 2009-04-06 10:19 PDT by Dão Gottwald [:dao]
Modified: 2011-06-13 11:23 PDT (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
patch (21.64 KB, patch)
2009-04-06 10:19 PDT, Dão Gottwald [:dao]
benjamin: review-
Details | Diff | Splinter Review
never throw (21.55 KB, patch)
2009-04-09 12:56 PDT, Dão Gottwald [:dao]
benjamin: review+
benjamin: superreview+
Details | Diff | Splinter Review

Description Dão Gottwald [:dao] 2009-04-06 10:19:49 PDT
Created attachment 371260 [details] [diff] [review]
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 [:bsmedberg] 2009-04-09 11:26:44 PDT
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] 2009-04-09 12:56:35 PDT
Created attachment 371910 [details] [diff] [review]
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] 2009-04-10 01:30:52 PDT
http://hg.mozilla.org/mozilla-central/rev/00e6498d0743
Comment 4 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2009-05-26 13:44:13 PDT
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 [:bsmedberg] 2009-05-26 13:47:22 PDT
Nobody asked for it to land there. I do not think it should.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-05-26 13:48:25 PDT
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) 2009-06-01 15:33:08 PDT
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] 2009-10-27 15:13:32 PDT
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] 2009-10-28 07:59:50 PDT
(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] 2009-10-28 08:02:02 PDT
Also, that sentence was added after this patch landed...
Comment 11 Mike Beltzner [:beltzner, not reading bugmail] 2009-10-28 09:34:49 PDT
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 2009-10-28 09:57:50 PDT
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) 2009-10-28 10:54:52 PDT
We should do any work on bug 524995. Lets move it over there.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2009-10-28 20:45:11 PDT
> 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] 2009-10-28 23:59:02 PDT
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 2009-10-29 13:34:03 PDT
On the relbranch:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3b49c063bb42
Comment 17 Henrik Skupin (:whimboo) 2009-10-29 13:42:13 PDT
(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 2009-10-29 15:55:06 PDT
(In reply to comment #17)
> Wrong bug Shawn.
Good catch.
Comment 19 Dão Gottwald [:dao] 2011-04-17 16:02:28 PDT
http://hg.mozilla.org/mozilla-central/rev/56d482ecbbc6
Comment 20 Shawn Wilsher :sdwilsh 2011-04-17 16:10:33 PDT
(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] 2011-04-17 16:16:47 PDT
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] 2011-04-17 19:09:08 PDT
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] 2011-04-17 23:01:53 PDT
(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) 2011-04-26 10:01:31 PDT
(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] 2011-04-26 10:30:37 PDT
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] 2011-06-13 11:23:10 PDT
Documentation updated:

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

And mentioned on Firefox 6 for developers.

Note You need to log in before you can comment on or make changes to this bug.