Summary: | CompactVariableMap::Handle's copy operator= leaks the previous data | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||
Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | benjamin, commit-queue, darin, ews-watchlist, fpizlo, ggaren, gskachkov, guijemont, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Saam Barati
2019-03-06 20:55:32 PST
Created attachment 363844 [details]
patch
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 on attachment 363844 [details]
patch
r=me
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 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 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 Created attachment 363917 [details]
patch for landing
Comment on attachment 363917 [details] patch for landing Clearing flags on attachment: 363917 Committed r242613: <https://trac.webkit.org/changeset/242613> All reviewed patches have been landed. Closing bug. 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 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. |