193031 - AddCategoryEntry ignores its aPersist parameter

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

Calling AddCategoryEntry will always persists the entry in compreg.dat, no
matter if aPersist is true or false.

http://lxr.mozilla.org/seamonkey/source/xpcom/components/nsCategoryManager.cpp#208
there is a single occurence of aPersist in this file, and that's in the argument
list

This was seen on linux, but the code shows that it occurs anywhere.

Comment 1

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

oh, and if I wasn't clear... this is of course nsICategoryManager::AddCategoryEntry.

Comment 2

[Doug Turner (:dougt)]

known issue - patches accepted. :-)

the workaround is to delete the entry before shutdown.

Comment 3

[Benjamin Smedberg]

So, what's the proper behavior for the following scenario:

Someone calls AddCategoryEntry("Category","Entry","Value1", PERSIST, REPLACE);

Someone else calls AddCategoryEntry("Category","Entry","Value2",NOPERSIST,REPLACE);

Should anything persist?

Comment 4

[Benjamin Smedberg]

OK, looking further at this:
nsComponentManagerImpl::WriteCategoryManagerToRegistry is what writes the
persistent registry, and it has no knowledge of the internals of
nsICategoryManager. Since nsICategoryManager is frozen, there is no way for
nsComponentManagerImpl::WriteCategoryManagerToRegistry to do its job. There are
two options to solve this:

1) make nsComponentManager aware of the internals of nsCategoryManager, and not
use XPCOM interfaces to do WriteCategoryManagerToRegistry. I prefer this,
because these are both XPCOM services that should never need to be "unlinked"
from eachother.

2) create a new interface that extends nsICategoryManager, with a method
EnumeratePersistentEntries or something similar.

Dougt: pick one

Comment 5

[Doug Turner (:dougt)]

1, of course. :-)

Comment 6

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

bsmedberg: as for comment 3... I think I would expect Value1 to be persisted.

however, whichever way you decide, please document it in nsICategoryManager.idl.

Comment 7

[Doug Turner (:dougt)]

Right,  I too think persisting Value1 is the right thing.

Comment 8

[Benjamin Smedberg]

OK, I think I have a design that works. nsComponentManagerImpl holds an
nsRefPtr<nsCategoryManager> and calls a pseudo-private method on
nsCategoryManager to write the persistent category entries. Each "entry" stores
two values: a persisting value and a non-persisting value. These are normally
the same, but in the case of comment #3 they would be different.

nsCategoryManager is now completely thread-safe. In addition, the enumerators
no longer make copies of the arena strings (they use nsDependentCString instead)

At this point the patch compiles and works (at least on win32 and linux) but I
haven't done any of strenuous testing, TS analysis, or codesighs numbers. Those
shall come.

I'm sorry this patch is going to be so ugly... there are systemic changes in
nsCategoryManager.cpp that "diff" just doesn't handle cleanly. 

Please note that there are comment changes in the FROZEN interface
nsICategoryManager, but they should just be clarifications/corrections, not
substantive changes.

Comment 9

[Benjamin Smedberg]

Attachment: make nsCategoryManager threadsafe, with more efficient hashtables and obey the aPersist directive

Oops, I just noticed a spelling error in nsICategoryManager.idl... that will be
fixed.

Comment 10

[Doug Turner (:dougt)]

Comment on attachment 123570 [details] [diff] [review]
make nsCategoryManager threadsafe, with more efficient hashtables and obey the aPersist directive

add this to my queue.

Comment 11

[Alec Flett]

hey, hopefully this isn't too late but did you notice that the "new"
nsIStringEnumerator stuff allows QI'ing back to a nsISimpleEnumerator? One of
the main reasons I did this was so that people could use the frozen
nsICategoryManager but query the results of enumerateCategory() into an
nsIStringEnumerator and avoid creation of nsISupportsCString and such.

Basically I like what you've done (haven't reviewed yet, but I scanned the file
for enumerator stuff) but it would be really cool if your enumerator could be
similarly QI'ed (or if you could add support for your non-copying string
enumerator to nsSimpleEnumerator...

My guess is supporting nsIUTF8StringEnumerator would probably be the easiest
thing (just adding a method or two) but it would be awesome to improve upon
nsStringEnumerator

Comment 12

[Benjamin Smedberg]

Attachment: Updated using nsIUTF8StringEnumerator

OK, a few updates...

Updated the enumerators to use nsIUTF8StringEnumerator per alecf's request... I
was duplicating code in the two enumerators, so I combined the *Enumerator impl
in a base class.

(this patch also includes the IsInitialized() change for bug 206254 because I
am lazy)

Comment 13

[Alec Flett]

I've only given this a quick perusal (but more than my last comment) and so far
this looks really great!

Comment 14

[Doug Turner (:dougt)]

Comment on attachment 123688 [details] [diff] [review]
Updated using nsIUTF8StringEnumerator

We don't use reader/writer lock too much in xpcom.  maybe we should.  have you
seen any benefit?  (at one point there 

weren't NPL/MPL.  I guess they are now.  We should investigate using this for
the component manager if there is any 

benefit.

Can we move nsCharHashKey into nsHashKeys.h?  seams simple enough such that
others use it?	Same goes for 

SupportsDependentCString and BaseStringEnumerator.  These classes seam like
they belong somewhere else?  xpcom/ds 

alec?

Nit: In nsCategoryManager.cpp, Cap the T in "the":

+  the leaf strings are allocated in an arena, because we assume they're not

In CategoryNode::AddLeaf, don't you want to be returning if you find that
ArenaStrDup fails instead of continuing?  I 

guess, a failure will result in another ArenaStrdup so I can't be concerned
with that.

+    const char* arenaEntryName = ArenaStrdup(aEntryName, aArena);
+    if (!arenaEntryName) {
+      rv = NS_ERROR_OUT_OF_MEMORY;
+    } else {


why this change?

-    nsCOMPtr<nsICategoryManager>  mCategoryManager;
+    nsRefPtr<nsCategoryManager>  mCategoryManager;


nit: please use consistent comments in nsCategoryManager.h.


Shoudln't the case be PR_fprintf(...) == (PRUint32)-1:

-    if (!PR_fprintf(fd, "\n[HEADER]\nVersion,%d,%d\n",
-		     PERSISTENT_REGISTRY_VERSION_MAJOR,
-		     PERSISTENT_REGISTRY_VERSION_MINOR))
+    if ((PRInt32) PR_fprintf(fd, "\n[HEADER]\nVersion,%d,%d\n",

spelling error in nsICategoryManager.idl 
+     * @param aPersist Delete persisten data from registry, if present?

In CategoryNode::GetLeaf, why do we only want the non persistent value?

Comment 15

[Benjamin Smedberg]

> We don't use reader/writer lock too much in xpcom.  maybe we should.  have you
> seen any benefit?  (at one point there 

Well, there currently is no benefit, because nobody is using the category
manager in a threadsafe way. But in a theoretical world, the manager gets "read"
frequently, but "written" only occasionally, so a RWLock prevents unncessary
deadlock. Do you have stats on how often the categoryManager is called from
multiple threads? 

> Can we move nsCharHashKey into nsHashKeys.h?  seams simple enough such
> that others use it? Same goes for SupportsDependentCString and
> BaseStringEnumerator.  These classes seam like they belong somewhere else?  

I am willing to move nsCharHashKey and SupportsDependentCString if you think
others could use them... because they rely on arena-allocated (or static)
strings they are pretty special-case; there is great potential for misuse unless
I write some sort of really scary warning.

> why this change?
> -    nsCOMPtr<nsICategoryManager>  mCategoryManager;
> +    nsRefPtr<nsCategoryManager>  mCategoryManager;

I have been told not to use nsCOMPtr on implementation classes... that's what
nsRefPtr is for. It will work, however, because nsCategoryManager
singly-inherits nsISupports.

> Shoudln't the case be PR_fprintf(...) == (PRUint32)-1:
I guess so, to match prprf.h

> In CategoryNode::GetLeaf, why do we only want the non persistent value?
The following case:
* Persistent Value1 added (become persistent and non-persistent)
* Non-persistent Value2 added
* DeleteCategoryEntry(non-persistent) is called...

at this point, we should not have any entry in the dynamic runtime, but we
should still persist Value1 (at least according to how I'm reading
nsICategoryManager.idl, though it's a bit vague).

Comment 16

[Doug Turner (:dougt)]

no one really calls the category manager on seperate threads.  However, this
really isn't the case for the component manager.  

nsCOMPtr and concrete classes:

http://groups.google.com/groups?q=scc+nsCOMPtr+concrete+xpcom&hl=en&lr=&ie=UTF-8&oe=UTF-8&selm=scc-3E1526.12182423042001%40h-204-29-187-152.netscape.com&rnum=1

I agree with your understanding of the nsICategoryManager interface. 


So, lets decide where the string and hash key implementations go.  Alec, do you
have any perference -- do you think that these should just remain where they are?

Comment 17

[Alec Flett]

personally, I lean towards making them more generic, but I'm not sure how easily
that can be done since we're depending on a specific implementation of
nsTHashtable<>.. so I'm fine where they are unless bsmedberg can figure out a
way of making them more generic, and then putting them into nsStringEnumerator.h

Comment 18

[Benjamin Smedberg]

Alec: for the enumerator-classes, I'm going to make them more generic as I find
other callers and can discern patterns. For the moment, though, I'd prefer to
leave them here.

nsCharHashKey and SupportsDependentCString are the more troubling question.

Comment 19

[Alec Flett]

oh, heh. I'd love to see SupportsDependentCString moved into
nsSupportsPrimitives - since you're not creating a new datatype, just creating a
new implementation behind nsISupportsCString, I'd say just add it directly to
nsSupportsPrimitives.h

Comment 20

[Alec Flett]

sorry, and nsCharHashKey, I don't know enough about - sounds like something to
be moved somewhere more generic! I'd support it.

Comment 21

[Benjamin Smedberg]

Attachment: Updated w/ nits

Updated with changes and relocations.

I removed a NS_NewISupportsPRBool function that was completely unused from the
SupportsPrimitives file, while I was in there.

Comment 22

[Benjamin Smedberg]

Comment on attachment 123841 [details] [diff] [review]
Updated w/ nits

alecf, could I get a r= from you on this also, espcially the changes in ds/

Comment 23

[Benjamin Smedberg]

This can't go in until bug 206726 is resolved.

Comment 24

[Alec Flett]

hmm. I have mixed feelings about removing NS_NewISupportsPRBool()
we already provide a default implementation, and the interfaces stuff are
frozen... I don't think we ought to be taking this function away.

Comment 25

[Doug Turner (:dougt)]

there are no callers and the function isn't frozen.  Why keep it? 

Comment 26

[Benjamin Smedberg]

I don't have a machine that's "quiet" enough to run TS tests on this patch. Is
there someone (@netscape, perhaps?) who has a machine setup for TS tests? I
believe that it will speed up TS, but I haven't actually measured to ensure that
PR_RWLock is efficient.

Comment 27

[Doug Turner (:dougt)]

i would prefer a normal lock as we don't have not much concurrent access to the
category manager.

Comment 28

[Benjamin Smedberg]

Attachment: same as above, using PRLock instead of PRRWlock

OK, PRLock it is.

Comment 29

[Doug Turner (:dougt)]

Comment on attachment 124576 [details] [diff] [review]
same as above, using PRLock instead of PRRWlock

nsBaseHashtable is still using reader/writer locks.  I would like to see data
on why they matter.  They do take up two regular prlocks.

fix that up, and i will mark it r=.

Comment 30

[Alec Flett]

Comment on attachment 124576 [details] [diff] [review]
same as above, using PRLock instead of PRRWlock

sr=alecf on everything except the removal of NS_ISupportsPRBool - I'm sorry but
I don't think we should be getting rid of this - we're providing a frozen
nsISupportsPRBool and this takes away the means to create it! lets not debate
it here and let it hold up this patch - file a new bug if its really that much
of an issue.

You might also add in the comments that the result of
enumerateCategory/enumerateCategories can also be QI'ed to an
nsIUTF8StringEnumerator.

Comment 31

[Doug Turner (:dougt)]

> we're providing a frozen nsISupportsPRBool and this takes away the means to
create it! 

you can (and should) create this via the component manager.  

Comment 32

[Alec Flett]

fine but lets file another bug. lets not piggyback this here.

Comment 33

[Doug Turner (:dougt)]

Comment on attachment 124576 [details] [diff] [review]
same as above, using PRLock instead of PRRWlock

what alec said.  bsmedbert, remove that change and you can check this into the
trunk.

Comment 34

[Benjamin Smedberg]

I cannot check this in until bug 206726 is resolved...

Comment 35

[Alec Flett]

nice! looks like this gave us a little Ts improvement across the board

Comment 36

[Benjamin Smedberg]

fixed-on-trunk. This cost a bit more in codesize than I expected... but it
wasn't the templates, it was mainly the enumerators... looking to see if I can
cut anything more there.