Skip to content

Commit d9a4a9f

Browse files
kateinoigakukunkateinoigakukun
authored
Fix JSClosure leak (#240)
* 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. * 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. * Report where the closure is created when it violates the lifetime contract This additional information will help developers to find the root cause * Flatten two-level object reference in ClosureEntry
1 parent 3b5af3d commit d9a4a9f

File tree

4 files changed

+103
-28
lines changed

4 files changed

+103
-28
lines changed

‎IntegrationTests/TestSuites/Sources/ConcurrencyTests/main.swift‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -123,12 +123,16 @@ func entrypoint() async throws {
123123
try expectEqual(result, .number(3))
124124
}
125125
try expectGTE(diff, 200)
126+
#if JAVASCRIPTKIT_WITHOUT_WEAKREFS
127+
delayObject.closure = nil
128+
delayClosure.release()
129+
#endif
126130
}
127131

128132
try await asyncTest("Async JSPromise: then") {
129133
let promise = JSPromise { resolve in
130134
_ = JSObject.global.setTimeout!(
131-
JSClosure { _ in
135+
JSOneshotClosure { _ in
132136
resolve(.success(JSValue.number(3)))
133137
return .undefined
134138
}.jsValue,
@@ -149,7 +153,7 @@ func entrypoint() async throws {
149153
try await asyncTest("Async JSPromise: then(success:failure:)") {
150154
let promise = JSPromise { resolve in
151155
_ = JSObject.global.setTimeout!(
152-
JSClosure { _ in
156+
JSOneshotClosure { _ in
153157
resolve(.failure(JSError(message: "test").jsValue))
154158
return .undefined
155159
}.jsValue,
@@ -168,7 +172,7 @@ func entrypoint() async throws {
168172
try await asyncTest("Async JSPromise: catch") {
169173
let promise = JSPromise { resolve in
170174
_ = JSObject.global.setTimeout!(
171-
JSClosure { _ in
175+
JSOneshotClosure { _ in
172176
resolve(.failure(JSError(message: "test").jsValue))
173177
return .undefined
174178
}.jsValue,

‎IntegrationTests/TestSuites/Sources/PrimaryTests/UnitTestUtils.swift‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ func expectThrow<T>(_ body: @autoclosure () throws -> T, file: StaticString = #f
111111
}
112112

113113
func wrapUnsafeThrowableFunction(_ body: @escaping () -> Void, file: StaticString = #file, line: UInt = #line, column: UInt = #column) throws -> Error {
114-
JSObject.global.callThrowingClosure.function!(JSClosure { _ in
114+
JSObject.global.callThrowingClosure.function!(JSOneshotClosure { _ in
115115
body()
116116
return .undefined
117117
})

‎IntegrationTests/TestSuites/Sources/PrimaryTests/main.swift‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -254,12 +254,18 @@ try test("Closure Lifetime") {
254254
do {
255255
let c1 = JSClosure { _ in .number(4) }
256256
try expectEqual(c1(), .number(4))
257+
#if JAVASCRIPTKIT_WITHOUT_WEAKREFS
258+
c1.release()
259+
#endif
257260
}
258261

259262
do {
260263
let c1 = JSClosure { _ in fatalError("Crash while closure evaluation") }
261264
let error = try expectThrow(try evalClosure.throws(c1)) as! JSValue
262265
try expectEqual(error.description, "RuntimeError: unreachable")
266+
#if JAVASCRIPTKIT_WITHOUT_WEAKREFS
267+
c1.release()
268+
#endif
263269
}
264270
}
265271

@@ -866,16 +872,24 @@ try test("Symbols") {
866872
// }.prop
867873
// Object.defineProperty(hasInstanceClass, Symbol.hasInstance, { value: () => true })
868874
let hasInstanceObject = JSObject.global.Object.function!.new()
869-
hasInstanceObject.prop = JSClosure { _ in .undefined }.jsValue
875+
let hasInstanceObjectClosure = JSClosure { _ in .undefined }
876+
hasInstanceObject.prop = hasInstanceObjectClosure.jsValue
870877
let hasInstanceClass = hasInstanceObject.prop.function!
871878
let propertyDescriptor = JSObject.global.Object.function!.new()
872-
propertyDescriptor.value = JSClosure { _ in .boolean(true) }.jsValue
879+
let propertyDescriptorClosure = JSClosure { _ in .boolean(true) }
880+
propertyDescriptor.value = propertyDescriptorClosure.jsValue
873881
_ = JSObject.global.Object.function!.defineProperty!(
874882
hasInstanceClass, JSSymbol.hasInstance,
875883
propertyDescriptor
876884
)
877885
try expectEqual(hasInstanceClass[JSSymbol.hasInstance].function!().boolean, true)
878886
try expectEqual(JSObject.global.Object.isInstanceOf(hasInstanceClass), true)
887+
#if JAVASCRIPTKIT_WITHOUT_WEAKREFS
888+
hasInstanceObject.prop = .undefined
889+
propertyDescriptor.value = .undefined
890+
hasInstanceObjectClosure.release()
891+
propertyDescriptorClosure.release()
892+
#endif
879893
}
880894

881895
struct AnimalStruct: Decodable {

‎Sources/JavaScriptKit/FundamentalObjects/JSClosure.swift‎

Lines changed: 79 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -19,17 +19,15 @@ public class JSOneshotClosure: JSObject, JSClosureProtocol {
1919
// 1. Fill `id` as zero at first to access `self` to get `ObjectIdentifier`.
2020
super.init(id: 0)
2121

22-
// 2. Create a new JavaScript function which calls the given Swift function.
23-
hostFuncRef = JavaScriptHostFuncRef(bitPattern: ObjectIdentifier(self))
24-
id = withExtendedLifetime(JSString(file)) { file in
25-
_create_function(hostFuncRef, line, file.asInternalJSRef())
26-
}
27-
28-
// 3. Retain the given body in static storage by `funcRef`.
29-
JSClosure.sharedClosures[hostFuncRef] = (self, {
22+
// 2. Retain the given body in static storage
23+
// Leak the self object globally and release once it's called
24+
hostFuncRef = JSClosure.sharedClosures.register(ObjectIdentifier(self), object: .strong(self), body: {
3025
defer { self.release() }
3126
return body($0)
3227
})
28+
id = withExtendedLifetime(JSString(file)) { file in
29+
_create_function(hostFuncRef, line, file.asInternalJSRef())
30+
}
3331
}
3432

3533
#if compiler(>=5.5)
@@ -42,7 +40,7 @@ public class JSOneshotClosure: JSObject, JSClosureProtocol {
4240
/// Release this function resource.
4341
/// After calling `release`, calling this function from JavaScript will fail.
4442
public func release() {
45-
JSClosure.sharedClosures[hostFuncRef] = nil
43+
JSClosure.sharedClosures.unregister(hostFuncRef)
4644
}
4745
}
4846

@@ -62,13 +60,13 @@ public class JSOneshotClosure: JSObject, JSClosureProtocol {
6260
/// ```
6361
///
6462
public class JSClosure: JSFunction, JSClosureProtocol {
65-
66-
// Note: Retain the closure object itself also to avoid funcRef conflicts
67-
fileprivate static var sharedClosures: [JavaScriptHostFuncRef: (object: JSObject, body: ([JSValue]) -> JSValue)] = [:]
63+
fileprivate static var sharedClosures = SharedJSClosureRegistry()
6864

6965
private var hostFuncRef: JavaScriptHostFuncRef = 0
7066

7167
#if JAVASCRIPTKIT_WITHOUT_WEAKREFS
68+
private let file: String
69+
private let line: UInt32
7270
private var isReleased: Bool = false
7371
#endif
7472

@@ -82,30 +80,35 @@ public class JSClosure: JSFunction, JSClosureProtocol {
8280
}
8381

8482
public init(_ body: @escaping ([JSValue]) -> JSValue, file: String = #fileID, line: UInt32 = #line) {
83+
#if JAVASCRIPTKIT_WITHOUT_WEAKREFS
84+
self.file = file
85+
self.line = line
86+
#endif
8587
// 1. Fill `id` as zero at first to access `self` to get `ObjectIdentifier`.
8688
super.init(id: 0)
8789

88-
// 2. Create a new JavaScript function which calls the given Swift function.
89-
hostFuncRef = JavaScriptHostFuncRef(bitPattern: ObjectIdentifier(self))
90+
// 2. Retain the given body in static storage
91+
hostFuncRef = Self.sharedClosures.register(
92+
ObjectIdentifier(self), object: .weak(self), body: body
93+
)
94+
9095
id = withExtendedLifetime(JSString(file)) { file in
9196
_create_function(hostFuncRef, line, file.asInternalJSRef())
9297
}
9398

94-
// 3. Retain the given body in static storage by `funcRef`.
95-
Self.sharedClosures[hostFuncRef] = (self, body)
9699
}
97100

98101
#if compiler(>=5.5)
99102
@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)
100-
public static func async(_ body: @escaping ([JSValue]) async throws -> JSValue) -> JSClosure {
101-
JSClosure(makeAsyncClosure(body))
103+
public static func async(_ body: @escaping ([JSValue]) async throws -> JSValue, file: String = #fileID, line: UInt32 = #line) -> JSClosure {
104+
JSClosure(makeAsyncClosure(body), file: file, line: line)
102105
}
103106
#endif
104107

105108
#if JAVASCRIPTKIT_WITHOUT_WEAKREFS
106109
deinit {
107110
guard isReleased else {
108-
fatalError("release() must be called on JSClosure objects manually before they are deallocated")
111+
fatalError("release() must be called on JSClosure object (\(file):\(line)) manually before they are deallocated")
109112
}
110113
}
111114
#endif
@@ -133,6 +136,60 @@ private func makeAsyncClosure(_ body: @escaping ([JSValue]) async throws -> JSVa
133136
}
134137
#endif
135138

139+
/// Registry for Swift closures that are referenced from JavaScript.
140+
private struct SharedJSClosureRegistry {
141+
struct ClosureEntry {
142+
// Note: Retain the closure object itself also to avoid funcRef conflicts.
143+
var object: AnyObjectReference
144+
var body: ([JSValue]) -> JSValue
145+
146+
init(object: AnyObjectReference, body: @escaping ([JSValue]) -> JSValue) {
147+
self.object = object
148+
self.body = body
149+
}
150+
}
151+
enum AnyObjectReference {
152+
case strong(AnyObject)
153+
case weak(WeakObject)
154+
155+
static func `weak`(_ object: AnyObject) -> AnyObjectReference {
156+
.weak(SharedJSClosureRegistry.WeakObject(underlying: object))
157+
}
158+
}
159+
struct WeakObject {
160+
weak var underlying: AnyObject?
161+
init(underlying: AnyObject) {
162+
self.underlying = underlying
163+
}
164+
}
165+
private var closures: [JavaScriptHostFuncRef: ClosureEntry] = [:]
166+
167+
/// Register a Swift closure to be called from JavaScript.
168+
/// - Parameters:
169+
/// - hint: A hint to identify the closure.
170+
/// - object: The object should be retained until the closure is released from JavaScript.
171+
/// - body: The closure to be called from JavaScript.
172+
/// - Returns: An unique identifier for the registered closure.
173+
mutating func register(
174+
_ hint: ObjectIdentifier,
175+
object: AnyObjectReference, body: @escaping ([JSValue]) -> JSValue
176+
) -> JavaScriptHostFuncRef {
177+
let ref = JavaScriptHostFuncRef(bitPattern: hint)
178+
closures[ref] = ClosureEntry(object: object, body: body)
179+
return ref
180+
}
181+
182+
/// Unregister a Swift closure from the registry.
183+
mutating func unregister(_ ref: JavaScriptHostFuncRef) {
184+
closures[ref] = nil
185+
}
186+
187+
/// Get the Swift closure from the registry.
188+
subscript(_ ref: JavaScriptHostFuncRef) -> (([JSValue]) -> JSValue)? {
189+
closures[ref]?.body
190+
}
191+
}
192+
136193
// MARK: - `JSClosure` mechanism note
137194
//
138195
// 1. Create a thunk in the JavaScript world, which has a reference
@@ -174,7 +231,7 @@ func _call_host_function_impl(
174231
_ argv: UnsafePointer<RawJSValue>, _ argc: Int32,
175232
_ callbackFuncRef: JavaScriptObjectRef
176233
) -> Bool {
177-
guard let (_, hostFunc) = JSClosure.sharedClosures[hostFuncRef] else {
234+
guard let hostFunc = JSClosure.sharedClosures[hostFuncRef] else {
178235
return true
179236
}
180237
let arguments = UnsafeBufferPointer(start: argv, count: Int(argc)).map(\.jsValue)
@@ -195,7 +252,7 @@ func _call_host_function_impl(
195252
extension JSClosure {
196253
public func release() {
197254
isReleased = true
198-
Self.sharedClosures[hostFuncRef] = nil
255+
Self.sharedClosures.unregister(hostFuncRef)
199256
}
200257
}
201258

@@ -213,6 +270,6 @@ extension JSClosure {
213270

214271
@_cdecl("_free_host_function_impl")
215272
func _free_host_function_impl(_ hostFuncRef: JavaScriptHostFuncRef) {
216-
JSClosure.sharedClosures[hostFuncRef] = nil
273+
JSClosure.sharedClosures.unregister(hostFuncRef)
217274
}
218275
#endif

0 commit comments

Comments
 (0)