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.
<rdar://problem/30331444>
Created attachment 300419 [details] Patch
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.
Committed r211579: <http://trac.webkit.org/changeset/211579>
(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!
(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
Re-opened since this is blocked by bug 167753
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.
Created attachment 300477 [details] Patch
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.
Tests are happy. I just want to have Anders (or maybe Darin) check that the move operator is written properly.
Comment on attachment 300477 [details] 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).
Comment on attachment 300477 [details] 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.
Committed r211646: <http://trac.webkit.org/changeset/211646>