Merge ServiceManager and ComponentManager

RESOLVED FIXED in mozilla0.9.5

Status

()

defect
P2
normal
RESOLVED FIXED
18 years ago
18 years ago

People

(Reporter: dp, Assigned: dougt)

Tracking

({perf})

Trunk
mozilla0.9.5
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

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
Blocks: 7251
Keywords: perf
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9.5
Doug, the new brave owner of xpcom has accepted to do these. Yeah! thanks doug.
Assignee: dp → dougt
Status: ASSIGNED → NEW
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?


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)
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 ?
the last patch fixes an assertion which would be hit in the general case.  
Blocks: 98275
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.
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)
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.
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;
}

started miller time too early; ignore last comment; 
The last patch includes remove nsRepository.
"Try standing downwind... it's Miller Time." - Kim Catrall, _Big Trouble in
Little China_.

staff@mozilla.org need to hear about module ownership assertions, btw.

/be
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.
Attachment #48397 - Flags: superreview+
Meant to say ``sr=waterson'', too.
Fix checked in.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
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),

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
r/sr=me.  dp, can you review this change too?
sr=shaver.  Nice catch, jband.
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.
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.
Posted patch Fixes assert on ReInit() (obsolete) — Splinter Review
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? 
Yes yes yes. Thanks. Will attach new patch.
Taking bug over to fix assert and replace flag handling.
Assignee: dougt → dp
Status: REOPENED → NEW
Attachment #48597 - Attachment is obsolete: true
shaver/jband : r=/sr= ?
Status: NEW → ASSIGNED
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.
Attachment #48622 - Flags: superreview+
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.
Assignee: dp → dougt
Status: ASSIGNED → NEW
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.  
Status: NEW → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.