[webkit-reviews] review denied: [Bug 64501] The JSC and V8 garbage collector can not properly remove an audio element created by JavaScript "new Audio". : [Attachment 100784] patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Hwang
<luxtella at company100.net>'s request for review:
Bug 64501: The JSC and V8 garbage collector can not properly remove an audio
element created by JavaScript "new Audio".
https://bugs.webkit.org/show_bug.cgi?id=64501

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=100784&action=review


> Source/WebCore/ChangeLog:17
> +	   1. LEAK : If playing the audio element and navigating another page,
> +	   audio.paused() returns "false".

This sounds like a bad bug if having a playing audio at the time of navigation
causes a world leak. However, what if the document goes to back/forward cache,
and is restored later? Should we continue playing the audio when that happens?

> Source/WebCore/ChangeLog:19
> +	   2. GC sweeps the audio element at any time, not during playing. It
makes web
> +	   developers confused because they can not play audio after GC.

I don't understand this. If an audio element created via "new Audio" is not
referenced from JavaScript, how can destroying it confuse developers? And if
there is a variable pointing to it, it won't be destroyed regardless of what
isReachableFromDOM() returns.

> Source/WebCore/bindings/js/JSNodeCustom.cpp:119
> +	       Document* doc = node->document();

An audio element always has a non-null document. Only DocumentType nodes may
have a null document(), as explained in a comment in Node.h.

While looking at this patch, I noticed some broken code in
HTMLMediaElement::mediaPlayerOwningDocument(). ownerDocument() can never return
non-null if document() returned null(). This method seems useless, and should
be simply removed. It's also virtual, but no subclass ever overrides it.

> Source/WebCore/bindings/js/JSNodeCustom.cpp:123
> +	       Page* pg = 0;
> +	       if (doc)
> +		   pg = doc->page();
> +	       bool inCurrentPage = doc && pg;

As mentioned above, this doesn't look like a correct check. Also, we try to not
have abbreviations like pg or even doc in new code.

I'm not sure what a correct check would be. You can have a look at
HTMLMediaElement::isPlaying() and HTMLMediaElement::inActiveDocument(). The
latter is confusing - it uses a data member in HTMLMediaElement, but you can
always easily figure out if an element is in active document without that.
Perhaps it doesn't do what it says, and perhaps it actually does what you need
here.

The first thing to do would be to figure out whether audio should continue
playing when a page returns from b/f cache.