LibWeb+WebContent: Do not reference-count file request objects

There is currently a memory leak with these file request objects due to
the callback on_file_request_finish referencing itself in its capture
list. This object does not need to be reference counted or allocated on
the heap. It is only ever stored in a HashMap until a response is
received from the browser, and it is not shared.

9 files changed, 29 insertions, 26 deletions

diff --git a/Userland/Libraries/LibWeb/Loader/FileRequest.cpp b/Userland/Libraries/LibWeb/Loader/FileRequest.cpp
index 69294ab883..f39e8f3ffd 100644
--- a/Userland/Libraries/LibWeb/Loader/FileRequest.cpp
+++ b/Userland/Libraries/LibWeb/Loader/FileRequest.cpp
@@ -8,8 +8,9 @@
 
 namespace Web {
 
-FileRequest::FileRequest(DeprecatedString path)
-    : m_path(move(path))
+FileRequest::FileRequest(DeprecatedString path, Function<void(ErrorOr<i32>)> on_file_request_finish_callback)
+    : on_file_request_finish(move(on_file_request_finish_callback))
+    , m_path(move(path))
 {
 }
 
diff --git a/Userland/Libraries/LibWeb/Loader/FileRequest.h b/Userland/Libraries/LibWeb/Loader/FileRequest.h
index a169d970b7..afa2b70ead 100644
--- a/Userland/Libraries/LibWeb/Loader/FileRequest.h
+++ b/Userland/Libraries/LibWeb/Loader/FileRequest.h
@@ -9,13 +9,12 @@
 #include <AK/DeprecatedString.h>
 #include <AK/Error.h>
 #include <AK/Function.h>
-#include <AK/RefCounted.h>
 
 namespace Web {
 
-class FileRequest : public RefCounted<FileRequest> {
+class FileRequest {
 public:
-    explicit FileRequest(DeprecatedString path);
+    FileRequest(DeprecatedString path, Function<void(ErrorOr<i32>)> on_file_request_finish);
 
     DeprecatedString path() const;
 
diff --git a/Userland/Libraries/LibWeb/Loader/ResourceLoader.cpp b/Userland/Libraries/LibWeb/Loader/ResourceLoader.cpp
index 981ed0b1d4..acba0370fb 100644
--- a/Userland/Libraries/LibWeb/Loader/ResourceLoader.cpp
+++ b/Userland/Libraries/LibWeb/Loader/ResourceLoader.cpp
@@ -236,9 +236,8 @@ void ResourceLoader::load(LoadRequest& request, Function<void(ReadonlyBytes, Has
 
         if (!m_page.has_value())
             return;
-        VERIFY(m_page.has_value());
-        auto file_ref = make_ref_counted<FileRequest>(url.path());
-        file_ref->on_file_request_finish = [this, success_callback = move(success_callback), error_callback = move(error_callback), log_success, log_failure, request, file_ref](ErrorOr<i32> file_or_error) {
+
+        FileRequest file_request(url.path(), [this, success_callback = move(success_callback), error_callback = move(error_callback), log_success, log_failure, request](ErrorOr<i32> file_or_error) {
             --m_pending_loads;
             if (on_load_counter_change)
                 on_load_counter_change();
@@ -271,8 +270,9 @@ void ResourceLoader::load(LoadRequest& request, Function<void(ReadonlyBytes, Has
             auto data = maybe_data.release_value();
             log_success(request);
             success_callback(data, {}, {});
-        };
-        m_page->client().request_file(file_ref);
+        });
+
+        m_page->client().request_file(move(file_request));
 
         ++m_pending_loads;
         if (on_load_counter_change)
diff --git a/Userland/Libraries/LibWeb/Page/Page.h b/Userland/Libraries/LibWeb/Page/Page.h
index ceaddf0548..2857920477 100644
--- a/Userland/Libraries/LibWeb/Page/Page.h
+++ b/Userland/Libraries/LibWeb/Page/Page.h
@@ -195,7 +195,7 @@ public:
     virtual void page_did_update_resource_count(i32) { }
     virtual void page_did_close_browsing_context(HTML::BrowsingContext const&) { }
 
-    virtual void request_file(NonnullRefPtr<FileRequest>&) = 0;
+    virtual void request_file(FileRequest) = 0;
 
     // https://html.spec.whatwg.org/multipage/input.html#show-the-picker,-if-applicable
     virtual void page_did_request_file_picker(WeakPtr<DOM::EventTarget>, [[maybe_unused]] bool multiple) {};
diff --git a/Userland/Services/WebContent/ConnectionFromClient.cpp b/Userland/Services/WebContent/ConnectionFromClient.cpp
index 44550f791b..3c4e00361f 100644
--- a/Userland/Services/WebContent/ConnectionFromClient.cpp
+++ b/Userland/Services/WebContent/ConnectionFromClient.cpp
@@ -562,20 +562,23 @@ Messages::WebContentServer::GetSessionStorageEntriesResponse ConnectionFromClien
 
 void ConnectionFromClient::handle_file_return(i32 error, Optional<IPC::File> const& file, i32 request_id)
 {
-    auto result = m_requested_files.get(request_id);
-    VERIFY(result.has_value());
+    auto file_request = m_requested_files.get(request_id);
 
-    VERIFY(result.value()->on_file_request_finish);
-    result.value()->on_file_request_finish(error != 0 ? Error::from_errno(error) : ErrorOr<i32> { file->take_fd() });
+    VERIFY(file_request.has_value());
+    VERIFY(file_request.value().on_file_request_finish);
+
+    file_request.value().on_file_request_finish(error != 0 ? Error::from_errno(error) : ErrorOr<i32> { file->take_fd() });
     m_requested_files.remove(request_id);
 }
 
-void ConnectionFromClient::request_file(NonnullRefPtr<Web::FileRequest>& file_request)
+void ConnectionFromClient::request_file(Web::FileRequest file_request)
 {
     i32 const id = last_id++;
-    m_requested_files.set(id, file_request);
 
-    async_did_request_file(file_request->path(), id);
+    auto path = file_request.path();
+    m_requested_files.set(id, move(file_request));
+
+    async_did_request_file(path, id);
 }
 
 void ConnectionFromClient::set_system_visibility_state(bool visible)
diff --git a/Userland/Services/WebContent/ConnectionFromClient.h b/Userland/Services/WebContent/ConnectionFromClient.h
index 32c9bb6728..9da0d31dcd 100644
--- a/Userland/Services/WebContent/ConnectionFromClient.h
+++ b/Userland/Services/WebContent/ConnectionFromClient.h
@@ -34,7 +34,7 @@ public:
 
     void initialize_js_console(Badge<PageHost>);
 
-    void request_file(NonnullRefPtr<Web::FileRequest>&);
+    void request_file(Web::FileRequest);
 
     Optional<int> fd() { return socket().fd(); }
 
@@ -117,7 +117,7 @@ private:
     OwnPtr<WebContentConsoleClient> m_console_client;
     JS::Handle<JS::GlobalObject> m_console_global_object;
 
-    HashMap<int, NonnullRefPtr<Web::FileRequest>> m_requested_files {};
+    HashMap<int, Web::FileRequest> m_requested_files {};
     int last_id { 0 };
 };
 
diff --git a/Userland/Services/WebContent/PageHost.cpp b/Userland/Services/WebContent/PageHost.cpp
index d64d893206..728c737722 100644
--- a/Userland/Services/WebContent/PageHost.cpp
+++ b/Userland/Services/WebContent/PageHost.cpp
@@ -373,9 +373,9 @@ void PageHost::page_did_update_resource_count(i32 count_waiting)
     m_client.async_did_update_resource_count(count_waiting);
 }
 
-void PageHost::request_file(NonnullRefPtr<Web::FileRequest>& file_request)
+void PageHost::request_file(Web::FileRequest file_request)
 {
-    m_client.request_file(file_request);
+    m_client.request_file(move(file_request));
 }
 
 }
diff --git a/Userland/Services/WebContent/PageHost.h b/Userland/Services/WebContent/PageHost.h
index e5aa149a28..fe160409f3 100644
--- a/Userland/Services/WebContent/PageHost.h
+++ b/Userland/Services/WebContent/PageHost.h
@@ -97,7 +97,7 @@ private:
     virtual void page_did_set_cookie(const URL&, Web::Cookie::ParsedCookie const&, Web::Cookie::Source) override;
     virtual void page_did_update_cookie(Web::Cookie::Cookie) override;
     virtual void page_did_update_resource_count(i32) override;
-    virtual void request_file(NonnullRefPtr<Web::FileRequest>&) override;
+    virtual void request_file(Web::FileRequest) override;
 
     explicit PageHost(ConnectionFromClient&);
 
diff --git a/Userland/Utilities/headless-browser.cpp b/Userland/Utilities/headless-browser.cpp
index 526e05a363..551e921d3d 100644
--- a/Userland/Utilities/headless-browser.cpp
+++ b/Userland/Utilities/headless-browser.cpp
@@ -236,10 +236,10 @@ public:
     {
     }
 
-    void request_file(NonnullRefPtr<Web::FileRequest>& request) override
+    void request_file(Web::FileRequest request) override
     {
-        auto const file = Core::System::open(request->path(), O_RDONLY);
-        request->on_file_request_finish(file);
+        auto const file = Core::System::open(request.path(), O_RDONLY);
+        request.on_file_request_finish(file);
     }
 
 private: