524995 - Fix clearUserPref usage, back out no-throw change to prefbranch

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect, P1)

Description

[:Gijs (he/him)]

For discussion, see: http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/c083cf990ff2f03f#

We need to check all the callsites ( http://mxr.mozilla.org/mozilla-central/search?string=clearUserPref, and possibly other repos?), fix any that need fixing, and back out the original change.

Comment 1

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

We should also check any documentation on MDC.

Comment 2

[Nochum Sossonko [:Natch]]

Ok, bug 499733 definitely had issues with this as well.

From a precursory glance at the tree, aside for bug 487059, it seems the only two files with a possible issue would be:

1) DownloadLastDir.jsm in toolkit/mozapps/downloads/src/
2) openLocationLastURL.jsm in browser/base/content/

Cc'ing ehsan as he worked on both.

Comment 3

[:Gijs (he/him)]

Attachment: Patch 1: DownloadLastDir.jsm, gOpenLocationLastURL.jsm, private browsing test [checked in: trunk]

This is a funny bunch. I'm assuming we don't want exceptions from "innocent" setters on these things, right?

There's still the EM to do, and the revert of the original patch. That seems to be it, AFAICT...

Comment 4

[:Gijs (he/him)]

Attachment: Patch for EM [checked in: trunk]

I think these are the callsites which we *do* need to change, and I think we can get away with leaving the other ones alone.

Comment 5

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

I remember hitting a problem with this in a sessionstore test (for bug 490040) & needing to try/catch for 1.9.1 but not trunk at the time, so just make sure you run all those tests too.

Comment 6

[:Gijs (he/him)]

Attachment: Revert patch of bug 487059

-R version of the original patch against current tip (with some bitrot fixed)

Comment 7

[:Gijs (he/him)]

(In reply to comment #5)
> I remember hitting a problem with this in a sessionstore test (for bug 490040)
> & needing to try/catch for 1.9.1 but not trunk at the time, so just make sure
> you run all those tests too.

We're not doing anything to 1.9.1 at the moment, I don't think. I just sent all the attached patches to tryserver on 1.9.2, though, so if something breaks, I guess I'll see it...

As for that particular test, it sets a pref, and resets it at the end. AIUI that ought to be fine, because there's a guarantee that the pref *will* have a "user" set value (unless the earlier pref call borked, but in that case you have bigger problems). Did I miss something?

Comment 8

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

Comment on attachment 408934 [details] [diff] [review]
Revert patch of bug 487059

>diff --git a/netwerk/test/unit/test_bug396389.js b/netwerk/test/unit/test_bug396389.js

>   for each (var pref in prefData) {
>     try {
>       pref.oldVal = prefs.getBoolPref(pref.name);
>+      pref.set = true;
>     } catch(e) {
>+      pref.set = false;
>     }
>     prefs.setBoolPref(pref.name, pref.newVal);
>   }

>     for each (var pref in prefData) {
>-      prefs.clearUserPref(pref.name);
>+      if (pref.set) {
>+        prefs.setBoolPref(pref.name, pref.oldVal);
>+      } else {
>+        prefs.clearUserPref(pref.name);
>+      }
>     }
>   }

This change, and the other few like it, seems unnecessary to revert (if the test is currently fine with just clearing the pref value, that should be sufficient even if we throw-on-nonexistence.) We can make that change only on the trunk, or in a followup, though, if you want to keep this a straight backout for simplicity's sake.

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #7)
> As for that particular test, it sets a pref, and resets it at the end. AIUI
> that ought to be fine, because there's a guarantee that the pref *will* have a
> "user" set value (unless the earlier pref call borked, but in that case you
> have bigger problems). Did I miss something?

You're missing that clearUserPref used to throw if the set value happens to equal the default value.

Comment 10

[:Gijs (he/him)]

http://hg.mozilla.org/mozilla-central/rev/d0b7ef28c23a for the EM and other callsite patch. I'll attach an updated revert patch, but per Dao's comment, we do need the try catch around those calls -- and in fact, there's probably other tests in the tree which will need it, based on that.

Comment 11

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

(In reply to comment #7)
> We're not doing anything to 1.9.1 at the moment, I don't think. I just sent all
> the attached patches to tryserver on 1.9.2, though, so if something breaks, I
> guess I'll see it...

Right. The test had counted on not throwing, so I had to backport to 1.9.1 and
try/catch there.

> Did I miss something?

What Dao said.

Comment 12

[:Gijs (he/him)]

Attachment: Revert Patch with test try/catch where appropriate, as opposed to plain revert

(In reply to comment #8)
> This change, and the other few like it, seems unnecessary to revert (if the
> test is currently fine with just clearing the pref value, that should be
> sufficient even if we throw-on-nonexistence.) We can make that change only on
> the trunk, or in a followup, though, if you want to keep this a straight
> backout for simplicity's sake.

I've added a prefHasUserValue call in two of them to make sure that if they're set to user values, they don't throw. The third case sets the update URL to a test value, which I'm sure can't be the same as the default, so I think that's fine like this.

Comment 13

[:Gijs (he/him)]

Attachment: Revert patch w/ changes suggested by gavin

Egh, correct patch this time.

Comment 14

[:Gijs (he/him)]

Attachment: Patch tests that have added clearUserPref since the change

There is one case visible in the patch, and one case in toolkit/crashreporter/test/browser/browser_aboutCrashesResubmit.js that I didn't wrap up, because I could not imagine the default pref and the value it's set to by the test ever possibly being the same. :-)

Comment 15

[:Gijs (he/him)]

Attachment: Further EM fixes based on tryserver failures

Comment 16

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

Comment on attachment 408993 [details] [diff] [review]
Patch tests that have added clearUserPref since the change

>diff --git a/content/events/test/test_bug226361.xhtml b/content/events/test/test_bug226361.xhtml

> function setOrRestoreTabFocus(newValue) {

>-  if (!newValue) {
>+  if (!newValue && prefs.prefHasUserValue("tabfocus")) {
>     prefs.clearUserPref("tabfocus");
>   } else {
>     prefs.setIntPref("tabfocus", newValue);

This is a behavior change (will setIntPref("tabfocus", null) if !newValue && !prefs.prefHasUserValue, which I don't think is desired given the function name). You should just put the prefHasUserValue check inside the if().

>diff --git a/content/xul/content/test/test_bug486990.xul b/content/xul/content/test/test_bug486990.xul

>     try {
>-      if (allow) {
>+      if (allow && prefs.prefHasUserValue("enabled")) {
>         prefs.clearUserPref("enabled");
>       } else {
>         prefs.setBoolPref("enabled", allow);
>       }

ditto

r=me with those changed.

Comment 17

[:Gijs (he/him)]

Attachment: Still further EM fixes

Per http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1256806880.1256820612.23642.gz&fulltext=1 , we need this checked too. :-(

Comment 18

[:Gijs (he/him)]

Comment on attachment 408917 [details] [diff] [review]
Patch 1: DownloadLastDir.jsm, gOpenLocationLastURL.jsm, private browsing test [checked in: trunk]

[checked in: trunk]

Comment 19

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

Comment on attachment 409072 [details] [diff] [review]
Still further EM fixes

r=beltzner, this is very straightforward

Comment 20

[:Gijs (he/him)]

http://hg.mozilla.org/mozilla-central/rev/9e89c0d4e570 for the last 4 patches.

Comment 21

[:Gijs (he/him)]

Attachment: Patch that went on branch

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b015a068cf8b

Comment 22

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

All tests went green on mozilla-1.9.2, please land this on relbranch

Comment 23

[:Gijs (he/him)]

(In reply to comment #22)
> All tests went green on mozilla-1.9.2, please land this on relbranch

Shawn landed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3b49c063bb42 .

I changed these devmo pages:

https://developer.mozilla.org/index.php?title=en/nsIPrefBranch&action=diff&revision=27&diff=29

https://developer.mozilla.org/index.php?title=en/Code_snippets/Preferences&action=diff&revision=53&diff=54

I think that's it for this bug... If I missed something, please feel free to reopen.

Comment 24

[juan becerra [:juanb]]

Gijs, could you suggest places to poke around in test this fix? Preferences and migration have been mentioned. Any other?

Comment 25

[:Gijs (he/him)]

(In reply to comment #24)
> Gijs, could you suggest places to poke around in test this fix? Preferences and
> migration have been mentioned. Any other?

When I checked what code added a clearUserPref call between this stuff no longer throwing, and going back to throwing (just now :-)), everything I found was in tests, with the only exception being the add-on manager, so that'd be my final suggestion... gOpenLocationLastURL technically changed, but the only callsite for that method (reset()) is in a test, according to MXR.

Comment 26

[Shawn Wilsher :sdwilsh]

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

Comment 27

[Eric Shepherd [:sheppy]]

I don't see what needs changing in the docs here. Isn't this fix simply reverting to the same behavior that's already documented?

Comment 28

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

(In reply to comment #16)
> (From update of attachment 408993 [details] [diff] [review])
> >diff --git a/content/events/test/test_bug226361.xhtml b/content/events/test/test_bug226361.xhtml

> >diff --git a/content/xul/content/test/test_bug486990.xul b/content/xul/content/test/test_bug486990.xul

> r=me with those changed.

Looks like neither of these were actually fixed?

Comment 29

[:Gijs (he/him)]

(In reply to comment #28)
> (In reply to comment #16)
> > (From update of attachment 408993 [details] [diff] [review] [details])
> > >diff --git a/content/events/test/test_bug226361.xhtml b/content/events/test/test_bug226361.xhtml
> 
> > >diff --git a/content/xul/content/test/test_bug486990.xul b/content/xul/content/test/test_bug486990.xul
> 
> > r=me with those changed.
> 
> Looks like neither of these were actually fixed?

Ugh. Really sorry - I remember making this change, but I must have committed an outdated version of the patch. I'll check the fix into trunk when it's green, and I guess we want this on branch too, but not on relbranch?

I should note this isn't breaking right now only because all the callers of those functions pass false or 0 explicitly, and so setBool/IntPref get called with those, which also works.

(In reply to comment #27)
> I don't see what needs changing in the docs here. Isn't this fix simply
> reverting to the same behavior that's already documented?

Yes. If that behaviour is documented enough (2 MDC pages and the interface file right now, AFAICT), then IMHO nothing else is needed.

Comment 30

[:Gijs (he/him)]

http://hg.mozilla.org/mozilla-central/rev/c2f0df485f9d for those tests.

Comment 31

[:Gijs (he/him)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/444be380ca8b for branch.