167744 2017-02-02 10:50:46 -0800 Correct memory leak in MediaConstraints 2017-02-03 12:55:02 -0800 1 1 1 Unclassified WebKit Media WebKit Nightly Build All All RESOLVED FIXED InRadar P2 Normal --- 167753 1 bfulgham bfulgham andersca bfulgham commit-queue darin ryanhaddad webkit-bug-importer oldest_to_newest 1272527 0 bfulgham 2017-02-02 10:50:46 -0800 Static analysis discovered a memory leak in the handling of ConstraintHolders. 1. ConstraintHolder::create returns a reference to a “new” object. 2. m_variants holds a set of ConstraintHolder objects. 3. m_variants.append(ConstraintHolder::create(…)) copies the returned reference, leaking the new object. I discussed this with Anders Carlsson, who confirmed that we should just return the object by value here, and return-value-optimization will make sure we efficiently use the memory. 1272528 1 bfulgham 2017-02-02 10:52:49 -0800 <rdar://problem/30331444> 1272531 2 300419 bfulgham 2017-02-02 10:55:19 -0800 Created attachment 300419 Patch 1272537 3 300419 andersca 2017-02-02 10:59:25 -0800 Comment on attachment 300419 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300419&action=review > Source/WebCore/platform/mediastream/MediaConstraints.h:716 > + static ConstraintHolder create(const MediaConstraint& value) { return *new ConstraintHolder(value); } This should just be return ConstraintHolder(value) otherwise we'll still leak. 1272555 4 bfulgham 2017-02-02 11:34:35 -0800 Committed r211579: <http://trac.webkit.org/changeset/211579> 1272556 5 bfulgham 2017-02-02 11:35:15 -0800 (In reply to comment #3) > Comment on attachment 300419 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=300419&action=review > > > Source/WebCore/platform/mediastream/MediaConstraints.h:716 > > + static ConstraintHolder create(const MediaConstraint& value) { return *new ConstraintHolder(value); } > > This should just be return ConstraintHolder(value) otherwise we'll still > leak. Done! 1272602 6 ryanhaddad 2017-02-02 13:15:57 -0800 (In reply to comment #4) > Committed r211579: <http://trac.webkit.org/changeset/211579> This change caused 5 mediastream LayoutTests to crash: https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r211582%20(3234)/results.html 1272615 7 commit-queue 2017-02-02 13:30:33 -0800 Re-opened since this is blocked by bug 167753 1272619 8 bfulgham 2017-02-02 13:31:42 -0800 This isn't quite right. I introduced some media stream crashes with this change, so I'm rolling it out under https://bugs.webkit.org/show_bug.cgi?id=167753. 1272745 9 300477 bfulgham 2017-02-02 17:37:05 -0800 Created attachment 300477 Patch 1272746 10 bfulgham 2017-02-02 17:39:05 -0800 The problem was that the move operator was not clearing the union, so when the "move source" was cleaned up, the memory was getting freed, even though the pointers had been moved to the "move target" location. 1272777 11 bfulgham 2017-02-02 20:22:40 -0800 Tests are happy. I just want to have Anders (or maybe Darin) check that the move operator is written properly. 1273023 12 300477 andersca 2017-02-03 12:39:12 -0800 Comment on attachment 300477 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300477&action=review > Source/WebCore/platform/mediastream/MediaConstraints.h:749 > + m_value.asInteger = WTFMove(other.m_value.asInteger); > + other.m_value.asInteger = nullptr; No need to use move with pointers, just do something like m_value.asInteger = std::exchange(other.m_value.asInteger, nullptr); (same thing for the other cases). 1273024 13 300477 bfulgham 2017-02-03 12:41:21 -0800 Comment on attachment 300477 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300477&action=review >> Source/WebCore/platform/mediastream/MediaConstraints.h:749 >> + other.m_value.asInteger = nullptr; > > No need to use move with pointers, just do something like > > m_value.asInteger = std::exchange(other.m_value.asInteger, nullptr); > > (same thing for the other cases). Oh, cool! Will change. 1273029 14 bfulgham 2017-02-03 12:55:02 -0800 Committed r211646: <http://trac.webkit.org/changeset/211646> 300419 2017-02-02 10:55:19 -0800 2017-02-02 17:37:02 -0800 Patch bug-167744-20170202105306.patch text/plain 1704 bfulgham SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIxMTU3MikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBACisyMDE3LTAyLTAyICBCcmVudCBG dWxnaGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgorCisgICAgICAgIENvcnJlY3QgbWVtb3J5IGxl YWsgaW4gTWVkaWFDb25zdHJhaW50cworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9MTY3NzQ0CisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS8zMDMzMTQ0ND4K KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBDb25zdHJh aW50SG9sZGVyIHJldHVybnMgYSByZWZlcmVuY2UgdG8gYW4gb2JqZWN0IGNyZWF0ZWQgYnkgb3Bl cmF0b3IgbmV3LiBXaGVuIHRoZQorICAgICAgICByZXR1cm5lZCB2YWx1ZSBpcyBhc3NpZ25lZCBv ciBzdG9yZWQgaW4gVmVjdG9yIG9yIG90aGVyIGNvbnRhaW5lcnMgd2UgbGVhayBtZW1vcnkuCisg ICAgICAgIEluc3RlYWQsIHRoaXMgdmFsdWUgc2hvdWxkIGp1c3QgYmUgcmV0dXJuZWQgYXMgYSBy ZWd1bGFyIG9iamVjdCBzbyB0aGF0IHRoZSByZXR1cm4KKyAgICAgICAgdmFsdWUgb3B0aW1pemF0 aW9uIGNhbiBtYWtlIHN1cmUgbWVtb3J5IGlzIHByb3Blcmx5IChhbmQgZWZmaWNpZW50bHkpIHVz ZWQuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9tZWRpYXN0cmVhbS9NZWRpYUNvbnN0cmFpbnRzLmg6 CisgICAgICAgIChXZWJDb3JlOjpGbGF0dGVuZWRDb25zdHJhaW50OjpDb25zdHJhaW50SG9sZGVy OjpjcmVhdGUpOiBSZXR1cm4gYnkgdmFsdWUuCisKIDIwMTctMDItMDIgIEFuZHJlYXMgS2xpbmcg IDxha2xpbmdAYXBwbGUuY29tPgogCiAgICAgICAgIFtNYWNdIEluLXByb2Nlc3MgbWVtb3J5IHBy ZXNzdXJlIG1vbml0b3IgZm9yIFdlYkNvbnRlbnQgcHJvY2Vzc2VzIEFLQSB3ZWJzYW0KSW5kZXg6 IFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL21lZGlhc3RyZWFtL01lZGlhQ29uc3RyYWludHMuaAo9 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9tZWRpYXN0cmVhbS9NZWRpYUNv bnN0cmFpbnRzLmgJKHJldmlzaW9uIDIxMTUwMSkKKysrIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3Jt L21lZGlhc3RyZWFtL01lZGlhQ29uc3RyYWludHMuaAkod29ya2luZyBjb3B5KQpAQCAtNzEzLDcg KzcxMyw3IEBAIHB1YmxpYzoKIHByaXZhdGU6CiAgICAgY2xhc3MgQ29uc3RyYWludEhvbGRlciB7 CiAgICAgcHVibGljOgotICAgICAgICBzdGF0aWMgQ29uc3RyYWludEhvbGRlciYgY3JlYXRlKGNv bnN0IE1lZGlhQ29uc3RyYWludCYgdmFsdWUpIHsgcmV0dXJuICpuZXcgQ29uc3RyYWludEhvbGRl cih2YWx1ZSk7IH0KKyAgICAgICAgc3RhdGljIENvbnN0cmFpbnRIb2xkZXIgY3JlYXRlKGNvbnN0 IE1lZGlhQ29uc3RyYWludCYgdmFsdWUpIHsgcmV0dXJuICpuZXcgQ29uc3RyYWludEhvbGRlcih2 YWx1ZSk7IH0KIAogICAgICAgICB+Q29uc3RyYWludEhvbGRlcigpCiAgICAgICAgIHsK 300477 2017-02-02 17:37:05 -0800 2017-02-03 12:45:33 -0800 Patch bug-167744-20170202173451.patch text/plain 4263 bfulgham SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIxMTYxNCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIyIEBACisyMDE3LTAyLTAyICBCcmVudCBG dWxnaGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgorCisgICAgICAgIENvcnJlY3QgbWVtb3J5IGxl YWsgaW4gTWVkaWFDb25zdHJhaW50cworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9MTY3NzQ0CisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS8zMDMzMTQ0ND4K KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBDb25zdHJh aW50SG9sZGVyIHJldHVybnMgYSByZWZlcmVuY2UgdG8gYW4gb2JqZWN0IGNyZWF0ZWQgYnkgb3Bl cmF0b3IgbmV3LiBXaGVuIHRoZQorICAgICAgICByZXR1cm5lZCB2YWx1ZSBpcyBhc3NpZ25lZCBv ciBzdG9yZWQgaW4gVmVjdG9yIG9yIG90aGVyIGNvbnRhaW5lcnMgd2UgbGVhayBtZW1vcnkuCisg ICAgICAgIEluc3RlYWQsIHRoaXMgdmFsdWUgc2hvdWxkIGp1c3QgYmUgcmV0dXJuZWQgYXMgYSBy ZWd1bGFyIG9iamVjdCBzbyB0aGF0IHRoZSByZXR1cm4KKyAgICAgICAgdmFsdWUgb3B0aW1pemF0 aW9uIGNhbiBtYWtlIHN1cmUgbWVtb3J5IGlzIHByb3Blcmx5IChhbmQgZWZmaWNpZW50bHkpIHVz ZWQuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9tZWRpYXN0cmVhbS9NZWRpYUNvbnN0cmFpbnRzLmg6 CisgICAgICAgIChXZWJDb3JlOjpGbGF0dGVuZWRDb25zdHJhaW50OjpDb25zdHJhaW50SG9sZGVy OjpjcmVhdGUpOiBSZXR1cm4gYnkgdmFsdWUuCisgICAgICAgIChXZWJDb3JlOjpGbGF0dGVuZWRD b25zdHJhaW50OjpDb25zdHJhaW50SG9sZGVyOjp+Q29uc3RyYWludEhvbGRlcik6IERvbid0IGF0 dGVtcHQgdG8KKyAgICAgICAgZGVsZXRlIG1lbW9yeSB0aGF0IHdhcyBtb3ZlZCBhd2F5LgorICAg ICAgICAoV2ViQ29yZTo6RmxhdHRlbmVkQ29uc3RyYWludDo6Q29uc3RyYWludEhvbGRlcjo6Q29u c3RyYWludEhvbGRlcik6IEFkZCBNb3ZlIGNvbnN0cnVjdG9yLgorCiAyMDE3LTAyLTAyICBXZW5z b24gSHNpZWggIDx3ZW5zb25faHNpZWhAYXBwbGUuY29tPgogCiAgICAgICAgIENyYXNoVHJhY2Vy OiBbVVNFUl0gY29tLmFwcGxlLldlYktpdC5XZWJDb250ZW50IGF0IGNvbS5hcHBsZS5XZWJDb3Jl OiBXZWJDb3JlOjpVUkw6Omhvc3QgY29uc3QgKyA5CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9wbGF0 Zm9ybS9tZWRpYXN0cmVhbS9NZWRpYUNvbnN0cmFpbnRzLmgKPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vbWVkaWFzdHJlYW0vTWVkaWFDb25zdHJhaW50cy5oCShyZXZpc2lv biAyMTE2MTQpCisrKyBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9tZWRpYXN0cmVhbS9NZWRpYUNv bnN0cmFpbnRzLmgJKHdvcmtpbmcgY29weSkKQEAgLTcxMywzMSArNzEzLDU3IEBAIHB1YmxpYzoK IHByaXZhdGU6CiAgICAgY2xhc3MgQ29uc3RyYWludEhvbGRlciB7CiAgICAgcHVibGljOgotICAg ICAgICBzdGF0aWMgQ29uc3RyYWludEhvbGRlciYgY3JlYXRlKGNvbnN0IE1lZGlhQ29uc3RyYWlu dCYgdmFsdWUpIHsgcmV0dXJuICpuZXcgQ29uc3RyYWludEhvbGRlcih2YWx1ZSk7IH0KKyAgICAg ICAgc3RhdGljIENvbnN0cmFpbnRIb2xkZXIgY3JlYXRlKGNvbnN0IE1lZGlhQ29uc3RyYWludCYg dmFsdWUpIHsgcmV0dXJuIENvbnN0cmFpbnRIb2xkZXIodmFsdWUpOyB9CiAKICAgICAgICAgfkNv bnN0cmFpbnRIb2xkZXIoKQogICAgICAgICB7Ci0gICAgICAgICAgICBzd2l0Y2ggKGRhdGFUeXBl KCkpIHsKKyAgICAgICAgICAgIGlmIChtX3ZhbHVlLmFzUmF3KSB7CisgICAgICAgICAgICAgICAg c3dpdGNoIChkYXRhVHlwZSgpKSB7CisgICAgICAgICAgICAgICAgY2FzZSBNZWRpYUNvbnN0cmFp bnQ6OkRhdGFUeXBlOjpJbnRlZ2VyOgorICAgICAgICAgICAgICAgICAgICBkZWxldGUgbV92YWx1 ZS5hc0ludGVnZXI7CisgICAgICAgICAgICAgICAgICAgIGJyZWFrOworICAgICAgICAgICAgICAg IGNhc2UgTWVkaWFDb25zdHJhaW50OjpEYXRhVHlwZTo6RG91YmxlOgorICAgICAgICAgICAgICAg ICAgICBkZWxldGUgbV92YWx1ZS5hc0RvdWJsZTsKKyAgICAgICAgICAgICAgICAgICAgYnJlYWs7 CisgICAgICAgICAgICAgICAgY2FzZSBNZWRpYUNvbnN0cmFpbnQ6OkRhdGFUeXBlOjpCb29sZWFu OgorICAgICAgICAgICAgICAgICAgICBkZWxldGUgbV92YWx1ZS5hc0Jvb2xlYW47CisgICAgICAg ICAgICAgICAgICAgIGJyZWFrOworICAgICAgICAgICAgICAgIGNhc2UgTWVkaWFDb25zdHJhaW50 OjpEYXRhVHlwZTo6U3RyaW5nOgorICAgICAgICAgICAgICAgICAgICBkZWxldGUgbV92YWx1ZS5h c1N0cmluZzsKKyAgICAgICAgICAgICAgICAgICAgYnJlYWs7CisgICAgICAgICAgICAgICAgY2Fz ZSBNZWRpYUNvbnN0cmFpbnQ6OkRhdGFUeXBlOjpOb25lOgorICAgICAgICAgICAgICAgICAgICBB U1NFUlRfTk9UX1JFQUNIRUQoKTsKKyAgICAgICAgICAgICAgICAgICAgYnJlYWs7CisgICAgICAg ICAgICAgICAgfQorICAgICAgICAgICAgfQorI2lmbmRlZiBOREVCVUcKKyAgICAgICAgICAgIG1f dmFsdWUuYXNSYXcgPSByZWludGVycHJldF9jYXN0PE1lZGlhQ29uc3RyYWludCo+KDB4YmJhZGJl ZWYpOworI2VuZGlmCisgICAgICAgIH0KKworICAgICAgICBDb25zdHJhaW50SG9sZGVyKENvbnN0 cmFpbnRIb2xkZXImJiBvdGhlcikKKyAgICAgICAgeworICAgICAgICAgICAgc3dpdGNoIChvdGhl ci5kYXRhVHlwZSgpKSB7CiAgICAgICAgICAgICBjYXNlIE1lZGlhQ29uc3RyYWludDo6RGF0YVR5 cGU6OkludGVnZXI6Ci0gICAgICAgICAgICAgICAgZGVsZXRlIG1fdmFsdWUuYXNJbnRlZ2VyOwor ICAgICAgICAgICAgICAgIG1fdmFsdWUuYXNJbnRlZ2VyID0gV1RGTW92ZShvdGhlci5tX3ZhbHVl LmFzSW50ZWdlcik7CisgICAgICAgICAgICAgICAgb3RoZXIubV92YWx1ZS5hc0ludGVnZXIgPSBu dWxscHRyOwogICAgICAgICAgICAgICAgIGJyZWFrOwogICAgICAgICAgICAgY2FzZSBNZWRpYUNv bnN0cmFpbnQ6OkRhdGFUeXBlOjpEb3VibGU6Ci0gICAgICAgICAgICAgICAgZGVsZXRlIG1fdmFs dWUuYXNEb3VibGU7CisgICAgICAgICAgICAgICAgbV92YWx1ZS5hc0RvdWJsZSA9IFdURk1vdmUo b3RoZXIubV92YWx1ZS5hc0RvdWJsZSk7CisgICAgICAgICAgICAgICAgb3RoZXIubV92YWx1ZS5h c0RvdWJsZSA9IG51bGxwdHI7CiAgICAgICAgICAgICAgICAgYnJlYWs7CiAgICAgICAgICAgICBj YXNlIE1lZGlhQ29uc3RyYWludDo6RGF0YVR5cGU6OkJvb2xlYW46Ci0gICAgICAgICAgICAgICAg ZGVsZXRlIG1fdmFsdWUuYXNCb29sZWFuOworICAgICAgICAgICAgICAgIG1fdmFsdWUuYXNCb29s ZWFuID0gV1RGTW92ZShvdGhlci5tX3ZhbHVlLmFzQm9vbGVhbik7CisgICAgICAgICAgICAgICAg b3RoZXIubV92YWx1ZS5hc0Jvb2xlYW4gPSBudWxscHRyOwogICAgICAgICAgICAgICAgIGJyZWFr OwogICAgICAgICAgICAgY2FzZSBNZWRpYUNvbnN0cmFpbnQ6OkRhdGFUeXBlOjpTdHJpbmc6Ci0g ICAgICAgICAgICAgICAgZGVsZXRlIG1fdmFsdWUuYXNTdHJpbmc7CisgICAgICAgICAgICAgICAg bV92YWx1ZS5hc1N0cmluZyA9IFdURk1vdmUob3RoZXIubV92YWx1ZS5hc1N0cmluZyk7CisgICAg ICAgICAgICAgICAgb3RoZXIubV92YWx1ZS5hc1N0cmluZyA9IG51bGxwdHI7CiAgICAgICAgICAg ICAgICAgYnJlYWs7CiAgICAgICAgICAgICBjYXNlIE1lZGlhQ29uc3RyYWludDo6RGF0YVR5cGU6 Ok5vbmU6CiAgICAgICAgICAgICAgICAgQVNTRVJUX05PVF9SRUFDSEVEKCk7CiAgICAgICAgICAg ICAgICAgYnJlYWs7CiAgICAgICAgICAgICB9Ci0KLSNpZm5kZWYgTkRFQlVHCi0gICAgICAgICAg ICBtX3ZhbHVlLmFzUmF3ID0gcmVpbnRlcnByZXRfY2FzdDxNZWRpYUNvbnN0cmFpbnQqPigweGJi YWRiZWVmKTsKLSNlbmRpZgogICAgICAgICB9CiAKICAgICAgICAgTWVkaWFDb25zdHJhaW50JiBj b25zdHJhaW50KCkgY29uc3QgeyByZXR1cm4gKm1fdmFsdWUuYXNSYXc7IH0K