From e8cf759d793707350fe2380b3a7f099187c39eef Mon Sep 17 00:00:00 2001 From: Yuta Saito Date: Sun, 7 Apr 2024 14:56:32 +0900 Subject: [PATCH 1/4] Fix retain cycle in JSClosure by weak reference to the closure holder ```swift let c1 = JSClosure { _ in .undefined } consume c1 ``` did not release the `JSClosure` itself and it leaked the underlying JavaScript closure too because `JSClosure` -> JS closure thunk -> Closure registry entry -> `JSClosure` reference cycle was not broken when using FinalizationRegistry. (Without FR, it was broken by manual `release` call.) Note that weakening the reference does not violates the contract that function reference should be unique because holding a weak reference does deinit but not deallocate the object, so ObjectIdentifier is not reused until the weak reference in the registry is removed. --- .../FundamentalObjects/JSClosure.swift | 81 ++++++++++++++----- 1 file changed, 62 insertions(+), 19 deletions(-) diff --git a/Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift b/Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift index 441dd2a6c..f0c2e7167 100644 --- a/Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift +++ b/Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift @@ -19,17 +19,14 @@ public class JSOneshotClosure: JSObject, JSClosureProtocol { // 1. Fill `id` as zero at first to access `self` to get `ObjectIdentifier`. super.init(id: 0) - // 2. Create a new JavaScript function which calls the given Swift function. - hostFuncRef = JavaScriptHostFuncRef(bitPattern: ObjectIdentifier(self)) - id = withExtendedLifetime(JSString(file)) { file in - _create_function(hostFuncRef, line, file.asInternalJSRef()) - } - - // 3. Retain the given body in static storage by `funcRef`. - JSClosure.sharedClosures[hostFuncRef] = (self, { + // 2. Retain the given body in static storage + hostFuncRef = JSClosure.sharedClosures.register(ObjectIdentifier(self), object: self, body: { defer { self.release() } return body($0) }) + id = withExtendedLifetime(JSString(file)) { file in + _create_function(hostFuncRef, line, file.asInternalJSRef()) + } } #if compiler(>=5.5) @@ -42,7 +39,7 @@ public class JSOneshotClosure: JSObject, JSClosureProtocol { /// Release this function resource. /// After calling `release`, calling this function from JavaScript will fail. public func release() { - JSClosure.sharedClosures[hostFuncRef] = nil + JSClosure.sharedClosures.unregister(hostFuncRef) } } @@ -62,9 +59,7 @@ public class JSOneshotClosure: JSObject, JSClosureProtocol { /// ``` /// public class JSClosure: JSFunction, JSClosureProtocol { - - // Note: Retain the closure object itself also to avoid funcRef conflicts - fileprivate static var sharedClosures: [JavaScriptHostFuncRef: (object: JSObject, body: ([JSValue]) -> JSValue)] = [:] + fileprivate static var sharedClosures = SharedJSClosureRegistry() private var hostFuncRef: JavaScriptHostFuncRef = 0 @@ -85,14 +80,16 @@ public class JSClosure: JSFunction, JSClosureProtocol { // 1. Fill `id` as zero at first to access `self` to get `ObjectIdentifier`. super.init(id: 0) - // 2. Create a new JavaScript function which calls the given Swift function. - hostFuncRef = JavaScriptHostFuncRef(bitPattern: ObjectIdentifier(self)) + // 2. Retain the given body in static storage + let objectHolder = SharedJSClosureRegistry.WeakBox(underlying: self) + hostFuncRef = Self.sharedClosures.register( + ObjectIdentifier(self), object: objectHolder, body: body + ) + id = withExtendedLifetime(JSString(file)) { file in _create_function(hostFuncRef, line, file.asInternalJSRef()) } - // 3. Retain the given body in static storage by `funcRef`. - Self.sharedClosures[hostFuncRef] = (self, body) } #if compiler(>=5.5) @@ -133,6 +130,52 @@ private func makeAsyncClosure(_ body: @escaping ([JSValue]) async throws -> JSVa } #endif +/// Registry for Swift closures that are referenced from JavaScript. +private struct SharedJSClosureRegistry { + class ClosureEntry { + // Note: Retain the closure object itself also to avoid funcRef conflicts. + var object: AnyObject + var body: ([JSValue]) -> JSValue + + init(object: AnyObject, body: @escaping ([JSValue]) -> JSValue) { + self.object = object + self.body = body + } + } + class WeakBox { + weak var underlying: AnyObject? + init(underlying: AnyObject) { + self.underlying = underlying + } + } + private var closures: [JavaScriptHostFuncRef: ClosureEntry] = [:] + + /// Register a Swift closure to be called from JavaScript. + /// - Parameters: + /// - hint: A hint to identify the closure. + /// - object: The object should be retained until the closure is released from JavaScript. + /// - body: The closure to be called from JavaScript. + /// - Returns: An unique identifier for the registered closure. + mutating func register( + _ hint: ObjectIdentifier, + object: AnyObject, body: @escaping ([JSValue]) -> JSValue + ) -> JavaScriptHostFuncRef { + let ref = JavaScriptHostFuncRef(bitPattern: hint) + closures[ref] = ClosureEntry(object: object, body: body) + return ref + } + + /// Unregister a Swift closure from the registry. + mutating func unregister(_ ref: JavaScriptHostFuncRef) { + closures[ref] = nil + } + + /// Get the Swift closure from the registry. + subscript(_ ref: JavaScriptHostFuncRef) -> (([JSValue]) -> JSValue)? { + closures[ref]?.body + } +} + // MARK: - `JSClosure` mechanism note // // 1. Create a thunk in the JavaScript world, which has a reference @@ -174,7 +217,7 @@ func _call_host_function_impl( _ argv: UnsafePointer, _ argc: Int32, _ callbackFuncRef: JavaScriptObjectRef ) -> Bool { - guard let (_, hostFunc) = JSClosure.sharedClosures[hostFuncRef] else { + guard let hostFunc = JSClosure.sharedClosures[hostFuncRef] else { return true } let arguments = UnsafeBufferPointer(start: argv, count: Int(argc)).map(\.jsValue) @@ -195,7 +238,7 @@ func _call_host_function_impl( extension JSClosure { public func release() { isReleased = true - Self.sharedClosures[hostFuncRef] = nil + Self.sharedClosures.unregister(hostFuncRef) } } @@ -213,6 +256,6 @@ extension JSClosure { @_cdecl("_free_host_function_impl") func _free_host_function_impl(_ hostFuncRef: JavaScriptHostFuncRef) { - JSClosure.sharedClosures[hostFuncRef] = nil + JSClosure.sharedClosures.unregister(hostFuncRef) } #endif From d08a1e487f5b3606e674563e2f27fd9b3268452a Mon Sep 17 00:00:00 2001 From: Yuta Saito Date: Sun, 7 Apr 2024 15:05:55 +0900 Subject: [PATCH 2/4] Fix the test suite for violation of the closure lifetime contract The test suite was not properly releasing the closures but they have not been revealed as a problem because those closures were leaked conservatively. --- .../Sources/ConcurrencyTests/main.swift | 10 +++++++--- .../Sources/PrimaryTests/UnitTestUtils.swift | 2 +- .../TestSuites/Sources/PrimaryTests/main.swift | 18 ++++++++++++++++-- 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/IntegrationTests/TestSuites/Sources/ConcurrencyTests/main.swift b/IntegrationTests/TestSuites/Sources/ConcurrencyTests/main.swift index ece58b317..46eeeab69 100644 --- a/IntegrationTests/TestSuites/Sources/ConcurrencyTests/main.swift +++ b/IntegrationTests/TestSuites/Sources/ConcurrencyTests/main.swift @@ -123,12 +123,16 @@ func entrypoint() async throws { try expectEqual(result, .number(3)) } try expectGTE(diff, 200) +#if JAVASCRIPTKIT_WITHOUT_WEAKREFS + delayObject.closure = nil + delayClosure.release() +#endif } try await asyncTest("Async JSPromise: then") { let promise = JSPromise { resolve in _ = JSObject.global.setTimeout!( - JSClosure { _ in + JSOneshotClosure { _ in resolve(.success(JSValue.number(3))) return .undefined }.jsValue, @@ -149,7 +153,7 @@ func entrypoint() async throws { try await asyncTest("Async JSPromise: then(success:failure:)") { let promise = JSPromise { resolve in _ = JSObject.global.setTimeout!( - JSClosure { _ in + JSOneshotClosure { _ in resolve(.failure(JSError(message: "test").jsValue)) return .undefined }.jsValue, @@ -168,7 +172,7 @@ func entrypoint() async throws { try await asyncTest("Async JSPromise: catch") { let promise = JSPromise { resolve in _ = JSObject.global.setTimeout!( - JSClosure { _ in + JSOneshotClosure { _ in resolve(.failure(JSError(message: "test").jsValue)) return .undefined }.jsValue, diff --git a/IntegrationTests/TestSuites/Sources/PrimaryTests/UnitTestUtils.swift b/IntegrationTests/TestSuites/Sources/PrimaryTests/UnitTestUtils.swift index 199a4cbdf..b6446add3 100644 --- a/IntegrationTests/TestSuites/Sources/PrimaryTests/UnitTestUtils.swift +++ b/IntegrationTests/TestSuites/Sources/PrimaryTests/UnitTestUtils.swift @@ -111,7 +111,7 @@ func expectThrow(_ body: @autoclosure () throws -> T, file: StaticString = #f } func wrapUnsafeThrowableFunction(_ body: @escaping () -> Void, file: StaticString = #file, line: UInt = #line, column: UInt = #column) throws -> Error { - JSObject.global.callThrowingClosure.function!(JSClosure { _ in + JSObject.global.callThrowingClosure.function!(JSOneshotClosure { _ in body() return .undefined }) diff --git a/IntegrationTests/TestSuites/Sources/PrimaryTests/main.swift b/IntegrationTests/TestSuites/Sources/PrimaryTests/main.swift index a3e27573a..dc9d8353d 100644 --- a/IntegrationTests/TestSuites/Sources/PrimaryTests/main.swift +++ b/IntegrationTests/TestSuites/Sources/PrimaryTests/main.swift @@ -254,12 +254,18 @@ try test("Closure Lifetime") { do { let c1 = JSClosure { _ in .number(4) } try expectEqual(c1(), .number(4)) +#if JAVASCRIPTKIT_WITHOUT_WEAKREFS + c1.release() +#endif } do { let c1 = JSClosure { _ in fatalError("Crash while closure evaluation") } let error = try expectThrow(try evalClosure.throws(c1)) as! JSValue try expectEqual(error.description, "RuntimeError: unreachable") +#if JAVASCRIPTKIT_WITHOUT_WEAKREFS + c1.release() +#endif } } @@ -824,16 +830,24 @@ try test("Symbols") { // }.prop // Object.defineProperty(hasInstanceClass, Symbol.hasInstance, { value: () => true }) let hasInstanceObject = JSObject.global.Object.function!.new() - hasInstanceObject.prop = JSClosure { _ in .undefined }.jsValue + let hasInstanceObjectClosure = JSClosure { _ in .undefined } + hasInstanceObject.prop = hasInstanceObjectClosure.jsValue let hasInstanceClass = hasInstanceObject.prop.function! let propertyDescriptor = JSObject.global.Object.function!.new() - propertyDescriptor.value = JSClosure { _ in .boolean(true) }.jsValue + let propertyDescriptorClosure = JSClosure { _ in .boolean(true) } + propertyDescriptor.value = propertyDescriptorClosure.jsValue _ = JSObject.global.Object.function!.defineProperty!( hasInstanceClass, JSSymbol.hasInstance, propertyDescriptor ) try expectEqual(hasInstanceClass[JSSymbol.hasInstance].function!().boolean, true) try expectEqual(JSObject.global.Object.isInstanceOf(hasInstanceClass), true) +#if JAVASCRIPTKIT_WITHOUT_WEAKREFS + hasInstanceObject.prop = .undefined + propertyDescriptor.value = .undefined + hasInstanceObjectClosure.release() + propertyDescriptorClosure.release() +#endif } struct AnimalStruct: Decodable { From 68e143571ac686d28250de3a243724517b5027e2 Mon Sep 17 00:00:00 2001 From: Yuta Saito Date: Sun, 7 Apr 2024 15:08:23 +0900 Subject: [PATCH 3/4] Report where the closure is created when it violates the lifetime contract This additional information will help developers to find the root cause --- .../JavaScriptKit/FundamentalObjects/JSClosure.swift | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift b/Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift index f0c2e7167..098622bba 100644 --- a/Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift +++ b/Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift @@ -64,6 +64,8 @@ public class JSClosure: JSFunction, JSClosureProtocol { private var hostFuncRef: JavaScriptHostFuncRef = 0 #if JAVASCRIPTKIT_WITHOUT_WEAKREFS + private let file: String + private let line: UInt32 private var isReleased: Bool = false #endif @@ -77,6 +79,10 @@ public class JSClosure: JSFunction, JSClosureProtocol { } public init(_ body: @escaping ([JSValue]) -> JSValue, file: String = #fileID, line: UInt32 = #line) { + #if JAVASCRIPTKIT_WITHOUT_WEAKREFS + self.file = file + self.line = line + #endif // 1. Fill `id` as zero at first to access `self` to get `ObjectIdentifier`. super.init(id: 0) @@ -94,15 +100,15 @@ public class JSClosure: JSFunction, JSClosureProtocol { #if compiler(>=5.5) @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) - public static func async(_ body: @escaping ([JSValue]) async throws -> JSValue) -> JSClosure { - JSClosure(makeAsyncClosure(body)) + public static func async(_ body: @escaping ([JSValue]) async throws -> JSValue, file: String = #fileID, line: UInt32 = #line) -> JSClosure { + JSClosure(makeAsyncClosure(body), file: file, line: line) } #endif #if JAVASCRIPTKIT_WITHOUT_WEAKREFS deinit { guard isReleased else { - fatalError("release() must be called on JSClosure objects manually before they are deallocated") + fatalError("release() must be called on JSClosure object (\(file):\(line)) manually before they are deallocated") } } #endif From 2a7269fadfd621c4c6275e012b2406b6147c3dbd Mon Sep 17 00:00:00 2001 From: Yuta Saito Date: Sun, 7 Apr 2024 15:50:30 +0900 Subject: [PATCH 4/4] Flatten two-level object reference in ClosureEntry --- .../FundamentalObjects/JSClosure.swift | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift b/Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift index 098622bba..c3a0c886e 100644 --- a/Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift +++ b/Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift @@ -20,7 +20,8 @@ public class JSOneshotClosure: JSObject, JSClosureProtocol { super.init(id: 0) // 2. Retain the given body in static storage - hostFuncRef = JSClosure.sharedClosures.register(ObjectIdentifier(self), object: self, body: { + // Leak the self object globally and release once it's called + hostFuncRef = JSClosure.sharedClosures.register(ObjectIdentifier(self), object: .strong(self), body: { defer { self.release() } return body($0) }) @@ -87,9 +88,8 @@ public class JSClosure: JSFunction, JSClosureProtocol { super.init(id: 0) // 2. Retain the given body in static storage - let objectHolder = SharedJSClosureRegistry.WeakBox(underlying: self) hostFuncRef = Self.sharedClosures.register( - ObjectIdentifier(self), object: objectHolder, body: body + ObjectIdentifier(self), object: .weak(self), body: body ) id = withExtendedLifetime(JSString(file)) { file in @@ -138,17 +138,25 @@ private func makeAsyncClosure(_ body: @escaping ([JSValue]) async throws -> JSVa /// Registry for Swift closures that are referenced from JavaScript. private struct SharedJSClosureRegistry { - class ClosureEntry { + struct ClosureEntry { // Note: Retain the closure object itself also to avoid funcRef conflicts. - var object: AnyObject + var object: AnyObjectReference var body: ([JSValue]) -> JSValue - init(object: AnyObject, body: @escaping ([JSValue]) -> JSValue) { + init(object: AnyObjectReference, body: @escaping ([JSValue]) -> JSValue) { self.object = object self.body = body } } - class WeakBox { + enum AnyObjectReference { + case strong(AnyObject) + case weak(WeakObject) + + static func `weak`(_ object: AnyObject) -> AnyObjectReference { + .weak(SharedJSClosureRegistry.WeakObject(underlying: object)) + } + } + struct WeakObject { weak var underlying: AnyObject? init(underlying: AnyObject) { self.underlying = underlying @@ -164,7 +172,7 @@ private struct SharedJSClosureRegistry { /// - Returns: An unique identifier for the registered closure. mutating func register( _ hint: ObjectIdentifier, - object: AnyObject, body: @escaping ([JSValue]) -> JSValue + object: AnyObjectReference, body: @escaping ([JSValue]) -> JSValue ) -> JavaScriptHostFuncRef { let ref = JavaScriptHostFuncRef(bitPattern: hint) closures[ref] = ClosureEntry(object: object, body: body)