[Webkit-unassigned] [Bug 64501] The JSC and V8 garbage collector can not properly remove an audio element created by JavaScript "new Audio".

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=64501


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #100784|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #4 from Alexey Proskuryakov <ap at webkit.org>  2011-07-14 09:24:28 PST ---
(From update of attachment 100784)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.