fix(perf_hooks): stop observer prototype dispatch recursion - #9295
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change updates perf_hooks prototype wiring to use class-default links. This avoids marking built-in prototype relationships as user overrides, preventing recursive native dispatch during Changesperf_hooks prototype dispatch
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change separates built-in perf_hooks setup from user-controlled prototype replacement, removing the recursive dispatch crash while preserving existing prototype behavior. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the cause, implementation, scope, linked issue, and test results. It omits the template headings for Changes, Related issue, and Checklist, but the required information is present and the description is mostly complete. Full details: Linked Issues checkExplanation The PR addresses the objectives in [ Full details: Docstring CoverageExplanation Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 1 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Audited the diff — correct fix, and I'd merge it. Swapping Naming it This is the second instance of the same underlying defectWorth connecting explicitly: this is #9251, which I filed after #9169. is documented and consumed as "the user replaced this instance's prototype", but it is set by ~20 runtime prototype-wiring sites that are not user The pattern is now established:
Both are correct locally. But that is two of ~20 sites fixed reactively, each discovered by a different expensive failure — #9169's cost four gap-test regressions on Not a blocker for this PR — fixing the flag's semantics is #9251's job, and a release blocker should not wait on it. But it is worth someone owning #9251 rather than continuing to pay per site. Verification note#9281 is a parity regression that passed on the previous candidate, so please confirm against the candidate SHA and CI rather than a local run alone — |
|
Correction to my comment above: one symbol name was eaten by shell interpretation. The sentence should read:
Nothing else in that comment changes. |
Summary
perf_hooksinstance and inheritance links as class-default prototype wiringObject.setPrototypeOfoverride pathCause
#9169 correctly made native method calls honor a receiver whose prototype was replaced by the user.
perf_hooks, however, used the same loud prototype setter for three pieces of built-in class wiring. That incorrectly setOBJECT_META_FLAG_PROTO_OVERRIDEon ordinaryPerformanceObserverinstances.Resolving
observer.observe(...)then returned an already-bound prototype method and fed it back throughjs_native_call_method. The bound value-call path re-entered the same override lookup indefinitely; the Linux failure was stack exhaustion after roughly 28,000 repeated dispatch frames, ending in SIGSEGV.The quiet class-default setter records the same prototype relationships without claiming that the user replaced the chain. Genuine user
Object.setPrototypeOfbehavior remains on the loud path.Testing
./run_parity_tests.sh --filter test_parity_perf_hooks— PASS, 1/1, 100% parity, 0 crashes on Linux using one coherent release buildcargo test --release -p perry --test issue_9131_prototype_method_replacement— 2 passedcargo test --release -p perry-runtime perf_hooks::— 2 passedcargo fmt --all -- --checkCloses #9281
Summary by CodeRabbit
Bug Fixes
PerformanceObserver.Documentation