Repository: denoland/deno_core
Pull request #616 (2024-02-28 16:11:02)

fix(core): ensure that timers aren't dispatched if cancelled in timer phase


Patch:
diff --git a/core/01_core.js b/core/01_core.js
index c543758..b65f811 100644
--- a/core/01_core.js
+++ b/core/01_core.js
@@ -16,6 +16,7 @@
     Proxy,
     setQueueMicrotask,
     SafeMap,
+    Set,
     StringPrototypeSlice,
     Symbol,
     SymbolFor,
@@ -134,6 +135,8 @@
 
   let unhandledPromiseRejectionHandler = () => false;
   let timerDepth = 0;
+  let timersRunning = false;
+  const cancelledTimers = new Set();
 
   const macrotaskCallbacks = [];
   const nextTickCallbacks = [];
@@ -189,17 +192,24 @@
 
     const timers = arguments[arguments.length - 2];
     if (timers) {
-      for (let i = 0; i < timers.length; i += 2) {
+      timersRunning = true;
+      for (let i = 0; i < timers.length; i += 3) {
         timerDepth = timers[i];
+        const id = timers[i + 1];
+        if (cancelledTimers.has(id)) {
+          continue;
+        }
         try {
-          const f = timers[i + 1];
+          const f = timers[i + 2];
           f.call(window);
         } catch (e) {
           reportExceptionCallback(e);
         }
         op_run_microtasks();
       }
+      timersRunning = false;
       timerDepth = 0;
+      cancelledTimers.clear();
     }
 
     // If we have any rejections for this tick, attempt to process them
@@ -718,7 +728,12 @@
     queueSystemTimer: (_associatedOp, repeat, timeout, task) =>
       op_timer_queue_system(repeat, timeout, task),
     queueImmediate: (task) => op_timer_queue_immediate(task),
-    cancelTimer: (id) => op_timer_cancel(id),
+    cancelTimer: (id) => {
+      if (timersRunning) {
+        cancelledTimers.add(id);
+      }
+      op_timer_cancel(id);
+    },
     refTimer: (id) => op_timer_ref(id),
     unrefTimer: (id) => op_timer_unref(id),
     getTimerDepth: () => timerDepth,
diff --git a/core/runtime/jsruntime.rs b/core/runtime/jsruntime.rs
index 6531f2d..ccf73b5 100644
--- a/core/runtime/jsruntime.rs
+++ b/core/runtime/jsruntime.rs
@@ -2275,10 +2275,12 @@ impl JsRuntime {
 
     args.push(rejections);
 
+    // TODO(mmastrac): timer dispatch should be done via direct function call, but we will have to start
+    // storing the exception-reporting callback.
     let timers =
       if let Poll::Ready(timers) = context_state.timers.poll_timers(cx) {
         let traces_enabled = context_state.activity_traces.is_enabled();
-        let arr = v8::Array::new(scope, (timers.len() * 2) as _);
+        let arr = v8::Array::new(scope, (timers.len() * 3) as _);
         #[allow(clippy::needless_range_loop)]
         for i in 0..timers.len() {
           if traces_enabled {
@@ -2287,10 +2289,13 @@ impl JsRuntime {
               .activity_traces
               .complete(RuntimeActivityType::Timer, timers[i].0 as _);
           }
+          // depth, id, function
           let value = v8::Integer::new(scope, timers[i].1 .1 as _);
-          arr.set_index(scope, (i * 2) as _, value.into());
+          arr.set_index(scope, (i * 3) as _, value.into());
+          let value = v8::Number::new(scope, timers[i].0 as _);
+          arr.set_index(scope, (i * 3 + 1) as _, value.into());
           let value = v8::Local::new(scope, timers[i].1 .0.clone());
-          arr.set_index(scope, (i * 2 + 1) as _, value.into());
+          arr.set_index(scope, (i * 3 + 2) as _, value.into());
         }
         arr.into()
       } else {


Test patch:
diff --git a/testing/unit/timer_test.ts b/testing/unit/timer_test.ts
index e124227..978ac30 100644
--- a/testing/unit/timer_test.ts
+++ b/testing/unit/timer_test.ts
@@ -115,3 +115,23 @@ test(async function testTimerThis() {
   }, 1);
   await promise;
 });
+
+test(async function testCancellationDuringDispatch() {
+  const timers: number[] = [];
+  let timeouts = 0;
+
+  // If a timer is ready to be dispatched, but is cancelled during the
+  // dispatch of a previous timer that is also ready, that timer should
+  // not be dispatched.
+  function onTimeout() {
+    ++timeouts;
+    for (let i = 1; i < timers.length; i++) {
+      clearTimeout(timers[i]);
+    }
+  }
+  for (let i = 0; i < 10; i++) {
+    timers[i] = setTimeout(onTimeout, 1);
+  }
+  await new Promise((r) => setTimeout(r, 10));
+  assertEquals(timeouts, 1);
+});