From f15090faac304744c34ba8512c06560e9a8ed963 Mon Sep 17 00:00:00 2001 From: Simon Hausmann Date: Mon, 6 Nov 2023 16:14:25 +0100 Subject: [PATCH 1/4] nodejs: Add a test for callback garbage collection Replaces #3772 Closes #3706 --- api/node/__test__/api.spec.mts | 50 ++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/api/node/__test__/api.spec.mts b/api/node/__test__/api.spec.mts index 628893e9f83..dfda922d07c 100644 --- a/api/node/__test__/api.spec.mts +++ b/api/node/__test__/api.spec.mts @@ -4,6 +4,11 @@ import test from 'ava' import * as path from 'node:path'; import { fileURLToPath } from 'url'; +import { setFlagsFromString } from 'v8'; +import { runInNewContext } from 'vm'; + +setFlagsFromString('--expose_gc'); +const gc = runInNewContext('gc'); import { loadFile, loadSource, CompileError } from '../index.js' @@ -159,3 +164,48 @@ test('loadSource component instances and modules are sealed', (t) => { test.no_such_callback = () => { }; }, { instanceOf: TypeError }); }) + +test('callback closure cyclic references do not prevent GC', async (t) => { + + // Setup: + // A component instance with a callback installed from JS: + // * The callback captures the surrounding environment, which + // includes an extra reference to the component instance itself + // --> a cyclic reference + // * Invoking the callback clears the reference in the outer captured + // environment. + // + // Note: WeakRef's deref is used to observe the GC. This means that we must + // separate the test into different jobs with await, to permit for collection. + // (See https://tc39.es/ecma262/multipage/managing-memory.html#sec-weak-ref.prototype.deref) + + let demo = loadFile(path.join(__dirname, "resources/test-constructor.slint")) as any; + let callback_invoked = false; + let demo_weak = new WeakRef(demo); + + demo.my_callback = () => { + demo = null; + callback_invoked = true; + }; + + t.true(demo_weak.deref() !== undefined); + + // After the first GC, the instance should not have been collected because the + // current environment's demo variable is a strong reference. + await new Promise(resolve => setTimeout(resolve, 0)); + gc(); + + t.true(demo_weak.deref() !== undefined); + + // Invoke the callback, to clear "demo" + demo.my_callback(); + t.true(callback_invoked); + t.true(demo === null); + + // After the this GC call, the instance should have been collected. Strong references + // in Rust should not keep it alive. + await new Promise(resolve => setTimeout(resolve, 0)); + gc(); + + t.is(demo_weak.deref(), undefined, "The demo instance should have been collected and the weak ref should deref to undefined"); +}) From f6cfd8dc751eb960b42eb650eaf90c8d982f7f5a Mon Sep 17 00:00:00 2001 From: Simon Hausmann Date: Mon, 6 Nov 2023 18:19:19 +0100 Subject: [PATCH 2/4] Extend GC test to actually instantiate the Rust component instance code --- api/node/__test__/api.spec.mts | 4 +++- api/node/__test__/resources/test-gc.slint | 10 ++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 api/node/__test__/resources/test-gc.slint diff --git a/api/node/__test__/api.spec.mts b/api/node/__test__/api.spec.mts index dfda922d07c..3e38acb0454 100644 --- a/api/node/__test__/api.spec.mts +++ b/api/node/__test__/api.spec.mts @@ -179,7 +179,9 @@ test('callback closure cyclic references do not prevent GC', async (t) => { // separate the test into different jobs with await, to permit for collection. // (See https://tc39.es/ecma262/multipage/managing-memory.html#sec-weak-ref.prototype.deref) - let demo = loadFile(path.join(__dirname, "resources/test-constructor.slint")) as any; + let demo_module = loadFile(path.join(__dirname, "resources/test-gc.slint")) as any; + let demo = new demo_module.Test(); + t.is(demo.check, "initial value"); let callback_invoked = false; let demo_weak = new WeakRef(demo); diff --git a/api/node/__test__/resources/test-gc.slint b/api/node/__test__/resources/test-gc.slint new file mode 100644 index 00000000000..db2ce3a28c0 --- /dev/null +++ b/api/node/__test__/resources/test-gc.slint @@ -0,0 +1,10 @@ +// Copyright © SixtyFPS GmbH +// SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-Royalty-free-1.1 OR LicenseRef-Slint-commercial +// Copyright © SixtyFPS GmbH +// SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-Royalty-free-1.1 OR LicenseRef-Slint-commercial + + +export component Test { + callback say_hello(); + in-out property check: "initial value"; +} From 6e8214ec944aab19326cd57d18d51c5f495e2264 Mon Sep 17 00:00:00 2001 From: Simon Hausmann Date: Mon, 6 Nov 2023 18:22:27 +0100 Subject: [PATCH 3/4] Fix callback name This should never have happened. I'll add a fix to seal our instance and test that. --- api/node/__test__/api.spec.mts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/api/node/__test__/api.spec.mts b/api/node/__test__/api.spec.mts index 3e38acb0454..4662c549cf8 100644 --- a/api/node/__test__/api.spec.mts +++ b/api/node/__test__/api.spec.mts @@ -182,10 +182,11 @@ test('callback closure cyclic references do not prevent GC', async (t) => { let demo_module = loadFile(path.join(__dirname, "resources/test-gc.slint")) as any; let demo = new demo_module.Test(); t.is(demo.check, "initial value"); + t.true(Object.hasOwn(demo, "say_hello")); let callback_invoked = false; let demo_weak = new WeakRef(demo); - demo.my_callback = () => { + demo.say_hello = () => { demo = null; callback_invoked = true; }; @@ -200,7 +201,7 @@ test('callback closure cyclic references do not prevent GC', async (t) => { t.true(demo_weak.deref() !== undefined); // Invoke the callback, to clear "demo" - demo.my_callback(); + demo.say_hello(); t.true(callback_invoked); t.true(demo === null); From b551105f0a38cf17ce0b16750d8214497034464a Mon Sep 17 00:00:00 2001 From: Simon Hausmann Date: Mon, 6 Nov 2023 18:39:10 +0100 Subject: [PATCH 4/4] Modify the GC test so that it fails as expected --- api/node/__test__/api.spec.mts | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/api/node/__test__/api.spec.mts b/api/node/__test__/api.spec.mts index 4662c549cf8..86ea57995a6 100644 --- a/api/node/__test__/api.spec.mts +++ b/api/node/__test__/api.spec.mts @@ -169,27 +169,28 @@ test('callback closure cyclic references do not prevent GC', async (t) => { // Setup: // A component instance with a callback installed from JS: - // * The callback captures the surrounding environment, which - // includes an extra reference to the component instance itself - // --> a cyclic reference - // * Invoking the callback clears the reference in the outer captured - // environment. + // The callback captures the surrounding environment, which + // includes an extra reference to the component instance itself + // --> a cyclic reference // // Note: WeakRef's deref is used to observe the GC. This means that we must // separate the test into different jobs with await, to permit for collection. // (See https://tc39.es/ecma262/multipage/managing-memory.html#sec-weak-ref.prototype.deref) - let demo_module = loadFile(path.join(__dirname, "resources/test-gc.slint")) as any; + let demo_module = loadFile(path.join(dirname, "resources/test-gc.slint")) as any; let demo = new demo_module.Test(); t.is(demo.check, "initial value"); t.true(Object.hasOwn(demo, "say_hello")); - let callback_invoked = false; + let demo_weak = new WeakRef(demo); - demo.say_hello = () => { - demo = null; - callback_invoked = true; - }; + function scope() { + let copy = demo; + copy.say_hello = () => { + console.log(copy.check); + }; + } + scope(); t.true(demo_weak.deref() !== undefined); @@ -200,10 +201,8 @@ test('callback closure cyclic references do not prevent GC', async (t) => { t.true(demo_weak.deref() !== undefined); - // Invoke the callback, to clear "demo" - demo.say_hello(); - t.true(callback_invoked); - t.true(demo === null); + // Clear the strong reference here + demo = null; // After the this GC call, the instance should have been collected. Strong references // in Rust should not keep it alive.