LibCore: Make CObject reference-counted

Okay, I've spent a whole day on this now, and it finally kinda works!
With this patch, CObject and all of its derived classes are reference
counted instead of tree-owned.

The previous, Qt-like model was nice and familiar, but ultimately also
outdated and difficult to reason about.

CObject-derived types should now be stored in RefPtr/NonnullRefPtr and
each class can be constructed using the forwarding construct() helper:

    auto widget = GWidget::construct(parent_widget);

Note that construct() simply forwards all arguments to an existing
constructor. It is inserted into each class by the C_OBJECT macro,
see CObject.h to understand how that works.

CObject::delete_later() disappears in this patch, as there is no longer
a single logical owner of a CObject.

6 files changed, 70 insertions, 142 deletions

diff --git a/Libraries/LibCore/CEvent.h b/Libraries/LibCore/CEvent.h
index 84b34d09af..8e49c07fd4 100644
--- a/Libraries/LibCore/CEvent.h
+++ b/Libraries/LibCore/CEvent.h
@@ -15,7 +15,6 @@ public:
         Timer,
         NotifierRead,
         NotifierWrite,
-        DeferredDestroy,
         DeferredInvoke,
         ChildAdded,
         ChildRemoved,
diff --git a/Libraries/LibCore/CEventLoop.cpp b/Libraries/LibCore/CEventLoop.cpp
index e0eeda332b..df05f95784 100644
--- a/Libraries/LibCore/CEventLoop.cpp
+++ b/Libraries/LibCore/CEventLoop.cpp
@@ -43,7 +43,7 @@ public:
             int nread = m_socket->read((u8*)&length, sizeof(length));
             if (nread == 0) {
                 dbg() << "RPC client disconnected";
-                delete_later();
+                shutdown();
                 return;
             }
             ASSERT(nread == sizeof(length));
@@ -52,7 +52,7 @@ public:
             auto request_json = JsonValue::from_string(request);
             if (!request_json.is_object()) {
                 dbg() << "RPC client sent invalid request";
-                delete_later();
+                shutdown();
                 return;
             }
 
@@ -111,11 +111,17 @@ public:
         }
 
         if (type == "Disconnect") {
-            delete_later();
+            shutdown();
             return;
         }
     }
 
+    void shutdown()
+    {
+        // FIXME: This is quite a hackish way to clean ourselves up.
+        delete this;
+    }
+
 private:
     ObjectPtr<CLocalSocket> m_socket;
 };
@@ -148,7 +154,8 @@ CEventLoop::CEventLoop()
         s_rpc_server->on_ready_to_accept = [&] {
             auto client_socket = s_rpc_server->accept();
             ASSERT(client_socket);
-            new RPCClient(move(client_socket));
+            // NOTE: RPCClient will delete itself in RPCClient::shutdown().
+            (void)RPCClient::construct(move(client_socket)).leak_ref();
         };
     }
 
@@ -246,6 +253,7 @@ void CEventLoop::pump(WaitMode mode)
 #endif
             static_cast<CDeferredInvocationEvent&>(event).m_invokee(*receiver);
         } else {
+            NonnullRefPtr<CObject> protector(*receiver);
             receiver->dispatch_event(event);
         }
 
diff --git a/Libraries/LibCore/CObject.cpp b/Libraries/LibCore/CObject.cpp
index 6d5bab5b8d..e13bbcf1aa 100644
--- a/Libraries/LibCore/CObject.cpp
+++ b/Libraries/LibCore/CObject.cpp
@@ -23,13 +23,18 @@ CObject::CObject(CObject* parent, bool is_widget)
 
 CObject::~CObject()
 {
+    // NOTE: We move our children out to a stack vector to prevent other
+    //       code from trying to iterate over them.
+    auto children = move(m_children);
+    // NOTE: We also unparent the children, so that they won't try to unparent
+    //       themselves in their own destructors.
+    for (auto& child : children)
+        child.m_parent = nullptr;
+
     all_objects().remove(*this);
     stop_timer();
     if (m_parent)
         m_parent->remove_child(*this);
-    auto children_to_delete = move(m_children);
-    for (auto* child : children_to_delete)
-        delete child;
 }
 
 void CObject::event(CEvent& event)
@@ -37,9 +42,6 @@ void CObject::event(CEvent& event)
     switch (event.type()) {
     case CEvent::Timer:
         return timer_event(static_cast<CTimerEvent&>(event));
-    case CEvent::DeferredDestroy:
-        delete this;
-        break;
     case CEvent::ChildAdded:
     case CEvent::ChildRemoved:
         return child_event(static_cast<CChildEvent&>(event));
@@ -58,19 +60,24 @@ void CObject::add_child(CObject& object)
     // FIXME: Should we support reparenting objects?
     ASSERT(!object.parent() || object.parent() == this);
     object.m_parent = this;
-    m_children.append(&object);
+    m_children.append(object);
     event(*make<CChildEvent>(CEvent::ChildAdded, object));
 }
 
 void CObject::remove_child(CObject& object)
 {
-    for (ssize_t i = 0; i < m_children.size(); ++i) {
-        if (m_children[i] == &object) {
+    for (int i = 0; i < m_children.size(); ++i) {
+        dbg() << i << "] " << m_children.at(i);
+        if (m_children.ptr_at(i).ptr() == &object) {
+            // NOTE: We protect the child so it survives the handling of ChildRemoved.
+            NonnullRefPtr<CObject> protector = object;
+            object.m_parent = nullptr;
             m_children.remove(i);
             event(*make<CChildEvent>(CEvent::ChildRemoved, object));
             return;
         }
     }
+    ASSERT_NOT_REACHED();
 }
 
 void CObject::timer_event(CTimerEvent&)
@@ -104,11 +111,6 @@ void CObject::stop_timer()
     m_timer_id = 0;
 }
 
-void CObject::delete_later()
-{
-    CEventLoop::current().post_event(*this, make<CEvent>(CEvent::DeferredDestroy));
-}
-
 void CObject::dump_tree(int indent)
 {
     for (int i = 0; i < indent; ++i) {
diff --git a/Libraries/LibCore/CObject.h b/Libraries/LibCore/CObject.h
index 95fea75590..1c182a322f 100644
--- a/Libraries/LibCore/CObject.h
+++ b/Libraries/LibCore/CObject.h
@@ -2,6 +2,8 @@
 
 #include <AK/Function.h>
 #include <AK/IntrusiveList.h>
+#include <AK/Noncopyable.h>
+#include <AK/NonnullRefPtrVector.h>
 #include <AK/StdLibExtras.h>
 #include <AK/String.h>
 #include <AK/Vector.h>
@@ -21,13 +23,18 @@ class CTimerEvent;
 public:                                                                \
     virtual const char* class_name() const override { return #klass; } \
     template<class... Args>                                            \
-    static inline ObjectPtr<klass> construct(Args&&... args)           \
+    static inline NonnullRefPtr<klass> construct(Args&&... args)       \
     {                                                                  \
-        return ObjectPtr<klass>(new klass(forward<Args>(args)...));    \
+        return adopt(*new klass(forward<Args>(args)...));              \
     }
 
-class CObject : public Weakable<CObject> {
+class CObject
+    : public RefCounted<CObject>
+    , public Weakable<CObject> {
     // NOTE: No C_OBJECT macro for CObject itself.
+
+    AK_MAKE_NONCOPYABLE(CObject)
+    AK_MAKE_NONMOVABLE(CObject)
 public:
     IntrusiveListNode m_all_objects_list_node;
 
@@ -39,14 +46,14 @@ public:
     const String& name() const { return m_name; }
     void set_name(const StringView& name) { m_name = name; }
 
-    Vector<CObject*>& children() { return m_children; }
-    const Vector<CObject*>& children() const { return m_children; }
+    NonnullRefPtrVector<CObject>& children() { return m_children; }
+    const NonnullRefPtrVector<CObject>& children() const { return m_children; }
 
     template<typename Callback>
     void for_each_child(Callback callback)
     {
-        for (auto* child : m_children) {
-            if (callback(*child) == IterationDecision::Break)
+        for (auto& child : m_children) {
+            if (callback(child) == IterationDecision::Break)
                 return;
         }
     }
@@ -66,8 +73,6 @@ public:
     void add_child(CObject&);
     void remove_child(CObject&);
 
-    void delete_later();
-
     void dump_tree(int indent = 0);
 
     void deferred_invoke(Function<void(CObject&)>);
@@ -95,7 +100,7 @@ private:
     String m_name;
     int m_timer_id { 0 };
     bool m_widget { false };
-    Vector<CObject*> m_children;
+    NonnullRefPtrVector<CObject> m_children;
 };
 
 template<typename T>
diff --git a/Libraries/LibCore/CoreIPCServer.h b/Libraries/LibCore/CoreIPCServer.h
index dbec46a6ae..f8c064e8fa 100644
--- a/Libraries/LibCore/CoreIPCServer.h
+++ b/Libraries/LibCore/CoreIPCServer.h
@@ -49,22 +49,22 @@ namespace Server {
     };
 
     template<typename T, class... Args>
-    T* new_connection_for_client(Args&&... args)
+    NonnullRefPtr<T> new_connection_for_client(Args&&... args)
     {
-        auto conn = new T(AK::forward<Args>(args)...) /* arghs */;
+        auto conn = T::construct(forward<Args>(args)...);
         conn->send_greeting();
         return conn;
     }
 
     template<typename T, class... Args>
-    T* new_connection_ng_for_client(Args&&... args)
+    NonnullRefPtr<T> new_connection_ng_for_client(Args&&... args)
     {
-        return new T(AK::forward<Args>(args)...) /* arghs */;
+        return T::construct(forward<Args>(args)...) /* arghs */;
     }
 
     template<typename ServerMessage, typename ClientMessage>
     class Connection : public CObject {
-    public:
+    protected:
         Connection(CLocalSocket& socket, int client_id)
             : m_socket(socket)
             , m_client_id(client_id)
@@ -76,6 +76,7 @@ namespace Server {
 #endif
         }
 
+    public:
         ~Connection()
         {
 #if defined(CIPC_DEBUG)
@@ -108,14 +109,12 @@ namespace Server {
                 switch (errno) {
                 case EPIPE:
                     dbgprintf("Connection::post_message: Disconnected from peer.\n");
-                    delete_later();
+                    shutdown();
                     return;
-                    break;
                 case EAGAIN:
                     dbgprintf("Connection::post_message: Client buffer overflowed.\n");
                     did_misbehave();
                     return;
-                    break;
                 default:
                     perror("Connection::post_message writev");
                     ASSERT_NOT_REACHED();
@@ -134,7 +133,6 @@ namespace Server {
                 ssize_t nread = recv(m_socket->fd(), &message, sizeof(ClientMessage), MSG_DONTWAIT);
                 if (nread == 0 || (nread == -1 && errno == EAGAIN)) {
                     if (!messages_received) {
-                        // TODO: is delete_later() sufficient?
                         CEventLoop::current().post_event(*this, make<DisconnectedEvent>(client_id()));
                     }
                     break;
@@ -171,8 +169,13 @@ namespace Server {
         void did_misbehave()
         {
             dbgprintf("Connection{%p} (id=%d, pid=%d) misbehaved, disconnecting.\n", this, client_id(), m_client_pid);
+            shutdown();
+        }
+
+        void shutdown()
+        {
             m_socket->close();
-            delete_later();
+            die();
         }
 
         int client_id() const { return m_client_id; }
@@ -182,13 +185,15 @@ namespace Server {
         // ### having this public is sad
         virtual void send_greeting() = 0;
 
+        virtual void die() = 0;
+
     protected:
         void event(CEvent& event)
         {
             if (event.type() == Event::Disconnected) {
                 int client_id = static_cast<const DisconnectedEvent&>(event).client_id();
                 dbgprintf("Connection: Client disconnected: %d\n", client_id);
-                delete this;
+                die();
                 return;
             }
 
@@ -228,13 +233,12 @@ namespace Server {
                 switch (errno) {
                 case EPIPE:
                     dbg() << "Connection::post_message: Disconnected from peer";
-                    delete_later();
+                    shutdown();
                     return;
                 case EAGAIN:
                     dbg() << "Connection::post_message: Client buffer overflowed.";
                     did_misbehave();
                     return;
-                    break;
                 default:
                     perror("Connection::post_message write");
                     ASSERT_NOT_REACHED();
@@ -252,7 +256,6 @@ namespace Server {
                 ssize_t nread = recv(m_socket->fd(), buffer, sizeof(buffer), MSG_DONTWAIT);
                 if (nread == 0 || (nread == -1 && errno == EAGAIN)) {
                     if (!messages_received) {
-                        // TODO: is delete_later() sufficient?
                         CEventLoop::current().post_event(*this, make<DisconnectedEvent>(client_id()));
                     }
                     break;
@@ -278,21 +281,28 @@ namespace Server {
         void did_misbehave()
         {
             dbg() << "Connection{" << this << "} (id=" << m_client_id << ", pid=" << m_client_pid << ") misbehaved, disconnecting.";
+            shutdown();
+        }
+
+        void shutdown()
+        {
             m_socket->close();
-            delete_later();
+            die();
         }
 
         int client_id() const { return m_client_id; }
         pid_t client_pid() const { return m_client_pid; }
         void set_client_pid(pid_t pid) { m_client_pid = pid; }
 
+        virtual void die() = 0;
+
     protected:
         void event(CEvent& event) override
         {
             if (event.type() == Event::Disconnected) {
                 int client_id = static_cast<const DisconnectedEvent&>(event).client_id();
                 dbgprintf("Connection: Client disconnected: %d\n", client_id);
-                delete this;
+                die();
                 return;
             }
 
diff --git a/Libraries/LibCore/ObjectPtr.h b/Libraries/LibCore/ObjectPtr.h
index ef41e509be..8490a8877e 100644
--- a/Libraries/LibCore/ObjectPtr.h
+++ b/Libraries/LibCore/ObjectPtr.h
@@ -1,99 +1,3 @@
 #pragma once
 
-#include <AK/StdLibExtras.h>
-
-// This is a stopgap pointer. It's not meant to stick around forever.
-
-template<typename T>
-class ObjectPtr {
-public:
-    ObjectPtr() {}
-    ObjectPtr(T* ptr)
-        : m_ptr(ptr)
-    {
-    }
-    ObjectPtr(T& ptr)
-        : m_ptr(&ptr)
-    {
-    }
-    ~ObjectPtr()
-    {
-        clear();
-    }
-
-    void clear()
-    {
-        if (m_ptr && !m_ptr->parent())
-            delete m_ptr;
-        m_ptr = nullptr;
-    }
-
-    ObjectPtr& operator=(std::nullptr_t)
-    {
-        clear();
-        return *this;
-    }
-
-    template<typename U>
-    ObjectPtr(U* ptr)
-        : m_ptr(static_cast<T*>(ptr))
-    {
-    }
-
-    ObjectPtr(const ObjectPtr& other)
-        : m_ptr(other.m_ptr)
-    {
-    }
-
-    template<typename U>
-    ObjectPtr(const ObjectPtr<U>& other)
-        : m_ptr(static_cast<T*>(const_cast<ObjectPtr<U>&>(other).ptr()))
-    {
-    }
-
-    ObjectPtr(ObjectPtr&& other)
-    {
-        m_ptr = other.leak_ptr();
-    }
-
-    template<typename U>
-    ObjectPtr(const ObjectPtr<U>&& other)
-    {
-        m_ptr = static_cast<T*>(const_cast<ObjectPtr<U>&>(other).leak_ptr());
-    }
-
-    ObjectPtr& operator=(const ObjectPtr& other)
-    {
-        if (this != &other) {
-            clear();
-            m_ptr = other.m_ptr;
-        }
-        return *this;
-    }
-
-    ObjectPtr& operator=(ObjectPtr&& other)
-    {
-        if (this != &other) {
-            clear();
-            m_ptr = exchange(other.m_ptr, nullptr);
-        }
-        return *this;
-    }
-
-    T* operator->() { return m_ptr; }
-    const T* operator->() const { return m_ptr; }
-
-    operator T*() { return m_ptr; }
-    operator const T*() const { return m_ptr; }
-
-    T& operator*() { return *m_ptr; }
-    const T& operator*() const { return *m_ptr; }
-
-    T* ptr() const { return m_ptr; }
-    T* leak_ptr() { return exchange(m_ptr, nullptr); }
-
-    operator bool() const { return !!m_ptr; }
-
-private:
-    T* m_ptr { nullptr };
-};
+#define ObjectPtr RefPtr