82421 2012-03-27 20:30:40 -0700 REGRESSION(r110064): audio elements can keep the DOM alive after the user has navigated away 2012-03-28 18:03:50 -0700 1 1 1 Unclassified WebKit Media 528+ (Nightly build) Mac Unspecified RESOLVED FIXED InRadar P1 Normal --- 1 slewis slewis ap eric.carlson ggaren jer.noble jonlee mrobinson pnormand slewis webkit-bug-importer xan.lopez oldest_to_newest 589630 0 slewis 2012-03-27 20:30:40 -0700 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. 589631 1 134210 slewis 2012-03-27 20:40:00 -0700 Created attachment 134210 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. 589691 2 pnormand 2012-03-27 23:58:35 -0700 *** Bug 81781 has been marked as a duplicate of this bug. *** 589693 3 pnormand 2012-03-28 00:02:34 -0700 (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 :) 589704 4 pnormand 2012-03-28 00:19:42 -0700 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. 590219 5 ap 2012-03-28 11:56:45 -0700 I don't think that this kind of change requires a regression test. Please upload an updated patch and mark it for review. 590311 6 ggaren 2012-03-28 12:55:19 -0700 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. 590478 7 slewis 2012-03-28 14:59:14 -0700 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. 590526 8 ggaren 2012-03-28 15:33:41 -0700 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. 590665 9 eric.carlson 2012-03-28 17:41:24 -0700 (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. 590680 10 slewis 2012-03-28 18:03:50 -0700 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) 134210 2012-03-27 20:40:00 -0700 2012-03-27 20:40:00 -0700 patch 0001-REGRESSION-audio-elements-can-keep-the-DOM-alive-aft.patch text/plain 2169 slewis RnJvbSAxOGE2YzE5NmJmMGU3NmM1MzA1YTI1MGIzNzYzNjY0ZDFkNmRkOGQ3IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBTdGVwaGFuaWUgTGV3aXMgPHNsZXdpc0BhcHBsZS5jb20+CkRh dGU6IFR1ZSwgMjcgTWFyIDIwMTIgMjA6Mzc6MjQgLTA3MDAKU3ViamVjdDogW1BBVENIXSBSRUdS RVNTSU9OOiBhdWRpbyBlbGVtZW50cyBjYW4ga2VlcCB0aGUgRE9NIGFsaXZlIGFmdGVyIHRoZQog dXNlciBoYXMgbmF2aWdhdGVkIGF3YXkKIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD04MjQyMQoKRml4IGFuIGluY29ycmVjdCBpZiBzdGF0ZW1lbnQgdGhhdCBjYXVzZWQg SFRNTEF1ZGlvRWxlbWVudHMgdG8gYmUgb2JzZXJ2YWJsZSB3aGVuIHRoZXJlIHdhcyBubyBwZW5k aW5nIGFjdGl2aXR5Cmluc3RlYWQgb2Ygd2hlbiB0aGVyZSB3YXMuCgpSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KCk5vIG5ldyB0ZXN0cy4gKE9PUFMhKQoKKiBiaW5kaW5ncy9qcy9KU05vZGVD dXN0b20uY3BwOgooV2ViQ29yZTo6aXNSZWFjaGFibGVGcm9tRE9NKToKLS0tCiBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cgICAgICAgICAgICAgICAgICAgIHwgICAxNSArKysrKysrKysrKysrKysK IFNvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL2pzL0pTTm9kZUN1c3RvbS5jcHAgfCAgICAyICstCiAy IGZpbGVzIGNoYW5nZWQsIDE2IGluc2VydGlvbnMoKyksIDEgZGVsZXRpb25zKC0pCgpkaWZmIC0t Z2l0IGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9n CmluZGV4IDdhZjc3ZmMuLjIyNTFmODUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZworKysgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxOCBAQAorMjAx Mi0wMy0yNyAgU3RlcGhhbmllIExld2lzICA8c2xld2lzQGFwcGxlLmNvbT4KKworICAgICAgICBS RUdSRVNTSU9OOiBhdWRpbyBlbGVtZW50cyBjYW4ga2VlcCB0aGUgRE9NIGFsaXZlIGFmdGVyIHRo ZSB1c2VyIGhhcyBuYXZpZ2F0ZWQgYXdheQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9ODI0MjEKKworICAgICAgICBGaXggYW4gaW5jb3JyZWN0IGlmIHN0 YXRlbWVudCB0aGF0IGNhdXNlZCBIVE1MQXVkaW9FbGVtZW50cyB0byBiZSBvYnNlcnZhYmxlIHdo ZW4gdGhlcmUgd2FzIG5vIHBlbmRpbmcgYWN0aXZpdHkKKyAgICAgICAgaW5zdGVhZCBvZiB3aGVu IHRoZXJlIHdhcy4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBObyBuZXcgdGVzdHMuIChPT1BTISkKKworICAgICAgICAqIGJpbmRpbmdzL2pzL0pTTm9k ZUN1c3RvbS5jcHA6CisgICAgICAgIChXZWJDb3JlOjppc1JlYWNoYWJsZUZyb21ET00pOgorCiAy MDEyLTAzLTI2ICBWYW5nZWxpcyBLb2trZXZpcyAgPHZhbmdlbGlzQGNocm9taXVtLm9yZz4KIAog ICAgICAgICBbY2hyb21pdW1dIFNpbXBsaWZ5IGFuZCBmaXggQ0NMYXllclNvcnRlcgpkaWZmIC0t Z2l0IGEvU291cmNlL1dlYkNvcmUvYmluZGluZ3MvanMvSlNOb2RlQ3VzdG9tLmNwcCBiL1NvdXJj ZS9XZWJDb3JlL2JpbmRpbmdzL2pzL0pTTm9kZUN1c3RvbS5jcHAKaW5kZXggN2QwMGMwOS4uNzg5 ZjJjZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvYmluZGluZ3MvanMvSlNOb2RlQ3VzdG9t LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9qcy9KU05vZGVDdXN0b20uY3BwCkBA IC0xMTYsNyArMTE2LDcgQEAgc3RhdGljIGlubGluZSBib29sIGlzUmVhY2hhYmxlRnJvbURPTShK U05vZGUqIGpzTm9kZSwgTm9kZSogbm9kZSwgU2xvdFZpc2l0b3ImIHYKICAgICAgICAgfQogICAg ICNpZiBFTkFCTEUoVklERU8pCiAgICAgICAgIGVsc2UgaWYgKG5vZGUtPmhhc1RhZ05hbWUoYXVk aW9UYWcpKSB7Ci0gICAgICAgICAgICBpZiAoIXN0YXRpY19jYXN0PEhUTUxBdWRpb0VsZW1lbnQq Pihub2RlKS0+aGFzUGVuZGluZ0FjdGl2aXR5KCkpCisgICAgICAgICAgICBpZiAoc3RhdGljX2Nh c3Q8SFRNTEF1ZGlvRWxlbWVudCo+KG5vZGUpLT5oYXNQZW5kaW5nQWN0aXZpdHkoKSkKICAgICAg ICAgICAgICAgICByZXR1cm4gdHJ1ZTsKICAgICAgICAgfQogICAgICNlbmRpZgotLSAKMS43LjUu NAoK