From cbc12728102e898da946edfd3a79fe476d02e95c Mon Sep 17 00:00:00 2001
From: Andreas Kling <awesomekling@gmail.com>
Date: Fri, 2 Aug 2019 11:56:55 +0200
Subject: [PATCH] AK: Fix ref leaks in RefPtr assignment operators.

Many of the RefPtr assignment operators would cause ref leaks when we
call them to assign a pointer that's already the one kept.
---
 AK/RefPtr.h             | 61 +++++++++++++++++------------------------
 AK/Tests/Makefile       |  5 +++-
 AK/Tests/TestRefPtr.cpp | 60 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 89 insertions(+), 37 deletions(-)
 create mode 100644 AK/Tests/TestRefPtr.cpp

diff --git a/AK/RefPtr.h b/AK/RefPtr.h
index 91e08be088..70dc84f97f 100644
--- a/AK/RefPtr.h
+++ b/AK/RefPtr.h
@@ -2,6 +2,7 @@
 
 #include <AK/LogStream.h>
 #include <AK/NonnullRefPtr.h>
+#include <AK/StdLibExtras.h>
 #include <AK/Types.h>
 
 namespace AK {
@@ -94,88 +95,76 @@ public:
     template<typename U>
     RefPtr& operator=(const OwnPtr<U>&) = delete;
 
+    template<typename U>
+    void swap(RefPtr<U>& other)
+    {
+        ::swap(m_ptr, other.m_ptr);
+    }
+
     RefPtr& operator=(RefPtr&& other)
     {
-        if (this != &other) {
-            deref_if_not_null(m_ptr);
-            m_ptr = other.leak_ref();
-        }
+        RefPtr tmp = move(other);
+        swap(tmp);
         return *this;
     }
 
     template<typename U>
     RefPtr& operator=(RefPtr<U>&& other)
     {
-        if (this != static_cast<void*>(&other)) {
-            deref_if_not_null(m_ptr);
-            m_ptr = static_cast<T*>(other.leak_ref());
-        }
+        RefPtr tmp = move(other);
+        swap(tmp);
         return *this;
     }
 
     template<typename U>
     RefPtr& operator=(NonnullRefPtr<U>&& other)
     {
-        deref_if_not_null(m_ptr);
-        m_ptr = &other.leak_ref();
+        RefPtr tmp = move(other);
+        swap(tmp);
         return *this;
     }
 
     RefPtr& operator=(const NonnullRefPtr<T>& other)
     {
-        if (m_ptr != other.ptr())
-            deref_if_not_null(m_ptr);
-        m_ptr = const_cast<T*>(other.ptr());
-        ASSERT(m_ptr);
-        ref_if_not_null(m_ptr);
+        RefPtr tmp = other;
+        swap(tmp);
         return *this;
     }
 
     template<typename U>
     RefPtr& operator=(const NonnullRefPtr<U>& other)
     {
-        if (m_ptr != other.ptr())
-            deref_if_not_null(m_ptr);
-        m_ptr = const_cast<T*>(other.ptr());
-        ASSERT(m_ptr);
-        ref_if_not_null(m_ptr);
+        RefPtr tmp = other;
+        swap(tmp);
         return *this;
     }
 
     RefPtr& operator=(const RefPtr& other)
     {
-        if (m_ptr != other.ptr())
-            deref_if_not_null(m_ptr);
-        m_ptr = const_cast<T*>(other.ptr());
-        ref_if_not_null(m_ptr);
+        RefPtr tmp = other;
+        swap(tmp);
         return *this;
     }
 
     template<typename U>
     RefPtr& operator=(const RefPtr<U>& other)
     {
-        if (m_ptr != other.ptr())
-            deref_if_not_null(m_ptr);
-        m_ptr = const_cast<T*>(other.ptr());
-        ref_if_not_null(m_ptr);
+        RefPtr tmp = other;
+        swap(tmp);
         return *this;
     }
 
     RefPtr& operator=(const T* ptr)
     {
-        if (m_ptr != ptr)
-            deref_if_not_null(m_ptr);
-        m_ptr = const_cast<T*>(ptr);
-        ref_if_not_null(m_ptr);
+        RefPtr tmp = ptr;
+        swap(tmp);
         return *this;
     }
 
     RefPtr& operator=(const T& object)
     {
-        if (m_ptr != &object)
-            deref_if_not_null(m_ptr);
-        m_ptr = const_cast<T*>(&object);
-        ref_if_not_null(m_ptr);
+        RefPtr tmp = object;
+        swap(tmp);
         return *this;
     }
 
diff --git a/AK/Tests/Makefile b/AK/Tests/Makefile
index f1a28322a9..dc9837e915 100644
--- a/AK/Tests/Makefile
+++ b/AK/Tests/Makefile
@@ -1,4 +1,4 @@
-PROGRAMS = TestString TestQueue TestVector TestHashMap TestJSON TestWeakPtr TestNonnullRefPtr
+PROGRAMS = TestString TestQueue TestVector TestHashMap TestJSON TestWeakPtr TestNonnullRefPtr TestRefPtr
 
 CXXFLAGS = -std=c++17 -Wall -Wextra -ggdb3 -O2 -I../ -I../../
 
@@ -45,6 +45,9 @@ TestWeakPtr: TestWeakPtr.o $(SHARED_TEST_OBJS)
 TestNonnullRefPtr: TestNonnullRefPtr.o $(SHARED_TEST_OBJS)
 	$(PRE_CXX) $(CXX) $(CXXFLAGS) -o $@ TestNonnullRefPtr.o $(SHARED_TEST_OBJS)
 
+TestRefPtr: TestRefPtr.o $(SHARED_TEST_OBJS)
+	$(PRE_CXX) $(CXX) $(CXXFLAGS) -o $@ TestRefPtr.o $(SHARED_TEST_OBJS)
+
 clean:
 	rm -f $(SHARED_TEST_OBJS)
 	rm -f $(PROGRAMS)
diff --git a/AK/Tests/TestRefPtr.cpp b/AK/Tests/TestRefPtr.cpp
new file mode 100644
index 0000000000..b01df43c19
--- /dev/null
+++ b/AK/Tests/TestRefPtr.cpp
@@ -0,0 +1,60 @@
+#include <AK/TestSuite.h>
+
+#include <AK/NonnullRefPtr.h>
+#include <AK/AKString.h>
+
+struct Object : public RefCounted<Object> {
+    int x;
+};
+
+TEST_CASE(basics)
+{
+    RefPtr<Object> object = adopt(*new Object);
+    EXPECT(object.ptr() != nullptr);
+    EXPECT_EQ(object->ref_count(), 1);
+    object->ref();
+    EXPECT_EQ(object->ref_count(), 2);
+    object->deref();
+    EXPECT_EQ(object->ref_count(), 1);
+
+    {
+        NonnullRefPtr another = *object;
+        EXPECT_EQ(object->ref_count(), 2);
+    }
+
+    EXPECT_EQ(object->ref_count(), 1);
+}
+
+TEST_CASE(assign_reference)
+{
+    RefPtr<Object> object = adopt(*new Object);
+    EXPECT_EQ(object->ref_count(), 1);
+    object = *object;
+    EXPECT_EQ(object->ref_count(), 1);
+}
+
+TEST_CASE(assign_ptr)
+{
+    RefPtr<Object> object = adopt(*new Object);
+    EXPECT_EQ(object->ref_count(), 1);
+    object = object.ptr();
+    EXPECT_EQ(object->ref_count(), 1);
+}
+
+TEST_CASE(assign_moved_self)
+{
+    RefPtr<Object> object = adopt(*new Object);
+    EXPECT_EQ(object->ref_count(), 1);
+    object = move(object);
+    EXPECT_EQ(object->ref_count(), 1);
+}
+
+TEST_CASE(assign_copy_self)
+{
+    RefPtr<Object> object = adopt(*new Object);
+    EXPECT_EQ(object->ref_count(), 1);
+    object = object;
+    EXPECT_EQ(object->ref_count(), 1);
+}
+
+TEST_MAIN(String)