[webkit-reviews] review granted: [Bug 66878] HTMLAudioElement can be garbage collected while it playing : [Attachment 110580] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Eugene Nalimov <enal at google.com>'s
request for review:
Bug 66878: HTMLAudioElement can be garbage collected while it playing
https://bugs.webkit.org/show_bug.cgi?id=66878

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

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


> Source/WebCore/ChangeLog:13
> +	   (b) fix v8 bindings bug that was in the code forever, event listener

> +	   could be garbage collected even if object it attached to is still
> +	   alive, resulting in the loss of events.

This part of the change doesn't appear to be in this patch anymore.  Maybe
update the ChangeLog?

> Source/WebCore/bindings/v8/custom/V8HTMLAudioElementConstructor.cpp:50
> -WrapperTypeInfo V8HTMLAudioElementConstructor::info = {
V8HTMLAudioElementConstructor::GetTemplate, 0, 0, 0 };
> +WrapperTypeInfo V8HTMLAudioElementConstructor::info = {
V8HTMLAudioElementConstructor::GetTemplate, V8HTMLAudioElement::derefObject,
V8HTMLAudioElement::toActiveDOMObject, 0 };

Do we need to do this for every ActiveDOMObject?  Why doesn't the code
generator do this for us?

> Source/WebCore/bindings/v8/custom/V8HTMLAudioElementConstructor.cpp:78
> -    V8DOMWrapper::setJSWrapperForDOMNode(audio.get(),
v8::Persistent<v8::Object>::New(args.Holder()));
> +    V8DOMWrapper::setJSWrapperForActiveDOMObject(audio.get(),
v8::Persistent<v8::Object>::New(args.Holder()));

Do we need to do this for every active dom object?  Should we check all the
constructors for active DOM objects?

> LayoutTests/fast/xpath/xpath-result-eventlistener-crash.html:29
> -    for (var i = 0; i < 5000; ++i)
> +    for (var i = 0; i < 2000; ++i)

This change seems unrelated.