22283 - memory leak of nsUnicodeEncodeHelper

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P3)

Description

[Frank Tang]

We are leaking nsUnicodeEncodeHelpe in the Linux tinderbox leaking report.
I think what happen is our coverter DLL hold nsUnicodeEncodeHelper (and probably
also decode helper ) and never release it. We should hookup a Dll based shutdown
rountine to free them.

Comment 1

[cata]

Ok, I'll look into it.

Comment 2

[Frank Tang]

dp said jband will check in a new macro for the generic module which have a way
to put in a shutdown routine. We can release the helper from the shutdown
routine.

Comment 3

[John Bandhauer]

I checked this in last night. See NS_IMPL_NSGETMODULE_WITH_DTOR in
nsIGenericFactory.h

Comment 4

[bobj]

Out of time for M13, moving to M14.

Comment 5

[Frank Tang]

Change OS and Platform to ALL

Comment 6

[leger]

We need to know how big the leak is on a per leak basis?  How often does the 
leak occur.  We need this for PDT to make right decision as to PDT+ or -.  
Thanks!

Comment 7

[bobj]

Agreed.  In fact that is what cat and I discussed yesterday...
From the latest Linux full log:
http://cvs-mirror.mozilla.org/webtools/tindertest/showlog.cgi?log=SeaMonkey/9495
36186.29691.gz&fulltext=1

   78 nsUnicodeEncodeHelpe        8       16     1602        2 (...

So it looks like we leaked 2 x 8 bytes for a grand total of 16 bytes...
So clearing beta1 keyword and moving to M15.

Comment 8

[cata]

I took a very brief look today at this bug and I don't see how we are leaking 
this helper, unless we leak a converter as well. We don't use it outside 
converters, where we realease it in destructors. Anyone has a more localized 
report? (Which file, a reproductible case, etc.) The link provided doesn't 
really work today... :(

Comment 9

[cata]

Oh, ok, I found it, I was searching for "nsUnicodeEncodeHelper" while the class 
name is truncated in reports to "nsUnicodeEncodeHelpe". Anyone knows what do I 
have to do to reproduce the bloat test and to get some more details? Thanks!

Comment 10

[cata]

Aha! We are also leaking the nsBasicDecoderSupport class! That must be the 
reason for the helper class...

Comment 11

[cata]

Please ingnore my last comment. We have no problem with the decoders. But we are 
leaking nsBasicEncoder. This is a base object which:
a) implements NS_IMPL_ISUPPORTS which has our leak detection hooks
b) is subclassed by detectors

Practically, we can't leak it, because we never allocate it. My belive is that 
we are leaking converters - and seeing this. And this could be also paired with 
the helper object leaks: we are leaking the helpers because we are leaking their 
owners, the converters.

So, the problem is not as simple as it seemed: we have to track conveter usage 
patterns and see *who* is leaking. (You know, xpcom, ref counting, yada, 
yada...)

PS. The DLL keeps no reference to the helper, so there's nothing to release 
there! Only converters do that.

Comment 12

[yueheng.xu]

Attachment: Fix for #22218 (converter leak)

Comment 13

[yueheng.xu]

There are may be more than one source of converter leaks. Here I fix one of 
them. See the attached diff file: nsFontMetricsGTK.cpp.diff

To look for more places, do a search on "->GetUnicodeEncoder". Whenever the
smart COMPtr is not used, and also the NS_IF_RELEASE is not used, there is
a possible leak in that section.
 

Comment 14

[yueheng.xu]

Attachment: new diff file nsFontMetricsGTK.cpp.diif

Comment 15

[yueheng.xu]

the fix is for #22283, (not for #22218). Last diff file has a typo, 
the 'convert' should be 'converter'.  

Comment 16

[yueheng.xu]

Attachment: Diff for a change that does reduce the leak count in bloatview

Comment 17

[yueheng.xu]

The last attachment shows a diff that can reduce the leak count (total of 2 ) 
of nsBasicEncoder and nsUnicodeEncoderHelp in the bloatview by one.  The 
remaining leak count is a spurious resulted from that fact that the bloatview 
logged earlier than the destructor releases the class.

I made this change in a way that we can easily comment out one line: 

  aSelf->mConverter = 0;  

and uncomment two lines

  aSelf->mConverter = converter;
  NS_ADDREF(aSelf->mConverter); 

and we cab see the bloatview leak count will go up by one count.

This change is attached here for review purpose. I don't see significant
performance penalty before and after above change. At least for the default 
page.  If for performance reason we need to let aSelf->Converter owns an 
instance of converter, we need a right place and right time to do the release.
 

Comment 18

[yueheng.xu]

Add [e-mail] at the Cc: list

Comment 19

[yueheng.xu]

Erik's recent change to FreeGlobals() seems fixed the problem at the upper 
level. The bloatview count did went down by one. The remaining one count is a 
result of logging before destructor. There is no real leak. So, consider this 
fix fixed.

Comment 20

[Teruko Kobayashi]

Changed QA contact to [e-mail].