Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions changelog.d/10588-mock-timer-dispatch-pin.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
Fixed a follow-on to #10447/#10538: a one-shot `node:test` mock timer
(`setTimeout` under `mock.timers`) dropped its own registry pin
(`ScheduledTimerId`) the moment it left the mock queue for dispatch, rather
than after its callback returned. A callback that then scheduled and cleared
more than the registry's 65,536-entry eviction cap's worth of other timers
before finishing evicted its own handle mid-dispatch — the same symptom
#10447 fixed for long-lived real timers, reopened narrowly for the mock
dispatch path. The pin now rides along in the dispatch action tuple and
drops only after `call_timer_callback` returns. Real (non-mock) timers and
mock intervals were never affected. Reproduced against clean `main` with a
new regression test before the fix, confirmed passing after.
24 changes: 22 additions & 2 deletions crates/perry-runtime/src/timer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -815,21 +815,41 @@ fn mock_timers_advance_to(target_ms: f64) {
};
state.current_ms = due_ms;
if is_interval {
// The interval entry stays in the queue (it re-fires), so its
// `_scheduled` pin is untouched here — nothing to carry.
let timer = &mut state.intervals[idx];
timer.next_ms = due_ms + timer.interval_ms.max(1) as f64;
Some((
timer.id,
timer.callback,
timer.args.clone(),
timer.context.clone(),
None,
))
} else {
// #10447 follow-up: `remove` takes the WHOLE entry, including
// its `_scheduled` pin. Move that pin into the action too and
// hand it back below, instead of leaving it behind on `timer`
// to drop (and retire the id) right here — before
// `call_timer_callback` has even run, let alone finished. A
// one-shot mock timer otherwise loses its own registry entry
// if its callback churns more than the eviction cap's worth of
// other timers while it is still dispatching.
let timer = state.callbacks.remove(idx);
Some((timer.id, timer.callback, timer.args, timer.context))
Some((
timer.id,
timer.callback,
timer.args,
timer.context,
Some(timer._scheduled),
))
}
};
if let Some((id, callback, args, context)) = action {
if let Some((id, callback, args, context, _pin)) = action {
call_timer_callback(id, callback, &args, &context);
// `_pin` (the one-shot case's `ScheduledTimerId`, moved out of the
// popped queue entry above) stays alive across that call and only
// retires the id here, after the callback has returned.
}
}
}
Expand Down
70 changes: 70 additions & 0 deletions crates/perry-runtime/src/timer/tests_inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,3 +244,73 @@ mod expired_batch_order_tests {
);
}
}

#[cfg(test)]
mod mock_dispatch_own_pin_tests {
use super::*;
use std::sync::atomic::{AtomicBool, AtomicI64, Ordering};

static SELF_ID: AtomicI64 = AtomicI64::new(0);
static SAW_KNOWN: AtomicBool = AtomicBool::new(false);
static SAW_HAS_REF: AtomicBool = AtomicBool::new(false);
static RAN: AtomicBool = AtomicBool::new(false);

/// A one-shot mock timer's own callback: churns more real one-shot timers
/// than the registry's eviction cap, then checks its OWN id. If this
/// timer's `_scheduled` pin already retired the moment it was popped off
/// the mock queue for dispatch (the bug), it is the OLDEST retired id in
/// the shared registry when the churn starts, so it is the very first one
/// evicted once the churn passes the cap — and this callback observes its
/// own eviction while it is still running.
extern "C" fn churn_then_check_self(_closure: *const crate::closure::ClosureHeader) -> f64 {
let id = SELF_ID.load(Ordering::SeqCst);
for _ in 0..(ref_states::TIMER_REF_STATES_CAP + 2_000) {
clearTimeout(js_set_timeout_callback(0, 1_000.0));
}
SAW_KNOWN.store(is_known_timer_id(id), Ordering::SeqCst);
SAW_HAS_REF.store(js_timer_has_ref(id) != 0, Ordering::SeqCst);
RAN.store(true, Ordering::SeqCst);
0.0
}

/// #10447 follow-up: `mock_timers_advance_to` used to pop a one-shot mock
/// timer off the queue with `state.callbacks.remove(idx)` and destructure
/// out `(id, callback, args, context)` — leaving the popped entry's
/// `_scheduled: ScheduledTimerId` behind to drop, and retire the id, right
/// there, before `call_timer_callback` had even run, let alone finished.
/// A callback that then churned more timers than the eviction cap evicted
/// its OWN handle mid-dispatch. The fix carries the pin into the dispatch
/// action and drops it only after the callback returns.
#[test]
fn a_one_shot_mock_timers_own_pin_survives_its_own_dispatch() {
let _serial = crate::gc::global_side_table_test_lock();
SAW_KNOWN.store(false, Ordering::SeqCst);
SAW_HAS_REF.store(false, Ordering::SeqCst);
RAN.store(false, Ordering::SeqCst);
js_mock_timers_reset();
js_mock_timers_enable(MOCK_TIMERS_API_SET_TIMEOUT, 0.0);

let closure = crate::closure::js_closure_alloc(churn_then_check_self as *const u8, 0);
let id = schedule_mock_callback_timer(
closure as i64,
10.0,
Vec::new(),
CallbackTimerKind::Timeout,
)
.expect("mock setTimeout must be enabled for this API set");
SELF_ID.store(id, Ordering::SeqCst);

js_mock_timers_tick(10.0);

assert!(RAN.load(Ordering::SeqCst), "the mock timer never fired");
assert!(
SAW_KNOWN.load(Ordering::SeqCst),
"timer {id} was evicted from the registry by its own callback's churn"
);
assert!(
SAW_HAS_REF.load(Ordering::SeqCst),
"timer {id}'s ref state was lost to its own callback's churn"
);
js_mock_timers_reset();
}
}
6 changes: 6 additions & 0 deletions scripts/gc_runtime_root_holders.json
Original file line number Diff line number Diff line change
Expand Up @@ -2529,6 +2529,12 @@
"name": "WINDOW_ROOTS",
"verdict": "not_a_gc_pointer",
"why": "Window-root registry maps numeric window handles to numeric root-widget handles; neither value is a JavaScript heap pointer."
},
{
"file": "crates/perry-runtime/src/timer/tests_inline.rs",
"name": "SELF_ID",
"verdict": "test_only",
"why": "AtomicI64 holding a scheduled mock timer's id (an i64 returned by schedule_mock_callback_timer, never a GC heap pointer) so the timer's own extern \"C\" callback can look itself up in the ref-state registry mid-dispatch. Declared under #[cfg(test)] only (tests_inline.rs, mock_dispatch_own_pin_tests), never live in a shipped binary."
}
],
"_FRONTIER_README": "Identity-pinned debt ratchet over new perry-ui* candidates and otherwise-unclassified core raw/Perry TLS declarations (see the census docstring, \u201cThe identity-pinned frontier\u201d). A new uncovered holder fails until it is scanned, receives a researched holders verdict, or is deliberately pinned as debt. Moving a researched false positive to holders graduates it from this list. A fixed or classified holder makes its old frontier pin stale, so the receipt must be deleted.",
Expand Down
Loading