From 1768d70823da997e14af7b462492e31f976a3b4c Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Sat, 22 Jul 2023 06:53:22 +0200 Subject: [PATCH] Revert "LibJS: Remove "uprooting" mechanism from garbage collector" This reverts commit 6232ad3a0d44a3833c476d37bf4084d39a10a74b. Unfortunately this introduced some flakiness on CI, so it wasn't quite this simple. --- Tests/LibJS/test-js.cpp | 28 +++++++++++++++++++ Userland/Libraries/LibJS/Heap/Heap.cpp | 10 +++++++ Userland/Libraries/LibJS/Heap/Heap.h | 4 +++ ...alizationRegistry.prototype.cleanupSome.js | 7 +++-- .../builtins/WeakMap/WeakMap.prototype.set.js | 25 ++++++----------- .../builtins/WeakSet/WeakSet.prototype.add.js | 19 ++++++------- 6 files changed, 65 insertions(+), 28 deletions(-) diff --git a/Tests/LibJS/test-js.cpp b/Tests/LibJS/test-js.cpp index e1528ce1f5b..352d48347c7 100644 --- a/Tests/LibJS/test-js.cpp +++ b/Tests/LibJS/test-js.cpp @@ -49,6 +49,34 @@ TESTJS_GLOBAL_FUNCTION(get_weak_map_size, getWeakMapSize) return JS::Value(weak_map.values().size()); } +TESTJS_GLOBAL_FUNCTION(mark_as_garbage, markAsGarbage) +{ + auto argument = vm.argument(0); + if (!argument.is_string()) + return vm.throw_completion(JS::ErrorType::NotAString, TRY_OR_THROW_OOM(vm, argument.to_string_without_side_effects())); + + auto& variable_name = argument.as_string(); + + // In native functions we don't have a lexical environment so get the outer via the execution stack. + auto outer_environment = vm.execution_context_stack().last_matching([&](auto& execution_context) { + return execution_context->lexical_environment != nullptr; + }); + if (!outer_environment.has_value()) + return vm.throw_completion(JS::ErrorType::UnknownIdentifier, TRY(variable_name.deprecated_string())); + + auto reference = TRY(vm.resolve_binding(TRY(variable_name.deprecated_string()), outer_environment.value()->lexical_environment)); + + auto value = TRY(reference.get_value(vm)); + + if (!can_be_held_weakly(value)) + return vm.throw_completion(JS::ErrorType::CannotBeHeldWeakly, DeprecatedString::formatted("Variable with name {}", TRY(variable_name.deprecated_string()))); + + vm.heap().uproot_cell(&value.as_cell()); + TRY(reference.delete_(vm)); + + return JS::js_undefined(); +} + TESTJS_GLOBAL_FUNCTION(detach_array_buffer, detachArrayBuffer) { auto array_buffer = vm.argument(0); diff --git a/Userland/Libraries/LibJS/Heap/Heap.cpp b/Userland/Libraries/LibJS/Heap/Heap.cpp index a3d0335183a..732647843a1 100644 --- a/Userland/Libraries/LibJS/Heap/Heap.cpp +++ b/Userland/Libraries/LibJS/Heap/Heap.cpp @@ -276,6 +276,11 @@ void Heap::mark_live_cells(HashTable const& roots) bytecode_interpreter->visit_edges(visitor); visitor.mark_all_live_cells(); + + for (auto& inverse_root : m_uprooted_cells) + inverse_root->set_marked(false); + + m_uprooted_cells.clear(); } bool Heap::cell_must_survive_garbage_collection(Cell const& cell) @@ -422,6 +427,11 @@ void Heap::undefer_gc(Badge) } } +void Heap::uproot_cell(Cell* cell) +{ + m_uprooted_cells.append(cell); +} + void register_safe_function_closure(void* base, size_t size) { if (!s_custom_ranges_for_conservative_scan) { diff --git a/Userland/Libraries/LibJS/Heap/Heap.h b/Userland/Libraries/LibJS/Heap/Heap.h index e2b68242172..946a2a5cc24 100644 --- a/Userland/Libraries/LibJS/Heap/Heap.h +++ b/Userland/Libraries/LibJS/Heap/Heap.h @@ -76,6 +76,8 @@ public: BlockAllocator& block_allocator() { return m_block_allocator; } + void uproot_cell(Cell* cell); + private: static bool cell_must_survive_garbage_collection(Cell const&); @@ -110,6 +112,8 @@ private: MarkedVectorBase::List m_marked_vectors; WeakContainer::List m_weak_containers; + Vector> m_uprooted_cells; + BlockAllocator m_block_allocator; size_t m_gc_deferrals { 0 }; diff --git a/Userland/Libraries/LibJS/Tests/builtins/FinalizationRegistry/FinalizationRegistry.prototype.cleanupSome.js b/Userland/Libraries/LibJS/Tests/builtins/FinalizationRegistry/FinalizationRegistry.prototype.cleanupSome.js index 7553d96b828..313a1649ab8 100644 --- a/Userland/Libraries/LibJS/Tests/builtins/FinalizationRegistry/FinalizationRegistry.prototype.cleanupSome.js +++ b/Userland/Libraries/LibJS/Tests/builtins/FinalizationRegistry/FinalizationRegistry.prototype.cleanupSome.js @@ -3,7 +3,9 @@ test("length is 0", () => { }); function registerInDifferentScope(registry) { - registry.register({}, {}); + const target = {}; + registry.register(target, {}); + return target; } test("basic functionality", () => { @@ -18,7 +20,8 @@ test("basic functionality", () => { expect(count).toBe(0); - registerInDifferentScope(registry); + const target = registerInDifferentScope(registry); + markAsGarbage("target"); gc(); registry.cleanupSome(increment); diff --git a/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.prototype.set.js b/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.prototype.set.js index 8be4f0d08ad..14d38d65129 100644 --- a/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.prototype.set.js +++ b/Userland/Libraries/LibJS/Tests/builtins/WeakMap/WeakMap.prototype.set.js @@ -1,9 +1,3 @@ -function registerInDifferentScope(registry) { - const target = {}; - registry.register(target, {}); - eval(""); -} - test("basic functionality", () => { expect(WeakMap.prototype.set).toHaveLength(2); @@ -27,26 +21,25 @@ test("invalid values", () => { }); }); -function setObjectKey(weakMap) { - expect(weakMap.set({ e: 3 }, 1)).toBe(weakMap); -} - -function setSymbolKey(weakMap) { - expect(weakMap.set(Symbol("foo"), "bar")).toBe(weakMap); -} - test("automatic removal of garbage-collected values", () => { const weakMap = new WeakMap(); + const objectKey = { e: 3 }; - setObjectKey(weakMap); + expect(weakMap.set(objectKey, 1)).toBe(weakMap); expect(getWeakMapSize(weakMap)).toBe(1); + markAsGarbage("objectKey"); gc(); + expect(getWeakMapSize(weakMap)).toBe(0); - setSymbolKey(weakMap); + const symbolKey = Symbol("foo"); + + expect(weakMap.set(symbolKey, "bar")).toBe(weakMap); expect(getWeakMapSize(weakMap)).toBe(1); + markAsGarbage("symbolKey"); gc(); + expect(getWeakMapSize(weakMap)).toBe(0); }); diff --git a/Userland/Libraries/LibJS/Tests/builtins/WeakSet/WeakSet.prototype.add.js b/Userland/Libraries/LibJS/Tests/builtins/WeakSet/WeakSet.prototype.add.js index 561cc9f8947..0d5fdda12cd 100644 --- a/Userland/Libraries/LibJS/Tests/builtins/WeakSet/WeakSet.prototype.add.js +++ b/Userland/Libraries/LibJS/Tests/builtins/WeakSet/WeakSet.prototype.add.js @@ -16,26 +16,25 @@ test("invalid values", () => { }); }); -function addObjectItem(weakSet) { - weakSet.add({ a: 1 }); -} - -function addSymbolItem(weakSet) { - weakSet.add(Symbol("foo")); -} - test("automatic removal of garbage-collected values", () => { const weakSet = new WeakSet(); + const objectItem = { a: 1 }; - addObjectItem(weakSet); + expect(weakSet.add(objectItem)).toBe(weakSet); expect(getWeakSetSize(weakSet)).toBe(1); + markAsGarbage("objectItem"); gc(); + expect(getWeakSetSize(weakSet)).toBe(0); - addSymbolItem(weakSet); + const symbolItem = Symbol("foo"); + + expect(weakSet.add(symbolItem)).toBe(weakSet); expect(getWeakSetSize(weakSet)).toBe(1); + markAsGarbage("symbolItem"); gc(); + expect(getWeakSetSize(weakSet)).toBe(0); });