Bug 82421

Summary: REGRESSION(r110064): audio elements can keep the DOM alive after the user has navigated away
Product: WebKit Reporter: Stephanie Lewis <slewis>
Component: MediaAssignee: Stephanie Lewis <slewis>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, eric.carlson, ggaren, jer.noble, jonlee, mrobinson, pnormand, slewis, webkit-bug-importer, xan.lopez
Priority: P1 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: Unspecified   
Attachments:
Description Flags
patch none

Stephanie Lewis
Reported 2012-03-27 20:30:40 PDT
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.
Attachments
patch (2.12 KB, patch)
2012-03-27 20:40 PDT, Stephanie Lewis 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Stephanie Lewis
Comment 1 2012-03-27 20:40:00 PDT
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
Comment 2 2012-03-27 23:58:35 PDT
*** Bug 81781 has been marked as a duplicate of this bug. ***
Philippe Normand
Comment 3 2012-03-28 00:02:34 PDT
(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
Comment 4 2012-03-28 00:19:42 PDT
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
Comment 5 2012-03-28 11:56:45 PDT
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
Comment 6 2012-03-28 12:55:19 PDT
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
Comment 7 2012-03-28 14:59:14 PDT
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
Comment 8 2012-03-28 15:33:41 PDT
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
Comment 9 2012-03-28 17:41:24 PDT
(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
Comment 10 2012-03-28 18:03:50 PDT
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)
Note You need to log in before you can comment on or make changes to this bug.