[webkit-reviews] review denied: [Bug 26347] UString leaks memory when creating sharedBuffers for SmallStrings : [Attachment 31243] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Andrew Wilson <atwilson at google.com>'s
request for review:
Bug 26347: UString leaks memory when creating sharedBuffers for SmallStrings
https://bugs.webkit.org/show_bug.cgi?id=26347

Attachment 31243: proposed patch
https://bugs.webkit.org/attachment.cgi?id=31243&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.