From 9d5a800c3da1afb124b16dfec75af253ef1d8dfd Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Sun, 15 Feb 2026 03:33:48 -0800 Subject: [PATCH] fix(napi,timers): callback scope (#27026) --- src/bun.js/api/Timer/TimerObjectInternals.zig | 60 +++++++++++++------ src/napi/napi.zig | 14 ++--- 2 files changed, 45 insertions(+), 29 deletions(-) diff --git a/src/bun.js/api/Timer/TimerObjectInternals.zig b/src/bun.js/api/Timer/TimerObjectInternals.zig index dac458e8e1..0466cd2150 100644 --- a/src/bun.js/api/Timer/TimerObjectInternals.zig +++ b/src/bun.js/api/Timer/TimerObjectInternals.zig @@ -4,7 +4,7 @@ const TimerObjectInternals = @This(); /// Identifier for this timer that is exposed to JavaScript (by `+timer`) id: i32 = -1, interval: u31 = 0, -strong_this: jsc.Strong.Optional = .empty, +this_value: jsc.JSRef = .empty(), flags: Flags = .{}, /// Used by: @@ -76,31 +76,41 @@ pub fn runImmediateTask(this: *TimerObjectInternals, vm: *VirtualMachine) bool { // loop alive other than setImmediates (!this.flags.is_keeping_event_loop_alive and !vm.isEventLoopAliveExcludingImmediates())) { + this.setEnableKeepingEventLoopAlive(vm, false); + this.this_value.downgrade(); this.deref(); return false; } - const timer = this.strong_this.get() orelse { + const timer = this.this_value.tryGet() orelse { if (Environment.isDebug) { @panic("TimerObjectInternals.runImmediateTask: this_object is null"); } + this.setEnableKeepingEventLoopAlive(vm, false); + this.deref(); return false; }; const globalThis = vm.global; - this.strong_this.deinit(); + this.this_value.downgrade(); this.eventLoopTimer().state = .FIRED; this.setEnableKeepingEventLoopAlive(vm, false); + timer.ensureStillAlive(); vm.eventLoop().enter(); const callback = ImmediateObject.js.callbackGetCached(timer).?; const arguments = ImmediateObject.js.argumentsGetCached(timer).?; - this.ref(); - const exception_thrown = this.run(globalThis, timer, callback, arguments, this.asyncID(), vm); - this.deref(); - if (this.eventLoopTimer().state == .FIRED) { - this.deref(); - } + const exception_thrown = brk: { + this.ref(); + defer { + if (this.eventLoopTimer().state == .FIRED) { + this.deref(); + } + this.deref(); + } + break :brk this.run(globalThis, timer, callback, arguments, this.asyncID(), vm); + }; + // --- after this point, the timer is no longer guaranteed to be alive --- vm.eventLoop().exitMaybeDrainMicrotasks(!exception_thrown) catch return true; @@ -120,7 +130,13 @@ pub fn fire(this: *TimerObjectInternals, _: *const timespec, vm: *jsc.VirtualMac this.eventLoopTimer().state = .FIRED; const globalThis = vm.global; - const this_object = this.strong_this.get().?; + const this_object = this.this_value.tryGet() orelse { + this.setEnableKeepingEventLoopAlive(vm, false); + this.flags.has_cleared_timer = true; + this.this_value.downgrade(); + this.deref(); + return; + }; const callback: JSValue, const arguments: JSValue, var idle_timeout: JSValue, var repeat: JSValue = switch (kind) { .setImmediate => .{ @@ -143,7 +159,7 @@ pub fn fire(this: *TimerObjectInternals, _: *const timespec, vm: *jsc.VirtualMac } this.setEnableKeepingEventLoopAlive(vm, false); this.flags.has_cleared_timer = true; - this.strong_this.deinit(); + this.this_value.downgrade(); this.deref(); return; @@ -152,7 +168,7 @@ pub fn fire(this: *TimerObjectInternals, _: *const timespec, vm: *jsc.VirtualMac var time_before_call: timespec = undefined; if (kind != .setInterval) { - this.strong_this.clearWithoutDeallocation(); + this.this_value.downgrade(); } else { time_before_call = timespec.msFromNow(.allow_mocked_time, this.interval); } @@ -239,7 +255,7 @@ fn convertToInterval(this: *TimerObjectInternals, global: *JSGlobalObject, timer // https://github.com/nodejs/node/blob/a7cbb904745591c9a9d047a364c2c188e5470047/lib/internal/timers.js#L613 TimeoutObject.js.idleTimeoutSetCached(timer, global, repeat); - this.strong_this.set(global, timer); + this.this_value.setStrong(timer, global); this.flags.kind = .setInterval; this.interval = new_interval; this.reschedule(timer, vm, global); @@ -297,7 +313,7 @@ pub fn init( this.reschedule(timer, vm, global); } - this.strong_this.set(global, timer); + this.this_value.setStrong(timer, global); } pub fn doRef(this: *TimerObjectInternals, _: *jsc.JSGlobalObject, this_value: JSValue) JSValue { @@ -327,7 +343,7 @@ pub fn doRefresh(this: *TimerObjectInternals, globalObject: *jsc.JSGlobalObject, return this_value; } - this.strong_this.set(globalObject, this_value); + this.this_value.setStrong(this_value, globalObject); this.reschedule(this_value, VirtualMachine.get(), globalObject); return this_value; @@ -350,12 +366,18 @@ pub fn cancel(this: *TimerObjectInternals, vm: *VirtualMachine) void { this.setEnableKeepingEventLoopAlive(vm, false); this.flags.has_cleared_timer = true; - if (this.flags.kind == .setImmediate) return; + if (this.flags.kind == .setImmediate) { + // Release the strong reference so the GC can collect the JS object. + // The immediate task is still in the event loop queue and will be skipped + // by runImmediateTask when it sees has_cleared_timer == true. + this.this_value.downgrade(); + return; + } const was_active = this.eventLoopTimer().state == .ACTIVE; this.eventLoopTimer().state = .CANCELLED; - this.strong_this.deinit(); + this.this_value.downgrade(); if (was_active) { vm.timer.remove(this.eventLoopTimer()); @@ -442,12 +464,12 @@ pub fn getDestroyed(this: *TimerObjectInternals) bool { } pub fn finalize(this: *TimerObjectInternals) void { - this.strong_this.deinit(); + this.this_value.finalize(); this.deref(); } pub fn deinit(this: *TimerObjectInternals) void { - this.strong_this.deinit(); + this.this_value.deinit(); const vm = VirtualMachine.get(); const kind = this.flags.kind; diff --git a/src/napi/napi.zig b/src/napi/napi.zig index b2a53b6d6e..5792849138 100644 --- a/src/napi/napi.zig +++ b/src/napi/napi.zig @@ -766,19 +766,13 @@ pub extern fn napi_type_tag_object(env: napi_env, _: napi_value, _: [*c]const na pub extern fn napi_check_object_type_tag(env: napi_env, _: napi_value, _: [*c]const napi_type_tag, _: *bool) napi_status; // do nothing for both of these -pub export fn napi_open_callback_scope(env_: napi_env, _: napi_value, _: *anyopaque, _: *anyopaque) napi_status { +pub export fn napi_open_callback_scope(_: napi_env, _: napi_value, _: *anyopaque, _: *anyopaque) napi_status { log("napi_open_callback_scope", .{}); - const env = env_ orelse { - return envIsNull(); - }; - return env.ok(); + return @intFromEnum(NapiStatus.ok); } -pub export fn napi_close_callback_scope(env_: napi_env, _: *anyopaque) napi_status { +pub export fn napi_close_callback_scope(_: napi_env, _: *anyopaque) napi_status { log("napi_close_callback_scope", .{}); - const env = env_ orelse { - return envIsNull(); - }; - return env.ok(); + return @intFromEnum(NapiStatus.ok); } pub extern fn napi_throw(env: napi_env, @"error": napi_value) napi_status; pub extern fn napi_throw_error(env: napi_env, code: [*c]const u8, msg: [*c]const u8) napi_status;