fix: clamp setTimeout value to prevent negative timeout crash#39273
fix: clamp setTimeout value to prevent negative timeout crash#39273yury-s merged 2 commits intomicrosoft:mainfrom
Conversation
| 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.
| 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.
| if (this.metadata.pauseStartTime && !this.metadata.pauseEndTime) | ||
| return; | ||
| const remaining = deadline - monotonicTime(); | ||
| if (remaining <= 0) { |
There was a problem hiding this comment.
We can't be doing this, we've already measured monotonicTime once and built deadline on top of it, so we should never end up in this branch. If there is deadline, ever so little, we set timer. Otherwise, if deadline is 0 we do not.
There was a problem hiding this comment.
You're right, sorry about that. Reverted progress.ts entirely -- if deadline is set then the timer should always have positive remaining time since it was just computed. The only change now is in timeoutRunner.ts: skip the timer when deadline is 0 instead of computing a ~24-day timeout from kMaxDeadline.
|
You're absolutely right — measuring monotonicTime twice defeats the purpose. I'll rework this to keep it simple: if deadline is set (deadline > 0), create the timer; if deadline is 0, skip the timer entirely. That aligns with the original measurement and avoids the redundant second check. Will push an updated approach shortly. |
When deadline is 0, return early from the promise executor instead of computing a huge timeout via (0 || kMaxDeadline). This avoids setting an unnecessary ~24-day timer. When deadline is set, cap the timeout with Math.min to stay within setTimeout's 32-bit limit.
veeceey
force-pushed
the
fix/issue-39166-negative-settimeout
branch
from
81656f6 to
18e6a9b
Compare
When deadline is 0 (no timeout), skip the timer entirely instead of computing a fallback from kMaxDeadline. When deadline is set, use deadline - monotonicTime() directly as the timer value. Remove the kMaxDeadline constant since it's no longer needed.
Test results for "tests 1"2 flaky37556 passed, 838 skipped Merge workflow run. |
3 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