Bug in timer initialisation steps: the id's of cleared but still live timers can be reused

[majaha]

What is the issue with the HTML Standard?

The timer initialisation steps have a bug related to how timers that have been cancelled are represented, and how new timer IDs can be generated that match cleared (but still existing) timers. This is best illustrated with an example, consider these things happening in order:

• there are two functions, A() and B()
• setTimeout(A, 1000) is called
• step 2 of the timer initialisation steps is executed, giving the timer a non-zero ID. It receives the value 103.
• step 12 is executed, adding the key 103 to the map of active timers
• clearTimeout(103) is called, removing the key 103 from the map of active timers
• setTimeout(B, 2000) is called
• step 2 of the timer initialisation steps is executed, giving the timer a non-zero ID that is not in the map of active timers. It receives the ID 103, which it could do because that key has been removed from the map.
• After 1000 milliseconds, the task created in setTimeout(A, 1000), step 8, is executed.
• In step 8.1, the ID of 103 exists in the map of active timers, and so function A() is executed, even though it was cancelled.

I can see two ways of attacking this problem:

1. Stop step 2 from generating IDs of cleared timers, perhaps by moving cleared IDs to a new map of cleared timers. Then the IDs would be removed from the map of cleared timers once the task executes, perhaps in an "else" after step 8.1.
2. Try to make run steps after a timeout directly cancellable somehow. I assume this is closer to how web browsers actually do it, but I'm not sure how to implement that in specification-ese.

[annevk]

Well spotted!

How about instead of removing the entry, we set its value to null? And then update all the call sites to account for that. This is similar to your second map idea I think, but seems easier to get right.

cc @domenic

[domenic]

It feels like whatwg/infra#396 might be related here, although I'm not sure it's exactly the same thing.

How Chromium's code does it is it keeps a "next timer ID" variable. That seems relatively simple and still allows us to clear the map entries. It also reduces implementation-defined behavior.

[majaha]

Ah okay, that's good info. That's also pretty much how Firefox does it.

By the way, I spotted some Undefined Behaviour in that Chromium code (signed int overflow), which I've sent in a patch for.

The Firefox code is also technically broken, because the value can overflow and use the ID 0, or reuse ID's. (The webIDL types should probably also be changed to unsigned long).

[domenic]

Nice! Do you want to work on a spec PR to incorporate a good algorithm here?

[majaha]

Alright, I've had a bash at fixing the spec.
#10505