Bug 155701 - SmallPtrSet leaks memory in its move assignment operator when !this->isSmall()
Summary: SmallPtrSet leaks memory in its move assignment operator when !this->isSmall()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-20 10:56 PDT by Saam Barati
Modified: 2016-03-23 01:21 PDT (History)
15 users (show)

See Also:


Attachments
patch (1.23 KB, patch)
2016-03-20 11:28 PDT, Saam Barati
darin: review+
Details | Formatted Diff | Diff
patch (1.28 KB, patch)
2016-03-22 17:53 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-03-20 10:56:44 PDT
...
Comment 1 Saam Barati 2016-03-20 11:28:12 PDT
Created attachment 274546 [details]
patch
Comment 2 Darin Adler 2016-03-20 16:35:00 PDT
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.
Comment 3 Saam Barati 2016-03-20 22:56:37 PDT
(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;
}
```
?
Comment 4 Darin Adler 2016-03-22 15:27:27 PDT
Yes, something like that. In simple classes there may be further tricks to do it more efficiently.
Comment 5 Saam Barati 2016-03-22 17:53:56 PDT
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?
Comment 6 Darin Adler 2016-03-22 21:24:50 PDT
If this generates more efficient code than the traditional swap idiom, I am OK with it.
Comment 7 WebKit Commit Bot 2016-03-23 01:21:06 PDT
Comment on attachment 274717 [details]
patch

Clearing flags on attachment: 274717

Committed r198579: <http://trac.webkit.org/changeset/198579>
Comment 8 WebKit Commit Bot 2016-03-23 01:21:12 PDT
All reviewed patches have been landed.  Closing bug.