fix(node:perf_hooks): report nodeTiming milestones as offsets from timeOrigin - #32481
fix(node:perf_hooks): report nodeTiming milestones as offsets from timeOrigin#324810xfandom wants to merge 1 commit into
Conversation
Walkthrough
ChangesPerformanceNodeTiming prototype refactor and offset fix
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/node/perf_hooks.ts`:
- Around line 64-78: The PerformanceNodeTiming.prototype.toJSON assignment
creates an enumerable property which differs from Node.js behavior where toJSON
is non-enumerable. Replace the direct assignment of
PerformanceNodeTiming.prototype.toJSON with Object.defineProperty, passing the
prototype object, the property name "toJSON", and a property descriptor object
that defines the value as the toJSON function while setting enumerable to false
to match Node.js implementation.
- Around line 55-62: The properties `name`, `entryType`, `startTime`, and
`duration` are currently defined only on the PerformanceNodeTiming.prototype,
but they must also be defined as own properties on the instance to match Node.js
v22+ behavior and ensure Object.getOwnPropertyNames returns them. In the
createPerformanceNodeTiming function, after creating the instance, apply
Object.defineProperties to the instance itself using the same property
descriptors (with the same getter functions for name, entryType, startTime, and
duration) that are defined on the prototype.
In `@test/js/node/perf_hooks/perf_hooks.test.ts`:
- Around line 25-30: The milestone offset validation loop iterates over
nodeStart, v8Start, environment, bootstrapComplete, and idleTime but is missing
loopStart, which was also converted from epoch timestamps to offsets. Add
loopStart to the array of keys being checked in the for loop that validates each
milestone is less than 1e12, ensuring that loopStart is properly validated
alongside the other milestone offsets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 25088c16-7ff1-4e8f-9c5c-ee16f79530a6
📒 Files selected for processing (2)
src/js/node/perf_hooks.tstest/js/node/perf_hooks/perf_hooks.test.ts
|
Addressed the review in ea92c93:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/node/perf_hooks.ts`:
- Around line 58-63: The performanceNodeTimingEntryDescriptors object is
vulnerable to prototype-pollution attacks because it inherits from
Object.prototype. Add __proto__: null as the first property in the
performanceNodeTimingEntryDescriptors descriptor object to create a
null-prototype object that prevents prototype chain pollution. Since
performanceNodeTimingEntryDescriptors is reused in the
createPerformanceNodeTiming() function, this single change will secure both
locations.
In `@test/js/node/perf_hooks/perf_hooks.test.ts`:
- Around line 25-29: The current assertion uses Object.getOwnPropertyNames()
which returns all own properties regardless of enumerability, but the comment
states these should be "own enumerable properties". Replace or augment the check
to use Object.keys() instead, which only returns enumerable own properties. This
will properly verify that the "name", "entryType", "startTime", and "duration"
fields are actually enumerable, aligning the test with its documented intent and
providing the same rigor as the separate Object.getOwnPropertyDescriptor() check
for toJSON non-enumerability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 143415b2-88c2-4de1-97a1-867e61b14d32
📒 Files selected for processing (2)
src/js/node/perf_hooks.tstest/js/node/perf_hooks/perf_hooks.test.ts
…meOrigin performance.nodeTiming reported absolute epoch timestamps: the startup milestones were set to performance.timeOrigin, and startTime returned nodeStart. Node measures all of them in milliseconds from timeOrigin, and a "node" entry is the timeOrigin reference, so its startTime is 0. Also define the name/entryType/startTime/duration accessors as own enumerable properties of the nodeTiming object, matching Node, using null-prototype descriptors so prototype pollution cannot inject extra descriptor keys. Fixes oven-sh#23041
91843ed to
95a2d0c
Compare
|
Rebased onto current main and force-pushed ( Main has since switched The earlier CodeRabbit points (own enumerable properties, null-prototype descriptors) are carried over into the smaller diff. |
|
Still relevant after #34518: that change fixed the throwing getters and closed #23041, but One small difference from node you may want to pick up while you are here: node defines For context, #35390 wires up real milestone timestamps; until that lands this PR is the right fix for the offsets. |
What does this PR do?
Fixes #23041.
performance.nodeTimingreports absolute epoch timestamps instead of offsets fromperformance.timeOrigin:Node measures every startup milestone in milliseconds from
timeOrigin, and a"node"entry is the timeOrigin reference, so itsstartTimeis0.createPerformanceNodeTiming()setbootstrapComplete/environment/nodeStart/v8Starttoperformance.timeOrigin; they are now offsets (0), withloopExitstaying-1. Bun does not record the individual startup milestones, so they are reported relative to process start.startTimereturnedthis.nodeStart(an epoch value); it now returns0.name/entryType/startTime/durationare now own enumerable properties of thenodeTimingobject, not only prototype accessors, soObject.keys(performance.nodeTiming)matches Node. They use null-prototype descriptors so prototype pollution before the module loads cannot inject extra descriptor keys (value/writable/get/set).The
startTime/durationgetters throwingThe PerformanceEntry.startTime getter can only be used on instances of PerformanceEntry— the other half of #23041 — is already fixed on main by the switch toObject.setPrototypeOf, which preserves the class-body accessors. This PR was rebased onto that and reduced to the remaining gap.How did you verify your code works?
Added a
nodeTimingtest intest/js/node/perf_hooks/perf_hooks.test.tsassertingstartTime === 0,name/entryType, a numericduration, that the entry accessors are own enumerable properties, and that every milestone is an offset (< 1e12) whiletimeOriginis epoch-scale — plus the exacttoJSON()shape. Values were checked againstnode -eon v22 and the documented Node behaviour (startTime: 0, small millisecond milestones).