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

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied 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 109669: Patch
https://bugs.webkit.org/attachment.cgi?id=109669&action=review

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


I'm not sure the part of this change that adds the hidden property is correct. 
There can be many listeners added for each event.  I thought it was the job of
V8EventListener (or one of those related classes) to stop the listener from
being collected by GC.

If I were making this change, I would break it down into two parts.

1) Make CodeGeneratorV8.pm respect the ActiveDOMObject IDL attribute.  That
will fix the proximate bug with HTMLAudioElement.
2) Fix the bug with event listeners getting garbage collected.

I suspect (2) is better fixed in the V8EventListener objects, which are already
involved in managing the lifetimes of these objects.

> 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

Ah!  That makes sense.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1174
> +	   // value. Spec guarantees that one can have only a single event
listener
> +	   // per event type. 

Why do we only have a single event listener for each event type?  Can't you add
more with addEventListener?  Maybe you mean the onclick-like properties in the
DOM?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2866
> -    # FIXME: Consider making this an .idl attribute.
> +    # FIXME: Make this an .idl attribute.

It is already an IDL attribute.  We need to respect the IDL attribute.