246085 - nsCategoryManager should notify observers when items are added/removed from categories

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[timeless]

Comment 1

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: notify observers when categories change

Comment 2

[Darin Fisher]

who will use this patch?  there isn't much explanation here about why this is a
good idea.  what problem are you trying to solve with this?  just looking for a
little justification ;)

Comment 3

[Christian Persch (GNOME) (away; not receiving bug mail)]

I'm trying to solve the problem from bug 246092: nsContentPolicy gets the list
of content policies from the 'content-policy' category at startup. That makes it
impossible to load (and unload again) new content policies whithout a restart. I
need this for adding a content policy from an extension (in Epiphany, where
extensions can be loaded/unloaded anytime without shutting down the browser).

Comment 4

[Darin Fisher]

I see.  Thanks for the details!

Comment 5

[timeless]

Comment on attachment 171372 [details] [diff] [review]
notify observers when categories change

>Index: xpcom/components/nsCategoryManager.cpp

this is adding an encoding marker, but it'll confuse other editors, please
remove it from your patches before you attach them (and from the files before
you commit, when you get cvs access):

>@@ -1,11 +1,11 @@
>-/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+ /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>@@ -465,20 +468,25 @@ nsCategoryManager::Create()
>   manager->mLock = PR_NewLock();
> 
>   if (!manager->mLock) {
>     delete manager;
>     return nsnull;
>   }
> 
>   return manager;
> }
> 

don't do this:
>+nsCategoryManager::nsCategoryManager()
>+{
>+  mObserverService = do_GetService("@mozilla.org/observer-service;1");
>+}
instead, put the code into
nsCategoryManager::Create(), and decide whether we should handle failure. if
you decide to fail when the observer service is unavailable, then you don't
need to null check mObserverService elsewhere: 

>@@ -487,20 +495,30 @@ nsCategoryManager::~nsCategoryManager()
>+inline void
>+nsCategoryManager::NotifyObservers( const char *aCategoryName )
>+{
>+  if (mObserverService) {
>+    mObserverService->NotifyObservers(this,
>+                                      NS_XPCOM_CATEGORY_CHANGE_OBSERVER_ID,
>+                                      NS_ConvertUTF8toUTF16(aCategoryName).get());
>+  }
>+}
>@@ -538,26 +556,31 @@ nsCategoryManager::AddCategoryEntry( con
i think we normally do nsresult rv = ...
>+  nsresult rv;
>+  rv = category->AddLeaf(aEntryName,
>+                         aValue,
>+                         aPersist,
>+                         aReplace,
>+                         _retval,
>+                         &mArena);
>+
why are you notifying observers if AddLeaf fails?
>+  NotifyObservers(aCategoryName);
>+
>+  return rv;
> }
>@@ -567,40 +590,47 @@ nsCategoryManager::DeleteCategoryEntry( 
>+  nsresult rv;
>+  rv = category->DeleteLeaf(aEntryName,
>+                            aDontPersist);
>+
same for delete...
>+  NotifyObservers(aCategoryName);
>+
>+  return rv;
> }
> 
> NS_IMETHODIMP
> nsCategoryManager::DeleteCategory( const char *aCategoryName )
> {
...
>   if (category)
>     category->Clear();
should you notify if there's no such category, or if it fails? 
>+  NotifyObservers (aCategoryName);
>+
>   return NS_OK;
> }
>Index: xpcom/components/nsCategoryManager.h
>===================================================================
>RCS file: /cvsroot/mozilla/xpcom/components/nsCategoryManager.h,v
>retrieving revision 3.3
>diff -p -u -u -p -U10 -r3.3 nsCategoryManager.h
>--- xpcom/components/nsCategoryManager.h	18 Apr 2004 14:18:12 -0000	3.3
>+++ xpcom/components/nsCategoryManager.h	15 Jan 2005 22:30:33 -0000
>@@ -37,20 +37,21 @@
> 
> 
> #ifndef NSCATEGORYMANAGER_H
> #define NSCATEGORYMANAGER_H
> 
> #include "prio.h"
> #include "prlock.h"
> #include "plarena.h"
> #include "nsClassHashtable.h"
> #include "nsICategoryManager.h"
>+#include "nsIObserverService.h"
> 
> #define NS_CATEGORYMANAGER_CLASSNAME     "Category Manager"
> 
> /* 16d222a6-1dd2-11b2-b693-f38b02c021b2 */
> #define NS_CATEGORYMANAGER_CID \
> { 0x16d222a6, 0x1dd2, 0x11b2, \
>   {0xb6, 0x93, 0xf3, 0x8b, 0x02, 0xc0, 0x21, 0xb2} }
> 
> /**
>  * a "leaf-node", managed by the nsCategoryNode hashtable.
>@@ -135,25 +136,27 @@ class nsCategoryManager
as mentioned earlier, don't do this:
>-  nsCategoryManager() { }
>+  nsCategoryManager();

btw, thank you very much for working on this, it came up again this past week,
i really do want this work :)

Comment 6

[Christian Persch (GNOME) (away; not receiving bug mail)]

> don't do this:
> >+nsCategoryManager::nsCategoryManager()
> >+{
> >+  mObserverService = do_GetService("@mozilla.org/observer-service;1");
> >+}
> instead, put the code into
> nsCategoryManager::Create(), and decide whether we should handle failure. if
> you decide to fail when the observer service is unavailable, then you don't
> need to null check mObserverService elsewhere: 
Done.

Attaching updated patch...

> >@@ -538,26 +556,31 @@ nsCategoryManager::AddCategoryEntry( con
> i think we normally do nsresult rv = ...
> >+  nsresult rv;
> >+  rv = category->AddLeaf(aEntryName,
> >+                         aValue,
> >+                         aPersist,
> >+                         aReplace,
> >+                         _retval,
> >+                         &mArena);
> >+
Fixed.

> why are you notifying observers if AddLeaf fails?
Fixed.

> >@@ -567,40 +590,47 @@ nsCategoryManager::DeleteCategoryEntry( 
> >+  nsresult rv;
> >+  rv = category->DeleteLeaf(aEntryName,
> >+                            aDontPersist);
> >+
> same for delete...
Fixed.

> > NS_IMETHODIMP
> > nsCategoryManager::DeleteCategory( const char *aCategoryName )
> > {
> ...
> >   if (category)
> >     category->Clear();
> should you notify if there's no such category, or if it fails? 
Fixed.

Is it ok to add the observer topic as a #define to the nsICategoryManager.idl?
(There are similar defines in nsIServiceManager.idl.)

Comment 7

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: updated patch

Comment 8

[timeless]

Comment on attachment 171657 [details] [diff] [review]
updated patch

generally we don't put a space after NS_SUCCEEDED.

Note to self: i bet ArenaStrdup can fail and that mTable will get unhappy when
it does.

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 171657 [details] [diff] [review]
updated patch

iirc, the servcie manage owns a reference to the category manager.  Having the
category manager hold a reference to the service manager seams like it is going
to cause a cycle.  Is this the case?

nsICategoryManager is frozen.  I do not think you should added the C++ code to
it.

Otherwise, looks good.	nice feature.

Comment 10

[timeless]

fwiw you can get a weakreference to the service manager, which is probably the
thing to do.

Comment 11

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: updated patch

(In reply to comment #9)
> (From update of attachment 171657 [details] [diff] [review] [edit])
> iirc, the servcie manage owns a reference to the category manager.  Having
the
> category manager hold a reference to the service manager seams like it is
going
> to cause a cycle.  Is this the case?

The service manager owns the category manager and the observer service. The
previous patch added a ref from cat man to obs service, not from cat man to
serv man. So I don't think it would create a cycle. Anyway, I've changed the
patch to only acquire the obs service while notifying.

I've also added a way to suppress notifications, and use it while the component
manager is loading the persistent category list on startup. That saves about
300 notifications.

> nsICategoryManager is frozen.  I do not think you should added the C++ code
to
> it.

Ok, removed it. Should it be added to some non-frozen header, or should
consumers just use the literal string?

Comment 12

[Benjamin Smedberg]

Please see my patch in bug 296430 (the XPCOM one): when that lands, you can add
the observer topic #define to nsXPCOM.h (presuming that we're planning on
freezing this functionality, which seems reasonable to me).

Comment 13

[Doug Turner (:dougt)]

Comment on attachment 185556 [details] [diff] [review]
updated patch

I use the literal string, others like seeing a #define somewhere.  Just don't
put it in a frozen idl.

Also, before you land this, please use the XPCOM refcnt balancer to ensure that
you don't leak after a shutdown.

Comment 14

[Christian Persch (GNOME) (away; not receiving bug mail)]

(In reply to comment #12)
> Please see my patch in bug 296430 (the XPCOM one): when that lands, you can add
> the observer topic #define to nsXPCOM.h (presuming that we're planning on
> freezing this functionality, which seems reasonable to me).

I'll post a follow-up patch doing that after that one lands.

(In reply to comment #13)
> (From update of attachment 185556 [details] [diff] [review] [edit])
> Also, before you land this, please use the XPCOM refcnt balancer to ensure that
> you don't leak after a shutdown.
> 

I've followed the instructions from
http://www.mozilla.org/performance/refcnt-balancer.html
with
XPCOM_MEM_LOG_CLASSES=nsComponentManagerImpl,nsObserverService,nsCategoryManager
and I get the following WITH and WITHOUT my patch, from a (start firefox, load
one web page, quit) cycle: leak-finder.pl prints

0x080D1148 (1) @ <nsCategoryManager>
0x080E6C00 (1) @ <nsObserverService>

(the addresses vary, obviously). So my patch doesn't add a leak. (And the patch
from attachment.cgi 185556 doesn't hold a ref to obs service for the cat man
lifetime anyway.)

Comment 15

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: updated patch

Moved the observer topic to nsXPCOM.h now that bug 296430 has landed. Otherwise
unchanged, transferring dougt's r+.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 185662 [details] [diff] [review]
updated patch

could you document the subject and data arguments in the nsXPCOM.h comment?

Comment 17

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: updated patch

Document the observer subject & data arguments for the category-changed
observer topic.

Comment 18

[Darin Fisher]

Comment on attachment 185696 [details] [diff] [review]
updated patch

>Index: xpcom/components/nsCategoryManager.cpp

>+inline void
>+nsCategoryManager::NotifyObservers( const char *aCategoryName )
>+{
>+  if (!mSuppressNotifications) {
>+    nsCOMPtr<nsIObserverService> observerService(do_GetService("@mozilla.org/observer-service;1"));
>+    if (observerService) {
>+      observerService->NotifyObservers(this,
>+                                       NS_XPCOM_CATEGORY_CHANGED_OBSERVER_ID,
>+                                       NS_ConvertUTF8toUTF16(aCategoryName).get());
>+    }
>+  }
>+}

Does this actually get inlined?  It seems to have enough
code to warrant not being inlined IMO.	Also, please be
sure to wrap long lines to 80 chars.  I would also 
recommend returning early if mSuppressNotifications is
true.  It is good to early return to minimize indentation.

I think it would be nice to have separate notifications
for addition and removal of category entries.  How would
someone discern addtion from removal with this API?

I'd like to see this revised and my questions answered
before approving this patch.

-Darin

Comment 19

[Christian Persch (GNOME) (away; not receiving bug mail)]

(In reply to comment #18)
> Does this actually get inlined?  It seems to have enough
> code to warrant not being inlined IMO.	
I'm not sure if it's really getting inlined (not sure how to check; I did check
libxpcom_core.so with nm -D and there's no symbol for it). I'll remove the
inlining in the next patch.

> Also, please be
> sure to wrap long lines to 80 chars.
Ok.

> I would also 
> recommend returning early if mSuppressNotifications is
> true.  It is good to early return to minimize indentation.
Ok.

> I think it would be nice to have separate notifications
> for addition and removal of category entries.  How would
> someone discern addtion from removal with this API?
I was going by what I do in the patch in bug 246092: enumerate the category and
just do all the work for each entry. Maybe I was too focused on that particular
patch.
Notification occurs in 3 places: added entry to category, removed entry from
category, and category cleared. The category name is transported in the data
argument of the observer notification; to make "added/removed" notification
useful, the name of the entry that was added/removed needs also to be
transported. I could use an nsISupportsCString as subject on the notification
for that; what do you think?

Comment 20

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: separate notification for added/removed/cleared

This separates the notifications into entry added/entry removed/category
cleared. Fixed line breaking, and removed inlining of ::NotifyObservers.

Comment 21

[Darin Fisher]

Comment on attachment 186108 [details] [diff] [review]
separate notification for added/removed/cleared

It's a little odd that for some events the subject is the category manager and
for others it is a nsISupportsCString, but I understand why you had to do it
that way.

Please be consistent with your use of whitespace.  I see that this file is
already not so consistent, and that's very unfortunate.  For example, decide
whether there should be spaces between function names and opening parans, and
be consistent.

Also, I think it is a good idea to put return statements on a newline so that a
breakpoint can be added on a return statement.

Otherwise, this patch looks fine.

Comment 22

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: final patch

Changed whitespace to not use space before parens in function call, and put
returns on their own line.
Transferring r+sr, and asking for approval.

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in xpcom/build/nsXPCOM.h;
/cvsroot/mozilla/xpcom/build/nsXPCOM.h,v  <--  nsXPCOM.h
new revision: 1.18; previous revision: 1.17
done
Checking in xpcom/components/nsCategoryManager.cpp;
/cvsroot/mozilla/xpcom/components/nsCategoryManager.cpp,v  <-- 
nsCategoryManager.cpp
new revision: 1.46; previous revision: 1.45
done
Checking in xpcom/components/nsCategoryManager.h;
/cvsroot/mozilla/xpcom/components/nsCategoryManager.h,v  <--  nsCategoryManager.h
new revision: 3.4; previous revision: 3.3
done
Checking in xpcom/components/nsComponentManager.cpp;
/cvsroot/mozilla/xpcom/components/nsComponentManager.cpp,v  <-- 
nsComponentManager.cpp
new revision: 1.266; previous revision: 1.265
done

Comment 24

[Doug Turner (:dougt)]

patch caused UMR.

https://bugzilla.mozilla.org/show_bug.cgi?id=300432.

Comment 25

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: notify on main thread

As discussed in bug 246092 comment 18 ff, the notification should always occur
on the main thread. This patch implements that and adds a note about it to the
observer topic documentation in nsXPCOM.h.
Also fixes the UMR by adding the initialisation to the constructor; sorry about
having introduced that bug in the earlier patch.

Comment 26

[Darin Fisher]

Comment on attachment 189593 [details] [diff] [review]
notify on main thread

>Index: components/nsCategoryManager.cpp

>+    obsProxy->NotifyObservers (entry, aTopic,
>+                               NS_ConvertUTF8toUTF16(aCategoryName).get());
>   } else {
>+    obsProxy->NotifyObservers (this, aTopic,
>+                               NS_ConvertUTF8toUTF16(aCategoryName).get());

please kill the extra whitespace between function name and opening
parenthesis, thx!


sr=darin

Comment 27

[Christian Persch (GNOME) (away; not receiving bug mail)]

(In reply to comment #26)
> (From update of attachment 189593 [details] [diff] [review] [edit])
> >Index: components/nsCategoryManager.cpp
> 
> >+    obsProxy->NotifyObservers (entry, aTopic,
> >+                               NS_ConvertUTF8toUTF16(aCategoryName).get());
> >   } else {
> >+    obsProxy->NotifyObservers (this, aTopic,
> >+                               NS_ConvertUTF8toUTF16(aCategoryName).get());
> 
> please kill the extra whitespace between function name and opening
> parenthesis, thx!

Fixed in my tree; I'll post an updated patch when I ask for a? after getting review.

Comment 28

[Doug Turner (:dougt)]

Comment on attachment 189593 [details] [diff] [review]
notify on main thread

this should land on the trunk as soon as possible.

Comment 29

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: patch for checkin, with the whitespace changes requested by the sr

Comment 30

[Darin Fisher]

Attachment 196667 [details] [diff] checked in on trunk:

Checking in build/nsXPCOM.h;
/cvsroot/mozilla/xpcom/build/nsXPCOM.h,v  <--  nsXPCOM.h
new revision: 1.20; previous revision: 1.19
done
Checking in components/nsCategoryManager.cpp;
/cvsroot/mozilla/xpcom/components/nsCategoryManager.cpp,v  <-- 
nsCategoryManager.cpp
new revision: 1.47; previous revision: 1.46
done
Checking in components/nsCategoryManager.h;
/cvsroot/mozilla/xpcom/components/nsCategoryManager.h,v  <--  nsCategoryManager.h
new revision: 3.5; previous revision: 3.4
done

Marking this bug FIXED.

Comment 31

[Peter van der Woude [:Peter6]]

I assume that last patch will land on branch aswell ?

Comment 32

[Doug Turner (:dougt)]

request approval from the drivers of the branch you are interested.

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 196667 [details] [diff] [review]
patch for checkin, with the whitespace changes requested by the sr

this would be good on the branch, so that users can rely on the fact that they are notified on the main thread (esp. for nsCategoryCache)

Comment 34

[timeless]

Comment on attachment 196667 [details] [diff] [review]
patch for checkin, with the whitespace changes requested by the sr

MOZILLA_1_8_BRANCH:
mozilla/xpcom/components/nsCategoryManager.h 	3.4.4.1
mozilla/xpcom/components/nsCategoryManager.cpp 	1.46.4.1
mozilla/xpcom/build/nsXPCOM.h 	1.19.2.2