Event: implement timeStamp instead of always returning 0#34894
Conversation
Event.prototype.timeStamp was a stubbed 'return 0.0' in Event::timeStampForBindings. The WebCore implementation relies on Performance::m_timeOrigin being a real MonotonicTime, but GlobalObject::performance() was storing wall-clock epoch milliseconds there so that Performance::timeOrigin() could hand them back for toJSON(). That made m_timeOrigin unusable as a monotonic reference. Store a proper MonotonicTime origin (current WTF clock minus Bun's origin_timer elapsed), move the wall-clock lookup for Performance::timeOrigin() onto Bun__readOriginTimerStart directly, and compute timeStamp as (m_createTime - monotonicTimeOrigin).milliseconds().
|
Updated 4:48 AM PT - Jul 21st, 2026
❌ @robobun, your commit 99ab564 has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 34894That installs a local version of the PR into your bun-34894 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline test-flake nits, I checked the other consumers of Performance::m_timeOrigin since its meaning changed from wall-clock-in-a-MonotonicTime-wrapper to a real MonotonicTime: Performance::timeOrigin() (used by performance.toJSON() at JSPerformance.cpp:408) was updated in the same commit and returns the identical value as before; addResourceTiming, relativeTimeFromTimeOriginInReducedResolution, and monotonicTimeFromRelativeTime have no live callers in Bun (or feed only commented-out inspector code), so no observable behavior regresses there.
Extended reasoning...
Verified the m_timeOrigin semantic change against every consumer in src/jsc/bindings: the only user-reachable path was Performance::timeOrigin() → performance.toJSON(), which now reads Bun__readOriginTimerStart directly and is numerically identical to the old m_timeOrigin.secondsSinceEpoch().milliseconds() round-trip. The PerformanceResourceTiming/UserTiming paths that also read m_timeOrigin are dead (no caller of addResourceTiming; monotonicTimeFromRelativeTime only feeds commented-out InspectorInstrumentation). Not approving — native JSC-bindings change with two flagged test-quality nits and no local test proof; worth a human look.
…pect snapshots
The Event.prototype.timeStamp test compared timeStamp against
performance.now() with zero tolerance. Because m_timeOrigin is derived
from two adjacent clock reads (Bun__readOriginTimer then
MonotonicTime::now()), there is a tiny fixed offset; on Windows release
that was ~1us, enough to trip a >= between back-to-back samples. Use a
1ms epsilon on the range checks and compare the 'later event' against
the previous timeStamp (same clock, same origin) instead of against
performance.now(). Also drop the strict empty-stderr assertion in favour
of a combined { stderr, exitCode } check.
inspect.test.js snapshots for CloseEvent/CustomEvent had the stubbed
'timeStamp: 0' baked in; normalize the now-dynamic value before
matching.
…ake-timer-aware one Bun__readOriginTimer honours jest.useFakeTimers() via vm.overridden_performance_now, so if the Performance object is lazily created while fake timers are active, m_timeOrigin is derived from the fake offset and every Event.timeStamp is permanently skewed. Add Bun__readOriginTimerRaw (origin_timer.elapsed() with no override check) and use it for the one-time origin derivation.
There was a problem hiding this comment.
No new issues after 99ab564 — the three earlier findings are all addressed. This is a real behavioral change to Event.timeStamp plus a semantic change to Performance::m_timeOrigin, so leaving it for a human to sign off.
What was reviewed:
Bun__readOriginTimerRawcorrectly bypassesvm.overridden_performance_now;Performance::now()still uses the fake-timer-aware variant.Performance::timeOrigin()'s only caller istoJSON(), which now matches the own-propertytimeOriginset inJSPerformance::finishCreation— previously they could diverge.- Other
m_timeOriginconsumers (monotonicTimeFromRelativeTimeinperformance.mark,PerformanceResourceTiming::create) were already receiving a bogus wall-clock-as-monotonic value; this change makes them coherent rather than regressing them. defaultGlobalObject(context.jsGlobalObject())intimeStampForBindingsresolves per-worker, so worker events use the worker VM'sorigin_timer.
Extended reasoning...
Overview
Implements Event.prototype.timeStamp (was hard-coded 0.0). To do so it changes what Performance::m_timeOrigin stores — from a wall-clock-epoch value smuggled through MonotonicTime::fromRawSeconds to an actual WTF MonotonicTime at VM-origin — and moves the wall-clock read for performance.timeOrigin/toJSON() to Bun__readOriginTimerStart directly. Adds Bun__readOriginTimerRaw so the one-time origin derivation cannot be poisoned by jest.useFakeTimers(). Test coverage in web-globals.test.js plus two inspect-snapshot normalizations.
Security risks
None. No untrusted input parsing, no auth/crypto, no allocation sizing. The only new surface is a timestamp read.
Level of scrutiny
Medium. The diff is small, but it redefines the meaning of a field (m_timeOrigin) that other WebCore code reads, and it interacts with fake timers, workers, and two independent monotonic clock sources. I checked each existing consumer of m_timeOrigin: monotonicTimeFromRelativeTime (only feeds a commented-out InspectorInstrumentation call), relativeTimeFromTimeOriginInReducedResolution (no live callers), and PerformanceResourceTiming::create (was already receiving garbage) — none regress. Performance::timeOrigin() uses the same scriptExecutionContext()->vm() pattern as the existing Performance::now() right above it.
Other factors
All three concerns I raised on earlier revisions (clock-derivation δ skew vs zero-tolerance assertions, expect(stderr).toBe(""), and fake-timer poisoning of the lazy m_timeOrigin init) have been addressed in 6c528ca and 99ab564. The 1 ms epsilon in the test is generous relative to the expected sub-µs gap between two adjacent clock reads but could in principle still be exceeded under extreme preemption on a debug+ASAN runner — worth a human glance but not something I'd block on. This is a user-visible Web API behavior change with WPT implications, which per the approval guidelines is not the kind of change to auto-approve.
|
CI status on 99ab564 (build #76789):
Happy to rebase onto e550f2c if you'd rather the size check run apples-to-apples. |
1 participant
Event.prototype.timeStampreturned0for every event (new Event,CustomEvent,MessageEvent,ErrorEvent, dispatched listener events,AbortSignalabort events), regardless of how long the process had been running. Node and browsers return aDOMHighResTimeStampon theperformance.now()scale; WPTdom/events/Event-constructors.any.jsassertsev.timeStamp > 0.Cause
Event::timeStampForBindingsinsrc/jsc/bindings/webcore/Event.cppwas a literalreturn 0.0; // TODO:. The WebCore path it is meant to use (m_createTime - Performance::m_timeOrigin) was also unusable becauseGlobalObject::performance()stored wall-clock epoch milliseconds inm_timeOriginviaMonotonicTime::fromRawSeconds, purely soPerformance::timeOrigin()could hand them back forperformance.toJSON(). Subtracting that from a realMonotonicTimeproduces garbage.Fix
GlobalObject::performance()now derives a realMonotonicTimeorigin:MonotonicTime::now() - Seconds(Bun__readOriginTimer(bunVM) / 1e9), i.e. the WTF clock value at the instantorigin_timerwas captured.Performance::timeOrigin()readsBun__readOriginTimerStartdirectly for the wall-clock value (matching theperformance.timeOriginown-property path inJSPerformance::finishCreation).Event::timeStampForBindingsreturns(m_createTime - performance->monotonicTimeOrigin()).milliseconds(), clamped to 0. Both operands are fixed once set, so repeated reads return the same value.Verification
New test in
test/js/web/web-globals.test.jsspawns a subprocess, capturesperformance.now()before and after constructing six event variants, and asserts eachtimeStampfalls within[before, after], plus a stability check that two reads of the same event'stimeStampare identical. Fails onmain(Received: 0), passes with this change.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/web-globals.test.js