Bug 167744 – Correct memory leak in MediaConstraints

• Component: Media
• Status: RESOLVED FIXED

[Brent Fulgham]

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.

[Brent Fulgham]

<rdar://problem/30331444>

[Brent Fulgham]

Created attachment 300419 [details]
Patch

[Anders Carlsson]

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.

[Brent Fulgham]

Committed r211579: <http://trac.webkit.org/changeset/211579>

[Brent Fulgham]

(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!

[Ryan Haddad]

(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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 167753

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 300477 [details]
Patch

[Brent Fulgham]

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.

[Brent Fulgham]

Tests are happy. I just want to have Anders (or maybe Darin) check that the move operator is written properly.

[Anders Carlsson]

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).

[Brent Fulgham]

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.

[Brent Fulgham]

Committed r211646: <http://trac.webkit.org/changeset/211646>