From b108d51c5b50558cadc71d466369c8a84dd4a9fc Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Wed, 13 Dec 2023 10:24:30 +0100 Subject: [PATCH] LibJS: Only consider VM-accessible execution contexts as strong roots Partially reverts 3dc5f467a8200b7c4043cf01d78b08745ddf8528 to fix GC memory leak that happens because we treated all execution contexts as strong roots. --- Userland/Libraries/LibJS/Heap/Heap.cpp | 3 --- Userland/Libraries/LibJS/Heap/Heap.h | 13 ---------- .../Runtime/AsyncFunctionDriverWrapper.cpp | 2 ++ .../LibJS/Runtime/AsyncGenerator.cpp | 1 + .../LibJS/Runtime/ExecutionContext.cpp | 2 -- .../LibJS/Runtime/GeneratorObject.cpp | 1 + Userland/Libraries/LibJS/Runtime/VM.cpp | 26 +++++++++++++++++++ Userland/Libraries/LibJS/SourceTextModule.cpp | 1 + .../LibWeb/HTML/Scripting/Environments.cpp | 1 + 9 files changed, 32 insertions(+), 18 deletions(-) diff --git a/Userland/Libraries/LibJS/Heap/Heap.cpp b/Userland/Libraries/LibJS/Heap/Heap.cpp index 7f88eb93f0..2b263ba473 100644 --- a/Userland/Libraries/LibJS/Heap/Heap.cpp +++ b/Userland/Libraries/LibJS/Heap/Heap.cpp @@ -450,9 +450,6 @@ void Heap::mark_live_cells(HashMap const& roots) MarkingVisitor visitor(*this, roots); - for (auto& execution_context : m_execution_contexts) - execution_context.visit_edges(visitor); - vm().bytecode_interpreter().visit_edges(visitor); visitor.mark_all_live_cells(); diff --git a/Userland/Libraries/LibJS/Heap/Heap.h b/Userland/Libraries/LibJS/Heap/Heap.h index 8ebe0bcb7d..79a0a67ed8 100644 --- a/Userland/Libraries/LibJS/Heap/Heap.h +++ b/Userland/Libraries/LibJS/Heap/Heap.h @@ -148,7 +148,6 @@ private: HandleImpl::List m_handles; MarkedVectorBase::List m_marked_vectors; WeakContainer::List m_weak_containers; - ExecutionContext::List m_execution_contexts; Vector> m_uprooted_cells; @@ -196,16 +195,4 @@ inline void Heap::did_destroy_weak_container(Badge, WeakContainer m_weak_containers.remove(set); } -inline void Heap::did_create_execution_context(Badge, ExecutionContext& set) -{ - VERIFY(!m_execution_contexts.contains(set)); - m_execution_contexts.append(set); -} - -inline void Heap::did_destroy_execution_context(Badge, ExecutionContext& set) -{ - VERIFY(m_execution_contexts.contains(set)); - m_execution_contexts.remove(set); -} - } diff --git a/Userland/Libraries/LibJS/Runtime/AsyncFunctionDriverWrapper.cpp b/Userland/Libraries/LibJS/Runtime/AsyncFunctionDriverWrapper.cpp index 535e614c63..7c02f50900 100644 --- a/Userland/Libraries/LibJS/Runtime/AsyncFunctionDriverWrapper.cpp +++ b/Userland/Libraries/LibJS/Runtime/AsyncFunctionDriverWrapper.cpp @@ -192,6 +192,8 @@ void AsyncFunctionDriverWrapper::visit_edges(Cell::Visitor& visitor) visitor.visit(m_top_level_promise); if (m_current_promise) visitor.visit(m_current_promise); + if (m_suspended_execution_context) + m_suspended_execution_context->visit_edges(visitor); } } diff --git a/Userland/Libraries/LibJS/Runtime/AsyncGenerator.cpp b/Userland/Libraries/LibJS/Runtime/AsyncGenerator.cpp index 3e18fcba79..c27e67b9cd 100644 --- a/Userland/Libraries/LibJS/Runtime/AsyncGenerator.cpp +++ b/Userland/Libraries/LibJS/Runtime/AsyncGenerator.cpp @@ -48,6 +48,7 @@ void AsyncGenerator::visit_edges(Cell::Visitor& visitor) if (m_frame) m_frame->visit_edges(visitor); visitor.visit(m_current_promise); + m_async_generator_context->visit_edges(visitor); } // 27.6.3.4 AsyncGeneratorEnqueue ( generator, completion, promiseCapability ), https://tc39.es/ecma262/#sec-asyncgeneratorenqueue diff --git a/Userland/Libraries/LibJS/Runtime/ExecutionContext.cpp b/Userland/Libraries/LibJS/Runtime/ExecutionContext.cpp index fea56cfc84..26feaf219c 100644 --- a/Userland/Libraries/LibJS/Runtime/ExecutionContext.cpp +++ b/Userland/Libraries/LibJS/Runtime/ExecutionContext.cpp @@ -21,12 +21,10 @@ NonnullOwnPtr ExecutionContext::create(Heap& heap) ExecutionContext::ExecutionContext(Heap& heap) : m_heap(heap) { - m_heap.did_create_execution_context({}, *this); } ExecutionContext::~ExecutionContext() { - m_heap.did_destroy_execution_context({}, *this); } NonnullOwnPtr ExecutionContext::copy() const diff --git a/Userland/Libraries/LibJS/Runtime/GeneratorObject.cpp b/Userland/Libraries/LibJS/Runtime/GeneratorObject.cpp index 0b9ecdbe5a..5a6d378db9 100644 --- a/Userland/Libraries/LibJS/Runtime/GeneratorObject.cpp +++ b/Userland/Libraries/LibJS/Runtime/GeneratorObject.cpp @@ -51,6 +51,7 @@ void GeneratorObject::visit_edges(Cell::Visitor& visitor) visitor.visit(m_previous_value); if (m_frame) m_frame->visit_edges(visitor); + m_execution_context->visit_edges(visitor); } // 27.5.3.2 GeneratorValidate ( generator, generatorBrand ), https://tc39.es/ecma262/#sec-generatorvalidate diff --git a/Userland/Libraries/LibJS/Runtime/VM.cpp b/Userland/Libraries/LibJS/Runtime/VM.cpp index 3cea062a37..ee473b9cb4 100644 --- a/Userland/Libraries/LibJS/Runtime/VM.cpp +++ b/Userland/Libraries/LibJS/Runtime/VM.cpp @@ -153,6 +153,20 @@ Bytecode::Interpreter& VM::bytecode_interpreter() return *m_bytecode_interpreter; } +struct ExecutionContextRootsCollector : public Cell::Visitor { + virtual void visit_impl(Cell& cell) override + { + roots.set(&cell); + } + + virtual void visit_possible_values(ReadonlyBytes) override + { + VERIFY_NOT_REACHED(); + } + + HashTable roots; +}; + void VM::gather_roots(HashMap& roots) { roots.set(m_empty_string, HeapRoot { .type = HeapRoot::Type::VM }); @@ -169,6 +183,18 @@ void VM::gather_roots(HashMap& roots) for (auto finalization_registry : m_finalization_registry_cleanup_jobs) roots.set(finalization_registry, HeapRoot { .type = HeapRoot::Type::VM }); + + auto gather_roots_from_execution_context_stack = [&roots](Vector const& stack) { + for (auto const& execution_context : stack) { + ExecutionContextRootsCollector visitor; + execution_context->visit_edges(visitor); + for (auto* cell : visitor.roots) + roots.set(cell, HeapRoot { .type = HeapRoot::Type::VM }); + } + }; + gather_roots_from_execution_context_stack(m_execution_context_stack); + for (auto& saved_stack : m_saved_execution_context_stacks) + gather_roots_from_execution_context_stack(saved_stack); } ThrowCompletionOr VM::named_evaluation_if_anonymous_function(ASTNode const& expression, DeprecatedFlyString const& name) diff --git a/Userland/Libraries/LibJS/SourceTextModule.cpp b/Userland/Libraries/LibJS/SourceTextModule.cpp index decb1142b3..0c0f7e1440 100644 --- a/Userland/Libraries/LibJS/SourceTextModule.cpp +++ b/Userland/Libraries/LibJS/SourceTextModule.cpp @@ -120,6 +120,7 @@ void SourceTextModule::visit_edges(Cell::Visitor& visitor) { Base::visit_edges(visitor); visitor.visit(m_import_meta); + m_execution_context->visit_edges(visitor); } // 16.2.1.6.1 ParseModule ( sourceText, realm, hostDefined ), https://tc39.es/ecma262/#sec-parsemodule diff --git a/Userland/Libraries/LibWeb/HTML/Scripting/Environments.cpp b/Userland/Libraries/LibWeb/HTML/Scripting/Environments.cpp index 2bf6505054..f2c1f74f05 100644 --- a/Userland/Libraries/LibWeb/HTML/Scripting/Environments.cpp +++ b/Userland/Libraries/LibWeb/HTML/Scripting/Environments.cpp @@ -45,6 +45,7 @@ void EnvironmentSettingsObject::visit_edges(Cell::Visitor& visitor) visitor.visit(target_browsing_context); visitor.visit(m_module_map); visitor.ignore(m_outstanding_rejected_promises_weak_set); + m_realm_execution_context->visit_edges(visitor); } JS::ExecutionContext& EnvironmentSettingsObject::realm_execution_context()