Bugfixes for setTimeout and setInterval (#66)

Before this patch, timer (i.e., `setTimeout`/`setInterval`) returned the pollable handles as IDs. This is problematic, because pollable handles are reused, leading to content potentially clearing the wrong timer.

Additionally, the code wasn't robust against a timer being cleared in its own callback, leading to a failing assertion.

-----

This PR also includes the following changes:

* Introduce an explicitly async test variant to the integration tests framework

This cuts down on redundancy and is a bit easier to use, IMO.

I didn't change all tests to use this instead of the existing async support, but we could consider doing that at some point.

* Fix issues with timer subtest

The test didn't properly clean up its timeout and interval, which meant that the former would throw an exception into the ether, and the latter would continue running the interval forever.

(The exception thrown by the former also happened to be the wrong one, since `AssertionError` wasn't imported.)

builtins/web/timers.cpp

@@ -6,6 +6,7 @@
 #include <host_api.h>
 #include <iostream>
 #include <list>
+#include <map>
 #include <vector>
 
 #define S_TO_NS(s) ((s) * 1000000000)
@@ -15,6 +16,11 @@ static api::Engine *ENGINE;
 
 class TimerTask final : public api::AsyncTask {
   using TimerArgumentsVector = std::vector<JS::Heap<JS::Value>>;
+
+  static std::map<int32_t, PollableHandle> timer_ids_;
+  static int32_t next_timer_id;
+
+  int32_t timer_id_;
   int64_t delay_;
   int64_t deadline_;
   bool repeat_;
@@ -34,6 +40,8 @@ class TimerTask final : public api::AsyncTask {
     }
 
     handle_ = host_api::MonotonicClock::subscribe(deadline_, true);
+    timer_id_ = next_timer_id++;
+    timer_ids_.emplace(timer_id_, handle_);
   }
 
   [[nodiscard]] bool run(api::Engine *engine) override {
@@ -55,14 +63,25 @@ class TimerTask final : public api::AsyncTask {
       return false;
     }
 
-    if (repeat_) {
-      engine->queue_async_task(new TimerTask(delay_, true, callback, argv));
+    // The task might've been canceled during the callback.
+    if (handle_ != INVALID_POLLABLE_HANDLE) {
+      host_api::MonotonicClock::unsubscribe(handle_);
     }
 
-    return cancel(engine);
+    if (repeat_ && timer_ids_.contains(timer_id_)) {
+      deadline_ = host_api::MonotonicClock::now() + delay_;
+      handle_ = host_api::MonotonicClock::subscribe(deadline_, true);
+      engine->queue_async_task(this);
+    }
+
+    return true;
   }
 
   [[nodiscard]] bool cancel(api::Engine *engine) override {
+    if (!timer_ids_.contains(timer_id_)) {
+      return false;
+    }
+
     host_api::MonotonicClock::unsubscribe(id());
     handle_ = -1;
     return true;
@@ -72,14 +91,31 @@ class TimerTask final : public api::AsyncTask {
     return deadline_;
   }
 
+  [[nodiscard]] int32_t timer_id() const {
+    return timer_id_;
+  }
+
   void trace(JSTracer *trc) override {
     TraceEdge(trc, &callback_, "Timer callback");
     for (auto &arg : arguments_) {
       TraceEdge(trc, &arg, "Timer callback arguments");
     }
   }
+
+  static bool clear(int32_t timer_id) {
+    if (!timer_ids_.contains(timer_id)) {
+      return false;
+    }
+
+    ENGINE->cancel_async_task(timer_ids_[timer_id]);
+    timer_ids_.erase(timer_id);
+    return true;
+  }
 };
 
+std::map<int32_t, PollableHandle> TimerTask::timer_ids_ = {};
+int32_t TimerTask::next_timer_id = 0;
+
 namespace builtins::web::timers {
 
 /**
@@ -123,7 +159,7 @@ template <bool repeat> bool setTimeout_or_interval(JSContext *cx, const unsigned
 
   const auto timer = new TimerTask(delay, repeat, handler, handler_args);
   ENGINE->queue_async_task(timer);
-  args.rval().setInt32(timer->id());
+  args.rval().setInt32(timer->timer_id());
 
   return true;
 }
@@ -144,7 +180,7 @@ template <bool interval> bool clearTimeout_or_interval(JSContext *cx, unsigned a
     return false;
   }
 
-  ENGINE->cancel_async_task(id);
+  TimerTask::clear(id);
 
   args.rval().setUndefined();
   return true;

runtime/event_loop.cpp

@@ -23,7 +23,10 @@ static PersistentRooted<TaskQueue> queue;
 
 namespace core {
 
-void EventLoop::queue_async_task(api::AsyncTask *task) { queue.get().tasks.emplace_back(task); }
+void EventLoop::queue_async_task(api::AsyncTask *task) {
+  MOZ_ASSERT(task);
+  queue.get().tasks.emplace_back(task);
+}
 
 bool EventLoop::cancel_async_task(api::Engine *engine, const int32_t id) {
   const auto tasks = &queue.get().tasks;
@@ -87,8 +90,8 @@ bool EventLoop::run_event_loop(api::Engine *engine, double total_compute) {
     size_t task_idx = api::AsyncTask::select(*tasks);
 
     auto task = tasks->at(task_idx);
-    bool success = task->run(engine);
     tasks->erase(tasks->begin() + task_idx);
+    bool success = task->run(engine);
     if (!success) {
       exit_event_loop();
       return false;

tests/integration/test-server.js

@@ -35,6 +35,21 @@ export function serveTest (handler) {
             curSubtest = null;
             subtestCnt++;
           }
+        },
+        async asyncTest (name, subtest) {
+          if (!filter(name))
+            return;
+          curSubtest = name;
+          let resolve, reject;
+          let promise = new Promise((res, rej) => {
+            resolve = res;
+            reject = rej;
+          });
+          subtest(resolve, reject);
+          evt.waitUntil(promise);
+          await promise;
+          curSubtest = null;
+          subtestCnt++;
         }
       });
     }

tests/integration/timers/timers.js

@@ -1,36 +1,41 @@
 import { serveTest } from "../test-server.js";
-import { assert, strictEqual, throws, deepStrictEqual } from "../assert.js";
+import { assert, strictEqual, throws, deepStrictEqual, AssertionError } from "../assert.js";
 
 export const handler = serveTest(async (t) => {
-  await t.test("setTimeout", async () => {
+  await t.asyncTest("setTimeout-order", (resolve, reject) => {
     let first = false;
-    return new Promise((resolve, reject) => {
-      setTimeout(() => {
-        first = true;
-      }, 10);
-      setTimeout(() => {
-        try {
-          assert(first, 'first timeout should trigger first');
-        } catch (e) {
-          reject(e);
-          return;
-        }
+    setTimeout(() => {
+      first = true;
+    }, 10);
+    setTimeout(() => {
+      try {
+        assert(first, 'first timeout should trigger first');
+      } catch (e) {
+        reject(e);
+        return;
+      }
+      resolve();
+    }, 20);
+  });
+  await t.asyncTest("setInterval-10-times", (resolve, reject) => {
+    let timeout = setTimeout(() => {
+      reject(new AssertionError("Expected setInterval to be called 10 times quickly"));
+    }, 1000);
+    let cnt = 0;
+    let interval = setInterval(() => {
+      cnt++;
+      if (cnt === 10) {
+        clearTimeout(timeout);
+        clearInterval(interval);
         resolve();
-      }, 20);
+      }
     });
   });
-  await t.test("setInterval", async () => {
-    return new Promise((resolve, reject) => {
-      setTimeout(() => {
-        reject(new AssertionError("Expected setInterval to be called 10 times quickly"));
-      }, 1000);
-      let cnt = 0;
-      setInterval(() => {
-        cnt++;
-        if (cnt === 10)
-          resolve();
-      });
-    });
+  await t.asyncTest("setTimeout-cleared-in-callback", (resolve, reject) => {
+    let id = setTimeout.call(undefined, () => {
+      clearTimeout(id);
+      resolve();
+    }, 1);
   });
   t.test("setInterval-exposed-as-global", () => {
     strictEqual(typeof setInterval, "function", `typeof setInterval`);
@@ -343,6 +348,9 @@ export const handler = serveTest(async (t) => {
   t.test("setTimeout-called-unbound", () => {
     setTimeout.call(undefined, () => {}, 1);
   });
+  t.test("setTimeout-cleared-in-callback", () => {
+    let id = setTimeout.call(undefined, () => { clearTimeout(id); }, 1);
+  });
 
   t.test("clearInterval-exposed-as-global", () => {
     deepStrictEqual(typeof clearInterval, "function", `typeof clearInterval`);