LibWeb: Make ESO "fetch group" weakly reference the fetch records
Some checks are pending
CI / Linux, x86_64, Fuzzers, Clang (push) Waiting to run
CI / Linux, x86_64, Sanitizer, GNU (push) Waiting to run
CI / macOS, arm64, Sanitizer, Clang (push) Waiting to run
CI / Linux, x86_64, Sanitizer, Clang (push) Waiting to run
Package the js repl as a binary artifact / Linux, arm64 (push) Waiting to run
Package the js repl as a binary artifact / macOS, arm64 (push) Waiting to run
Package the js repl as a binary artifact / Linux, x86_64 (push) Waiting to run
Run test262 and test-wasm / run_and_update_results (push) Waiting to run
Lint Code / lint (push) Waiting to run
Label PRs with merge conflicts / auto-labeler (push) Waiting to run
Push notes / build (push) Waiting to run

Otherwise we end up holding on to every fetch record indefinitely.

Found by analyzing GC heap graphs on Discord.
This commit is contained in:
Andreas Kling 2025-07-29 21:23:30 +02:00 committed by Alexander Kalenik
commit 66a19b8550
Notes: github-actions[bot] 2025-07-30 00:01:41 +00:00
6 changed files with 20 additions and 9 deletions

View file

@ -1789,9 +1789,9 @@ WebIDL::ExceptionOr<GC::Ref<PendingResponse>> http_network_or_cache_fetch(JS::Re
auto& group = http_request->client()->fetch_group(); auto& group = http_request->client()->fetch_group();
// 3. Let inflightRecords be the set of fetch records in group whose requests keepalive is true and done flag is unset. // 3. Let inflightRecords be the set of fetch records in group whose requests keepalive is true and done flag is unset.
Vector<GC::Ref<Infrastructure::FetchRecord>> in_flight_records; GC::RootVector<GC::Ref<Infrastructure::FetchRecord>> in_flight_records(vm.heap());
for (auto const& fetch_record : group) { for (auto& fetch_record : group) {
if (fetch_record->request()->keepalive() && !fetch_record->request()->done()) if (fetch_record.request()->keepalive() && !fetch_record.request()->done())
in_flight_records.append(fetch_record); in_flight_records.append(fetch_record);
} }

View file

@ -5,6 +5,7 @@
*/ */
#include <LibWeb/Fetch/Infrastructure/FetchRecord.h> #include <LibWeb/Fetch/Infrastructure/FetchRecord.h>
#include <LibWeb/Fetch/Infrastructure/HTTP/Requests.h>
namespace Web::Fetch::Infrastructure { namespace Web::Fetch::Infrastructure {
@ -38,4 +39,10 @@ void FetchRecord::visit_edges(Visitor& visitor)
visitor.visit(m_fetch_controller); visitor.visit(m_fetch_controller);
} }
void FetchRecord::finalize()
{
Base::finalize();
m_list_node.remove();
}
} }

View file

@ -7,12 +7,11 @@
#pragma once #pragma once
#include <LibWeb/Fetch/Infrastructure/FetchController.h> #include <LibWeb/Fetch/Infrastructure/FetchController.h>
#include <LibWeb/Fetch/Infrastructure/HTTP/Requests.h>
namespace Web::Fetch::Infrastructure { namespace Web::Fetch::Infrastructure {
// https://fetch.spec.whatwg.org/#concept-fetch-record // https://fetch.spec.whatwg.org/#concept-fetch-record
class FetchRecord : public JS::Cell { class FetchRecord final : public JS::Cell {
GC_CELL(FetchRecord, JS::Cell); GC_CELL(FetchRecord, JS::Cell);
GC_DECLARE_ALLOCATOR(FetchRecord); GC_DECLARE_ALLOCATOR(FetchRecord);
@ -31,6 +30,7 @@ private:
FetchRecord(GC::Ref<Infrastructure::Request>, GC::Ptr<FetchController>); FetchRecord(GC::Ref<Infrastructure::Request>, GC::Ptr<FetchController>);
virtual void visit_edges(Visitor&) override; virtual void visit_edges(Visitor&) override;
virtual void finalize() override;
// https://fetch.spec.whatwg.org/#concept-request // https://fetch.spec.whatwg.org/#concept-request
// A fetch record has an associated request (a request) // A fetch record has an associated request (a request)
@ -39,6 +39,11 @@ private:
// https://fetch.spec.whatwg.org/#fetch-controller // https://fetch.spec.whatwg.org/#fetch-controller
// A fetch record has an associated controller (a fetch controller or null) // A fetch record has an associated controller (a fetch controller or null)
GC::Ptr<FetchController> m_fetch_controller { nullptr }; GC::Ptr<FetchController> m_fetch_controller { nullptr };
IntrusiveListNode<FetchRecord> m_list_node;
public:
using List = IntrusiveList<&FetchRecord::m_list_node>;
}; };
} }

View file

@ -22,7 +22,6 @@
#include <LibWeb/Fetch/Infrastructure/HTTP/Bodies.h> #include <LibWeb/Fetch/Infrastructure/HTTP/Bodies.h>
#include <LibWeb/Fetch/Infrastructure/HTTP/Headers.h> #include <LibWeb/Fetch/Infrastructure/HTTP/Headers.h>
#include <LibWeb/HTML/PolicyContainers.h> #include <LibWeb/HTML/PolicyContainers.h>
#include <LibWeb/HTML/Scripting/Environments.h>
namespace Web::Fetch::Infrastructure { namespace Web::Fetch::Infrastructure {

View file

@ -62,7 +62,6 @@ void EnvironmentSettingsObject::visit_edges(Cell::Visitor& visitor)
visitor.visit(m_responsible_event_loop); visitor.visit(m_responsible_event_loop);
visitor.visit(m_module_map); visitor.visit(m_module_map);
m_realm_execution_context->visit_edges(visitor); m_realm_execution_context->visit_edges(visitor);
visitor.visit(m_fetch_group);
visitor.visit(m_storage_manager); visitor.visit(m_storage_manager);
visitor.visit(m_service_worker_registration_object_map); visitor.visit(m_service_worker_registration_object_map);
visitor.visit(m_service_worker_object_map); visitor.visit(m_service_worker_object_map);

View file

@ -11,6 +11,7 @@
#include <LibJS/Forward.h> #include <LibJS/Forward.h>
#include <LibURL/Origin.h> #include <LibURL/Origin.h>
#include <LibURL/URL.h> #include <LibURL/URL.h>
#include <LibWeb/Fetch/Infrastructure/FetchRecord.h>
#include <LibWeb/Forward.h> #include <LibWeb/Forward.h>
#include <LibWeb/HTML/EventLoop/EventLoop.h> #include <LibWeb/HTML/EventLoop/EventLoop.h>
#include <LibWeb/HTML/Scripting/ModuleMap.h> #include <LibWeb/HTML/Scripting/ModuleMap.h>
@ -116,7 +117,7 @@ public:
EventLoop& responsible_event_loop(); EventLoop& responsible_event_loop();
// https://fetch.spec.whatwg.org/#concept-fetch-group // https://fetch.spec.whatwg.org/#concept-fetch-group
Vector<GC::Ref<Fetch::Infrastructure::FetchRecord>>& fetch_group() { return m_fetch_group; } auto& fetch_group() { return m_fetch_group; }
SerializedEnvironmentSettingsObject serialize(); SerializedEnvironmentSettingsObject serialize();
@ -146,7 +147,7 @@ private:
// https://fetch.spec.whatwg.org/#concept-fetch-record // https://fetch.spec.whatwg.org/#concept-fetch-record
// A fetch group holds an ordered list of fetch records // A fetch group holds an ordered list of fetch records
Vector<GC::Ref<Fetch::Infrastructure::FetchRecord>> m_fetch_group; Fetch::Infrastructure::FetchRecord::List m_fetch_group;
// https://storage.spec.whatwg.org/#api // https://storage.spec.whatwg.org/#api
// Each environment settings object has an associated StorageManager object. // Each environment settings object has an associated StorageManager object.