Bug 82421 – REGRESSION(r110064): audio elements can keep the DOM alive after the user has navigated away

• Component: Media
• Status: RESOLVED FIXED

[Stephanie Lewis]

Testing on Lion.  Regression occurred http://trac.webkit.org/projects/webkit/changeset/110064.  

I believe this change has a typo.  I think the ! needs to be eliminated.

-            if (!static_cast<HTMLAudioElement*>(node)->paused())
+            if (!static_cast<HTMLAudioElement*>(node)->hasPendingActivity())


As it stands an HTMLAudioElement is observable when it is not waiting for any events.  This means that the DOM can never be cleaned up when a user navigates away from the page.  Observed on http://twitter.com.

<rdar://problem/11135186> REGRESSION: audio elements with pending activity progressively slow down the PLT3.

[Stephanie Lewis]

Created attachment 134210 [details]
patch

Uploaded a patch to fix the if statement, but I'm not sure if it is possible to write a test to show the bug.

[Philippe Normand]

*** Bug 81781 has been marked as a duplicate of this bug. ***

[Philippe Normand]

(In reply to comment #1)
> Created an attachment (id=134210) [details]
> patch
> 
> Uploaded a patch to fix the if statement, but I'm not sure if it is possible to write a test to show the bug.

A nice list of media tests are flaky on GTK because of this issue, thanks for looking into it and sorry, I'm a bit embarassed :)

[Philippe Normand]

I've just checked that with your patch those media tests are no longer flaky on GTK:

media/W3C/audio/networkState/networkState_during_loadstart.html
media/audio-delete-while-step-button-clicked.html
media/audio-mpeg-supported.html
media/audio-repaint.html
media/media-controls-clone-crash.html
media/audio-repaint.html


If you could unskip them as part of your patch it'd be great.

media/audio-garbage-collect.html is now failing though. I'll have a look at this one again soon.

[Alexey Proskuryakov]

I don't think that this kind of change requires a regression test. Please upload an updated patch and mark it for review.

[Geoffrey Garen]

I think it would be even better just to revert http://trac.webkit.org/projects/webkit/changeset/110064 and consult Eric and Jer about what the best fix is. It's not clear that HTMLAudioElement::hasPendingActivity is the right test for GC.

[Stephanie Lewis]

Jer wasn't convinced that the original fix was correct, but said I should talk to Eric.  Since it is such a bad performance bug I'm rolling out until I can catch up with Eric and find the right fix.

[Geoffrey Garen]

In the end, what we need is a DOM API that is true when the audio node is waiting for an event to fire, for a load to complete, or for playing to end, and is *guaranteed* to become false if the node is removed from the document or the window is closed or navigated away from.

[Eric Carlson]

(In reply to comment #6)
> I think it would be even better just to revert http://trac.webkit.org/projects/webkit/changeset/110064 and consult Eric and Jer about what the best fix is. It's not clear that HTMLAudioElement::hasPendingActivity is the right test for GC.

HTMLAudioElement::hasPendingActivity is definitely NOT the correct test. The reason that we care about this is that the spec says when a media element can be collected:

    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.

and "potentially playing" is defined as:

    A media element is said to be potentially playing when its paused 
    attribute is false, the element has not ended playback, playback has 
    not stopped due to errors, the element either has no current media 
    controller or has a current media controller but is not blocked on its 
    media controller, and the element is not a blocked media element.

As it happens, we have HTMLMediaElement::potentiallyPlaying because "potentially playing" is used in other parts of the media element spec.

[Stephanie Lewis]

Rolled out in http://trac.webkit.org/projects/webkit/changeset/112483

I reopened the original bug for doing the correct fix. (https://bugs.webkit.org/show_bug.cgi?id=72698)