Bug 155701 – SmallPtrSet leaks memory in its move assignment operator when !this->isSmall()

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

...

[Saam Barati]

Created attachment 274546 [details]
patch

[Darin Adler]

Comment on attachment 274546 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274546&action=review

> Source/WTF/wtf/SmallPtrSet.h:60
> +        m_capacity = SmallArraySize;

Normally I prefer to write operator= based on the constructor rather than vice versa. This line of code shows what happens when we don't do it that way. Might be good to return here later and set that right.

[Saam Barati]

(In reply to comment #2)
> Comment on attachment 274546 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274546&action=review
> 
> > Source/WTF/wtf/SmallPtrSet.h:60
> > +        m_capacity = SmallArraySize;
> 
> Normally I prefer to write operator= based on the constructor rather than
> vice versa. This line of code shows what happens when we don't do it that
> way. Might be good to return here later and set that right.

Do you mean something like this:

```
SmallPtrSet& operator=(SmallPtrSet&& other)
{
    SmallPtrSet newThis(WTFMove(other));
    swap(newThis);
    return *this;
}
```
?

[Darin Adler]

Yes, something like that. In simple classes there may be further tricks to do it more efficiently.

[Saam Barati]

Created attachment 274717 [details]
patch

I stole this idea from the B3 code.
We can destruct ourself and then construct the new object
in our place inside the move assignment operator. Maybe this
is too sketchy. But it's also clever. I'm not 100% sure.

What do you think?

[Darin Adler]

If this generates more efficient code than the traditional swap idiom, I am OK with it.

[WebKit Commit Bot]

Comment on attachment 274717 [details]
patch

Clearing flags on attachment: 274717

Committed r198579: <http://trac.webkit.org/changeset/198579>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.