88500 - getChildList should return relative prefs

Status: VERIFIED FIXED

(Core :: Preferences: Backend, defect)

Description

[Scott MacGregor]

If you get a pref branch with a root of say:
browser.tempFile

Let's assume I have a couple children:
browser.tempFile.temp-0
browser.tempFile.temp-1

If you call GetChildList to get the list of children and then in a for loop call
GetComplexValue for each child, we fail to retrieve the preference. 

GetChildList currently returns a fully qualified pref name (NOT relative to the
root). So it returns
browser.tempFile.temp-0
and 
browser.tempFile.temp-1.

However when you call GetComplexValue passing in childNames[index] with an
interface ID of nsILocalFile, the pref's branch code re-applies the branch root
to the passed in pref name. So it ends up looking up a pref for:

browser.tempFile.temp-0.browser.tempFile.temp-0

*doh*

Either GetComplexValue shouldn't pre-pend the branch root or GetChildList
shouldn't return a pref name with the branch root pre-pended to it. 

I'm also having problems getting nsIPrefBranch::DeleteBranch("") to work. It
won't delete the pref entries from my prefs file. I stepped through the debugger
and we are properly telling the hash table to remove these entries but when we
quit they are still in my prefs file.

Comment 1

[Alec Flett]

ah, ok.. we just need to make getChildList return a relative pref.. otherwise
this is broken for all prefs (not just getComplexPref, but also getCharPref,
setIntPref, etc etc)

Comment 2

[Brian Nesse (gone)]

Yes, we definitely can not change getComplexPref. The only other possibility
would be to compare the input parameter against the stored root... and we don't
want to be doing that everytime you request a pref.

Ack... I wonder how much currently existing code this will break... LXR here I
come...

Comment 3

[Scott MacGregor]

I don't think it can break much. =) I didn't see many consumers of
nsIPrefBranch.GetCompexPref and GetchildList. Yes I agree the right fix seems to
be making getChildList return a relative pref. 

On a semi-related note, do you know why nsIPrefBranch::DeleteBranch("") wouldn't
dleete my pref branch? I see the delete enumerator properly return
PR_Remove(????) from it's function for my prefs but when the app quit ths pref
values are still there. 

Comment 4

[Brian Nesse (gone)]

It has been suggested to me by others that DeleteBranch() doesn't work. Others
say it works fine... I'm sure there is something going on there. It's on my list
of things to look into.

Comment 5

[Alec Flett]

I'll confirm that this won't break anything, because every instance (all 2 of
them) of getChildList uses the root branch:
http://lxr.mozilla.org/seamonkey/search?string=getchildlist%28

Comment 6

[Scott MacGregor]

I think I found out why DeleteBranch doesn't work. I'll file a separate bug to
track this issue. Basically, I'm calling DeleteBranch when XPCOM tells me we are
about to shutdown. I'm an observer on that topic. 

Unfortunately, before we send out this observer notification, our main routine
is hard coded to save the prefs file b4 sending out the notification:

  if (NS_SUCCEEDED(mainResult)) {
    rv = DoOnShutdown();    // DoOnShutdown saves our prefs file to disk
    NS_ASSERTION(NS_SUCCEEDED(rv), "DoOnShutdown failed");
  }

  rv = NS_ShutdownXPCOM( NULL );  

So any subsequent changes to prefs inside of ::Observe methods for the shutdown
topic are completely ignored. 

Ideally prefs should save the user prefs to disk when it actually gets destroyed
or maybe in response to a shutdown notification. Although in that later case, it
would now be a race condition between whether an observer which uses prefs gets
called b4 or after the prefs service does it's last save to disk. 

Seems like we've got a flaw in how we shut down prefs. I'm not sure what the
right answer is. 

Comment 7

[Brian Nesse (gone)]

Ok, to take this example to "the next level"... let's say instead that you:
 - create a branch with a root of "browser."
 - call GetChildList with an "aStartingAt" value of "tempFile."

Should GetChildList return "temp-0" so your GetComplexValue call becomes:
 - GetComplexValue("tempFile." + childNames[index], ...)
Or should it return "tempFile.temp-0" so you can simply say:
- GetComplexValue(childNames[index], ...)

Option 1 is probably more functionally correct, but option 2 is probably easier
to use...

Comment 8

[Scott MacGregor]

from a consumer's point of view, I'd vote for option 2. I want something I
should be able to pass directly to getComplexValue without having to look at or
append things too. Just my 2 cents.

Comment 9

[Brian Nesse (gone)]

Yeah, my inclination is to go with option 2 as well, of course my patch
currently implements option 1 :)

Comment 10

[Brian Nesse (gone)]

Attachment: Patch to strip off branch root from child list data.

Comment 11

[Alec Flett]

yeah, the intention of the branch concept was that the consumer would not have
to think about the prefix once they had obtained the appropriate branch.

Comment 12

[Alec Flett]

patch looks good, I'd just like to see the casting be a little more explicit..
I'm thinking

((char*)prefArray.ElementAt(dwIndex)) + mPrefRootLength;

just so it's clear that we're adding something to a char* string... nitpicky, I
know :)

Comment 13

[Brian Nesse (gone)]

Attachment: Patch with stronger typecasting.

Comment 14

[Alec Flett]

cool, thanks :)
sr=alecf

Comment 15

[Brian Nesse (gone)]

mscott, you want to r= this?

Comment 16

[Scott MacGregor]

r=mscott

Comment 17

[Brian Nesse (gone)]

Checked in.

Comment 18

[sairuh (rarely reading bugmail)]

rubberstamp.