Closed Bug 20182 Opened 21 years ago Closed 20 years ago

Leaking nsScriptNameSetRegistry (and others?)

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED
Future

People

(Reporter: beard, Assigned: law)

References

()

Details

(Keywords: embed, memory-leak)

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:
Assignee: rpotts → law
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 :-)
Status: NEW → ASSIGNED
Target Milestone: M13
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.
Priority: P3 → P1
M13.
QA Contact: claudius → paulmac
Target Milestone: M13 → M14
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).
Keywords: mlk
Summary: [MLK] Leaking nsScriptNameSetRegistry (and others?) → Leaking nsScriptNameSetRegistry (and others?)
Move to M16 ...
Target Milestone: M14 → M16
changing qa contact to jrgm@netscape.com on some random bugs
QA Contact: paulmac → jrgm
Target Milestone: M16 → Future
Why is this stuck off in the future?
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
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...
What's wrong with beard's patch, other than it being old?

Bruce, do you have a patch?

/be

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!!!)
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.
Keywords: embed
Agreed.  This code is gone, so the bug was fixed sometime by removing the leaky
code.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
What he said. (verified).
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.