Skip to content

fix(perf_hooks): stop observer prototype dispatch recursion - #9295

Merged
proggeramlug merged 1 commit into
PerryTS:mainfrom
proggeramlug:fix/9281-perf-hooks
Aug 31, 2026
Merged

fix(perf_hooks): stop observer prototype dispatch recursion#9295
proggeramlug merged 1 commit into
PerryTS:mainfrom
proggeramlug:fix/9281-perf-hooks

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Summary

  • record runtime-created perf_hooks instance and inheritance links as class-default prototype wiring
  • keep those objects out of the per-instance Object.setPrototypeOf override path
  • document the user-visible crash fix without changing any Cargo or npm version

Cause

#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 set OBJECT_META_FLAG_PROTO_OVERRIDE on ordinary PerformanceObserver instances.

Resolving observer.observe(...) then returned an already-bound prototype method and fed it back through js_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.setPrototypeOf behavior 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 build
  • cargo test --release -p perry --test issue_9131_prototype_method_replacement — 2 passed
  • cargo test --release -p perry-runtime perf_hooks:: — 2 passed
  • cargo fmt --all -- --check

Closes #9281

Summary by CodeRabbit

  • Bug Fixes

    • Fixed a stack overflow that could occur when observing performance entries with PerformanceObserver.
    • Restored normal dispatch behavior for performance-related entries, including marks, measures, resource timings, and observer entry lists.
  • Documentation

    • Added a changelog entry describing the performance observer dispatch fix.

@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 418f90a9-994f-46a4-8c3a-a1647d82b94b

📥 Commits

Reviewing files that changed from the base of the PR and between 7a6d5a1 and 68cf94e.

📒 Files selected for processing (2)
  • changelog.d/9281-perf-hooks-prototype-dispatch.md
  • crates/perry-runtime/src/perf_hooks/prototypes.rs

Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The 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 PerformanceObserver.observe().

Changes

perf_hooks prototype dispatch

Layer / File(s) Summary
Class-default prototype wiring
crates/perry-runtime/src/perf_hooks/prototypes.rs
Adds a helper for class-default prototype links and uses it for runtime-created perf objects, PerformanceMark, PerformanceMeasure, and PerformanceResourceTiming.
Dispatch fix record
changelog.d/9281-perf-hooks-prototype-dispatch.md
Documents the prevention of recursive PerformanceObserver.observe() dispatch and the affected built-in classes.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 68cf9

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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the primary change: stopping recursive prototype dispatch in perf_hooks observers.
Description check ✅ Passed 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 pre…
Linked Issues check ✅ Passed The PR addresses the objectives in [#9281]. It fixes the perf_hooks prototype-wiring cause, preserves user Object.setPrototypeOf behavior, and reports parity testing for test_parity_perf_hooks with no…
Out of Scope Changes check ✅ Passed The changes are limited to perf_hooks prototype wiring and a changelog entry documenting the crash fix. No unrelated code or version changes are present.
Full details: Description check

Explanation

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 check

Explanation

The PR addresses the objectives in [#9281]. It fixes the perf_hooks prototype-wiring cause, preserves user Object.setPrototypeOf behavior, and reports parity testing for test_parity_perf_hooks with no Linux crashes.

Full details: Docstring Coverage

Explanation

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.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Audited the diff — correct fix, and I'd merge it.

Swapping object_set_static_prototype for object_link_class_default_prototype is right on the merits: the perf_hooks class hierarchy is class-default wiring, not a user Object.setPrototypeOf. The loud variant additionally sets OBJECT_META_FLAG_PROTO_OVERRIDE, bumps the prop-plan epoch and retires element-shape proofs — none of which this wiring wants.

Naming it link_perf_class_default_prototype with the reasoning inline is better than a bare call swap; the next person to touch this file will see why the loud setter is wrong here rather than 'fixing' it back.

This is the second instance of the same underlying defect

Worth 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 setPrototypeOf calls — and perf_hooks/prototypes.rs is one of the sites named in that issue.

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 main, this one cost a release-blocking parity failure. The remaining sites (intl, cluster×4, node_vm×2, wasi, node_inspector×3, web_storage, disposable, dyn_eval, intl/locale) are still mis-signalling and will surface the same way.

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 — test_parity_perf_hooks was reported as a linux-only new failure.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Correction to my comment above: one symbol name was eaten by shell interpretation. The sentence should read:

OBJECT_META_FLAG_PROTO_OVERRIDE 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 setPrototypeOf calls — and perf_hooks/prototypes.rs is one of the sites named in #9251.

Nothing else in that comment changes.

@proggeramlug
proggeramlug merged commit 0515179 into PerryTS:main Aug 31, 2026
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

release blocker: test_parity_perf_hooks regressed on linux (parity shard 4)

1 participant