Skip to content

Commit d3f96f4

Browse files
committed
Fix cross-thread JSString deinit by wrapping JSObject instead of raw ref
JSString.Guts previously held a raw JavaScriptObjectRef and released it unconditionally on whatever thread ran deinit. In the multithreaded Wasm runtime each thread has its own JSObjectSpace, so releasing a ref on the wrong thread caused a TypeError crash (BugSnag: rc property of undefined). JSObject already handles this correctly by capturing ownerTid at init time and routing deinit through swjs_release_remote when destroyed off-thread. This change makes Guts hold a JSObject instead of a raw ref, delegating the correct cross-thread release behaviour to the existing JSObject.deinit path. Adds a regression test testDeinitJSStringOnDifferentThread that reproduces the crash deterministically: it forces JS ref allocation on the main thread via asInternalJSRef(), then drops the last reference on a worker thread. Fixes the crash seen in v292745-rc4 after upgrading to JavaScriptKit 0.50.2. https://claude.ai/code/session_01Qhg5GLXZYNJtH63Gs1SwJH
1 parent 43ed78f commit d3f96f4

2 files changed

Lines changed: 36 additions & 16 deletions

File tree

Sources/JavaScriptKit/FundamentalObjects/JSString.swift

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -16,25 +16,26 @@ import _CJavaScriptKit
1616
///
1717
public struct JSString: LosslessStringConvertible, Equatable {
1818
/// The internal representation of JS compatible string
19-
/// The initializers of this type must initialize `jsRef` or `buffer`.
19+
/// The initializers of this type must initialize `jsObject` or `buffer`.
2020
/// And the uninitialized one will be lazily initialized
2121
class Guts {
22-
var shouldDeallocateRef: Bool = false
23-
lazy var jsRef: JavaScriptObjectRef = {
24-
self.shouldDeallocateRef = true
25-
return buffer.withUTF8 { bufferPtr in
22+
// Owns the JS-side ref via JSObject, whose deinit routes the release to
23+
// the correct thread via swjs_release_remote when destroyed off-owner-thread.
24+
lazy var jsObject: JSObject = {
25+
let ref = buffer.withUTF8 { bufferPtr in
2626
return swjs_decode_string(bufferPtr.baseAddress!, Int32(bufferPtr.count))
2727
}
28+
return JSObject(id: ref) // captures ownerTid = current thread here
2829
}()
2930

3031
lazy var buffer: String = {
3132
var bytesRef: JavaScriptObjectRef = 0
32-
let bytesLength = Int(swjs_encode_string(jsRef, &bytesRef))
33+
let bytesLength = Int(swjs_encode_string(jsObject.id, &bytesRef))
3334
// +1 for null terminator
3435
let buffer = UnsafeMutablePointer<UInt8>.allocate(capacity: bytesLength + 1)
3536
defer {
3637
buffer.deallocate()
37-
swjs_release(bytesRef)
38+
swjs_release(bytesRef) // bytesRef is a same-thread temporary
3839
}
3940
swjs_load_string(bytesRef, buffer)
4041
buffer[bytesLength] = 0
@@ -46,13 +47,7 @@ public struct JSString: LosslessStringConvertible, Equatable {
4647
}
4748

4849
init(from jsRef: JavaScriptObjectRef) {
49-
self.jsRef = jsRef
50-
self.shouldDeallocateRef = true
51-
}
52-
53-
deinit {
54-
guard shouldDeallocateRef else { return }
55-
swjs_release(jsRef)
50+
self.jsObject = JSObject(id: jsRef)
5651
}
5752
}
5853

@@ -79,7 +74,7 @@ public struct JSString: LosslessStringConvertible, Equatable {
7974
public static func == (lhs: JSString, rhs: JSString) -> Bool {
8075
withExtendedLifetime(lhs.guts) { lhsGuts in
8176
withExtendedLifetime(rhs.guts) { rhsGuts in
82-
return swjs_value_equals(lhsGuts.jsRef, rhsGuts.jsRef)
77+
return swjs_value_equals(lhsGuts.jsObject.id, rhsGuts.jsObject.id)
8378
}
8479
}
8580
}
@@ -95,6 +90,6 @@ extension JSString: ExpressibleByStringLiteral {
9590
extension JSString {
9691

9792
func asInternalJSRef() -> JavaScriptObjectRef {
98-
guts.jsRef
93+
guts.jsObject.id
9994
}
10095
}

Tests/JavaScriptEventLoopTests/WebWorkerTaskExecutorTests.swift

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -789,5 +789,30 @@ final class WebWorkerTaskExecutorTests: XCTestCase {
789789
// await task.value
790790
// executor.terminate()
791791
// }
792+
793+
func testDeinitJSStringOnDifferentThread() async throws {
794+
final class Box: @unchecked Sendable {
795+
var string: JSString?
796+
init(_ string: JSString) { self.string = string }
797+
}
798+
799+
let executor = try await WebWorkerTaskExecutor(numberOfThreads: 1)
800+
defer { executor.terminate() }
801+
802+
// Force JS ref allocation on the main thread so ownerTid = main thread.
803+
var string: JSString? = JSString("main-thread-owned-key")
804+
_ = string!.asInternalJSRef()
805+
806+
let box = Box(string!)
807+
string = nil
808+
809+
// Drop the last reference on a worker — deinit fires on the worker.
810+
// Before the fix this crashed: TypeError: Cannot read properties of undefined (reading 'rc')
811+
let task = Task(executorPreference: executor) {
812+
XCTAssertFalse(isMainThread())
813+
box.string = nil
814+
}
815+
await task.value
816+
}
792817
}
793818
#endif

0 commit comments

Comments
 (0)