Bug 64467 – [V8] Avoid memory leaks with hidden references.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Vitaly Repeshko]

[V8] Avoid memory leaks with hidden references.

[Vitaly Repeshko]

Created attachment 100689 [details]
Patch

[Vitaly Repeshko]

See also http://crbug.com/84613

[Adam Barth]

Comment on attachment 100689 [details]
Patch

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.

[Vitaly Repeshko]

Created attachment 100698 [details]
Patch

[Vitaly Repeshko]

Created attachment 100700 [details]
Patch

[Adam Barth]

Comment on attachment 100700 [details]
Patch

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).

[WebKit Review Bot]

Comment on attachment 100700 [details]
Patch

Attachment 100700 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9016662

[Vitaly Repeshko]

	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