107617 - Adding an observer to a non-toplevel prefbranch doesn't work

Status: VERIFIED FIXED

(Core :: Preferences: Backend, defect)

Description

[Bradley Baetz (:bbaetz)]

In working on bug 43567, I've added a new pref and a pref listener. There 
was a question on whether I was allowed to do:

rv = prefSrv->GetBranch("network.ftp.", getter_AddRefs(branch));
...
pbi->AddObserver("idleConnectionTimeout", this, PR_TRUE);

and be notified when the network.ftp.idleConnectionTimeout pref changed. 
Alecf said yes, but looking at the nsPrefBranch.cpp code, I can't see 
where mPrefRoot is used before calling PREF_RegisterCallback.

I did some testing, and if I replace the branch with nsnull, (and observe
"network.ftp.idleConnectionTimeout" instead) then the observer is called.
Interestingly, if I use the code as above (observing the network.ftp branch),
then changing a pref called "idleConnectionTimeout" doesn't appear to fire the
observer either, so either my quick test hacking one of the pref panels wasn't
quite right, or something else is using mPrefRoot correctly, and is getting
confused.

Comment 1

[Brian Nesse (gone)]

Hmm, I'm pretty sure I had a note to myself somewhere about this exact issue...I
wonder where it went. :(

I expect that mPrefRoot will also need to be removed before calling Observe() to
eliminate confusion on the caller's part...

Comment 2

[Bradley Baetz (:bbaetz)]

My Observe method is going to be given a prefbranch as one of the params. I
should be able to expect that passing in the same string to GetFooPref as I did
to AddObserver should work, and that I get back the same pref branch I called
AddObserver on.

Its up to the nsIObserver implementation to avoid ambiguities. I suspect that
those would be rather rare, though.

Comment 3

[Brian Nesse (gone)]

Attachment: Proposed patch.

Comment 4

[Alec Flett]

well, the one concern I have here is that the OBSERVER needs to get the
prefbranch that they originally passed in, not just the root prefbranch.. is
that happening here? (I've forgotten if the pref branches have their own
observer lists or not)

Comment 5

[Brian Nesse (gone)]

The only reason this works is because the prefBranch associated with the
callback is stored in the callback structure. :)

The is, however, one gotcha I did notice. Because the prefService keeps a
reference to the default branch you can register an observer, release the
branch, and it works. If you supply a root though, you must retain a reference
to the branch, otherwise the branch is released as soon as it goes out of scope.

Comment 6

[Brian Nesse (gone)]

bbaetz, do you want to review this?

Comment 7

[Bradley Baetz (:bbaetz)]

I'm not in a position to test this now, but:

"If you supply a root though, you must retain a reference
to the branch, otherwise the branch is released as soon as it goes out of scope."
That doesn't sound intuative. Why can't you just addref the branch in the
addobserver call?

Comment 8

[Brian Nesse (gone)]

Until recently this would have been a really bad idea because it would have
practically guaranteed that no branch objects would ever been released. With the
recent shutdown obeserver changes, this might be safer. I'll have to think on it
a little...

Comment 9

[Brian Nesse (gone)]

I don't see any potential issues with doing an addref on the "top level" branch
because it will never be released anyway (because it is cached/owned by the pref
service.)

I can see where doing an addref on a non top level branch could cause bloat/leak
situations if the object doing the observing doesn't call RemoveObserver(). When
an object adds itself as an observer it is Addref()'ed. If this object is
subsequently Release()'d, it won't be deleted because the Observer queue still
has a hold on it.

The aHoldWeak parameter was added to get around this. This allows the object to
be deleted, while only retaining a stale reference in the observer list (which
is subsequently removed if a notification to the deleted object is attempted.)
If you add branch ref counting into this situation the (unused) branch will
remain along with the stale object reference. This will remain in memory until a
situation which would trigger notification happens, or until xpcom shutdown.

This might also add some complexities when removing observers as it would need
to insure that the branch deletion is not triggered while it is removing the
observer (due to the observer list having the only branch reference.)

Comment 10

[Brian Nesse (gone)]

After explaining my, apparently confusing, comments to alecf we came up with a
workable solution for this. I implemented it. It works great. But... I'm not
going to post the revised patch. Here's why...

The main purpose of the additional changes would be to allow a consumer to
create a non top level branch and not keep a reference to it. Unfortunately the
consumer might very well be doing something similar to the code I used for my
test case:
 http://lxr.mozilla.org/seamonkey/source/netwerk/cache/src/nsCacheService.cpp

In the Install() method at line 129, the prefservice is grabbed, and a branch
requested from it. The observers are added to this branch, and the branch is
released. In the Remove() method at line 177 this process is repeated to remove
the observers.

The problem is that, unless the prefservice caches the branch, 2 different (new)
branches will be returned. The RemoveObserver calls will fail because there are
no observers on the (newly created) branch.

The right answer is to have the prefservice cache the non top level branch, but
that may have additional issues associated with it.

Comment 11

[Brian Nesse (gone)]

My recommendation on this would be to check in the current patch to fix the this
bug, and to open a new bug to add caching for non top level branches to the
prefservice.

Comment 12

[Bradley Baetz (:bbaetz)]

I was under the impression that doing things using a subbranch was more
efficient, because we didn't have to look up each component in the branch all
the time.

Is this not (currently) the case?

Comment 13

[Alec Flett]

Comment on attachment 56756 [details] [diff] [review]
Proposed patch.

I agree with bnesse. sr=alecf

bbaetz: it isn't the case right now, but we designed things so that we could do
that in the future..

Comment 14

[Brian Nesse (gone)]

Attachment: A slightly better version without excessive error checking.

Comment 15

[Alec Flett]

Comment on attachment 60397 [details] [diff] [review]
A slightly better version without excessive error checking.

can't use static_cast<> - you have to use NS_STATIC_CAST()

also, seems like you're reversing the value of "domain" - I thought domain was
the prefix of a pref, not the suffix?

Comment 16

[Brian Nesse (gone)]

Attachment: Patch addressing alecf's comments

Changed static_cast to NS_STATIC_CAST. Changed variable name from domain to
suffix. :)

Comment 17

[Alec Flett]

Comment on attachment 60405 [details] [diff] [review]
Patch addressing alecf's comments

thanks
sr=alecf

Comment 18

[Conrad Carlen (not reading bugmail)]

Comment on attachment 60405 [details] [diff] [review]
Patch addressing alecf's comments

r=ccarlen

Comment 19

[Brian Nesse (gone)]

Created bug 113635 to address the caching of non-top level branches.

Comment 20

[Brian Nesse (gone)]

Fix checked in.

Comment 21

[sairuh (rarely reading bugmail)]

rs verify.