276956 - XPCOM service registered under two contract IDs gets instantiated twice

Status: RESOLVED WORKSFORME

(Core :: XPCOM, defect)

Description

[Simon Fraser [no longer active]]

In Camino I'm trying to replace the global history implementation with a custom one. This implements 
two services, nsIGlobalHistory2 and nsIAutoCompleteSession which are obtained in the code via 
contract IDs. It gets registered via nsComponentManagerImpl::RegisterFactory() in CHBrowserService::
RegisterAppComponents().

I'm seeing my global history implementation get instantiated twice, once for each of the do_GetService 
calls (one for each of the 2 contract IDs), which is very bad as both instances will try to write the history 
database.

Some debugging in nsComponentManager indicates a possible cause. nsComponentManagerImpl::
RegisterFactory() adds a new nsFactoryEntry to the mFactories hash for each call (rather than looking up 
via CID to see if one exists already). It then puts the new entry in the contractID hash for the given 
contractID.

This means that two RegisterFactory() calls with the same CID, but different contractIDs will result in a 
single entry in the mFactories hash for the CID, but two entries in the contractID table pointing to 
different nsFactoryEntry objects. This breaks the service singleton check when instantiating services by 
contractID.

Comment 1

[Simon Fraser [no longer active]]

Attachment: Possible patch

Read the comments in the patch to see what I'm doing here. I think it's the
right thing, but the problem is that we almost never use the same nsIFactory
object even for two instances of the same CID (we tend to make new
nsIGenericFactory objects wrapped around the same constructor), so other code
would have to be changed to fix the original problem.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It's worth noting that ReadPersistentRegistry, RegisterService and
RegisterComponentCommon do this right (associating multiple ContractID table
entries with the same nsFactoryEntry), while RegisterFactory does it wrong.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And, FWIW, I don't think it's necessary to fix other code to produce the same
factory object -- it should be safe to assume that the different factory object
is a factory for the same class, as those other three functions do.  This also
avoids the need to modify nsGenericFactory extensively.

Comment 4

[Benjamin Smedberg]

Just for my reference, why are you using registerFactory instead of
registerFactoryLocation? Are these static components or not in the components
directory?

Comment 5

[Simon Fraser [no longer active]]

As mentioned in the first comment, these are camino-specific component overrides
registered via CHBrowserService::RegisterAppComponents(). They live in app code,
not in a library.

Comment 6

[Simon Fraser [no longer active]]

I started on a patch that makes RegisterFactory() behave the same way that
RegisterComponentCommon() does, with requires an overloaded ReInit() on
nsFactoryEntry(). However, I'm not sure what checks to do on mTypeIndex in that
ReInit(). We seem to hit factory entries of all types (-2, -1, 0).

Comment 7

[Simon Fraser [no longer active]]

Attachment: Proto patch for a version that uses nsFactoryEntry::ReInit()

This is a WIP patch, which spits out warnings about
@mozilla.org/xpcom/memory-service;1, @mozilla.org/principal;1 and
@mozilla.org/systemprincipal;1 in addition to things that Camino is overriding.

Comment 8

[Benjamin Smedberg]

The XPCOM rewrite in FF4 fixed this.