57035 - nsString recycler does not recycle entries...

Status: RESOLVED DUPLICATE of bug 65219

(Core :: XPCOM, defect, P3)

Description

[rpotts (gone)]

It turns out that both nsString and nsCString have recyclers...  

So, if you call nsString::Recycle() the nsString instance is placed on a 
nsDeque.  Unfortunately, once strings are in the recycler, they are never used 
again :-(  They just hang out in the nsDeque for the lifetime of the app and 
ultimately leak!

I'm attaching a patch which removes the recycler lists from both nsString and 
nsCString.

-- rick

Comment 1

[rpotts (gone)]

Attachment: Patch to remove recyclers from nsString and nsCString

Comment 2

[rickg]

This particular bit of stupidity is my fault. The intent was to allow folks to 
call CreateString(), which (does) retrieve a string from the recycle deque. 

RickP: when I run the top 100, I don't see this getting called at all. Is there 
a situation where it get's called a bunch? Mail perhaps?

Comment 3

[rickg]

Wait a second. If you read the code closely, which I just did, it shows that the 
static string recycler is destroyed at shutdown. It in turn creates a 
deallocator which deletes all the strings in the deque. RPotts: are you sure 
this is a leak at all? (Again, I'm not even sure it's used).

Comment 4

[rpotts (gone)]

hey rick,

I was observing three things:

1. nsHTMLDocument recycles 2 nsStrings - The last-modified-date and the referer 
(if they are set).  More strings are recycled if either SetLastModified(...) or 
SetReferrer(...) are called.  However, in the simple page transition case I did 
not see these methods being called.

2. nsString::CreateString(...) is never called.

3. Even if nsString::CreateString(...) were called, it would return NULL.  The 
code in nsString::CreateString() fails to capture the result from 
GetRecycler().CreateString().  So, unfortunately, result will always be NULL :-(

When I found this, I was focusing on inter-page bloat.  It looks like the 
strings do get deleted from the recycler at shutdown, but during the lifetime of 
the app, they just build up...

It is not a huge leak - it is just another case where a few things get lost 
during a page transition...  Unfortunately, I don't see any *huge* leaks during 
page transitions - just lots of small ones...

-- rick

Comment 5

[Scott Collins]

The patch looks good.  I don't actually see any callers of |CreateString|
anywhere in the app (which is good, since you wouldn't really want |nsString|s
allocated on the heap very often).  The pattern appears in the IMAP parser, but
it implements this function itself.  Maybe we could totally get rid of
|CreateString| at the same time?  Any caller should really use the more
idiomatic |new nsString| in any case, if that's what they mean.  Why obfuscate?
 I'm happy to sr= this as is, but would like it even more if you went the extra
step and totally chopped out |CreateString|.

sr=scc (but feel free to attach a new patch that completely removes |CreateString|)

Comment 6

[K Carter]

Is this related to bug #65219 that was just marked fixed?

Comment 7

[Scott Collins]

Yeah, I think this is a duplicate.  Rick(s) ... I'm going to mark it a duplicate
of bug #65219, for which I just checked in dbaron's patch.

*** This bug has been marked as a duplicate of 65219 ***