All users were logged out of Bugzilla on October 13th, 2018

Leaking nsScriptNameSetRegistry (and others?)

VERIFIED FIXED in Future

Status

()

P1
normal
VERIFIED FIXED
19 years ago
17 years ago

People

(Reporter: beard, Assigned: law)

Tracking

({embed, memory-leak})

Trunk
Future
embed, memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

(Reporter)

Description

19 years ago
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:

Updated

19 years ago
Assignee: rpotts → law

Comment 1

19 years ago
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 :-)
(Assignee)

Updated

19 years ago
Status: NEW → ASSIGNED
Target Milestone: M13
(Assignee)

Comment 2

19 years ago
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.

Updated

19 years ago
Priority: P3 → P1

Comment 3

19 years ago
M13.

Updated

19 years ago
QA Contact: claudius → paulmac
(Assignee)

Updated

19 years ago
Target Milestone: M13 → M14
(Assignee)

Comment 4

19 years ago
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).

Updated

19 years ago
Keywords: mlk

Updated

19 years ago
Summary: [MLK] Leaking nsScriptNameSetRegistry (and others?) → Leaking nsScriptNameSetRegistry (and others?)

Comment 5

19 years ago
Move to M16 ...
Target Milestone: M14 → M16

Comment 6

19 years ago
changing qa contact to jrgm@netscape.com on some random bugs
QA Contact: paulmac → jrgm

Updated

19 years ago
Target Milestone: M16 → Future

Comment 7

19 years ago
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
(Assignee)

Comment 9

19 years ago
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

Comment 11

19 years ago
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

19 years ago
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.

Updated

19 years ago
Keywords: embed
Agreed.  This code is gone, so the bug was fixed sometime by removing the leaky
code.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Updated

18 years ago
Status: RESOLVED → VERIFIED

Comment 14

18 years ago
What he said. (verified).
You need to log in before you can comment on or make changes to this bug.