@@ -1,3 +1,24 @@
+2019-03-07  Saam Barati  <sbarati@apple.com>
+
+        CompactVariableMap::Handle's copy operator= leaks the previous data
+        https://bugs.webkit.org/show_bug.cgi?id=195398
+
+        Reviewed by Yusuke Suzuki.
+
+        The copy constructor was just assigning |this| to the new value,
+        forgetting to decrement the ref count of the thing pointed to by
+        the |this| handle. Based on Yusuke's suggestion, this patch refactors
+        the move constructor, move operator=, and copy operator= to use the
+        swap() primitive and the copy constructor primitive.
+
+        * parser/VariableEnvironment.cpp:
+        (JSC::CompactVariableMap::Handle::Handle):
+        (JSC::CompactVariableMap::Handle::swap):
+        (JSC::CompactVariableMap::Handle::operator=): Deleted.
+        * parser/VariableEnvironment.h:
+        (JSC::CompactVariableMap::Handle::Handle):
+        (JSC::CompactVariableMap::Handle::operator=):
+
 2019-03-07  Tadeu Zagallo  <tzagallo@apple.com>
 
         Lazily decode cached bytecode
 
@@ -193,26 +193,20 @@ CompactVariableMap::Handle::~Handle()
 }
 
 CompactVariableMap::Handle::Handle(const CompactVariableMap::Handle& other)
+    : m_environment(other.m_environment)
+    , m_map(other.m_map)
 {
-    *this = other;
-}
-
-CompactVariableMap::Handle& CompactVariableMap::Handle::operator=(const Handle& other)
-{
-    m_map = other.m_map;
-    m_environment = other.m_environment;
-
-    if (!m_map) {
-        ASSERT(!m_environment);
-        // This happens if `other` were moved into a different handle.
-        return *this;
+    if (m_map) {
+        auto iter = m_map->m_map.find(CompactVariableMapKey { *m_environment });
+        RELEASE_ASSERT(iter != m_map->m_map.end());
+        ++iter->value;
     }
+}
 
-    auto iter = m_map->m_map.find(CompactVariableMapKey { *m_environment });
-    RELEASE_ASSERT(iter != m_map->m_map.end());
-    ++iter->value;
-
-    return *this;
+CompactVariableMap::Handle::Handle(CompactVariableEnvironment& environment, CompactVariableMap& map)
+    : m_environment(&environment)
+    , m_map(&map)
+{ 
 }
 
 } // namespace JSC
@@ -214,21 +214,26 @@ class CompactVariableMap : public RefCounted<CompactVariableMap> {
     public:
         Handle() = default;
 
-        Handle(CompactVariableEnvironment& environment, CompactVariableMap& map)
-            : m_environment(&environment)
-            , m_map(&map)
-        { }
+        Handle(CompactVariableEnvironment&, CompactVariableMap&);
+
         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);
+        }
+        Handle& operator=(Handle&& other)
+        {
+            Handle handle(WTFMove(other));
+            swap(handle);
+            return *this;
         }
 
         Handle(const Handle&);
-        Handle& operator=(const Handle&);
+        Handle& operator=(const Handle& other)
+        {
+            Handle handle(other);
+            swap(handle);
+            return *this;
+        }
 
         ~Handle();
 
@@ -240,6 +245,12 @@ class CompactVariableMap : public RefCounted<CompactVariableMap> {
         }
 
     private:
+        void swap(Handle& other)
+        {
+            std::swap(other.m_environment, m_environment);
+            std::swap(other.m_map, m_map);
+        }
+
         CompactVariableEnvironment* m_environment { nullptr };
         RefPtr<CompactVariableMap> m_map;
     };