Bug 195398 - CompactVariableMap::Handle's copy operator= leaks the previous data
Summary: CompactVariableMap::Handle's copy operator= leaks the previous data
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: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-06 20:55 PST by Saam Barati
Modified: 2019-03-08 12:58 PST (History)
16 users (show)

See Also:


Attachments
patch (4.27 KB, patch)
2019-03-06 21:07 PST, Saam Barati
ysuzuki: review+
Details | Formatted Diff | Diff
patch for landing (4.27 KB, patch)
2019-03-07 13:26 PST, 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 2019-03-06 20:55:32 PST
...
Comment 1 Saam Barati 2019-03-06 21:07:02 PST
Created attachment 363844 [details]
patch
Comment 2 Saam Barati 2019-03-06 21:07:50 PST
<rdar://problem/48153216>
Comment 3 Saam Barati 2019-03-06 21:08:15 PST
Comment on attachment 363844 [details]
patch

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

> Source/JavaScriptCore/parser/VariableEnvironment.cpp:-203
> -    m_map = other.m_map;
> -    m_environment = other.m_environment;

The prior bug was here. We needed to deref the hash table entry for m_environment.
Comment 4 Yusuke Suzuki 2019-03-06 21:08:44 PST
Comment on attachment 363844 [details]
patch

r=me
Comment 5 EWS Watchlist 2019-03-06 21:10:18 PST
Attachment 363844 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/VariableEnvironment.h:217:  The parameter name "environment" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/VariableEnvironment.h:217:  The parameter name "map" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Yusuke Suzuki 2019-03-06 21:38:52 PST
Comment on attachment 363844 [details]
patch

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

> Source/JavaScriptCore/parser/VariableEnvironment.h:222
>          Handle(Handle&& other)
> -            : m_environment(other.m_environment)
> -            , m_map(WTFMove(other.m_map))
>          {
> -            RELEASE_ASSERT(!!m_environment == !!m_map);
> -            ASSERT(!other.m_map);
> -            other.m_environment = nullptr;
> +            swap(other);
> +        }

I've just investigated more, and, the following way is the another possible change.

1. defining move constructor too, as like the old one (move, and assign null to the original one)
2. defining move assignment operator as move-and-swap.

Handle& operator=(Handle&& other)
{
    Handle handle(WTFMove(other));
    swap(handle);
    return *this;
}

This would be better since (1) we can nullify the original `other`, and (2) move-and-swap idiom is well aligned to copy-and-swap in copy assignment operator.
Comment 7 Saam Barati 2019-03-06 21:55:45 PST
Comment on attachment 363844 [details]
patch

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

>> Source/JavaScriptCore/parser/VariableEnvironment.h:222
>> +        }
> 
> I've just investigated more, and, the following way is the another possible change.
> 
> 1. defining move constructor too, as like the old one (move, and assign null to the original one)
> 2. defining move assignment operator as move-and-swap.
> 
> Handle& operator=(Handle&& other)
> {
>     Handle handle(WTFMove(other));
>     swap(handle);
>     return *this;
> }
> 
> This would be better since (1) we can nullify the original `other`, and (2) move-and-swap idiom is well aligned to copy-and-swap in copy assignment operator.

This move constructor does nullify “other”. I like your suggestion for move assignment operator
Comment 8 Saam Barati 2019-03-07 13:26:09 PST
Created attachment 363917 [details]
patch for landing
Comment 9 WebKit Commit Bot 2019-03-07 14:41:23 PST
Comment on attachment 363917 [details]
patch for landing

Clearing flags on attachment: 363917

Committed r242613: <https://trac.webkit.org/changeset/242613>
Comment 10 WebKit Commit Bot 2019-03-07 14:41:25 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Darin Adler 2019-03-08 12:57:32 PST
Comment on attachment 363917 [details]
patch for landing

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

> Source/JavaScriptCore/parser/VariableEnvironment.h:227
> +            Handle handle(WTFMove(other));
> +            swap(handle);
> +            return *this;

This can instead just be:

    swap(other);
    return *this;
Comment 12 Darin Adler 2019-03-08 12:58:03 PST
Comment on attachment 363917 [details]
patch for landing

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

>> Source/JavaScriptCore/parser/VariableEnvironment.h:227
>> +            return *this;
> 
> This can instead just be:
> 
>     swap(other);
>     return *this;

Oh, I see, it wouldn’t nullify if we did it that way. OK, I retract that suggestion.