Closed Bug 524995 Opened 10 years ago Closed 10 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

()

Details

(Keywords: regression)

Attachments

(7 files, 2 obsolete files)

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.
Flags: blocking1.9.2?
Priority: -- → P1
Flags: blocking1.9.2? → blocking1.9.2+
We should also check any documentation on MDC.
Keywords: dev-doc-needed
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.
Depends on: 499733
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...
Attachment #408917 - Flags: review?(gavin.sharp)
Attachment #408917 - Flags: review?(gavin.sharp) → review+
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.
Attachment #408928 - Flags: review?
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.
Attached patch Revert patch of bug 487059 (obsolete) — Splinter Review
-R version of the original patch against current tip (with some bitrot fixed)
Attachment #408934 - Flags: review?(gavin.sharp)
Attachment #408928 - Flags: review? → review?(gavin.sharp)
(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 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.
Attachment #408934 - Flags: review?(gavin.sharp) → review+
Attachment #408928 - Flags: review?(gavin.sharp) → review+
(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.
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.
(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.
(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.
Attachment #408934 - Attachment is obsolete: true
Egh, correct patch this time.
Attachment #408953 - Attachment is obsolete: true
Attachment #408955 - Flags: review?(gavin.sharp)
Attachment #408955 - Flags: review?(gavin.sharp) → review+
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. :-)
Attachment #408993 - Flags: review?(gavin.sharp)
Attachment #408994 - Flags: review?(gavin.sharp)
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.
Attachment #408993 - Flags: review?(gavin.sharp) → review+
Attachment #408994 - Flags: review?(gavin.sharp) → review+
Attachment #409072 - Flags: review?
Attachment #408928 - Attachment description: Patch for EM → Patch for EM [checked in: trunk]
Comment on attachment 408917 [details] [diff] [review]
Patch 1: DownloadLastDir.jsm, gOpenLocationLastURL.jsm, private browsing test [checked in: trunk]

[checked in: trunk]
Attachment #408917 - Attachment description: Patch 1: DownloadLastDir.jsm, gOpenLocationLastURL.jsm, private browsing test → Patch 1: DownloadLastDir.jsm, gOpenLocationLastURL.jsm, private browsing test [checked in: trunk]
Comment on attachment 409072 [details] [diff] [review]
Still further EM fixes

r=beltzner, this is very straightforward
Attachment #409072 - Flags: review? → review+
All tests went green on mozilla-1.9.2, please land this on relbranch
(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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Gijs, could you suggest places to poke around in test this fix? Preferences and migration have been mentioned. Any other?
(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.
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?
(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?
(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.
You need to log in before you can comment on or make changes to this bug.