Bug 64501 – The JSC and V8 garbage collector can not properly remove an audio element created by JavaScript "new Audio".

• Component: DOM
• Status: UNCONFIRMED

[Dongseong Hwang]

There are two problems.

1. If playing the audio element and navigating another page, the audio element is not removed.
2. GC sweeps the audio element at any time, not during playing.

[Dongseong Hwang]

Created attachment 100764 [details]
patch

The audio element's life time should synchronize with document's life time.

V8 has following problem.
1. V8 GC removes an audio element at any time.

r45537 made JS GC collect an audio element created JavaScript only if (!audio.paused()).
The condition causes 2 bugs.
1. LEAK : If playing the audio element and navigating another page,
audio.paused() returns "false".
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.

[Dongseong Hwang]

Created attachment 100784 [details]
patch

[Dongseong Hwang]

I'm worry that it is possible that I use GCController.getJSObjectCount for the leak test.
Most of the tests using GCController.getJSObjectCount were disabled because the buildbot failed the tests.

[Alexey Proskuryakov]

Comment on attachment 100784 [details]
patch

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.

[Eric Carlson]

(In reply to comment #0)
> There are two problems.
> 
> 1. If playing the audio element and navigating another page, the audio element is not removed.
> 2. GC sweeps the audio element at any time, not during playing.

Not collecting an un-paused media element that is not in the Document is correct according to the spec:

    Media elements that are potentially playing while not in a Document must not play
    any video, but should play any audio component. Media elements must not stop
    playing just because all references to them have been removed; only once a media
    element is in a state where no further audio could ever be played by that element
    may the element be garbage collected. [1]

I see that the current code is wrong in that it only considers <audio> elements, I wrote bug 64617 for this.

[1] http://www.w3.org/TR/html5/video.html#media-playback