258381 - ###!!! ASSERTION: You are calling CreateInstance "{eaca2576-0d4a-11d3-9d7e-0060088f9ff7}" when a service for this CID already exists!: 'Error', file r:/mozilla/xpcom/components/nsComponentManager.cpp, line 1877

Status: NEW

(Core :: CSS Parsing and Computation, defect)

Description

[timeless]

###!!! ASSERTION: You are calling CreateInstance
"{eaca2576-0d4a-11d3-9d7e-0060088f9ff7}" when a service for this CID already
exists!: 'Error', file r:/mozilla/xpcom/components/nsComponentManager.cpp, line 1877

http://lxr.mozilla.org/seamonkey/search?string=eaca2576
129 // {eaca2576-0d4a-11d3-9d7e-0060088f9ff7}
130 #define NS_CSS_LOADER_CID \
131 { 0xeaca2576, 0x0d4a, 0x11d3, { 0x9d, 0x7e, 0x00, 0x60, 0x08, 0x8f, 0x9f,
0xf7 } }

http://lxr.mozilla.org/seamonkey/ident?i=NS_CSS_LOADER_CID
 42 static NS_DEFINE_CID(kCSSLoaderCID, NS_CSS_LOADER_CID);

issue: components should generally be treated as either singletons or non
singletons. CSSLoader is being used as both, the fix is to find all instances of:

Components.classesByID["{eaca2576-0d4a-11d3-9d7e-0060088f9ff7}"].createInstance
Components.classesByID["{eaca2576-0d4a-11d3-9d7e-0060088f9ff7}"].getService
do_CreateInstance(kCSSLoaderCID)
do_GetService(kCSSLoaderCID)

and similar ilk and standardize on either createInstance or getService. Pick
createInstance if there is internal state which needs to be maintained per
instance and multiple instances can peacefully coexist. Pick getService if there
is no internal state or if multiple instances cannot peacefully coexist and the
object internally manages multiple requests.

the only two createInstances to be found are in:
# content/xbl/src/nsXBLPrototypeResources.cpp:
# layout/html/tests/ParseCSS.cpp:

this would seem to imply that getService is the right choice. it's suggested
that the person who writes this patch verifies this change with bz.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> this would seem to imply that getService is the right choice

bzzzt.  Wrong.  CSSLoaders exist one per document, so can't possibly be a
singleton.  See various impls of nsIDocument::GetCSSLoader, which don't use the
CID (using NS_NewCSSLoader instead), so weren't caught in your searches.

All the users of getService on the CSSLoader should be fixed, though; it looks
like most of them are just copy-paste off each other and are just begging to be
replaced with the user/ua sheet api we need....  Perhaps we need a single
"random css loader" instance hanging out in the layout lib to handle this sort
of thing?

Comment 2

[timeless]

perhaps...

this bug can focus on replacing getService with createInstance (it's part of a
series of bugs...)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Pure createInstance may have a negative effect on performance (eg the document
viewer caller of getService would create an extra CSSLoader for every document...)

Comment 4

[Benjamin Smedberg]

I specifically made a CSSLoader service (for the chrome registry and the
layoutstylesheetcache) and the per-doc objects. I object to this assertion, this
is a valid usage.

Comment 5

[timeless]

in many cases it isn't, and if you want to hold onto a single instance, you
should do it yourself.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> I object to this assertion, this is a valid usage.

I don't agree with that.  Either something is a service or it's not.  It can't
somehow magically be both (for one thing, using createInstance to instantiate a
typical service is utterly wrong -- that's what this assertion is about).

I'm sorry that I missed that in my initial review of that patch...

Comment 7

[Benjamin Smedberg]

> I don't agree with that.  Either something is a service or it's not.  It can't

Restating the claim doesn't make it any more true... but as the component's in
your module, I'll let you decide how the chromereg and layout stylesheet cache
are supposed to handle it. Basically, we need one CSSLoader to "act like a
service" and be global, plus the per-document CSSLoaders. Seems like both a
service and a createable component to me.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Perhaps it's best to explain the reason for the assertion more clearly.  The
problem is that consumers of Gecko have no way to tell whether something is
meant to be a service or a creatable component by looking at our public api (the
contract id with its lack of documentation and the idl that's implemented by the
component).  So it's not uncommon for people to use createInstance to create
things that really _do_ need to be services and services only.  Hence the
assertion, to save them a lot of debugging trouble.

The situation is not symmetric.  Something that's "not really a service" is just
fine with being used as both service and component (you just end up with a
global instance, as you said).  But something that is "really a service" is not
at all OK with being used as both, typically.  If you can think of a way to
detect the one use case (and assert or prevent) without catching the other,
that's great and we should do it.