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,071	51,071	0
Ignored	1,482	1,482	0
Failed	572	572	0
Panics	0	0	0
Conformance	96.13%	96.13%	0.00%

Tested main commit: 8f5ef6542d641fd22320e51234e914b59e623717
Tested PR commit: 6389f83dd0c9c85639c849858f4602ff8eccd303
Compare commits: 8f5ef65...6389f83

[codecov]

Codecov Report

❌ Patch coverage is 58.62069% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.04%. Comparing base (6ddc2b4) to head (6389f83).
⚠️ Report is 967 commits behind head on main.

Files with missing lines	Patch %	Lines
core/runtime/src/interval.rs	73.91%	6 Missing ⚠️
core/engine/src/job.rs	0.00%	4 Missing ⚠️
core/engine/src/context/mod.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #5379       +/-   ##
===========================================
+ Coverage   47.24%   60.04%   +12.79%     
===========================================
  Files         476      566       +90     
  Lines       46892    63037    +16145     
===========================================
+ Hits        22154    37850    +15696     
- Misses      24738    25187      +449     

☔ 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.

[hansl]

No need to remove.

[iammdzaidalam]

Oops, accidental removal while fixing path handling. Restoring it.

[hansl]

Which test requires this?

[iammdzaidalam]

The test runner needs it, not any specific test. Workspace reqwest is pinned with rustls-no-provider so reqwest::blocking::Client panics with No provider set without a CryptoProvider installed. create_context() builds a WptFetcher (and a reqwest Client) for every test so the suite won't run without it.

I removed the line locally to verify, encoding and timers both panic the same way. Checked main too and cargo test encoding fails identically there, so this is a latent harness issue ig, not something this PR introduced.

Same install already exists in cli/src/main.rs and examples/src/bin/module_fetch_async.rs. Happy to split it out or add a comment, whichever works.

[hansl]

If it's not needed, I'd rather like to leave it out of the PR.

[iammdzaidalam]

yep not needed anymore after the switch to interval::clear_all. will remove in next push.