109130 - Possible memory leak in nsExternalHelperAppService::AddMimeInfoToCache

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

If one adds a mime info which has the same type or extension as a mime info
that's already in the cache, the refcounts will go all screwy. Eg:

Add "text/plain", ".c"
Add "text/plain", ".txt"

The result will be that each of those mime info objects has a refcount of 2,
even though there are only 3 entries in the cache.

Yes, callers should avoid doing this.  But that's the topic of separate bugs.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Add some calls to NS_IF_RELEASE

Comment 2

[Boris Zbarsky [:bzbarsky]]

Oh, by the way.  We hit this memory leak at least on Unix and OS2 in certain
circumstances, and possibly on Mac  (not exactly sure how the internet config
stuff works there).

Comment 3

[Bill Law]

Comment on attachment 57092 [details] [diff] [review]
Add some calls to NS_IF_RELEASE

r=law, but see comments coming later...

Comment 4

[Bill Law]

There used to be a problem with that cache in that there was no means of
updating or removing cache entries.  This meant that you couldn't have the user
pref changs show up until you stopped Mozilla and restarted it.

I know I removed some calls to AddMimeInfoToCache, and meant to remove all of
them.  Perhaps we should do that now, or otherwise fix that problem.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Agreed, we should come up with a way to invalidate cache entries or something...
We cache:

1)  IC lookups on the mac
2)  mailcap/mime.types lookups on Linux
3)  hardcoded sniffing on OS/2

#3 could be not cached without much trouble.
#1 I don't know anything about
#2 is a pretty slow operation -- read a file, parse it, do some matching, read
   another file, parse it, create a mimeinfo.  (this is best-case scenario).  So 
   the cache is pretty nice there as long as users don't change their mime
   settings.  :)  Here I would much prefer to just store the last-modified time 
   for the relevant files or something and invalidate the cache if we notice a
   change....

Separate bug on that issue, perhaps?

Failing a good cache-invalidation fix, a complete lack of caching would be OK by
me and preferable to what we do now....

Comment 6

[rpotts (gone)]

Comment on attachment 57092 [details] [diff] [review]
Add some calls to NS_IF_RELEASE

sr=[e-mail]

Comment 7

[Boris Zbarsky [:bzbarsky]]

checked in

Comment 8

[Chris Petersen]

Verified.