fix: clamp setTimeout value to prevent negative timeout crash#39273
fix: clamp setTimeout value to prevent negative timeout crash#39273veeceey wants to merge 4 commits intomicrosoft:mainfrom
Conversation
When a Node.js process has been running for more than ~24.8 days, monotonicTime() exceeds 2^31-1 ms. This causes the computed timeout in raceAgainstDeadline() to become negative, which Node.js coerces to 1ms, resulting in an immediate spurious timeout. Clamp the timeout to [0, 2^31-1] to prevent both negative values and values exceeding setTimeout's maximum safe delay. Fixes microsoft#39166
| new Promise<{ timedOut: true }>(resolve => { | ||
| const kMaxDeadline = 2147483647; // 2^31-1 | ||
| const timeout = (deadline || kMaxDeadline) - monotonicTime(); | ||
| const rawTimeout = (deadline || kMaxDeadline) - monotonicTime(); |
There was a problem hiding this comment.
It is better to not set a timer at all when there is no deadline, here and in other places across the code.
There was a problem hiding this comment.
Good call -- updated to skip the timer entirely when there's no deadline instead of clamping. If deadline is 0, the promise executor returns early so no setTimeout is created at all. The race then just waits on the callback.
Instead of falling back to kMaxDeadline and clamping, just return early from the promise executor when deadline is 0. This avoids the negative timeout issue altogether by not creating a timer when none is needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tor.ts Apply the same negative timeout prevention to other places in the codebase that compute deadline - monotonicTime() without clamping. This prevents passing negative values to setTimeout when the deadline has already passed.
| throw new Error(`Could not resolve ${this._selector} to DOM Element`); | ||
| try { | ||
| return await task(handle, deadline ? deadline - monotonicTime() : 0); | ||
| return await task(handle, deadline ? Math.max(deadline - monotonicTime(), 0) : 0); |
There was a problem hiding this comment.
Why did this change? I don't think Math.max(deadline - monotonicTime(), 0) will be different from deadline - monotonicTime().
There was a problem hiding this comment.
You're right, this was a pointless change — the timeout value here is just passed through to the task, not to setTimeout. Reverted it back to the original.
| this._controller.abort(timeoutError); | ||
| } | ||
| }, deadline - monotonicTime()); | ||
| }, Math.max(deadline - monotonicTime(), 0)); |
There was a problem hiding this comment.
Same fix here — when remaining time is <= 0, the timeout error is fired immediately without scheduling a timer.
| const timeout = (deadline || kMaxDeadline) - monotonicTime(); | ||
| timer = setTimeout(() => resolve({ timedOut: true }), timeout); | ||
| const timeout = Math.max(deadline - monotonicTime(), 0); | ||
| timer = setTimeout(() => resolve({ timedOut: true }), Math.min(timeout, kMaxDeadline)); |
There was a problem hiding this comment.
I thought we agreed to not set timer at all. Here we are at risk of firing in 0ms.
There was a problem hiding this comment.
You're right, clamping to 0 still fires the timer. Updated to check deadline - monotonicTime() <= 0 and resolve immediately with { timedOut: true } — no setTimeout at all when the deadline has already passed.
Instead of clamping to Math.max(..., 0) which still schedules a 0ms timer, check if the deadline has passed and either resolve/reject immediately or skip the setTimeout altogether. Reverted the unnecessary Math.max change in locator.ts where it doesn't apply.
2 participants
Problem
When a Node.js process has been running for more than ~24.8 days (2^31-1 milliseconds),
monotonicTime()exceeds the max signed 32-bit integer. InraceAgainstDeadline(), the timeout is computed as:When
monotonicTime()exceedskMaxDeadline(e.g. no explicit timeout was set, sodeadlineis0and falls back tokMaxDeadline), this produces a negative value. Node.js then coerces it to 1ms and fires the timer immediately, causing operations likebrowserType.connect()to fail with:Fix
Clamp the computed timeout to
[0, kMaxDeadline]:0prevents negative values that Node.js coerces to 1mskMaxDeadline(2^31-1) keeps it within setTimeout's valid rangeHow I verified this
Reproduced the issue by overriding
performance.nowto return a value > 2^31-1, confirmingconnect()fails before the fix and succeeds after.Fixes #39166