XPCOM prematurely removes factory entries from compreg.dat

VERIFIED FIXED in mozilla1.4final

Status

()

--
critical
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

Trunk
mozilla1.4final
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

16 years ago
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.
(Assignee)

Updated

16 years ago
Severity: normal → critical
Flags: blocking1.4?
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → mozilla1.4final
You said you had a patch?
(Assignee)

Comment 2

16 years ago
Created attachment 122766 [details] [diff] [review]
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.
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?
(Assignee)

Comment 4

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

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

Updated

16 years ago
Attachment #122766 - Flags: review?(darin)

Comment 6

16 years ago
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...
(Assignee)

Comment 7

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

darin and I spoke.  we came up with a better solution.
Attachment #122766 - Flags: review?(darin) → review-
(Assignee)

Comment 8

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

darin and I spoke.  we came up with a better solution.
Attachment #122766 - Attachment is obsolete: true
(Assignee)

Comment 9

16 years ago
Created attachment 122815 [details] [diff] [review]
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.

Updated

16 years ago
Attachment #122815 - Flags: review?(darin)
(Assignee)

Comment 10

16 years ago
Created attachment 122879 [details] [diff] [review]
patch v.3

This avoids two hashtable hits per RegistryFactory.  (thanks to brendan for
setting me straight).
Attachment #122815 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #122879 - Flags: superreview?
Attachment #122879 - Flags: review?(darin)

Comment 11

16 years ago
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
Attachment #122879 - Flags: review?(darin) → review+
Comment on attachment 122879 [details] [diff] [review]
patch v.3

a=blizzard for 1.4 (assuming dougt fixes darin's issues)
Attachment #122879 - Flags: approval1.4+
(Assignee)

Comment 13

16 years ago
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
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Updated

16 years ago
Flags: blocking1.4?

Comment 14

16 years ago
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.

Status: RESOLVED → VERIFIED
Comment on attachment 122879 [details] [diff] [review]
patch v.3

removing obsolete review request
Attachment #122879 - Flags: superreview?

Updated

16 years ago
Attachment #122815 - Flags: review?(darin)
You need to log in before you can comment on or make changes to this bug.