65616 - pref observers do not support multiple observers per domain

Status: VERIFIED FIXED

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

Description

[Alec Flett]

The pref observer system gets really confused if you try to add multiple
observers on the same domain.
i.e. if from JS you say:

pref.addObserver("browser.toolbar.showbuttons", foo);
pref.addObserver("browser.toolbar.showbuttons", bar);

then the backend gets all confused because the mObservers hashtable throws out
it's reference to "foo" but keeps a pointer to it as a callback.

I have a fix, patch forthcoming. Looking for reviewers, super reviewers

Comment 1

[Daniel Veditz [:dveditz]]

*** Bug 62935 has been marked as a duplicate of this bug. ***

Comment 2

[Daniel Veditz [:dveditz]]

Carrying over keywords from bug 62935 where there is a little more detail. I
closed that one in favor of this because you seem hot on the trail of a fix and
it was just middle-of-the-pile for me.

Comment 3

[Alec Flett]

Attachment: fix observer mechanism

Comment 4

[Alec Flett]

so what I'm doing now is keeping two arrays, mObservers, and mObserverDomains,
which must be maintained in lock-step. mObservers keeps a refcount to each
observer, and mObserverDomains keeps the domain that they were registered with.
This is so that we can unregister all the domains in unregisterObservers()

I just realized that the patch is slightly dangerous if you have the same
observer observing two domains, and you remove one of them.

new patch forthcoming which walks the observer list, looking for an exact match

Comment 5

[Alec Flett]

Attachment: better fix - safer removeObserver()

Comment 6

[Alec Flett]

adding dveditz to CC for review, blizzard and bryner for possible super reviews

Comment 7

[Brian Ryner (not reading)]

thanks, but i'm not a super-reviewer :)

Comment 8

[timeless]

 nsPref* nsPref::gInstance = NULL;
could we make this nsnull?

+nsresult
+nsPref::unregisterObservers()
+    PRUint32 i;
+    for (i=0; i< count; i++) {
is there any reason for i to survive the for loop?

i suppose parallelism to
+NS_IMETHODIMP nsPref::RemoveObserver(const char *aDomain,
+                                     nsIObserver *aObserver)
however, if that's the case then you have two styles for function definitions:
returntype\nfunctiondef and returntype\sfunctiondef

otherwise this code looks good.

Comment 9

[Alec Flett]

the reason for the for() syntax as it is is because certain different compilers
scope for (PRInt32 i=0;...) differently

Comment 10

[Alec Flett]

got an sr from blizzard... thanks!
 fix is in.

Comment 11

[sairuh (rarely reading bugmail)]

other than checking lxr to confirm alecf's changes to nsPref.cpp, am giving this
a rubberstamp...