Bug 64467 - [V8] Avoid memory leaks with hidden references.
Summary: [V8] Avoid memory leaks with hidden references.
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2011-07-13 11:08 PDT by Vitaly Repeshko
Modified: 2011-07-13 14:45 PDT (History)
4 users (show)

See Also:

Patch (14.84 KB, patch)
2011-07-13 11:13 PDT, Vitaly Repeshko
no flags Details | Formatted Diff | Diff
Patch (17.77 KB, patch)
2011-07-13 11:47 PDT, Vitaly Repeshko
no flags Details | Formatted Diff | Diff
Patch (18.63 KB, patch)
2011-07-13 11:52 PDT, Vitaly Repeshko
abarth: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vitaly Repeshko 2011-07-13 11:08:18 PDT
[V8] Avoid memory leaks with hidden references.
Comment 1 Vitaly Repeshko 2011-07-13 11:13:57 PDT
Created attachment 100689 [details]
Comment 2 Vitaly Repeshko 2011-07-13 11:20:15 PDT
See also http://crbug.com/84613
Comment 3 Adam Barth 2011-07-13 11:23:34 PDT
Comment on attachment 100689 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=100689&action=review

Looks reasonable, but I'd like to see one more iteration with Vector and the namespace question questions addressed.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:861
> -            push(@implContentDecls, "            V8DOMWrapper::setHiddenReference(info.Holder(), wrapper);\n");
> +            push(@implContentDecls, "            V8DOMWrapper::setNamedHiddenReference(info.Holder(), \"${attrName}\", wrapper);\n");

Do we want to put attribute names into their own namespace?

> Source/WebCore/bindings/v8/V8DOMWrapper.cpp:193
> +static const char hiddenPrefix[] = "WebCore::";

Don't we have this constant already in V8HiddenPropertyNames somewhere?

> Source/WebCore/bindings/v8/V8DOMWrapper.cpp:199
> +    char* prefixedName = new char[prefixLength + nameLength + 1];

Can we use Vector<char> instead?  All this bare-handed lifting looks wrong.  Vector<char> also can have an inline capacity, which means we can avoid the malloc in the common case of a short name.

> Source/WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:164
> +    const char* referenceName;

I'd initialize this to zero.
Comment 4 Vitaly Repeshko 2011-07-13 11:47:12 PDT
Created attachment 100698 [details]
Comment 5 Vitaly Repeshko 2011-07-13 11:52:28 PDT
Created attachment 100700 [details]
Comment 6 Adam Barth 2011-07-13 13:12:32 PDT
Comment on attachment 100700 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=100700&action=review

Thanks.  Very nice.

> Source/WebCore/bindings/v8/V8HiddenPropertyName.cpp:58
> +    return v8::String::NewSymbol(prefixedName.data(), static_cast<int>(prefixedName.size()));

Beautiful, except for the int cast!  (but I presume that's a problem with the V8 API).
Comment 7 WebKit Review Bot 2011-07-13 13:14:46 PDT
Comment on attachment 100700 [details]

Attachment 100700 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9016662
Comment 8 Vitaly Repeshko 2011-07-13 14:45:25 PDT
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/bindings/scripts/CodeGeneratorV8.pm
	M	Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp
	M	Source/WebCore/bindings/v8/V8DOMWrapper.cpp
	M	Source/WebCore/bindings/v8/V8DOMWrapper.h
	M	Source/WebCore/bindings/v8/V8HiddenPropertyName.cpp
	M	Source/WebCore/bindings/v8/V8HiddenPropertyName.h
	M	Source/WebCore/bindings/v8/WrapperTypeInfo.h
	M	Source/WebCore/bindings/v8/custom/V8CSSStyleSheetCustom.cpp
	M	Source/WebCore/bindings/v8/custom/V8DOMStringMapCustom.cpp
	M	Source/WebCore/bindings/v8/custom/V8DOMTokenListCustom.cpp
	M	Source/WebCore/bindings/v8/custom/V8LocationCustom.cpp
	M	Source/WebCore/bindings/v8/custom/V8MessageChannelConstructor.cpp
	M	Source/WebCore/bindings/v8/custom/V8NamedNodeMapCustom.cpp
	M	Source/WebCore/bindings/v8/custom/V8StyleSheetCustom.cpp
	M	Source/WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp
Committed r90949