45306 2010-09-07 09:57:47 -0700 Media elements should derive from ActiveDOMObjects 2011-01-24 12:18:00 -0800 1 1 1 Unclassified WebKit Media 528+ (Nightly build) All All RESOLVED FIXED InRadar P2 Normal --- 51249 1 eric.carlson eric.carlson ademar ap simon.fraser webkit.review.bot oldest_to_newest 275104 0 eric.carlson 2010-09-07 09:57:47 -0700 Media elements should derive from ActiveDOMObjects instead of implementing documentWillBecomeInactive/documentDidBecomeActive. It will allow us to prevent the object from being collected when it has pending events in the async event queue, which causes an assert in a debug build and a potential crash in a release build, and to remove the <audio> specific code from isObservableThroughDOM. 275106 1 eric.carlson 2010-09-07 09:58:54 -0700 <rdar://problem/7929062> 275143 2 66739 eric.carlson 2010-09-07 10:39:35 -0700 Created attachment 66739 proposed patch 275145 3 webkit.review.bot 2010-09-07 10:41:34 -0700 Attachment 66739 did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/html/HTMLMediaElement.h:32: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style. 275146 4 eric.carlson 2010-09-07 10:42:26 -0700 This patch doesn't add a test case because https://bugs.webkit.org/show_bug.cgi?id=45309 allows the object to be collected even when it has pending events. Getting these changes in now will make it easier to reproduce that crash. 275151 5 simon.fraser 2010-09-07 10:50:13 -0700 Want to take bug 24030 too? 275157 6 66739 darin 2010-09-07 10:59:14 -0700 Comment on attachment 66739 proposed patch Do we already have tests that cover a media element with pending events where we force the last reference to be garbage collected? > + // ActiveDOMObject methods. I prefer that we use the C++ terminology. So "functions" rather than "methods". > + virtual bool canSuspend() const { return true; } There doesn’t seem to be significant benefit to putting this definition in the header, since it’s a virtual function that will only be called through a function pointer. I suggest putting it in the .cpp file instead. It's too bad that the stop function is calling another virtual function, suspend, doing an unnecessary virtual dispatch. Not a major problem, but slightly unpleasant. Instead it could call HTMLMediaElement::suspend(), but that would strange enough that we probably shouldn't do it. The headers in HTMLMediaElement.h should stay sorted alphabetically. I bet that is what our stylebot is complaining about. 275165 7 eric.carlson 2010-09-07 11:09:13 -0700 (In reply to comment #6) > (From update of attachment 66739 [details]) > Do we already have tests that cover a media element with pending events where we force the last reference > to be garbage collected? > The crashing test case attached to https://bugs.webkit.org/show_bug.cgi?id=45309 does exactly that. > > + // ActiveDOMObject methods. > > I prefer that we use the C++ terminology. So "functions" rather than "methods". > Done. > > + virtual bool canSuspend() const { return true; } > > There doesn’t seem to be significant benefit to putting this definition in the header, since it’s a virtual function > that will only be called through a function pointer. I suggest putting it in the .cpp file instead. > Done. > > The headers in HTMLMediaElement.h should stay sorted alphabetically. I bet that is what our stylebot is > complaining about. Fixed that too. Thanks! 275183 8 eric.carlson 2010-09-07 11:30:34 -0700 http://trac.webkit.org/changeset/66895 276016 9 ap 2010-09-08 13:13:35 -0700 As discussed in person, a problem with this is that ActiveDOMObjects are also suspended below modal dialogs and alerts, as well as by Web Inspector. We probably don't want that for media elements, although we do want to consolidate document inactivation code. 279121 10 eric.carlson 2010-09-14 13:11:48 -0700 (In reply to comment #9) > As discussed in person, a problem with this is that ActiveDOMObjects are also suspended below > modal dialogs and alerts, as well as by Web Inspector. We probably don't want that for media > elements, although we do want to consolidate document inactivation code. http://trac.webkit.org/changeset/67432 fix this. 339177 11 ademar 2011-01-24 12:18:00 -0800 Revision r66895 cherry-picked into qtwebkit-2.2 with commit e5e1392 <http://gitorious.org/webkit/qtwebkit/commit/e5e1392> 66739 2010-09-07 10:39:35 -0700 2010-09-07 10:59:13 -0700 proposed patch patch_4.txt text/plain 4744 eric.carlson SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA2Njg4OCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMjAgQEAKKzIwMTAtMDktMDcgIEVyaWMgQ2FybHNvbiAgPGVyaWMuY2FybHNvbkBh cHBsZS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgTWVkaWEgZWxlbWVudHMgc2hvdWxkIGRlcml2ZSBmcm9tIEFjdGl2ZURPTU9iamVjdHMKKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTQ1MzA2CisKKyAg ICAgICAgKiBodG1sL0hUTUxNZWRpYUVsZW1lbnQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6SFRN TE1lZGlhRWxlbWVudDo6SFRNTE1lZGlhRWxlbWVudCk6IEluaXRpYWxpemUgQWN0aXZlRE9NT2Jq ZWN0CisgICAgICAgIChXZWJDb3JlOjpIVE1MTWVkaWFFbGVtZW50OjpzdG9wKTogQ2FsbCBzdXNw ZW5kLCB3ZSB3YW50IHRvIGRvIHRoZSBzYW1lIHRoaW5nIGluIGJvdGggY2FzZXMuCisgICAgICAg IChXZWJDb3JlOjpIVE1MTWVkaWFFbGVtZW50OjpzdXNwZW5kKTogUmVuYW1lIGZyb20gZG9jdW1l bnRXaWxsQmVjb21lSW5hY3RpdmUuCisgICAgICAgIChXZWJDb3JlOjpIVE1MTWVkaWFFbGVtZW50 OjpyZXN1bWUpOiBSZW5hbWUgZnJvbSBkb2N1bWVudERpZEJlY29tZUFjdGl2ZS4KKyAgICAgICAg KFdlYkNvcmU6OkhUTUxNZWRpYUVsZW1lbnQ6Omhhc1BlbmRpbmdBY3Rpdml0eSk6IFJldHVybiB0 cnVlIGlmIHRoZSBldmVudCBxdWV1ZSBpcyBub3QgZW1wdHkKKyAgICAgICAgc28gdGhlIGVsZW1l bnQgY2FuJ3QgYmUgY29sbGVjdGVkIGJlZm9yZSB0aGV5IGFyZSBzZW50LgorICAgICAgICAqIGh0 bWwvSFRNTE1lZGlhRWxlbWVudC5oOgorICAgICAgICAoV2ViQ29yZTo6SFRNTE1lZGlhRWxlbWVu dDo6Y2FuU3VzcGVuZCk6CisKIDIwMTAtMDktMDcgIEtyaXN0aWFuIE1vbnNlbiAgPGtyaXN0aWFu bUBnb29nbGUuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IFN0ZXZlIEJsb2NrLgpJbmRleDog V2ViQ29yZS9odG1sL0hUTUxNZWRpYUVsZW1lbnQuY3BwCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUv aHRtbC9IVE1MTWVkaWFFbGVtZW50LmNwcAkocmV2aXNpb24gNjY1MTcpCisrKyBXZWJDb3JlL2h0 bWwvSFRNTE1lZGlhRWxlbWVudC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTgwLDggKzgwLDkgQEAg bmFtZXNwYWNlIFdlYkNvcmUgewogCiB1c2luZyBuYW1lc3BhY2UgSFRNTE5hbWVzOwogCi1IVE1M TWVkaWFFbGVtZW50OjpIVE1MTWVkaWFFbGVtZW50KGNvbnN0IFF1YWxpZmllZE5hbWUmIHRhZ05h bWUsIERvY3VtZW50KiBkb2MpCi0gICAgOiBIVE1MRWxlbWVudCh0YWdOYW1lLCBkb2MpCitIVE1M TWVkaWFFbGVtZW50OjpIVE1MTWVkaWFFbGVtZW50KGNvbnN0IFF1YWxpZmllZE5hbWUmIHRhZ05h bWUsIERvY3VtZW50KiBkb2N1bWVudCkKKyAgICA6IEhUTUxFbGVtZW50KHRhZ05hbWUsIGRvY3Vt ZW50KQorICAgICwgQWN0aXZlRE9NT2JqZWN0KGRvY3VtZW50LCB0aGlzKQogICAgICwgbV9sb2Fk VGltZXIodGhpcywgJkhUTUxNZWRpYUVsZW1lbnQ6OmxvYWRUaW1lckZpcmVkKQogICAgICwgbV9h c3luY0V2ZW50VGltZXIodGhpcywgJkhUTUxNZWRpYUVsZW1lbnQ6OmFzeW5jRXZlbnRUaW1lckZp cmVkKQogICAgICwgbV9wcm9ncmVzc0V2ZW50VGltZXIodGhpcywgJkhUTUxNZWRpYUVsZW1lbnQ6 OnByb2dyZXNzRXZlbnRUaW1lckZpcmVkKQpAQCAtMTMyLDggKzEzMyw4IEBAIEhUTUxNZWRpYUVs ZW1lbnQ6OkhUTUxNZWRpYUVsZW1lbnQoY29uc3QKICAgICAsIG1fbG9hZEluaXRpYXRlZEJ5VXNl ckdlc3R1cmUoZmFsc2UpCiAgICAgLCBtX2NvbXBsZXRlbHlMb2FkZWQoZmFsc2UpCiB7Ci0gICAg ZG9jdW1lbnQoKS0+cmVnaXN0ZXJGb3JEb2N1bWVudEFjdGl2YXRpb25DYWxsYmFja3ModGhpcyk7 Ci0gICAgZG9jdW1lbnQoKS0+cmVnaXN0ZXJGb3JNZWRpYVZvbHVtZUNhbGxiYWNrcyh0aGlzKTsK KyAgICBkb2N1bWVudC0+cmVnaXN0ZXJGb3JEb2N1bWVudEFjdGl2YXRpb25DYWxsYmFja3ModGhp cyk7CisgICAgZG9jdW1lbnQtPnJlZ2lzdGVyRm9yTWVkaWFWb2x1bWVDYWxsYmFja3ModGhpcyk7 CiB9CiAKIEhUTUxNZWRpYUVsZW1lbnQ6On5IVE1MTWVkaWFFbGVtZW50KCkKQEAgLTE4NDksNyAr MTg1MCwxMiBAQCB2b2lkIEhUTUxNZWRpYUVsZW1lbnQ6OnVzZXJDYW5jZWxsZWRMb2FkCiAgICAg bV9yZWFkeVN0YXRlID0gSEFWRV9OT1RISU5HOwogfQogCi12b2lkIEhUTUxNZWRpYUVsZW1lbnQ6 OmRvY3VtZW50V2lsbEJlY29tZUluYWN0aXZlKCkKK3ZvaWQgSFRNTE1lZGlhRWxlbWVudDo6c3Rv cCgpCit7CisgICAgc3VzcGVuZCgpOworfQorCit2b2lkIEhUTUxNZWRpYUVsZW1lbnQ6OnN1c3Bl bmQoKQogewogICAgIGlmIChtX2lzRnVsbHNjcmVlbikKICAgICAgICAgZXhpdEZ1bGxzY3JlZW4o KTsKQEAgLTE4NjcsNyArMTg3Myw3IEBAIHZvaWQgSFRNTE1lZGlhRWxlbWVudDo6ZG9jdW1lbnRX aWxsQmVjb20KICAgICBjYW5jZWxQZW5kaW5nRXZlbnRzQW5kQ2FsbGJhY2tzKCk7CiB9CiAKLXZv aWQgSFRNTE1lZGlhRWxlbWVudDo6ZG9jdW1lbnREaWRCZWNvbWVBY3RpdmUoKQordm9pZCBIVE1M TWVkaWFFbGVtZW50OjpyZXN1bWUoKQogewogICAgIG1faW5BY3RpdmVEb2N1bWVudCA9IHRydWU7 CiAgICAgc2V0UGF1c2VkSW50ZXJuYWwoZmFsc2UpOwpAQCAtMTg4NSw2ICsxODkxLDEzIEBAIHZv aWQgSFRNTE1lZGlhRWxlbWVudDo6ZG9jdW1lbnREaWRCZWNvbWUKICAgICAgICAgcmVuZGVyZXIo KS0+dXBkYXRlRnJvbUVsZW1lbnQoKTsKIH0KIAorYm9vbCBIVE1MTWVkaWFFbGVtZW50OjpoYXNQ ZW5kaW5nQWN0aXZpdHkoKSBjb25zdAoreworICAgIC8vIFJldHVybiB0cnVlIHdoZW4gd2UgaGF2 ZSBwZW5kaW5nIGV2ZW50cyBzbyB3ZSBjYW4ndCBmaXJlIGV2ZW50cyBhZnRlciB0aGUgSlMgCisg ICAgLy8gb2JqZWN0IGdldHMgY29sbGVjdGVkLgorICAgIHJldHVybiBtX3BlbmRpbmdFdmVudHMu c2l6ZSgpOworfQorCiB2b2lkIEhUTUxNZWRpYUVsZW1lbnQ6Om1lZGlhVm9sdW1lRGlkQ2hhbmdl KCkKIHsKICAgICB1cGRhdGVWb2x1bWUoKTsKSW5kZXg6IFdlYkNvcmUvaHRtbC9IVE1MTWVkaWFF bGVtZW50LmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9odG1sL0hUTUxNZWRpYUVsZW1lbnQuaAko cmV2aXNpb24gNjY1MTcpCisrKyBXZWJDb3JlL2h0bWwvSFRNTE1lZGlhRWxlbWVudC5oCSh3b3Jr aW5nIGNvcHkpCkBAIC0yOSw2ICsyOSw3IEBACiAjaWYgRU5BQkxFKFZJREVPKQogCiAjaW5jbHVk ZSAiSFRNTEVsZW1lbnQuaCIKKyNpbmNsdWRlICJBY3RpdmVET01PYmplY3QuaCIKICNpbmNsdWRl ICJNZWRpYUNhblN0YXJ0TGlzdGVuZXIuaCIKICNpbmNsdWRlICJNZWRpYVBsYXllci5oIgogCkBA IC01MSw3ICs1Miw3IEBAIGNsYXNzIFdpZGdldDsKIC8vIEJ1dCBpdCBjYW4ndCBiZSB1bnRpbCB0 aGUgQ2hyb21pdW0gV2ViTWVkaWFQbGF5ZXJDbGllbnRJbXBsIGNsYXNzIGlzIGZpeGVkIHNvIGl0 CiAvLyBubyBsb25nZXIgZGVwZW5kcyBvbiB0eXBlY2FzdGluZyBhIE1lZGlhUGxheWVyQ2xpZW50 IHRvIGFuIEhUTUxNZWRpYUVsZW1lbnQuCiAKLWNsYXNzIEhUTUxNZWRpYUVsZW1lbnQgOiBwdWJs aWMgSFRNTEVsZW1lbnQsIHB1YmxpYyBNZWRpYVBsYXllckNsaWVudCwgcHJpdmF0ZSBNZWRpYUNh blN0YXJ0TGlzdGVuZXIgeworY2xhc3MgSFRNTE1lZGlhRWxlbWVudCA6IHB1YmxpYyBIVE1MRWxl bWVudCwgcHVibGljIE1lZGlhUGxheWVyQ2xpZW50LCBwcml2YXRlIE1lZGlhQ2FuU3RhcnRMaXN0 ZW5lciwgcHJpdmF0ZSBBY3RpdmVET01PYmplY3QgewogcHVibGljOgogICAgIE1lZGlhUGxheWVy KiBwbGF5ZXIoKSBjb25zdCB7IHJldHVybiBtX3BsYXllci5nZXQoKTsgfQogICAgIApAQCAtMTk1 LDggKzE5NiwxMyBAQCBwcml2YXRlOgogICAgIGZsb2F0IGdldFRpbWVPZmZzZXRBdHRyaWJ1dGUo Y29uc3QgUXVhbGlmaWVkTmFtZSYsIGZsb2F0IHZhbHVlT25FcnJvcikgY29uc3Q7CiAgICAgdm9p ZCBzZXRUaW1lT2Zmc2V0QXR0cmlidXRlKGNvbnN0IFF1YWxpZmllZE5hbWUmLCBmbG9hdCB2YWx1 ZSk7CiAgICAgCi0gICAgdmlydHVhbCB2b2lkIGRvY3VtZW50V2lsbEJlY29tZUluYWN0aXZlKCk7 Ci0gICAgdmlydHVhbCB2b2lkIGRvY3VtZW50RGlkQmVjb21lQWN0aXZlKCk7CisgICAgLy8gQWN0 aXZlRE9NT2JqZWN0IG1ldGhvZHMuCisgICAgdmlydHVhbCBib29sIGNhblN1c3BlbmQoKSBjb25z dCB7IHJldHVybiB0cnVlOyB9CisgICAgdmlydHVhbCB2b2lkIHN1c3BlbmQoKTsKKyAgICB2aXJ0 dWFsIHZvaWQgcmVzdW1lKCk7CisgICAgdmlydHVhbCB2b2lkIHN0b3AoKTsKKyAgICB2aXJ0dWFs IGJvb2wgaGFzUGVuZGluZ0FjdGl2aXR5KCkgY29uc3Q7CisgICAgCiAgICAgdmlydHVhbCB2b2lk IG1lZGlhVm9sdW1lRGlkQ2hhbmdlKCk7CiAKICAgICB2aXJ0dWFsIHZvaWQgdXBkYXRlRGlzcGxh eVN0YXRlKCkgeyB9Cg==