From f20aa82c2770d1cf725384f64e9990df4d44b33a Mon Sep 17 00:00:00 2001
From: Timothy Flynn <trflynn89@pm.me>
Date: Sun, 9 Feb 2025 13:00:10 -0500
Subject: [PATCH] LibThreading: Avert a reference cycle in BackgroundAction

When a BackgroundAction completes, it resolves a Promise (stored on the
BackgroundAction object) with a reference to itself. The Promise will
never unset this resolved value, thus it will hold a strong reference to
the BackgroundAction until it is destroyed. But because the Promise is
owned by the BackgroundAction itself, we have a reference cycle, and
neither object can be destroyed.

The only user of BackgroundAction is the ImageDecoder process. The
consequence was that the ImageDecoder process would never release any
image data for successfully decoded images.

To fix this, instead of storing the promise on the class itself, we can
just create it as a local variable and pass it around.
---
 Libraries/LibThreading/BackgroundAction.h | 24 +++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/Libraries/LibThreading/BackgroundAction.h b/Libraries/LibThreading/BackgroundAction.h
index c783c87696..263b94f674 100644
--- a/Libraries/LibThreading/BackgroundAction.h
+++ b/Libraries/LibThreading/BackgroundAction.h
@@ -56,12 +56,13 @@ public:
 
 private:
     BackgroundAction(ESCAPING Function<ErrorOr<Result>(BackgroundAction&)> action, ESCAPING Function<ErrorOr<void>(Result)> on_complete, ESCAPING Optional<Function<void(Error)>> on_error = {})
-        : m_promise(Promise::try_create().release_value_but_fixme_should_propagate_errors())
-        , m_action(move(action))
+        : m_action(move(action))
         , m_on_complete(move(on_complete))
     {
+        auto promise = Promise::construct();
+
         if (m_on_complete) {
-            m_promise->on_resolution = [](NonnullRefPtr<Core::EventReceiver>& object) -> ErrorOr<void> {
+            promise->on_resolution = [](NonnullRefPtr<Core::EventReceiver>& object) -> ErrorOr<void> {
                 auto self = static_ptr_cast<BackgroundAction<Result>>(object);
                 VERIFY(self->m_result.has_value());
                 if (auto maybe_error = self->m_on_complete(self->m_result.value()); maybe_error.is_error())
@@ -69,24 +70,27 @@ private:
 
                 return {};
             };
-            Core::EventLoop::current().add_job(m_promise);
+            Core::EventLoop::current().add_job(promise);
         }
 
         if (on_error.has_value())
             m_on_error = on_error.release_value();
 
-        enqueue_work([self = NonnullRefPtr(*this), origin_event_loop = &Core::EventLoop::current()]() {
+        enqueue_work([self = NonnullRefPtr(*this), promise = move(promise), origin_event_loop = &Core::EventLoop::current()]() mutable {
             auto result = self->m_action(*self);
+
             // The event loop cancels the promise when it exits.
-            self->m_canceled |= self->m_promise->is_rejected();
+            self->m_canceled |= promise->is_rejected();
+
             // All of our work was successful and we weren't cancelled; resolve the event loop's promise.
             if (!self->m_canceled && !result.is_error()) {
                 self->m_result = result.release_value();
+
                 // If there is no completion callback, we don't rely on the user keeping around the event loop.
                 if (self->m_on_complete) {
-                    origin_event_loop->deferred_invoke([self] {
+                    origin_event_loop->deferred_invoke([self, promise = move(promise)] {
                         // Our promise's resolution function will never error.
-                        (void)self->m_promise->resolve(*self);
+                        (void)promise->resolve(*self);
                     });
                     origin_event_loop->wake();
                 }
@@ -96,7 +100,8 @@ private:
                 if (result.is_error())
                     error = result.release_error();
 
-                self->m_promise->reject(Error::from_errno(ECANCELED));
+                promise->reject(Error::from_errno(ECANCELED));
+
                 if (!self->m_canceled && self->m_on_error) {
                     origin_event_loop->deferred_invoke([self, error = move(error)]() mutable {
                         self->m_on_error(move(error));
@@ -109,7 +114,6 @@ private:
         });
     }
 
-    NonnullRefPtr<Promise> m_promise;
     Function<ErrorOr<Result>(BackgroundAction&)> m_action;
     Function<ErrorOr<void>(Result)> m_on_complete;
     Function<void(Error)> m_on_error = [](Error error) {