fix(runtime): make timer IDs 1-based to avoid sentinel collision

[iammdzaidalam]

Closes #5378

Problem

IntervalInnerState::next_id() returns the internal counter before incrementing it. Since the counter starts at 0 via Default, the first valid timer gets handle 0:

fn next_id(&mut self) -> JsResult<u32> {
    self.active_map.retain(|_, v| !v.revoked());
    let id = self.id;
    self.id = id.checked_add(1)...;
    Ok(id)
}

At the same time, both set_timeout and set_interval return Ok(0) when no callback is provided. That makes the first real timer and the "nothing was scheduled" sentinel indistinguishable. It also violates WHATWG HTML timer semantics, which require positive timer handles, and breaks common JavaScript code that treats a returned timer ID as truthy.

Fix

Update next_id() to increment the counter before returning it, so valid timer IDs become 1-based again. This preserves 0 for the no-callback sentinel and restores the pre-#5289 behavior.

Tests

• timer_ids_are_positive_and_unique asserts that timer IDs returned by setTimeout and setInterval are greater than 0 and unique.
• no_callback_returns_zero_sentinel asserts that setTimeout() with no callback still returns 0 and does not collide with a valid timer ID.

Verification

• cargo test -p boa_runtime
• cargo run -p boa_cli -- -e "console.log(setTimeout(() => {}, 10)); console.log(setTimeout(() => {}, 10)); console.log(setTimeout())"

Before the fix, the first valid timer returned 0. After the fix, the output is 1, 2, 0.

[github-actions]

Test262 conformance changes

Test result	main count	PR count	difference
Total	53,125	53,125	0
Passed	51,072	51,072	0
Ignored	1,482	1,482	0
Failed	571	571	0
Panics	0	0	0
Conformance	96.14%	96.14%	0.00%

Tested main commit: 06b3a45524f55ed7e94149b7b5eaece20b8d22a1
Tested PR commit: 119b9be6a15c342d605d67c47a922b4b8fd5eba5
Compare commits: 06b3a45...119b9be

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.12%. Comparing base (6ddc2b4) to head (119b9be).
⚠️ Report is 968 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #5379       +/-   ##
===========================================
+ Coverage   47.24%   60.12%   +12.87%     
===========================================
  Files         476      566       +90     
  Lines       46892    63031    +16139     
===========================================
+ Hits        22154    37895    +15741     
- Misses      24738    25136      +398     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[iammdzaidalam]

hello @hansl @jedel1043, updated the pr.

• moved timer ids to NonZeroU32
• fixed clearTimeout / clearInterval behavior
• switched delay conversion to to_i32() (for webidl long behavior)
• enabled the WPT timer tests

also had to fix a windows WPT path issue, add clear_jobs for interval tests, and add the missing experimental cfg flags in iterable/tests.rs.

If needed i can split the last part.

[hansl]

I'm not convinced we need clear_jobs. We shouldn't need to clear jobs and should fail if jobs are not completed by the end of the run. Let me think a bit more about it.

[iammdzaidalam]

hi @hansl. i went with this because without clear_jobs() the WPT timer tests were hanging forever. some tests call done() but leave setIntervals running, so run_jobs_async() never exits because the jobs keep adding themselves back to the queue.

for now i used clear_jobs() to simulate browser teardown and get the tests passing. not sure if this is the best long term approach for Boa though, so would really love your guidance on handling this.

[hansl]

@iammdzaidalam Hey Zaid. Sorry for the late reply I was out on a trip last weekend, and thinking about how to approach this.

What you actually want to do is not cancel jobs (there are jobs in there that aren't timers/intervals). You want to clear timers themselves and let the engine exit gracefully. Am I right?

In that case, what you could do is add a clear_all() (or clear_timers or similar) function to the runtime (public, in interval) that specifically cancel intervals and timers. Since you have access to the list of valid timers, it should be trivial to cancel all of them. Then the test can exit run_jobs and if there is a job hanging that is likely an actual bug we would need to fix.

What do you think?

[iammdzaidalam]

Hey @hansl, hope the trip was good 😊.

tried a repro with a Promise.resolve().then() and an uncleared setInterval. With clear_jobs() the promise callback never ran since it clears more than just timers. With clear_all() the promise resolves normally and run_jobs_async() exits once the intervals are cancelled.

Also confirmed the hang directly. run_jobs_async() stayed Pending with a live interval until timers were cleared.
All 7 WPT timer tests still pass.

Also i am keeping the 10 second deadline in the harness so we notice if something else holds the loop open later.

seems like the right direction, thanks!

[hansl]

Nothing major, but I'm curious why the rustls dependency was added.

[iammdzaidalam]

added a test coverage for the clear_all function... missed it in last commit

[hansl]

Waiting for #5389 to go in then I'll merge main and re-run CI.

[hansl]

@jedel1043 need your approval, PTAL

[jedel1043]

Looks good!