8161 - Leaking nsIServiceMgr in nsIAppShellComponentImpl

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P1)

Description

[scottputterman]

I was looking into why none of my services are ever getting freed and one of the
many reasons seems to be because mServiceMgr in nsIAppShellComponentImpl is
never released. Therefore the global service manager can never get shutdown.
Also, because this header file gets included in multiple libraries, there is
actually a copy of mServiceMgr in each one.  Since we currently have 3
components that include this file, there are 3 refcounts left over when we
shutdown.  I tried to figure this out but didn't have much luck.

Comment 1

[don]

Bill, any luck we could fix this for M7?

Comment 2

[Bill Law]

I think I need to add a Release() call on mServiceMgr somewhere in the component
shutdown code.  I'll look at this later today.

Comment 3

[Bill Law]

I have a fix in hand so I'm changing this to M7.  The change consisted of simply
adding a call to mServiceMgr->Release() in nsAppShellComponentImpl::Shutdown().

Comment 4

[Bill Law]

This fix was just checked in:

Index: nsIAppShellComponentImpl.h
===================================================================
RCS file: /cvsroot/mozilla/xpfe/components/public/nsIAppShellComponentImpl.h,v
retrieving revision 1.6
diff -r1.6 nsIAppShellComponentImpl.h
154,155c154,159
<     if ( mServiceMgr && Is_Service() ) { \
<         rv = mServiceMgr->ReleaseService( progId, this ); \
---
>     if ( mServiceMgr ) { \
>         if ( Is_Service() ) { \
>             rv = mServiceMgr->ReleaseService( progId, this ); \
>         } \
>         mServiceMgr->Release(); \
>         mServiceMgr = 0; \

Comment 5

[scottputterman]

I tried this out and it still leaks the refcounts, so I'm reopening.

Apparently, NSCanUnload( nsISupports* aServiceMgr ) gets called after ShutDown.
In this function you call SetServiceManager which creates (and therefore
AddRefs) another service manager if the value is currently NULL, which it is
after you released it in ShutDown.  So, this extra reference still exists. Is
SetServiceManager necessary in NSCanUnload?

Comment 6

[leger]

Clearing Fixed resolution.

Comment 7

[scottputterman]

In case anyone's holding up M7 for this, I just wanted to point out that since
there are other ref counting problems on nsServiceManager that aren't going to
be fixed for M7, this bug fix will help, but it won't actually change anything.

Comment 8

[Bill Law]

Doh!  I've fixed this so it doesn't addref the service manager from NSCanUnload.

Comment 9

[scottputterman]

It looks fixed now.  Marking verified.