96457 - Merge ServiceManager and ComponentManager

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Suresh Duddi (gone)]

Merging them will give us the following wins:

a) Eliminate the need to maintain another hash table
b) GetService(contractid) will happen with 1 hash lookup as opposed to 2

Comment 1

[Suresh Duddi (gone)]

Doug, the new brave owner of xpcom has accepted to do these. Yeah! thanks doug.

Comment 2

[Doug Turner (:dougt)]

Attachment: xpcom/build patch v.1

Comment 3

[Doug Turner (:dougt)]

Attachment: Working xpcom/components patch v.1

Comment 4

[Doug Turner (:dougt)]

nsRepository.cpp, nsRepository.h  and nsServiceManager.cpp can be removed.

I am concerned at the shutdown deletion order of that hashtables.  I have
reordered them based on the ownership between these hashtables.  dp, please
confirm that it is okay do delete the contractID has prior to both unloading
libraries and deletion of the cid hash?

Comment 5

[Doug Turner (:dougt)]

dp, I think that we are overly using mMon in the last  patch.  As cited in
13009, we don't have to enter a monitor while accessing the hash.    Should we
treat this complete seperate (eg. land this patch before removing the monitor)

Comment 6

[Suresh Duddi (gone)]

Yeah let us keep this purely to merging the two objects and rerouting
GetService(contracid) to one hash lookup than 2.

Hey, Can you attach the latest patch with all our fixes merged in ?

Comment 7

[Doug Turner (:dougt)]

Attachment: xpcom/components diff v.2

Comment 8

[Doug Turner (:dougt)]

the last patch fixes an assertion which would be hit in the general case.  

Comment 9

[Doug Turner (:dougt)]

in the last patch to components, ingore the change to nsIComponentManager.idl. 
If you don't, you will not compile without sprinkling #include love everywhere.

Comment 10

[Suresh Duddi (gone)]

r=dp
Comments:
mContractIDsDestroy: We should assert if CID is not empty and the type of entry
is SERVICE_ONLY

Testing: The following cases should be tested:
- a case where shutdown listener is firing. I think there were only two
registering a shutdown listener that havent been weened off
- Test File->OpenFile.. menu item (tests js component loader)

Comment 11

[Suresh Duddi (gone)]

Oh yeah. We need to test to make sure we dont break static builds with this. I
doubt if that will happen from the review of code. Thanks Waterson for the yell.

Comment 12

[Doug Turner (:dougt)]

That cast in the xpcom/build from the component manager to the service manager
does not work xp.  I had to do this instead:        

rv = compMgr->QueryInterface(NS_GET_IID(nsIServiceManager),
(void**)&gServiceManager);
if (NS_FAILED(rv)) {
  NS_RELEASE(compMgr);
  return rv;
}

Comment 13

[Doug Turner (:dougt)]

started miller time too early; ignore last comment; 

Comment 14

[Doug Turner (:dougt)]

Attachment: Complete Changes - Tree wide

Comment 15

[Doug Turner (:dougt)]

The last patch includes remove nsRepository.

Comment 16

[Brendan Eich [:brendan]]

"Try standing downwind... it's Miller Time." - Kim Catrall, _Big Trouble in
Little China_.

[e-mail] need to hear about module ownership assertions, btw.

/be

Comment 17

[Chris Waterson]

Comment on attachment 48397 [details] [diff] [review]
Complete Changes - Tree wide

Changes look good modulo MOZ_COUNT_CTOR() and hard-tabs, which we discussed on IRC.

Comment 18

[Chris Waterson]

Meant to say ``sr=waterson'', too.

Comment 19

[Doug Turner (:dougt)]

Fix checked in.

Comment 20

[John Bandhauer]

Heh!

Am I the only one who cares that this asserts on startup?

The current code in nsXPComInit.cpp registers the nsMemory *service* early on 
and then tries to register all the components in its list. This includes  
nsMemory. But the code for that registration asserts and fails because the 
component is already registered as 'service only'. Assuming that we want to 
stick with this exclusive 'service only' scheme here then I think we just want 
to apply the following patch to avoid the doomed attempt to register nsMemory 
the second time...

Index: nsXPComInit.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpcom/build/nsXPComInit.cpp,v
retrieving revision 1.101
diff -u -r1.101 nsXPComInit.cpp
--- nsXPComInit.cpp     2001/09/06 21:12:52     1.101
+++ nsXPComInit.cpp     2001/09/07 02:34:40
@@ -177,8 +177,6 @@
    &NS_CLASSINFO_NAME(Class) }

 static nsModuleComponentInfo components[] = {
-    COMPONENT(MEMORY, nsMemoryImpl::Create),
-
 #define NS_ERRORSERVICE_CLASSNAME NS_ERRORSERVICE_NAME
     COMPONENT(ERRORSERVICE, nsErrorService::Create),

Comment 21

[Doug Turner (:dougt)]

r/sr=me.  dp, can you review this change too?

Comment 22

[Mike Shaver (:shaver emeritus)]

sr=shaver.  Nice catch, jband.

Comment 23

[Suresh Duddi (gone)]

Yeah nice catch. Wow. Why are asserts not firing on my build... I need to turn
them on.

So the thing is this registration should succeed. Something could be registered
as a service and as a component. Let me see what the problem is.

Comment 24

[Suresh Duddi (gone)]

Found two problems:
- ReInit() should be able to promote a SERVICE_ONLY entry to another of any type
(FACTORY_ONLY or NATIVE)
- RegisterFactory() and friends should not err out if replace is not specified
and the entry type is SERVICE_ONLY

working on a patch.

Comment 25

[Suresh Duddi (gone)]

Attachment: Fixes assert on ReInit()

Comment 26

[John Bandhauer]

dp: Thanks for digging out the real problem. I wasn't sure if you guys were 
trying to add a mechanism to ensure that some components were registered as 
services only or what.

Did you really mean to use || in one place and && in the other? 

Comment 27

[Suresh Duddi (gone)]

Yes yes yes. Thanks. Will attach new patch.

Comment 28

[Suresh Duddi (gone)]

Taking bug over to fix assert and replace flag handling.

Comment 29

[Suresh Duddi (gone)]

Attachment: Fixing assert patch again. This time with the correct logical conditions. (thanks jband)

Comment 30

[Suresh Duddi (gone)]

shaver/jband : r=/sr= ?

Comment 31

[John Bandhauer]

Comment on attachment 48622 [details] [diff] [review]
Fixing assert patch again. This time with the correct logical conditions. (thanks jband)

sr=jband.
worksforme.
I didn't closely review the initial patch. But as far as I understand things this looks right.

Comment 32

[Suresh Duddi (gone)]

Checked it in. The only thing that is left is:

- when we get a replace flag of FALSE for component registrations, we return
error whenever we find an entry for the cid. Finding an entry of type
SERVICE_ONLY should be ok.

This wont affect us yet. Sending doug's way.

Comment 33

[Doug Turner (:dougt)]

dp, spawning a new bug report for this problem:

http://bugzilla.mozilla.org/show_bug.cgi?id=99109

I am marking this bug fixed.  DP, thanks for your help.