[webkit-reviews] review denied: [Bug 93661] MutationObservers should not create circular, leaky references : [Attachment 161068] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Elliott Sprehn
<esprehn at chromium.org>'s request for review:
Bug 93661: MutationObservers should not create circular, leaky references
https://bugs.webkit.org/show_bug.cgi?id=93661

Attachment 161068: Patch
https://bugs.webkit.org/attachment.cgi?id=161068&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=161068&action=review


This looks like a very plausible approach.  I agree with inferno that we should
try to do this with composition.

> Source/WebCore/bindings/v8/V8HiddenPropertyName.cpp:63
>      Vector<char, 64> prefixedName;
>      prefixedName.append(V8_HIDDEN_PROPERTY_PREFIX,
sizeof(V8_HIDDEN_PROPERTY_PREFIX) - 1);
>      ASSERT(name && strlen(name));
>      prefixedName.append(name, strlen(name));

We should add a call to prefixedName.reserveCapacity(...)

> Source/WebCore/bindings/v8/V8HiddenPropertyName.h:60
> -	   static v8::Handle<v8::String> hiddenReferenceName(const char* name);

> +	   static v8::Handle<v8::String> hiddenReferenceName(const char* name,
bool symbol = true);

Rather than using a raw boolean parameter, can we use an enum?

> Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:51
> +	   m_value.makeWeak();

I don't think you need to keep a handle to m_value.

> Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:52
> +	   m_wrapper.makeWeak();

It makes sense to keep a reference to m_wrapper so that you can remove the
property if C++ tells you that you don't need it anymore.

> Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:65
> +    v8::Handle<v8::Object> scriptValue() const
> +    {
> +	   return m_value.get();
> +    }

This doesn't appear to be needed.

> Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:80
> +	   static double nextId = 0;

Please add an ASSERT(mainThread()) because this code isn't thread-safe.

> Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:83
> +	   return
v8::Persistent<v8::String>::New(V8HiddenPropertyName::hiddenReferenceName(numbe
rToString(nextId++, buffer), false));

This isn't sufficient to guarantee the uniqueness of the name.	We need to add
the string "V8WrapperRetainedValue" in there somewhere.

> Source/WebCore/bindings/v8/V8WrapperRetainedValue.h:88
> +    v8::Persistent<v8::String> m_propertyName;

This should be a ScopedPersistent.  Currently you're leaking the v8::String.