fix(event-loop): no exception should be thrown when an invalid timeoutID is passed to clearTimeout, silently do nothing instead

Xmader · Xmader · commit b155c8c1a08f · 2023-04-24T19:18:39.000Z
diff --git a/include/PyEventLoop.hh b/include/PyEventLoop.hh
@@ -55,8 +55,12 @@ public:
       _timeoutIdMap.push_back(std::move(handle));
       return _timeoutIdMap.size() - 1; // the index in `_timeoutIdMap`
     }
-    static inline AsyncHandle &fromId(uint32_t timeoutID) {
-      return _timeoutIdMap.at(timeoutID);
+    static inline AsyncHandle *fromId(uint32_t timeoutID) {
+      try {
+        return &_timeoutIdMap.at(timeoutID);
+      } catch (...) { // std::out_of_range&
+        return nullptr; // invalid timeoutID
+      }
     }
 
     /**
diff --git a/src/modules/pythonmonkey/pythonmonkey.cc b/src/modules/pythonmonkey/pythonmonkey.cc
@@ -234,13 +234,22 @@ static bool setTimeout(JSContext *cx, unsigned argc, JS::Value *vp) {
 static bool clearTimeout(JSContext *cx, unsigned argc, JS::Value *vp) {
   using AsyncHandle = PyEventLoop::AsyncHandle;
   JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
+  JS::HandleValue timeoutIdArg = args.get(0);
+
+  args.rval().setUndefined();
+
+  // silently does nothing when an invalid timeoutID is passed in
+  if (!timeoutIdArg.isNumber()) {
+    return true;
+  }
 
   // Retrieve the AsyncHandle by `timeoutID`
-  double timeoutID = args[0].toNumber();
-  AsyncHandle &handle = AsyncHandle::fromId((uint32_t)timeoutID);
+  double timeoutID = timeoutIdArg.toNumber();
+  AsyncHandle *handle = AsyncHandle::fromId((uint32_t)timeoutID);
+  if (!handle) return true; // does nothing on invalid timeoutID
 
   // Cancel this job on Python event-loop
-  handle.cancel();
+  handle->cancel();
 
   return true;
 }
diff --git a/tests/python/test_pythonmonkey_eval.py b/tests/python/test_pythonmonkey_eval.py
@@ -677,6 +677,14 @@ def to_raise(msg):
         # TODO (Tom Tang): test `setTimeout` setting delay to 0 if < 0
         # TODO (Tom Tang): test `setTimeout` accepting string as the delay, coercing to a number like parseFloat
 
+        # passing an invalid ID to `clearTimeout` should silently do nothing; no exception is thrown.
+        pm.eval("clearTimeout(NaN)")
+        pm.eval("clearTimeout(999)")
+        pm.eval("clearTimeout(-1)")
+        pm.eval("clearTimeout('a')")
+        pm.eval("clearTimeout(undefined)")
+        pm.eval("clearTimeout()")
+
         # making sure the async_fn is run
         return True
     assert asyncio.run(async_fn())

Original file line number	Diff line number	Diff line change
`@@ -55,8 +55,12 @@ public:`
`55`	`55`	`_timeoutIdMap.push_back(std::move(handle));`
`56`	`56`	return _timeoutIdMap.size() - 1; // the index in `_timeoutIdMap`
`57`	`57`	`}`
`58`		`- static inline AsyncHandle &fromId(uint32_t timeoutID) {`
`59`		`- return _timeoutIdMap.at(timeoutID);`
	`58`	`+ static inline AsyncHandle *fromId(uint32_t timeoutID) {`
	`59`	`+ try {`
	`60`	`+ return &_timeoutIdMap.at(timeoutID);`
	`61`	`+ } catch (...) { // std::out_of_range&`
	`62`	`+ return nullptr; // invalid timeoutID`
	`63`	`+ }`
`60`	`64`	`}`
`61`	`65`
`62`	`66`	`/**`