Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nit: timer initialization steps should define maximum for id #7358

Open
annevk opened this issue Nov 23, 2021 · 3 comments
Open

Nit: timer initialization steps should define maximum for id #7358

annevk opened this issue Nov 23, 2021 · 3 comments
Labels
clarification Standard could be clearer

Comments

@annevk
Copy link
Member

annevk commented Nov 23, 2021

After the refactoring this section will say

If previousId was given, let id be previousId; otherwise, let id be an implementation-defined integer that is greater than zero that will identify the timeout to be set by this call in global's map of active timers.

which is equivalent to what it says today. However, it seems a little bad that you have to infer from IDL what the maximum is here. I suspect to some extent it will depend on whatwg/infra#87. Using typed integers to indicate the range would make the most sense to me as that is what implementations do already.

@annevk annevk added the clarification Standard could be clearer label Nov 23, 2021
@domenic
Copy link
Member

domenic commented Nov 23, 2021

Well, what happens if you hit the maximum? The return value is only long (so, 2^31 possibilities) which is pretty easy to hit... (i.e. only takes a few minutes on modern hardware).

Looks like Chromium just loops back to 1, or... will infinite-loop, if the map is full?? https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/frame/dom_timer_coordinator.cc;l=46-47;drc=de68be3f18ba99cc01d75903e167ca09bade253c

I was also wondering if this should get more specific and specify that it's an incrementing per-global counter, since I'm pretty sure that's what everyone implements.

@domenic
Copy link
Member

domenic commented Nov 24, 2021

Talking about this issue a bit with Chromium folks and they're OK with our current infinite-loop behavior... basically, the reasoning is: timers (including the function or string) occupy at least a few bytes, so you're very likely to run out of memory and crash before hitting 2^31 timers. (E.g., 2^31 * 16 bytes = 34 GiB.) And, we already allow while (true) {}.

I'm still not super-comfortable with the situation since I don't know what to write in the spec text. Like, the spec does acknowledge resource limitations, but as written this part of the timer initialization steps spec will basically be saying "choose a not-previously-used integer within the range 1 to 2^31" which is clearly impossible after a certain point. This feels different in kind than writing spec text like "increase counter by 1" where it's implicitly understood than in most cases implementations will not use a bigint, and so increasing the counter will start being weird around 2^32 or 2^64.

For the record, I spent a bit of time perusing Firefox's code and from what I can tell they just loop back to 1 and don't care about the collision.

Maybe the easy path here is just something like

If there is no available ID in the range (due to these steps having been called 2^31 times), and for some reason [resource limitations] have not yet been hit, behavior is implementation-defined.

Although we don't like implementation-defined behavior in the general case, in this case it's probably not worth spending time on since it's pretty theoretical.

@annevk
Copy link
Member Author

annevk commented Nov 24, 2021

Adding @smaug---- and @farre for thoughts from Firefox. I suppose implementation-defined is reasonable for the limit case. At least until it starts happening in the future for one reason or another. It behaving as incrementing per global might be nice to define though. And if that's defined it also makes it possible for developers to bail before they hit the limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Standard could be clearer
Development

No branches or pull requests

2 participants