Closed Bug 315401 Opened 19 years ago Closed 19 years ago

XPTI_GetInterfaceInfoManager is a bad signature, and we should just use do_GetService anyway

Categories

(Core :: XPCOM, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

(Whiteboard: [has patch])

Attachments

(1 file, 1 obsolete file)

XPTI_GetInterfaceInfoManager returns an addrefed pointer but doesn't document that in any way. And we should really just be using do_GetService. Johnny said he doesn't think that XPTI_GetInterfaceInfoManager is typically called in perf-sensitive code, so I'm going to try and replace the XPTI_GetInterfaceInfoManager calls with do_GetService and see what happens to tbox perf numbers.
Attachment #202122 - Flags: superreview?(jst)
Attachment #202122 - Flags: review?(darin)
The XPCOM_API bits are from bug 305949 and aren't part of this patch.
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [has patch]
Comment on attachment 202122 [details] [diff] [review]
Use do_GetService for the interfaceinfomanager

>Index: xpcom/components/nsComponentManager.cpp

>-    nsCOMPtr<nsIInterfaceInfoManager> iim =
>-        dont_AddRef(XPTI_GetInterfaceInfoManager());
>+    nsCOMPtr<nsIInterfaceInfoManager> iim
>+        (do_GetService(NS_INTERFACEINFOMANAGER_SERVICE_CONTRACTID));

Why can't the component manager access the interface info manager directly?


>Index: xpcom/proxy/src/nsProxyEventClass.cpp

>-        nsCOMPtr<nsIInterfaceInfoManager> iimgr = getter_AddRefs(XPTI_GetInterfaceInfoManager());
>+        nsCOMPtr<nsIInterfaceInfoManager> iimgr
>+            (do_GetService(NS_INTERFACEINFOMANAGER_SERVICE_CONTRACTID));

ditto.. we're inside the xpcom dll, so why not get the service
directly?  take an AddRef if necessary.


>Index: modules/plugin/base/src/nsPluginHostImpl.cpp

>+    nsCOMPtr<nsIInterfaceInfoManager> iim
>+      (do_GetService(NS_INTERFACEINFOMANAGER_SERVICE_CONTRACTID));

This coding style seems pretty out-of-place in nsPluginHostImpl.cpp

Please stick with the prevailing coding style of the files being
modified.


>Index: js/src/xpconnect/src/nsXPConnect.cpp

> nsXPConnect::nsXPConnect()
>     :   mRuntime(nsnull),
>-        mInterfaceInfoManager(nsnull),
>+        mInterfaceInfoManager(do_GetService(NS_INTERFACEINFOMANAGER_SERVICE_CONTRACTID)),

looks like this line exceeds 80 chars :(


>Index: js/src/xpconnect/src/xpccomponents.cpp

>+nsXPCComponents_Interfaces::nsXPCComponents_Interfaces() :
>+    mManager(do_GetService(NS_INTERFACEINFOMANAGER_SERVICE_CONTRACTID))
> {
>-    mManager = dont_AddRef(XPTI_GetInterfaceInfoManager());
> }

XPConnect is probably not too happy if used from xpcom-shutdown :(
Could that be an issue with this patch?  How does a JS component
handle the xpcom-shutdown event if GetService would fail?  Yikes!
I'm open to NS_GetInterfaceInfoManager being added as an exported
function from xpcom.dll.

Hopefully mManager is null checked properly in places where it is used.


>Index: extensions/webservices/interfaceinfo/src/nsScriptableInterfaceInfo.cpp

>+    nsCOMPtr<nsIInterfaceInfoManager> iim 
>+        (do_GetService(NS_INTERFACEINFOMANAGER_SERVICE_CONTRACTID));

ding... keep coding style consistent.  same nit applies elsewhere.
Attachment #202122 - Flags: review?(darin) → review-
Attachment #202122 - Attachment is obsolete: true
Attachment #202147 - Flags: superreview?(jst)
Attachment #202147 - Flags: review?(darin)
Attachment #202122 - Flags: superreview?(jst)
Comment on attachment 202147 [details] [diff] [review]
Use do_GetService for the interfaceinfomanager, rev. 1.1

sr=jst
Attachment #202147 - Flags: superreview?(jst) → superreview+
OK, it turns out that it is possible to call GetService from xpcom-shutdown.  It just isn't possible to call it after xpcom-shutdown.  Hopefully, we do not need to access IIM after xpcom-shutdown.  If we do, then we have a bug.

CC'ing Javier since this may impact javaconnect bindings.
One possible issue is the fact that we do not call ProcessPendingEvents until after we set the gXPCOMShuttingDown flag.  That means that if we have some event listener (like a nsIStreamListener) implemented in a non-native language, then we could encounter problems dispatching those events, which could cause us to not clean things up properly.  Necko doesn't do anything to enforce that all of its pending events run inside the xpcom-shutdown event handler, so this could be a problem.  Should we maybe process pending events before setting gXPCOMShuttingDown to true?
Yeah, this affects pyxpcom also.

Can we leave the xpcom shutdown stuff for another bug? I have some fairly specific thoughts about how to manage XPCOM shutdown much more cleanly so that we can unload xpconnect (and other module loaders) that I haven't written down yet, and how to process pending events is part of that writeup. I don't think it will affect the current situation at all (if script is processing events after gXPCOMShuttingDown is set we're in a world of hurt already, so I don't think it's common).
The reason why I made a big deal about xpcom-shutdown is because it might mean that we need a NS_GetInterfaceInfoManager instead of do_GetService, which if true would greatly change the current patch.


> if script is processing events after gXPCOMShuttingDown is set we're in a world 
> of hurt already, so I don't think it's common

Are you sure?  I think it should be possible for simple things to still work today, and as I described it's really easy to hit this scenario... just have a necko stream listener waiting on events and shutdown the app.
What code is going to running a necko stream listener after xpcom-shutdown and need the XPTI manager? Remember that xpconnect has already mostly shut down and will assert on any usage to Components.* that does anything useful. And xpconnect keeps a reference to the interfaceinfomanager in any case.
> What code is going to running a necko stream listener after xpcom-shutdown and
> need the XPTI manager? Remember that xpconnect has already mostly shut down and
> will assert on any usage to Components.* that does anything useful. And
> xpconnect keeps a reference to the interfaceinfomanager in any case.

hmm... i don't see any xpcom-shutdown observers in xpconnect.  is there some module outside of xpconnect that listens to xpcom-shutdown and then calls into xpconnect to have it shutdown?  clearly, it wouldn't make much sense for that to be the case since it would possibly prevent JS components from handling the xpcom-shutdown event.  so, xpconnect must shutdown after xpcom-shutdown.  given that the next thing that happens after xpcom-shutdown is ProcessPendingEvents, it seems that some JS could run at that time and do useful things.

even if we take JS out of the picture, other language bindings may need the IIM to do finalization stuff.  all i'm saying is that it is hard to know if there isn't a problem.
Comment on attachment 202147 [details] [diff] [review]
Use do_GetService for the interfaceinfomanager, rev. 1.1

r=darin but don't say i didn't warn you! :)
Attachment #202147 - Flags: review?(darin) → review+
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Dveditz, this was not fixed and should not be fixed on the 1.8/1.8.0 branches, why did you mark it so?
You need to log in before you can comment on or make changes to this bug.