Closed Bug 535128 Opened 13 years ago Closed 11 years ago

Additional home page preferences not cleaned up when reducing the home page group

Categories

(SeaMonkey :: Preferences, defect)

defect
Not set
trivial

Tracking

(seamonkey2.7 fixed)

RESOLVED FIXED
seamonkey2.7
Tracking Status
seamonkey2.7 --- fixed

People

(Reporter: rsx11m.pub, Assigned: rsx11m.pub)

Details

Attachments

(1 file)

Edit > Preferences > Browser, home page settings.

> (Observation from bug 532596 comment #5, first part) Apparently removing a
> home page from the list does not reset the related browser.startup.homepage.*
> preference, even though browser.startup.homepage.count is reduced to the new
> value and HomePagePrefCleanup() should take care of resetting those prefs.
> That's done in UpdateHomePagePref() with a reference to bug 410562 workaround.

Steps to reproduce:

1. Create a home page group aaa/bbb/ccc;

2. check preference settings:
   browser.startup.homepage = aaa
   browser.startup.homepage.1 = bbb
   browser.startup.homepage.2 = ccc
   browser.startup.homepage.count = 3

3. reduce home page group to ddd;

4. check preference settings:
   browser.startup.homepage = ddd
   browser.startup.homepage.1 = bbb
   browser.startup.homepage.2 = ccc
   browser.startup.homepage.count = 1

The two additional preferences have not been reset as intended in HomePagePrefCleanup(). While this doesn't affect any operation (therefore "trivial" severity), it implies that something is quite working as planned. This part has been redesigned during bug 394522.
> it implies that something is quite working as planned.
Make that "is *not* working as planned" of course...
> function HomePagePrefCleanup()
> {
>   // remove the old user prefs values that we didn't overwrite
>   var count = GetHomePagePrefCount();
>   for (var j = count; j < gHomePagePrefPeak; ++j)
>   {
>     // clear <preference>
>     var pref = GetHomePagePref(j);
>     pref.reset();
...........^^^^^^^^
This is wrong.
We should use |valueFromPreferences| instead e.g.

>     pref.valueFromPreferences = undefined;

See: http://hg.mozilla.org/mozilla-central/annotate/edcd50167446/toolkit/content/widgets/preferences.xml#l340

rsx11m: do you want to try this out tosee if it works?
Sure, I have no clue what this does :-) but can give it a try over the weekend.
I see, this was part of bug 410562 which defers processing of the actual clearing with preferences.rootBranch.clearUserPref() on .reset(), which for some reason never seems to happen. So, if I understand this correctly, the assignment should execute the <setter> method, thus clearing the pref again immediately as desired.
> So, if I understand this correctly, the assignment should execute the <setter> method, > thus clearing the pref again immediately as desired.
You betchum, Red Ryder!
Ok, I've tried that and it works as advertised, now resetting any additional browser.startup.homepage.* prefs which are no longer needed. I've tested various scenarios with adding and removing prefs (including "clear all") and they all worked fine without any visible issues in functionality.

What I observed though - with or without this patch - are a couple of exceptions thrown when adding or removing those prefs, not in HomePagePrefCleanup() though.

When a new browser.startup.homepage.* pref is added, I see the following:

Error: Couldn't get browser.startup.homepage.x pref: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch2.getComplexValue]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://communicator/content/utilityOverlay.js :: GetLocalizedStringPref :: line 194"  data: no]
Source File: chrome://communicator/content/utilityOverlay.js
Line: 196

When the number of home pages in the group is reduced, I get another message:

Error: this.preferences is null
Source File: chrome://global/content/bindings/preferences.xml
Line: 122

Thus, the question which comes up is if the fix for bug 410562 caused some other regression here (apparently a mismatch between browser.startup.homepage.count and the actual number of prefs at some intermediate time) and if that should be fixed here if identified. The other option is to just go with this patch for now and clean up anything else that needs cleaning in the wake of bug 410562 in another follow-up bug.

What do you think?
Attachment #568840 - Flags: feedback?(philip.chee)
After reducing, then increasing the number of homepage prefs used, the following may show up in the Error Console (again, with or without the patch applied):

Error: [Exception... "'JavaScript component does not have a method named: "observe"' when calling method: [nsIObserver::observe]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: chrome://global/content/bindings/preferences.xml :: set_valueFromPreferences :: line 350"  data: no]
Source File: chrome://global/content/bindings/preferences.xml
Line: 350
Comment on attachment 568840 [details] [diff] [review]
Patch based on Phil's suggestion

> I see, this was part of bug 410562 which defers processing of the actual clearing with
> preferences.rootBranch.clearUserPref() on .reset(), which for some reason never seems to
> happen.

That's because we nuke the <preference> in the next line:
>      pref.parentNode.removeChild(pref);
So it doesn't exist when it's time to close up.

For the other errors that occur with and without your patch, you should file a new bug.

I'm giving this patch a rubber stamp because it's a trivial/obvious fix rs=me
Attachment #568840 - Flags: feedback?(philip.chee) → feedback+
Thanks, thus I assume that rs=Ratty means I can set the checkin-needed flag.  :-)

I've done some further testing on Windows 7 and noticed that the exceptions in comment #6 don't exist in SM 2.0.4, thus neither caused by bug 410562 nor by the changes made in bug bug 532596. The comment #7 error shows up in 2.0.4 already.

I'll double-check the exceptions on the Linux side and will file a new bug.
Assignee: nobody → rsx11m.pub
Keywords: checkin-needed
Exceptions filed as bug 696608.
Comment on attachment 568840 [details] [diff] [review]
Patch based on Phil's suggestion

Err, no but you get a r+ anyway. Just to formalize things.
Attachment #568840 - Flags: review+
Ok, I figured "rubber stamp" means "any further reviews waived" - sorry!
Pushed to comm-central.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Target Milestone: --- → seamonkey2.7
Thanks, that was http://hg.mozilla.org/comm-central/rev/2fb357773d13

I don't think this is needed on aurora or beta, no visible UX impact nor a stability issue, thus not nominating (unless it's wanted for 2.5/2.6).
Blocks: 702211
No longer blocks: 702211
You need to log in before you can comment on or make changes to this bug.