196759 2019-04-09 19:34:49 -0700 OfflineAudioDestinationNode::startRendering leaks OfflineAudioDestinationNode if offlineRender exists early 2019-04-10 13:31:06 -0700 1 1 1 Unclassified WebKit Web Audio WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 rniwa rniwa ddkilzer dino eric.carlson jer.noble youennf oldest_to_newest 1525861 0 rniwa 2019-04-09 19:34:49 -0700 OfflineAudioDestinationNode::startRendering calls ref() unconditionally but offlineRender() calls deref() conditionally. 1525862 1 rniwa 2019-04-09 19:34:58 -0700 <rdar://problem/47900062> 1525864 2 367098 rniwa 2019-04-09 19:40:23 -0700 Created attachment 367098 Fixes the bug 1526051 3 367098 eric.carlson 2019-04-10 12:02:09 -0700 Comment on attachment 367098 Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=367098&action=review > Source/WebCore/ChangeLog:9 > + But offlineRender can early exit without ever calling deref() in the main thread, leaking to the leak of s/leaking/leading/ > Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.h:69 > + bool startRenderingIfPossible(); Nit: looks like this isn't necessary. 1526054 4 367098 youennf 2019-04-10 12:06:38 -0700 Comment on attachment 367098 Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=367098&action=review > Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp:94 > + m_renderThread = Thread::create("offline renderer", [this] { Instead of ref/deref, could the thread lambda take a protectedThis=makeRef(*this) and move protectedThis to the callOnMainThread lambda below? That will make it clear that 'this' is protected and ref/deref count is always ok. 1526089 5 rniwa 2019-04-10 13:23:57 -0700 (In reply to youenn fablet from comment #4) > Comment on attachment 367098 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=367098&action=review > > > Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.cpp:94 > > + m_renderThread = Thread::create("offline renderer", [this] { > > Instead of ref/deref, could the thread lambda take a > protectedThis=makeRef(*this) and move protectedThis to the callOnMainThread > lambda below? > That will make it clear that 'this' is protected and ref/deref count is > always ok. That's a bit dangerous since OfflineAudioDestinationNode is not thread-safe ref counted. We need to very carefully copy & not copy things in each lambdas. 1526090 6 rniwa 2019-04-10 13:25:15 -0700 (In reply to Eric Carlson from comment #3) > Comment on attachment 367098 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=367098&action=review > > > Source/WebCore/ChangeLog:9 > > + But offlineRender can early exit without ever calling deref() in the main thread, leaking to the leak of > > s/leaking/leading/ Fixed. > > Source/WebCore/Modules/webaudio/OfflineAudioDestinationNode.h:69 > > + bool startRenderingIfPossible(); > > Nit: looks like this isn't necessary. Indeed. Removed. 1526092 7 rniwa 2019-04-10 13:31:06 -0700 Committed r244145: <https://trac.webkit.org/changeset/244145> 367098 2019-04-09 19:40:23 -0700 2019-04-10 12:02:09 -0700 Fixes the bug bug-196759-20190409194023.patch text/plain 5036 rniwa SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDI0NDEwMykKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI0IEBACisyMDE5LTA0LTA5ICBSeW9zdWtl IE5pd2EgIDxybml3YUB3ZWJraXQub3JnPgorCisgICAgICAgIE9mZmxpbmVBdWRpb0Rlc3RpbmF0 aW9uTm9kZTo6c3RhcnRSZW5kZXJpbmcgbGVha3MgT2ZmbGluZUF1ZGlvRGVzdGluYXRpb25Ob2Rl IGlmIG9mZmxpbmVSZW5kZXIgZXhpc3RzIGVhcmx5CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xOTY3NTkKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBPZmZsaW5lQXVkaW9EZXN0aW5hdGlvbk5vZGU6OnN0YXJ0 UmVuZGVyaW5nIHVuY29uZGl0aW9uYWxseSByZWYncyBpdHNlbGYgYmVmb3JlIGludm9raW5nIG9m ZmxpbmVSZW5kZXIoKSBpbiBhIG5ldyB0aHJlYWQuCisgICAgICAgIEJ1dCBvZmZsaW5lUmVuZGVy IGNhbiBlYXJseSBleGl0IHdpdGhvdXQgZXZlciBjYWxsaW5nIGRlcmVmKCkgaW4gdGhlIG1haW4g dGhyZWFkLCBsZWFraW5nIHRvIHRoZSBsZWFrIG9mCisgICAgICAgIE9mZmxpbmVBdWRpb0Rlc3Rp bmF0aW9uTm9kZS4gRml4ZWQgdGhlIGxlYWsgYnkgYWx3YXlzIGNhbGxpbmcgZGVyZWYgaW4gdGhl IG1haW4gdGhyZWFkIGFmdGVyIGNhbGxpbmcgb2ZmbGluZVJlbmRlcigpLgorCisgICAgICAgIEFs c28gcmVtb3ZlZCB0aGUgZGVidWcgYXNzZXJ0aW9uIGluIG9mZmxpbmVSZW5kZXIgd2hpY2ggYWx3 YXlzIGhpdHMgd2hlbiB3ZSBydW4gdGhlIHJlbGV2YW50IHRlc3QuCisKKyAgICAgICAgVGVzdDog aW1wb3J0ZWQvdzNjL3dlYi1wbGF0Zm9ybS10ZXN0cy93ZWJhdWRpby90aGUtYXVkaW8tYXBpL3Ro ZS1vZmZsaW5lYXVkaW9jb250ZXh0LWludGVyZmFjZS9jdXJyZW50LXRpbWUtYmxvY2stc2l6ZS5o dG1sCisKKyAgICAgICAgKiBNb2R1bGVzL3dlYmF1ZGlvL09mZmxpbmVBdWRpb0Rlc3RpbmF0aW9u Tm9kZS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpPZmZsaW5lQXVkaW9EZXN0aW5hdGlvbk5vZGU6 OnN0YXJ0UmVuZGVyaW5nKToKKyAgICAgICAgKFdlYkNvcmU6Ok9mZmxpbmVBdWRpb0Rlc3RpbmF0 aW9uTm9kZTo6b2ZmbGluZVJlbmRlcik6CisgICAgICAgIChXZWJDb3JlOjpPZmZsaW5lQXVkaW9E ZXN0aW5hdGlvbk5vZGU6Om5vdGlmeUNvbXBsZXRlKTogTWVyZ2VkIGludG8gc3RhcnRSZW5kZXJp bmcuCisgICAgICAgICogTW9kdWxlcy93ZWJhdWRpby9PZmZsaW5lQXVkaW9EZXN0aW5hdGlvbk5v ZGUuaDoKKwogMjAxOS0wNC0wOSAgS2VpdGggUm9sbGluICA8a3JvbGxpbkBhcHBsZS5jb20+CiAK ICAgICAgICAgVW5yZXZpZXdlZCBidWlsZCBtYWludGVuYW5jZSAtLSB1cGRhdGUgLnhjZmlsZWxp c3RzLgpJbmRleDogU291cmNlL1dlYkNvcmUvTW9kdWxlcy93ZWJhdWRpby9PZmZsaW5lQXVkaW9E ZXN0aW5hdGlvbk5vZGUuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL01vZHVsZXMv d2ViYXVkaW8vT2ZmbGluZUF1ZGlvRGVzdGluYXRpb25Ob2RlLmNwcAkocmV2aXNpb24gMjQzOTYx KQorKysgU291cmNlL1dlYkNvcmUvTW9kdWxlcy93ZWJhdWRpby9PZmZsaW5lQXVkaW9EZXN0aW5h dGlvbk5vZGUuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC04NCwzNyArODQsNDYgQEAgdm9pZCBPZmZs aW5lQXVkaW9EZXN0aW5hdGlvbk5vZGU6OnN0YXJ0UgogICAgIGlmICghbV9yZW5kZXJUYXJnZXQu Z2V0KCkpCiAgICAgICAgIHJldHVybjsKICAgICAKLSAgICBpZiAoIW1fc3RhcnRlZFJlbmRlcmlu ZykgewotICAgICAgICBtX3N0YXJ0ZWRSZW5kZXJpbmcgPSB0cnVlOwotICAgICAgICByZWYoKTsg Ly8gU2VlIGNvcnJlc3BvbmRpbmcgZGVyZWYoKSBjYWxsIGluIG5vdGlmeUNvbXBsZXRlRGlzcGF0 Y2goKS4KLSAgICAgICAgbV9yZW5kZXJUaHJlYWQgPSBUaHJlYWQ6OmNyZWF0ZSgib2ZmbGluZSBy ZW5kZXJlciIsIFt0aGlzXSB7Ci0gICAgICAgICAgICBvZmZsaW5lUmVuZGVyKCk7CisgICAgaWYg KG1fc3RhcnRlZFJlbmRlcmluZykKKyAgICAgICAgcmV0dXJuOworCisgICAgbV9zdGFydGVkUmVu ZGVyaW5nID0gdHJ1ZTsKKyAgICByZWYoKTsKKyAgICAvLyBGSVhNRTogU2hvdWxkIHdlIGNhbGwg bGF6eUluaXRpYWxpemUgaGVyZT8KKyAgICAvLyBGSVhNRTogV2Ugc2hvdWxkIHByb2JhYmx5IGxp bWl0IHRoZSBudW1iZXIgb2YgdGhyZWFkcyB3ZSBjcmVhdGUgZm9yIG9mZmxpbmUgYXVkaW8uCisg ICAgbV9yZW5kZXJUaHJlYWQgPSBUaHJlYWQ6OmNyZWF0ZSgib2ZmbGluZSByZW5kZXJlciIsIFt0 aGlzXSB7CisgICAgICAgIGJvb2wgZGlkUmVuZGVyID0gb2ZmbGluZVJlbmRlcigpOworICAgICAg ICBjYWxsT25NYWluVGhyZWFkKFt0aGlzLCBkaWRSZW5kZXJdIHsKKyAgICAgICAgICAgIGlmIChk aWRSZW5kZXIpCisgICAgICAgICAgICAgICAgY29udGV4dCgpLmZpcmVDb21wbGV0aW9uRXZlbnQo KTsKKyAgICAgICAgICAgIGRlcmVmKCk7CiAgICAgICAgIH0pOwotICAgIH0KKyAgICB9KTsKIH0K IAotdm9pZCBPZmZsaW5lQXVkaW9EZXN0aW5hdGlvbk5vZGU6Om9mZmxpbmVSZW5kZXIoKQorYm9v bCBPZmZsaW5lQXVkaW9EZXN0aW5hdGlvbk5vZGU6Om9mZmxpbmVSZW5kZXIoKQogewogICAgIEFT U0VSVCghaXNNYWluVGhyZWFkKCkpOwogICAgIEFTU0VSVChtX3JlbmRlckJ1cy5nZXQoKSk7CiAg ICAgaWYgKCFtX3JlbmRlckJ1cy5nZXQoKSkKLSAgICAgICAgcmV0dXJuOworICAgICAgICByZXR1 cm4gZmFsc2U7CiAKICAgICBib29sIGlzQXVkaW9Db250ZXh0SW5pdGlhbGl6ZWQgPSBjb250ZXh0 KCkuaXNJbml0aWFsaXplZCgpOwotICAgIEFTU0VSVChpc0F1ZGlvQ29udGV4dEluaXRpYWxpemVk KTsKKyAgICAvLyBGSVhNRTogV2UgdXNlZCB0byBhc3NlcnQgdGhhdCBpc0F1ZGlvQ29udGV4dElu aXRpYWxpemVkIGlzIHRydWUgaGVyZS4KKyAgICAvLyBCdXQgaXQncyB0cml2aWFsbHkgZmFsc2Ug aW4gaW1wb3J0ZWQvdzNjL3dlYi1wbGF0Zm9ybS10ZXN0cy93ZWJhdWRpby90aGUtYXVkaW8tYXBp L3RoZS1vZmZsaW5lYXVkaW9jb250ZXh0LWludGVyZmFjZS9jdXJyZW50LXRpbWUtYmxvY2stc2l6 ZS5odG1sIAogICAgIGlmICghaXNBdWRpb0NvbnRleHRJbml0aWFsaXplZCkKLSAgICAgICAgcmV0 dXJuOworICAgICAgICByZXR1cm4gZmFsc2U7CiAKICAgICBib29sIGNoYW5uZWxzTWF0Y2ggPSBt X3JlbmRlckJ1cy0+bnVtYmVyT2ZDaGFubmVscygpID09IG1fcmVuZGVyVGFyZ2V0LT5udW1iZXJP ZkNoYW5uZWxzKCk7CiAgICAgQVNTRVJUKGNoYW5uZWxzTWF0Y2gpOwogICAgIGlmICghY2hhbm5l bHNNYXRjaCkKLSAgICAgICAgcmV0dXJuOwotICAgICAgICAKKyAgICAgICAgcmV0dXJuIGZhbHNl OworCiAgICAgYm9vbCBpc1JlbmRlckJ1c0FsbG9jYXRlZCA9IG1fcmVuZGVyQnVzLT5sZW5ndGgo KSA+PSByZW5kZXJRdWFudHVtU2l6ZTsKICAgICBBU1NFUlQoaXNSZW5kZXJCdXNBbGxvY2F0ZWQp OwogICAgIGlmICghaXNSZW5kZXJCdXNBbGxvY2F0ZWQpCi0gICAgICAgIHJldHVybjsKLSAgICAg ICAgCisgICAgICAgIHJldHVybiBmYWxzZTsKKwogICAgIC8vIEJyZWFrIHVwIHRoZSByZW5kZXIg dGFyZ2V0IGludG8gc21hbGxlciAicmVuZGVyIHF1YW50aXplIiBzaXplZCBwaWVjZXMuCiAgICAg Ly8gUmVuZGVyIHVudGlsIHdlJ3JlIGZpbmlzaGVkLgogICAgIHNpemVfdCBmcmFtZXNUb1Byb2Nl c3MgPSBtX3JlbmRlclRhcmdldC0+bGVuZ3RoKCk7CkBAIC0xMzYsMTcgKzE0NSw4IEBAIHZvaWQg T2ZmbGluZUF1ZGlvRGVzdGluYXRpb25Ob2RlOjpvZmZsaW4KICAgICAgICAgbiArPSBmcmFtZXNB dmFpbGFibGVUb0NvcHk7CiAgICAgICAgIGZyYW1lc1RvUHJvY2VzcyAtPSBmcmFtZXNBdmFpbGFi bGVUb0NvcHk7CiAgICAgfQotICAgIAotICAgIC8vIE91ciB3b3JrIGlzIGRvbmUuIExldCB0aGUg QXVkaW9Db250ZXh0IGtub3cuCi0gICAgY2FsbE9uTWFpblRocmVhZChbdGhpc10gewotICAgICAg ICBub3RpZnlDb21wbGV0ZSgpOwotICAgICAgICBkZXJlZigpOwotICAgIH0pOwotfQogCi12b2lk IE9mZmxpbmVBdWRpb0Rlc3RpbmF0aW9uTm9kZTo6bm90aWZ5Q29tcGxldGUoKQotewotICAgIGNv bnRleHQoKS5maXJlQ29tcGxldGlvbkV2ZW50KCk7CisgICAgcmV0dXJuIHRydWU7CiB9CiAKIH0g Ly8gbmFtZXNwYWNlIFdlYkNvcmUKSW5kZXg6IFNvdXJjZS9XZWJDb3JlL01vZHVsZXMvd2ViYXVk aW8vT2ZmbGluZUF1ZGlvRGVzdGluYXRpb25Ob2RlLmgKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dl YkNvcmUvTW9kdWxlcy93ZWJhdWRpby9PZmZsaW5lQXVkaW9EZXN0aW5hdGlvbk5vZGUuaAkocmV2 aXNpb24gMjQzOTYxKQorKysgU291cmNlL1dlYkNvcmUvTW9kdWxlcy93ZWJhdWRpby9PZmZsaW5l QXVkaW9EZXN0aW5hdGlvbk5vZGUuaAkod29ya2luZyBjb3B5KQpAQCAtNjYsMTAgKzY2LDggQEAg cHJpdmF0ZToKICAgICAvLyBSZW5kZXJpbmcgdGhyZWFkLgogICAgIFJlZlB0cjxUaHJlYWQ+IG1f cmVuZGVyVGhyZWFkOwogICAgIGJvb2wgbV9zdGFydGVkUmVuZGVyaW5nOwotICAgIHZvaWQgb2Zm bGluZVJlbmRlcigpOwotICAgIAotICAgIC8vIEZvciBjb21wbGV0aW9uIGNhbGxiYWNrIG9uIG1h aW4gdGhyZWFkLgotICAgIHZvaWQgbm90aWZ5Q29tcGxldGUoKTsKKyAgICBib29sIHN0YXJ0UmVu ZGVyaW5nSWZQb3NzaWJsZSgpOworICAgIGJvb2wgb2ZmbGluZVJlbmRlcigpOwogfTsKIAogfSAv LyBuYW1lc3BhY2UgV2ViQ29yZQo=