204870 - XPCOM prematurely removes factory entries from compreg.dat

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[Doug Turner (:dougt)]

When overriding a component, you want to be able to register your nsIFactory
object and not corrupt the compreg.dat file.  Today, if you call
RegisterFactory() with a contractID/CID that was registered by
RegisterFactoryLocation, that entry will not be written out to disk.  

A classic example of this is the requirement to replace the prompt service. 
During auto-registration, the default prompt service is recorded.  When an
embedding application starts up, it may want to replace this service with its
own.  The recommended way to do this is to call RegisterFactory() passing the
same CID and ContractID as the default service.  If the embedding application
does this, the next time another application starts up using the same components
registry, they will not have access to either prompt service.  That is, the
default prompt service information will no longer be recorded in the compreg.dat.

Comment 1

[Christopher Blizzard (:blizzard)]

You said you had a patch?

Comment 2

[Doug Turner (:dougt)]

Attachment: patch v.1

this doesn't fix that fact that Unregister* is destructive.  So, if you call
UnregisterFactory(cid, factory), you will delete the CID and associated
contract id's in the component registery.  This may be a seperate bug.

Comment 3

[Christopher Blizzard (:blizzard)]

Is this clean?  I can't get a sense from looking at the patch.

Do we need a seperate way to register factories that aren't to be dropped in
compreg.dat?

Comment 4

[Doug Turner (:dougt)]

there is no need to create new methods.  We just have to properly implement the
ones we have.  

Comment 5

[Christopher Blizzard (:blizzard)]

Do you need this reviewed by one of the XPCOM folks, then?

Comment 6

[Darin Fisher]

Comment on attachment 122766 [details] [diff] [review]
patch v.1

>Index: nsComponentManager.cpp

>+    // When clients override an existing component, we can no clobber information 
>+    // about how the origanal factory was registered.  If an object was registered
>+    // by passing an nsIFactory object, we want to make sure that we don't remove the 
>+    // origanal factory entry.  That is, registering with a nsIFactory object should
>+    // not modify the component registry. We accomplish perserving the origanal 
>+    // factory entry by skipping factory entries that have a loader type of 
>+    // NS_COMPONENT_TYPE_FACTORY_ONLY (-1) and that have a factory entry as null.
>+    // Note: This state can only be reached if the factory a factory is explictly
>+    // unregistered or if we load a component registry and discover that a factory 
>+    // entry has no assocated loader.

typos in the comment...

s/origanal/original/

 "we can no clobber information..."
 "if the factory a factory is explicitly..."

still reviewing the code itself...

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 122766 [details] [diff] [review]
patch v.1

darin and I spoke.  we came up with a better solution.

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 122766 [details] [diff] [review]
patch v.1

darin and I spoke.  we came up with a better solution.

Comment 9

[Doug Turner (:dougt)]

Attachment: patch v.2

I am stacking the nsFactoryEntries and only writting out the first entry.  
Some member variable name clean up noise included.

Current problem (that exist with or without this patch)
 Unregister* still has problems but not are much easier to solve.
 There is no history for contractid <-> cid mapping.

Comment 10

[Doug Turner (:dougt)]

Attachment: patch v.3

This avoids two hashtable hits per RegistryFactory.  (thanks to brendan for
setting me straight).

Comment 11

[Darin Fisher]

Comment on attachment 122879 [details] [diff] [review]
patch v.3

>Index: nsComponentManager.cpp

>+	// nsFactoryEntry is arena allocated. So we dont delete it.
>+    // We call the destructor by hand.
>+    if (mParent)
>+		mParent->~nsFactoryEntry();

nit: tabs


>+/*
> nsresult
> nsFactoryEntry::ReInit(const nsCID &aClass, nsIFactory *aFactory)
...
>+*/

nit: delete commented out code?


>     if (PR_LOG_TEST(nsComponentManagerLog, PR_LOG_ALWAYS))
>     {
...
>     }

hmm.. shouldn't this be wrapped with |#ifdef PR_LOGGING|?  maybe there are 
other places like this?

r=darin

Comment 12

[Christopher Blizzard (:blizzard)]

Comment on attachment 122879 [details] [diff] [review]
patch v.3

a=blizzard for 1.4 (assuming dougt fixes darin's issues)

Comment 13

[Doug Turner (:dougt)]

I fixed darin's issues with the above patch and checked the patch in for 1.4.

Checking in nsComponentManager.cpp;
/cvsroot/mozilla/xpcom/components/nsComponentManager.cpp,v  <-- 
nsComponentManager.cpp
new revision: 1.234; previous revision: 1.233
done
Checking in nsComponentManager.h;
/cvsroot/mozilla/xpcom/components/nsComponentManager.h,v  <--  nsComponentManager.h
new revision: 1.87; previous revision: 1.86
done

Comment 14

[Charles Rosendahl]

20030516 builds.

Verified using mfcembed (prompt service) and a simple unit test (appshell,
message service, others in the dat file) that registering entries with the same
contract ids as an existing component does not remove existing entries.

Comment 15

[Matthias Versen [:Matti]]

Comment on attachment 122879 [details] [diff] [review]
patch v.3

removing obsolete review request