Bug 195398 – CompactVariableMap::Handle's copy operator= leaks the previous data

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

Created attachment 363844 [details]
patch

[Saam Barati]

<rdar://problem/48153216>

[Saam Barati]

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.

[Yusuke Suzuki]

Comment on attachment 363844 [details]
patch

r=me

[EWS Watchlist]

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.

[Yusuke Suzuki]

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.

[Saam Barati]

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

[Saam Barati]

Created attachment 363917 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 363917 [details]
patch for landing

Clearing flags on attachment: 363917

Committed r242613: <https://trac.webkit.org/changeset/242613>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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;

[Darin Adler]

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.