112190 - leaking nsCaseConversionImpl2

Status: RESOLVED INVALID

(Core :: Layout, defect, P2)

Description

[Alec Flett]

from the bloat logs, I noticed we're leaking the nsCaseConversionImpl2 service,
as well as 2 nsCompressedCharMaps (32 bytes + a 256 byte cache, each)

The leaker was nsTextTransformer, and the problem was that it wasn't releasing
the global service. The fix is to observe XPCOM shutdown and release the global
object.

Patch forthcoming. This is the same way we've fixed similar service leaks.

can I get r=nhotta, sr=attinasi?

Comment 1

[Alec Flett]

Attachment: fix leak with an observer

Comment 2

[Alec Flett]

er, the compressed char maps are 16 bytes each

all told this is a 556 byte leak.

Comment 3

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

Well, as I said in my review comments on bug 112209, where this patch was
accidentally included:

>Index: layout/html/base/src/nsTextTransformer.cpp

It would be much simpler if you just did this work from the layout
module destructor rather than having your own observer.  If, when
(if?) we start unloading DLLs, we find that the module destructors
are doing some of these things too late, we could have one XPCOM
shutdown observer per-module and move some of the things from the
module destructor into it.

Comment 4

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

But actually, now that I look at it, we *already* have
nsTextTransformer::Shutdown to fix this.  Why doesn't that work?  (It should
just use NS_IF_RELEASE rather than ReleaseService, though...)

Comment 5

[Alec Flett]

I agree. :)
I'll go whip up a new patch.

Comment 6

[Alec Flett]

now this is odd. according to layout's destructor, nsTextTransformer::Shutdown
should be shutting it down, but I'm not sure it is. Maybe I need to use the
refcnt balancer :)

Comment 7

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

Oh, you didn't?  I was previously under the impression here that the reason this
was happening was that dougt broke nsIShutdownListener.

Comment 8

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

See bug 106466 and bug 99152 (particularly
http://bugzilla.mozilla.org/show_bug.cgi?id=99152#c1 ).

Comment 9

[Roy Yokoyama]

alecf: do we need to check for aTopic on ::Observe()
+nsCaseConvShutdown::Observe(nsISupports*, const char *aTopic, const PRUnichar*)
+{
----> if(!nsCRT::strcmp(aTopic, NS_XPCOM_SHUTDOWN_OBSERVER_ID))
+        NS_IF_RELEASE(gCaseConv);
+      return NS_OK;
+}

or 
+ obs->AddObserver(observer, NS_XPCOM_SHUTDOWN_OBSERVER_ID, PR_FALSE);
guarantees the ::Observe() to be called only at shutdown?

Comment 10

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

A refcount log shows that HandleCaseConversionShutdown::Observe isn't getting
called.  I wonder if this is the problem that some observers aren't being
notified because of dougt's observer changes that don't handle the case of
observers removing themselves during notification again.

Comment 11

[Alec Flett]

I think this is just bogus - I just fixed bug 110226, so this will just go away.