Bug 26347 - UString leaks memory when creating sharedBuffers for SmallStrings
Summary: UString leaks memory when creating sharedBuffers for SmallStrings
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Andrew Wilson
Depends on:
Reported: 2009-06-12 11:22 PDT by Andrew Wilson
Modified: 2009-06-15 10:34 PDT (History)
1 user (show)

See Also:

proposed patch (5.84 KB, patch)
2009-06-13 12:12 PDT, Andrew Wilson
darin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Wilson 2009-06-12 11:22:10 PDT
The sharedBuffer code in UString creates shared buffers for small strings, because the code just checks the BaseString.len field which is 256 (the BaseString is shared for all the SmallStrings).

Since the items in SmallStringStorage.m_reps never have destroy() called on them, the sharedBuffer leaks.

I think it's a good idea to fix the bug so we don't create sharedBuffer objects for SmallStrings, but it's also a good idea to change SmallStringsStorage to properly clean up its contents (right now, none of its Rep instances or its BaseString instance have their destroy() function invoked, which will undoubtedly lead to more leaks in the future).
Comment 1 David Levin 2009-06-12 11:30:08 PDT
I know what to do for this one.
Comment 2 Andrew Wilson 2009-06-12 11:34:08 PDT
I'm working on a patch locally to change how SmallStringsStorage handles its internal references, btw. You should still make the sharedBuffer change though.
Comment 3 Andrew Wilson 2009-06-13 12:12:45 PDT
Created attachment 31243 [details]
proposed patch
Comment 4 Eric Seidel (no email) 2009-06-14 02:22:08 PDT
Comment on attachment 31243 [details]
proposed patch

We run run-webkit-tests with --leaks from time to time.  So it is theoretically possible to make a test case for this if the tests don't already trip the leak.
Comment 5 Andrew Wilson 2009-06-14 09:05:03 PDT
With levin's fix for bug 26360, there's no longer a leak. I'm presuming that his layout tests for that bug will cover this case as well. This patch is a cleanup to make sure a similar class of leaks can't happen in the future.

That said, I have been able to reproduce this using my layout tests for MessagePorts and workers, but I can't check in that test yet as it is pending yet another patch.
Comment 6 Darin Adler 2009-06-14 11:41:14 PDT
Comment on attachment 31243 [details]
proposed patch

This seems quite awkward to me. Is there some way we can address this without adding so much new explicit code to the SmallStringsStorage class?

> +        (JSC::SmallStringsStorage::~SmallStringsStorage):
> +	Manually invokes destroy on the locally-defined Rep objects.

Tab in the change log here. The person landing this will have to fix it.

> +        * runtime/UString.cpp:
> +        (JSC::UString::Rep::destroy):
> +        Kept for backwards compatibility (exported API)

Compatibility with what? Exported where? These implementation details of JavaScriptCore should be internal and WebCore and JavaScriptGlue should be the only clients. I think this comment is a bit misleading.

> +        * runtime/UString.h:
> +        (JSC::UString::Rep::deref):
> +        Changed to call the new destroyInternal() API.

This patch puts its new ChangeLog entry in the middle of the new file, not at the top.

> +            // Cleans up the internal references for this object and frees it
>              void destroy();
> +            // Cleans up internal references with the option of freeing the object
> +            void destroyInternal(bool freeObject);

Maybe we can come up with a name clearer than "destroyInternal" for this. It seems weak to have a boolean just to control whether "delete this" is called. I suggest adding a function that does all of destroy except for the "delete this" part and then have the normal destroy call it first, and then do "delete this". If we can't think of a better name, then I suggest destroyWithoutDeleting() for the new function. Lets avoid the mysterious boolean design pattern.

I think it's confusing to refer to "delete this" in comments as "freeing the object". I'd much prefer that it say "deleting".

I'm going to say review- due to the comments above, but I agree it is crucial that we fix this soon since we just introduced the leak.
Comment 7 David Levin 2009-06-14 13:04:36 PDT
I hate to add this comment because I know you spent a lot time tracking this down (and it was my bug).  While it could be argued that the particular structure of this class caused the bug, it really was a bug in my change that happened in UString, and it has now been fixed.

SmallString is tricky in how it does things but it addresses the use of single letter string which seems to be common.  I'm concerned that this change may make the performance worse because it changes the locality of the items in this class.  

Right now all of the data is sequential in one memory location, so the data layout looks like this:
m_characters 512 bytes
m_base 48 bytes
m_reps 6144 bytes

After the change, the data layout would look more like this
m_characters 4 bytes --> pointer to real location
m_base 4 bytes -> pointer to real location
m_reps 6144 bytes
Those pointers go to some other page(s) in memory.

Given this and that the bug has been fixed.  I don't think this patch should be done.

However, if this change goes forward anyway, you need to change the "new UChar[numCharactersToStore]" to use fastMalloc (because that is how UString allocates it m_data) but you could just use Rep::createEmptyBuffer to create the BaseString (and there is no need to keep the m_characters member no matter what).
Comment 8 Andrew Wilson 2009-06-15 10:34:41 PDT
Dave has a good point about changing the memory locality of SmallStrings. Rather than try to address Darin's comments, I'm marking this as WONTFIX. The code is leak-free now, and it doesn't seem worth it to make potentially performance-affecting changes in code that is fragile but not actually broken.