20182 - Leaking nsScriptNameSetRegistry (and others?)

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect, P1)

Description

[Patrick C. Beard]

Unmatched GetService() call causes a leak here. You should use NS_GET_SERVICE
macro. Here's a patch that cleans all of this code up. It's a big patch, so I'd
ask you to review it carefully. I removed all of the goto's in favor of returns.

Index: mozilla/xpfe/appshell/src/nsAppShellService.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpfe/appshell/src/nsAppShellService.cpp,v
retrieving revision 1.96
diff -r1.96 nsAppShellService.cpp
196,199c196
<   nsIEventQueueService* eventQService;
<   rv = nsServiceManager::GetService(kEventQueueServiceCID,
<                                     kIEventQueueServiceIID,
<                                     (nsISupports **)&eventQService);
---
>   NS_WITH_SERVICE(nsIEventQueueService, eventQService, kEventQueueServiceCID, &rv);
202a200
>     if (NS_FAILED(rv)) return rv;
206,212c204,205
<   nsIScriptNameSetRegistry *registry;
<   rv = nsServiceManager::GetService(kCScriptNameSetRegistryCID,
<                                     kIScriptNameSetRegistryIID,
<                                     (nsISupports **)&registry);
<   if (NS_FAILED(rv)) {
<     goto done;
<   }
---
>   NS_WITH_SERVICE(nsIScriptNameSetRegistry, registry, kCScriptNameSetRegistryCID, &rv);
>   if (NS_FAILED(rv)) return rv;
221,223c214
<   if (NS_FAILED(rv)) {
<     goto done;
<   }
---
>   if (NS_FAILED(rv)) return rv;
232,257c223,231
<
<   nsIMetaCharsetService* metacharset;
<   rv = nsServiceManager::GetService(kMetaCharsetCID,
<                                     kIMetaCharsetServiceIID,
<                                      (nsISupports **) &metacharset);
<    if(NS_FAILED(rv)) {
<       goto done;
<    }
<    rv = metacharset->Start();
<    if(NS_FAILED(rv)) {
<       goto done;
<    }
<    rv = nsServiceManager::ReleaseService(kMetaCharsetCID, metacharset);
<
<   nsIXMLEncodingService* xmlencoding;
<   rv = nsServiceManager::GetService(kXMLEncodingCID,
<                                     nsIXMLEncodingService::GetIID(),
<                                      (nsISupports **) &xmlencoding);
<    if(NS_FAILED(rv)) {
<       goto done;
<    }
<    rv = xmlencoding->Start();
<    if(NS_FAILED(rv)) {
<       goto done;
<    }
<    rv = nsServiceManager::ReleaseService(kXMLEncodingCID, xmlencoding);
---
>   NS_WITH_SERVICE(nsIMetaCharsetService, metacharset, kMetaCharsetCID, &rv);
>   if (NS_FAILED(rv)) return rv;
>   rv = metacharset->Start();
>   if (NS_FAILED(rv)) return rv;
>
>   NS_WITH_SERVICE(nsIXMLEncodingService, xmlencoding, kXMLEncodingCID, &rv);
>   if (NS_FAILED(rv)) return rv;
>   rv = xmlencoding->Start();
>   if (NS_FAILED(rv)) return rv;
262,264c236
<   if (NS_FAILED(rv)) {
<     goto done;
<   }
---
>   if (NS_FAILED(rv)) return rv;
267,270c239
<
<   if (NS_FAILED(rv)) {
<       goto done;
<   }
---
>   if (NS_FAILED(rv)) return rv;
282,283d250
<
< done:

Comment 1

[rpotts (gone)]

hey bill,

Is this AppShell ScriptNameSetRegistry stuff still being used?  The oroginal CVS
checkin comment says that it was for the XPConnect factory (which has long since
gone - I think)...

I would rather not merge this patch, since I don't own the code and haven't been
in it for quite a while :-)  So, I'm handing it off to you :-)

Comment 2

[Bill Law]

I recall doing something somewhere to remove some JS name set stuff (I think it
was appcore related in xpfe/appcores).  This "XPCOMFactory" object is still
there, apparently.

I will look into this and excise the code.  In the off chance it is still used
and needed, I'll incorporate this patch to fix the leak.  Thanks for the patch,
regardless of whether we actually end up using it.

Comment 3

[don]

M13.

Comment 4

[Bill Law]

I'm got a fix for this in hand but I'm not going to push it into M13.  Higher
priority bugs are requiring more attention (build and other bustage is causing
grief).

Comment 5

[don]

Move to M16 ...

Comment 6

[Paul MacQuiddy]

changing qa contact to [e-mail] on some random bugs

Comment 7

[Bruce Mitchener]

Why is this stuck off in the future?

Comment 8

[Brendan Eich [:brendan]]

Does the patch need to be updated to match the latest rev?  Bill, if it looks 
good to you, I'm going to approve it for checkin by Bruce.

/be

Comment 9

[Bill Law]

Well, it seems I said I had a fix four and a half months ago.  As I recall, 
rpotts and I talked about this and we concluded the whole "name set registry" 
thing was obsolete so fixing the refcounting of it was polishing a turd.  I 
think we should dig up (or recreate) my patch that cleaned this up properly.

I can't estimate when it will rise to the top of my bug list, though...

Comment 10

[Brendan Eich [:brendan]]

What's wrong with beard's patch, other than it being old?

Bruce, do you have a patch?

/be

Comment 11

[Bruce Mitchener]

Nope, I just noticed the bug searching for another bug and figured it'd be nice
to get old stuff like this out of the way before I get cranking again.  (I
didn't mean cranky!!!)

Comment 12

[rusty.lynch]

The code that this bug originally referred to has change a lot. A
nsScriptNameSetRegistry service is no longer used, and the two remaining
non-xpcom GetService/ReleaseService calls match up correctly.

I think we should mark this as fixed.

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Agreed.  This code is gone, so the bug was fixed sometime by removing the leaky
code.

Comment 14

[John Morrison]

What he said. (verified).