fix(engine): audit object-reference threadvars and rebuild prototype members per realm#904
Merged
Conversation
Follow-up to #885/#891 (managed threadvars), auditing object-reference threadvars per #892. FPC never auto-finalizes object-reference threadvars. - Remove the dead, write-only GSymbolRegistry threadvar (every symbol added itself, nothing read it, so it grew unboundedly per thread). - Route the leaking object-reference container threadvars (ObjectModel.GPublishedGetterHosts, Compiler.Statements GUsingResources / GLabelControls) through Goccia.ThreadCleanupRegistry; document the already-safe ones (refcounted symbol registry, non-owning current pointers). - Migrate 7 prototype method-host threadvars (Object/Array/String/Number/ Boolean/Symbol/Iterator) to per-realm slots so the realm pins the host on SetSlot and unpins it on Destroy; rebuild member definitions per realm. - Drop BigInt's redundant write-only host threadvar (its host is the 0n process singleton, already pinned process-wide, deliberately not migrated). - Extend Goccia.ThreadCleanupLeak.Test with an idempotency case; add ADR 0083. Closes #892 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAcross ~30 built-in value units, per-thread ChangesRealm-owned prototype method hosts and threadvar audit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Suite TimingTest Runner (interpreted: 11,046 passed; bytecode: 11,046 passed)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Test runner worker shutdown frees thread-local heaps in bulk; that shutdown reclamation is not counted as GC collections or collected objects.
Benchmarks (interpreted: 437; bytecode: 437)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Benchmark runner performs explicit between-file collections, so collection and collected-object counts can be much higher than the test runner.
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results437 benchmarks · PR vs same-runner Interpreted: 🟢 27 improved · 🔴 38 regressed · 372 unchanged · avg -0.2% Typical per-run noise (median variance): interpreted ±3.6%, bytecode ±2.1%. Deltas within noise overlap and read as unchanged. arraybuffer.js — Interp: 🔴 1, 13 unch. · avg +0.3% · Bytecode: 🔴 1, 13 unch. · avg +0.5%
arrays.js — Interp: 🔴 2, 17 unch. · avg -2.4% · Bytecode: 🔴 1, 18 unch. · avg -0.0%
async-await.js — Interp: 🟢 1, 5 unch. · avg +3.2% · Bytecode: 6 unch. · avg +1.1%
async-generators.js — Interp: 2 unch. · avg -10.6% · Bytecode: 2 unch. · avg +4.2%
atomics.js — Interp: 6 unch. · avg +5.7% · Bytecode: 🟢 1, 5 unch. · avg -1.2%
base64.js — Interp: 🟢 3, 7 unch. · avg +0.8% · Bytecode: 10 unch. · avg +0.9%
classes.js — Interp: 🟢 1, 30 unch. · avg -0.9% · Bytecode: 🟢 1, 🔴 6, 24 unch. · avg -0.8%
closures.js — Interp: 🔴 2, 9 unch. · avg -3.5% · Bytecode: 11 unch. · avg +0.9%
collections.js — Interp: 12 unch. · avg +2.8% · Bytecode: 🔴 2, 10 unch. · avg -2.6%
csv.js — Interp: 13 unch. · avg +1.4% · Bytecode: 🔴 2, 11 unch. · avg -2.9%
destructuring.js — Interp: 🟢 3, 🔴 1, 18 unch. · avg +1.8% · Bytecode: 🟢 1, 🔴 3, 18 unch. · avg +0.5%
fibonacci.js — Interp: 8 unch. · avg -2.1% · Bytecode: 🔴 1, 7 unch. · avg -3.2%
float16array.js — Interp: 🟢 2, 30 unch. · avg -0.6% · Bytecode: 🟢 5, 🔴 2, 25 unch. · avg +2.7%
for-in/for-in.js — Interp: 🔴 1, 2 unch. · avg -3.4% · Bytecode: 3 unch. · avg +0.7%
for-of.js — Interp: 🔴 1, 6 unch. · avg -3.1% · Bytecode: 🔴 1, 6 unch. · avg -2.7%
generators.js — Interp: 4 unch. · avg +1.0% · Bytecode: 🔴 2, 2 unch. · avg -3.1%
intl.js — Interp: 🔴 1, 5 unch. · avg -2.0% · Bytecode: 6 unch. · avg -1.5%
iterators.js — Interp: 🔴 9, 33 unch. · avg -4.7% · Bytecode: 🟢 9, 🔴 3, 30 unch. · avg +0.3%
json.js — Interp: 🟢 3, 20 unch. · avg +3.1% · Bytecode: 🟢 4, 19 unch. · avg +2.9%
jsx.jsx — Interp: 🟢 1, 🔴 1, 19 unch. · avg -1.8% · Bytecode: 🟢 2, 🔴 5, 14 unch. · avg -3.6%
modules.js — Interp: 9 unch. · avg -3.3% · Bytecode: 9 unch. · avg +0.1%
numbers.js — Interp: 12 unch. · avg -1.3% · Bytecode: 🔴 1, 11 unch. · avg -4.8%
objects.js — Interp: 7 unch. · avg +4.0% · Bytecode: 7 unch. · avg -1.1%
promises.js — Interp: 🔴 1, 11 unch. · avg -4.7% · Bytecode: 🟢 1, 🔴 2, 9 unch. · avg +1.4%
property-access.js — Interp: 5 unch. · avg -4.2% · Bytecode: 5 unch. · avg -1.8%
regexp.js — Interp: 🟢 2, 🔴 1, 8 unch. · avg +1.4% · Bytecode: 🔴 3, 8 unch. · avg -3.4%
strings.js — Interp: 19 unch. · avg +0.1% · Bytecode: 🟢 1, 🔴 1, 17 unch. · avg -0.8%
temporal.js — Interp: 🟢 1, 5 unch. · avg +1.8% · Bytecode: 🔴 1, 5 unch. · avg -2.9%
tsv.js — Interp: 🔴 2, 7 unch. · avg +0.6% · Bytecode: 🔴 1, 8 unch. · avg -2.3%
typed-arrays.js — Interp: 🟢 5, 🔴 4, 13 unch. · avg -2.9% · Bytecode: 🟢 5, 🔴 4, 13 unch. · avg +14.9%
uint8array-encoding.js — Interp: 🟢 2, 🔴 7, 9 unch. · avg +10.5% · Bytecode: 🔴 10, 8 unch. · avg -16.0%
weak-collections.js — Interp: 🟢 3, 🔴 4, 8 unch. · avg +5.5% · Bytecode: 🔴 8, 7 unch. · avg -14.5%
Deterministic profile diffDeterministic profile diff: no significant changes. Measured on ubuntu-latest x64. Each PR run also builds the |
Contributor
test262 Conformance
Areas closest to 100%
Per-test deltas (+1 / -0)Newly passing (1):
Steady-state failures are non-blocking; regressions vs the cached main baseline (lower total pass count, or any PASS → non-PASS transition) fail the conformance gate. Measured on ubuntu-latest x64, bytecode mode. Areas grouped by the first two test262 path components; minimum 25 attempted tests, areas already at 100% excluded. Δ vs main compares against the most recent cached |
…safety Follow-up to #892 / ADR 0083. The ~30 value units that build their prototype via TGocciaSharedPrototype.Create(Self) keep a cross-realm FPrototypeMembers cache bound to a realm-owned (freeable) host — the combination ADR 0083 calls unsafe. Investigated whether it is an exploitable use-after-free. It is not: every bound prototype callback re-derives its receiver from the JS `this` (AThisValue) and never dereferences the bound host, and none are virtual, so a later realm reusing the cache passes the earlier realm's freed host as Self but never reads it. A sweep of all 30 units confirmed this (the only two deviations, ZonedDateTimeSubtract / YearMonthSubtract, call a non-virtual sibling that also re-binds AThisValue, so the freed host is still never read). - Add Goccia.SharedPrototypeRealmReuse.Test: runs many realms on one thread with a forced GC collect + heap stomp between them and exercises the shared-prototype types in later realms; a callback that began dereferencing the freed host would crash or corrupt there. - Document the invariant at the binding site (Goccia.SharedPrototype) and record the investigation + the discrepancy with ADR 0083's rationale in ADR 0084. No production behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…otype units Deep fix for the cross-realm member-cache hazard investigated as a #892 follow-up (ADR 0083). The ~30 value units that build their prototype via TGocciaSharedPrototype.Create(Self) had a realm-owned (freeable) host but cached their member definitions in a cross-realm FPrototypeMembers threadvar, so a later realm reused callbacks bound to an earlier realm's freed host. Not observably exploitable (callbacks take their receiver from the JS `this`, never the host), but it relied on that implicit invariant. Apply #892's per-realm rebuild to all ~30 units: drop the cross-realm FPrototypeMembers cache, rebuild member definitions per realm bound to the current realm's host, and drop their #891 ClearThreadvarMembers. This removes the hazard class outright and incidentally makes the only two host-referencing callbacks (ZonedDateTimeSubtract / YearMonthSubtract) safe. - Fold the follow-up investigation + this migration into ADR 0083; delete the separate ADR 0084 (one ADR per workstream; ADRs lock at merge, not commit). - Update the TGocciaSharedPrototype invariant comment and the regression-test header to reflect per-realm rebuild. Full JS suite 11046/11046 both modes; SharedPrototypeRealmReuse / ThreadCleanupLeak / Threading Pascal tests green; format clean. No production behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add the engine-construction A/B result for the thirty-unit per-realm member rebuild: no measurable wall-clock change and a deterministic +0.2-0.3% bytes allocated per engine (allocator-mitigated), with no per-engine retention. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…teLength Code-review follow-up on the per-realm prototype rebuild (PR #904). With the cross-realm member cache removed, SharedArrayBuffer's InitializePrototype now runs per realm; its byteLength getter used AddPublishedGetter, which appends to the thread-global, never-pruned GPublishedGetterHosts list — so it leaked one getter host per realm. Switch byteLength to AddAccessor(SharedArrayBufferByteLengthGetter) (the callback already existed, unused), matching ArrayBuffer and its sibling getters: no GPublishedGetterHosts growth, behavior unchanged. Also from the review: - Fix stale comments that still described the removed cross-realm cache (Goccia.Values.MapValue header; the SharedPrototypeRealmReuse test body). - Correct ADR 0083: the two Temporal *.Subtract callbacks are made safe (their Self is now the current realm's live host), not removed; and record GlobalRegExp as a tracked, out-of-scope follow-up with the same pattern. JS suite 11046/11046 both modes; SharedArrayBuffer/ArrayBuffer 218/218; format clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
frostney
changed the title
…UAF) Goccia.Builtins.GlobalRegExp cached its prototype and static member definitions in cross-realm threadvars (FPrototypeMembers/FStaticMembers, build-once guard), so every later realm's RegExp prototype bound to the FIRST realm's TGocciaGlobalRegExp host. That host is realm-owned — TGocciaEngine frees it on Destroy — so a second realm on the same thread dispatched cached callbacks against a freed host. Unlike the ~30 TGocciaSharedPrototype units migrated in this PR (whose callbacks never dereference the host), two RegExp prototype callbacks do: RegExp.prototype[Symbol.matchAll] and [Symbol.split] read the host's FRegExpConstructor and dereference it on the SpeciesConstructor fallback path (receiver constructor = undefined). That makes GlobalRegExp the one genuinely exploitable instance of the pattern — a multi-realm repro crashes with an access violation. Drop both caches and rebuild the member definitions per realm bound to the current host (same shape as the TGocciaSharedPrototype migration), and drop the unit's ThreadCleanupRegistry wiring. Goccia.SharedPrototypeRealmReuse.Test gains RegExp cases, an FPC-heap stomp (the host is freed to the FPC heap, not the GC heap), and the species-fallback exploit: it faults before this change and passes after. ADR 0083 records the finding. Pre-existing; surfaced by the PR #904 code review. JS suite 11046/11046 both modes; SharedPrototypeRealmReuse/ThreadCleanupLeak/Threading green; format clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code-review follow-ups on the GlobalRegExp per-realm rebuild test: - Add a RegExp.prototype[Symbol.split] species-fallback case so BOTH host-dereferencing callbacks (matchAll and split) are gated; the comments claimed both but only matchAll was actually exercised on the fallback path. - Document the stomp magnitudes (cycles / per-cycle FPC allocations / GC churn) as empirical generous floors, not tuned thresholds. - Correct "caught below" -> caught by the shared test runner (the catch is in the test runner, not this file). Test compiles and passes (3/3 deterministic); format clean. No production code change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
coderabbitai
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
source/units/Goccia.SharedPrototypeRealmReuse.Test.pas (1)
62-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct SharedArrayBuffer byteLength coverage.
This regression gate covers
ArrayBuffer.byteLength, but the PR also fixes aSharedArrayBuffer.prototype.byteLengthgetter-host leak. Add a smallSharedArrayBufferassertion here so that exact fixed path is locked down.Suggested test addition
'const ab = new ArrayBuffer(8); if (ab.byteLength !== 8) throw new Error("ArrayBuffer");' + sLineBreak + 'const ab2 = ab.slice(0, 4); if (ab2.byteLength !== 4) throw new Error("ArrayBuffer.slice");' + sLineBreak + + 'const sab = new SharedArrayBuffer(8); if (sab.byteLength !== 8) throw new Error("SharedArrayBuffer");' + sLineBreak +🤖 Prompt for 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. In `@source/units/Goccia.SharedPrototypeRealmReuse.Test.pas` around lines 62 - 63, Add a direct SharedArrayBuffer byteLength assertion in the existing test string so the fix is covered on the exact getter path. Update the JavaScript snippet built in the test around the ArrayBuffer checks to also create a SharedArrayBuffer and verify its byteLength is correct, using the same style as the current ArrayBuffer assertions in Goccia.SharedPrototypeRealmReuse.Test. This should live alongside the existing byteLength coverage so the SharedArrayBuffer.prototype.byteLength host-leak regression is locked down.source/units/Goccia.Values.TemporalZonedDateTime.pas (1)
2039-2066: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared Temporal property-name constants in the rebuilt member list.
These changed lines reintroduce hardcoded Temporal property names that already have constants (
PROP_CALENDAR_ID,PROP_YEAR,PROP_MONTH,PROP_MONTH_CODE,PROP_DAY, and time-unit constants). Prefer the shared constants here.♻️ Proposed direction
- Members.AddAccessor('calendarId', GetCalendarId, nil, [pfConfigurable]); + Members.AddAccessor(PROP_CALENDAR_ID, GetCalendarId, nil, [pfConfigurable]); ... - Members.AddAccessor('year', GetYear, nil, [pfConfigurable]); - Members.AddAccessor('month', GetMonth, nil, [pfConfigurable]); - Members.AddAccessor('monthCode', GetMonthCode, nil, [pfConfigurable]); - Members.AddAccessor('day', GetDay, nil, [pfConfigurable]); + Members.AddAccessor(PROP_YEAR, GetYear, nil, [pfConfigurable]); + Members.AddAccessor(PROP_MONTH, GetMonth, nil, [pfConfigurable]); + Members.AddAccessor(PROP_MONTH_CODE, GetMonthCode, nil, [pfConfigurable]); + Members.AddAccessor(PROP_DAY, GetDay, nil, [pfConfigurable]); ... - Members.AddAccessor('hour', GetHour, nil, [pfConfigurable]); + Members.AddAccessor(PROP_HOUR, GetHour, nil, [pfConfigurable]);Based on learnings, Temporal-related units should use the shared Temporal property-name constants from
Goccia.Constants.PropertyNamesinstead of hardcoded Temporal property names.🤖 Prompt for 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. In `@source/units/Goccia.Values.TemporalZonedDateTime.pas` around lines 2039 - 2066, The rebuilt member list in the TemporalZonedDateTime setup is using hardcoded Temporal property names again; switch these AddAccessor calls to the shared constants from Goccia.Constants.PropertyNames. Update the relevant member registrations in the TemporalZonedDateTime member-building routine so symbols like GetCalendarId, GetYear, GetMonth, GetMonthCode, GetDay, and the time-unit accessors all reference the existing PROP_* constants instead of string literals.Source: Learnings
source/units/Goccia.Values.TemporalPlainYearMonth.pas (1)
290-295: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Temporal property-name constants here.
calendarId,year,month, andmonthCodeshould use the shared constants to match the rest of the Temporal units and avoid drift.Proposed cleanup
- Members.AddAccessor('calendarId', GetCalendarId, nil, [pfConfigurable]); + Members.AddAccessor(PROP_CALENDAR_ID, GetCalendarId, nil, [pfConfigurable]); Members.AddAccessor('era', GetEra, nil, [pfConfigurable]); Members.AddAccessor('eraYear', GetEraYear, nil, [pfConfigurable]); - Members.AddAccessor('year', GetYear, nil, [pfConfigurable]); - Members.AddAccessor('month', GetMonth, nil, [pfConfigurable]); - Members.AddAccessor('monthCode', GetMonthCode, nil, [pfConfigurable]); + Members.AddAccessor(PROP_YEAR, GetYear, nil, [pfConfigurable]); + Members.AddAccessor(PROP_MONTH, GetMonth, nil, [pfConfigurable]); + Members.AddAccessor(PROP_MONTH_CODE, GetMonthCode, nil, [pfConfigurable]);Based on learnings, Temporal-related units should use shared property-name constants instead of hardcoded Temporal property names.
🤖 Prompt for 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. In `@source/units/Goccia.Values.TemporalPlainYearMonth.pas` around lines 290 - 295, The accessor registrations in the TemporalPlainYearMonth setup are using hardcoded property names, which should be replaced with the shared Temporal property-name constants to stay consistent with the other Temporal units. Update the Members.AddAccessor calls for calendarId, year, month, and monthCode in the TemporalPlainYearMonth registration code to use the existing constants instead of string literals, matching the pattern used elsewhere in the Temporal value classes.Source: Learnings
source/units/Goccia.Values.TemporalPlainTime.pas (1)
271-276: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared time-unit property constants here.
The rebuilt prototype member list still hardcodes
'hour','minute','second','millisecond','microsecond', and'nanosecond'. UsePROP_HOUR,PROP_MINUTE, etc., to keep Temporal property names centralized. Based on learnings, Temporal units should use shared property-name constants fromGoccia.Constants.PropertyNamesinstead of hardcoded Temporal property strings. <retrieved_learnings>♻️ Suggested direction
- Members.AddAccessor('hour', GetHour, nil, [pfConfigurable]); - Members.AddAccessor('minute', GetMinute, nil, [pfConfigurable]); + Members.AddAccessor(PROP_HOUR, GetHour, nil, [pfConfigurable]); + Members.AddAccessor(PROP_MINUTE, GetMinute, nil, [pfConfigurable]);🤖 Prompt for 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. In `@source/units/Goccia.Values.TemporalPlainTime.pas` around lines 271 - 276, The rebuilt prototype member list in TemporalPlainTime still hardcodes the time-unit accessor names, so update the member registration calls in the AddAccessor block to use the shared constants from Goccia.Constants.PropertyNames instead of literal strings. Replace the inline property names with PROP_HOUR, PROP_MINUTE, PROP_SECOND, PROP_MILLISECOND, PROP_MICROSECOND, and PROP_NANOSECOND so the Temporal property names stay centralized and consistent.Source: Learnings
source/units/Goccia.Values.TemporalPlainDateTime.pas (1)
612-633: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Temporal property-name constants in the rebuilt member list.
The changed registration block still hardcodes Temporal names like
'calendarId','year','month', and the time-unit properties. Prefer the sharedPROP_*constants where available so the per-realm rebuild stays aligned with the rest of Temporal. Based on learnings, Temporal units should use shared property-name constants fromGoccia.Constants.PropertyNamesinstead of hardcoded Temporal property strings. <retrieved_learnings>♻️ Example replacement pattern
- Members.AddAccessor('calendarId', GetCalendarId, nil, [pfConfigurable]); + Members.AddAccessor(PROP_CALENDAR_ID, GetCalendarId, nil, [pfConfigurable]); ... - Members.AddAccessor('hour', GetHour, nil, [pfConfigurable]); + Members.AddAccessor(PROP_HOUR, GetHour, nil, [pfConfigurable]);🤖 Prompt for 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. In `@source/units/Goccia.Values.TemporalPlainDateTime.pas` around lines 612 - 633, The rebuilt member registration in TemporalPlainDateTime still hardcodes Temporal property names instead of using the shared property-name constants. Update the accessor setup in the member list to reference the matching PROP_* symbols from Goccia.Constants.PropertyNames wherever they exist, keeping the registrations in sync with the rest of Temporal and avoiding duplicated string literals in the TemporalPlainDateTime rebuild path.Source: Learnings
🤖 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 `@source/units/Goccia.ThreadCleanupLeak.Test.pas`:
- Around line 103-108: The idempotency assertion in the thread cleanup test is
too strict because GetHeapStatus.TotalAllocated can fluctuate between collector
and heap-manager calls. Update the check around RunThreadvarCleanups and
TGarbageCollector.Instance.Collect in Goccia.ThreadCleanupLeak.Test to allow a
small bounded delta instead of relying on exact non-increase, while still
proving the second drain does not crash and does not cause meaningful growth.
---
Nitpick comments:
In `@source/units/Goccia.SharedPrototypeRealmReuse.Test.pas`:
- Around line 62-63: Add a direct SharedArrayBuffer byteLength assertion in the
existing test string so the fix is covered on the exact getter path. Update the
JavaScript snippet built in the test around the ArrayBuffer checks to also
create a SharedArrayBuffer and verify its byteLength is correct, using the same
style as the current ArrayBuffer assertions in
Goccia.SharedPrototypeRealmReuse.Test. This should live alongside the existing
byteLength coverage so the SharedArrayBuffer.prototype.byteLength host-leak
regression is locked down.
In `@source/units/Goccia.Values.TemporalPlainDateTime.pas`:
- Around line 612-633: The rebuilt member registration in TemporalPlainDateTime
still hardcodes Temporal property names instead of using the shared
property-name constants. Update the accessor setup in the member list to
reference the matching PROP_* symbols from Goccia.Constants.PropertyNames
wherever they exist, keeping the registrations in sync with the rest of Temporal
and avoiding duplicated string literals in the TemporalPlainDateTime rebuild
path.
In `@source/units/Goccia.Values.TemporalPlainTime.pas`:
- Around line 271-276: The rebuilt prototype member list in TemporalPlainTime
still hardcodes the time-unit accessor names, so update the member registration
calls in the AddAccessor block to use the shared constants from
Goccia.Constants.PropertyNames instead of literal strings. Replace the inline
property names with PROP_HOUR, PROP_MINUTE, PROP_SECOND, PROP_MILLISECOND,
PROP_MICROSECOND, and PROP_NANOSECOND so the Temporal property names stay
centralized and consistent.
In `@source/units/Goccia.Values.TemporalPlainYearMonth.pas`:
- Around line 290-295: The accessor registrations in the TemporalPlainYearMonth
setup are using hardcoded property names, which should be replaced with the
shared Temporal property-name constants to stay consistent with the other
Temporal units. Update the Members.AddAccessor calls for calendarId, year,
month, and monthCode in the TemporalPlainYearMonth registration code to use the
existing constants instead of string literals, matching the pattern used
elsewhere in the Temporal value classes.
In `@source/units/Goccia.Values.TemporalZonedDateTime.pas`:
- Around line 2039-2066: The rebuilt member list in the TemporalZonedDateTime
setup is using hardcoded Temporal property names again; switch these AddAccessor
calls to the shared constants from Goccia.Constants.PropertyNames. Update the
relevant member registrations in the TemporalZonedDateTime member-building
routine so symbols like GetCalendarId, GetYear, GetMonth, GetMonthCode, GetDay,
and the time-unit accessors all reference the existing PROP_* constants instead
of string literals.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ef6ca06c-f17c-43b9-9a26-7cf83af8b351
📒 Files selected for processing (51)
docs/adr/0083-realm-owned-prototype-method-hosts.mddocs/adr/README.mdsource/units/Goccia.Builtins.GlobalRegExp.passource/units/Goccia.Compiler.Statements.passource/units/Goccia.Generator.Continuation.passource/units/Goccia.ObjectModel.passource/units/Goccia.Realm.passource/units/Goccia.RegExp.Runtime.passource/units/Goccia.SharedPrototype.passource/units/Goccia.SharedPrototypeRealmReuse.Test.passource/units/Goccia.SourcePipeline.passource/units/Goccia.ThreadCleanupLeak.Test.passource/units/Goccia.VM.passource/units/Goccia.Values.ArrayBufferValue.passource/units/Goccia.Values.ArrayValue.passource/units/Goccia.Values.BigIntValue.passource/units/Goccia.Values.BooleanObjectValue.passource/units/Goccia.Values.FFILibrary.passource/units/Goccia.Values.FFIPointer.passource/units/Goccia.Values.FinalizationRegistryValue.passource/units/Goccia.Values.HeadersValue.passource/units/Goccia.Values.IntlCollator.passource/units/Goccia.Values.IntlDateTimeFormat.passource/units/Goccia.Values.IntlDisplayNames.passource/units/Goccia.Values.IntlDurationFormat.passource/units/Goccia.Values.IntlListFormat.passource/units/Goccia.Values.IntlLocale.passource/units/Goccia.Values.IntlNumberFormat.passource/units/Goccia.Values.IntlPluralRules.passource/units/Goccia.Values.IntlRelativeTimeFormat.passource/units/Goccia.Values.IteratorValue.passource/units/Goccia.Values.MapValue.passource/units/Goccia.Values.NumberObjectValue.passource/units/Goccia.Values.ObjectValue.passource/units/Goccia.Values.PromiseValue.passource/units/Goccia.Values.ResponseValue.passource/units/Goccia.Values.SetValue.passource/units/Goccia.Values.SharedArrayBufferValue.passource/units/Goccia.Values.StringObjectValue.passource/units/Goccia.Values.SymbolValue.passource/units/Goccia.Values.TemporalDuration.passource/units/Goccia.Values.TemporalInstant.passource/units/Goccia.Values.TemporalPlainDateTime.passource/units/Goccia.Values.TemporalPlainMonthDay.passource/units/Goccia.Values.TemporalPlainTime.passource/units/Goccia.Values.TemporalPlainYearMonth.passource/units/Goccia.Values.TemporalZonedDateTime.passource/units/Goccia.Values.TextDecoderValue.passource/units/Goccia.Values.WeakMapValue.passource/units/Goccia.Values.WeakRefValue.passource/units/Goccia.Values.WeakSetValue.pas
Address a CodeRabbit review on PR #904: the second-drain idempotency assertion used exact heap-total non-increase (AfterSecondDrain <= AfterFirstDrain), which GetHeapStatus.TotalAllocated can fluctuate on across collector/heap-manager calls -- a potential CI flake unrelated to cleanup idempotency. Allow a bounded 1 KiB delta (MAX_IDEMPOTENT_DRAIN_NOISE_BYTES, local to the test) so the check still catches a double-free re-allocation or a real leak without flaking on bookkeeping noise. Test passes (3/3); format clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
coderabbitai
Bot
added
documentation
main landed #893 (migrate explicit per-thread cache clears into the cleanup registry, ADR 0083) and bumped the test262 / toml-test / tc39-mcp pins. Resolutions: - ADR number collision: renumber this PR's ADR 0083 -> 0084 (realm-owned prototype method hosts) so main's #893 keeps 0083; update docs/adr/README.md and all references, and add a sibling cross-link between the two #891 follow-ups. - Goccia.ThreadCleanupLeak.Test: keep all three tests -- this PR's idempotency (#892, with the bounded-tolerance fix) and reclaim tests, plus main's migrated-cache-cleanup-registration test (#893). Verified after merge: clean testrunner build, JS suite 11046/11046 in both interpreter and bytecode modes, and ThreadCleanupLeak / SharedPrototypeRealmReuse / Threading Pascal tests green; format clean.
coderabbitai
Bot
added
bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #885 / #891 (ADR 0078), which released managed threadvars at worker-thread exit but deliberately scoped out object-reference threadvars. This audits those (#892). FPC never auto-finalizes object-reference threadvars.
Object-reference container threadvars
GSymbolRegistry(Goccia.Values.SymbolValue): everyTGocciaSymbolValueadded itself and nothing ever read the map, so it grew unboundedly per thread for no purpose.Goccia.ObjectModel.GPublishedGetterHosts(aTObjectListthat owns its entries) andGoccia.Compiler.Statements'GUsingResources/GLabelControlsworking-state lists — throughGoccia.ThreadCleanupRegistrywithFreeAndNil-based cleanups, released on both the worker-exit and main-thread-finalization paths (the same mechanism fix(engine): release managed threadvars on worker-thread exit #891 established). The other compiler working-state lists are balanced (nil at rest) and documented as such.GSharedSymbolRegistry(freed when the lastTGocciaGlobalSymbolon the thread dies), the non-owning "current" pointers (CurrentRealm, active generator continuation, active bytecode generator, RegExp prototype cache, source-pipeline scope), and the explicitly-shut-down per-threadInstancesingletons.Manually-pinned prototype method hosts
Object,Array,String,Number,Boolean,Symbol,Iterator— from a process-wide-pinnedthreadvarhost (never unpinned) to a per-realm slot. The realm pins the host onSetSlotand unpins it onDestroy, so it is reclaimed with its realm just like the prototype object; member definitions are rebuilt per realm because they bind to the per-realm host. Their now-dead fix(engine): release managed threadvars on worker-thread exit #891ClearThreadvarMembersregistrations are dropped.BigIntZero— the0nprocess-wide singleton (aclass var), the same fixed-value-singleton category as the primitive singletons and already pinned process-wide. Routing it through a realm slot made the realm unpin0nonDestroy, freeing the shared singleton and breaking every engine created after the first on a thread (surfaced during development; see the ADR).Constraints / non-goals: threadvar lifetime only — no JS-observable behavior or semantics change. The only behavioral delta is that the 7 host-bearing types rebuild their member definitions per realm instead of caching them once per thread (bounded to the units this issue names; the other ~57 cached units are untouched, a possible later migration). Decision recorded in ADR 0084.
Closes #892
Testing
GocciaTestRunner testsand--mode=bytecodeboth 11046/11046 (100%). (No JS-observable behavior; the regression was caught here because workers run many engines per thread.)Goccia.ThreadCleanupLeak.Testextended with an idempotency case proving the newFreeAndNilcontainer cleanups survive a repeated registry drain (2/2);Goccia.Threading.Test(12/12);Goccia.Engine.Realm.Test(18/18);Goccia.GarbageCollector.Test(1/1)../format.pas --checkclean.Follow-up: the other ~30
TGocciaSharedPrototypeunitsThese units (
Map,Set, the weak collections,Promise,ArrayBuffer/SharedArrayBuffer, allIntl*/Temporal*,Headers,Response,TextDecoder, the FFI values,FinalizationRegistry) kept a cross-realmFPrototypeMemberscache bound to a realm-owned (freeable) host — the combination ADR 0084 calls unsafe.I first confirmed it was not observably exploitable: every bound callback re-derives its receiver from the JS
this, none is virtual, so a later realm reusing the cache passes the earlier realm's freed host asSelfbut never dereferences it (verified by a sweep of all 30 units plus a multi-realm repro test). But it relied on that implicit invariant, with two callbacks (ZonedDateTimeSubtract/YearMonthSubtract) one refactor away from a real UAF.So I applied the deep fix: extend #892's per-realm member rebuild to all ~30 units — drop the cross-realm
FPrototypeMemberscache, rebuild member definitions per realm bound to the current realm's host, and drop their #891ClearThreadvarMembers. This removes the hazard class entirely (no reliance on the invariant) and makes the twoSubtractcallbacks safe for free. The investigation + migration are folded into ADR 0084 (renumbered from 0083 on baseline merge — main landed its own ADR 0083), andGoccia.SharedPrototypeRealmReuse.Test(many realms on one thread + forced GC collect + heap stomp) guards against a regression. Both JS modes stay 11046/11046, and the per-realm rebuild's allocation cost is negligible and allocator-mitigated (benchmarked in ADR 0084).Code review
A high-effort review of the branch caught one regression the deep fix introduced:
SharedArrayBuffer.prototype.byteLengthusedAddPublishedGetter, which appends to the never-pruned thread-globalGPublishedGetterHostslist. Harmless while the prototype was built once per thread, but once members are rebuilt per realm it leaks one getter host per realm. Switched it toAddAccessorover the already-present (previously unused)SharedArrayBufferByteLengthGettercallback — matchingArrayBufferandSharedArrayBuffer's own sibling getters;byteLengthbehavior is unchanged (SharedArrayBuffer/ArrayBufferbuilt-ins 218/218). The review also fixed two stale comments that still described the removed cross-realm cache, corrected an ADR claim (the two TemporalSubtractcallbacks are made safe, not removed), and flagged a pre-existing instance of the same cross-realm-cache pattern inGoccia.Builtins.GlobalRegExp(a constructor-style builtin, different mechanism) — audited and fixed below.RegExp builtin — the one genuinely exploitable instance
Auditing the flagged
Goccia.Builtins.GlobalRegExpshowed its host (TGocciaGlobalRegExp) is realm-owned —TGocciaEnginefrees it onDestroy— and it cachedFPrototypeMembers/FStaticMemberscross-realm, so later realms bound to the first realm's freed host. Crucially, unlike the ~30TGocciaSharedPrototypeunits (whose callbacks never touch the host), two RegExp prototype callbacks dereference it:RegExp.prototype[Symbol.matchAll]and[Symbol.split]read the host'sFRegExpConstructorand use it on theSpeciesConstructorfallback (receiverconstructor=undefined). That makes it the one genuinely exploitable instance of the pattern — a multi-realm repro crashes with an access violation.Fixed the same way: drop both caches, rebuild per realm bound to the current host, drop the
ThreadCleanupRegistrywiring.Goccia.SharedPrototypeRealmReuse.Testgains RegExp cases + an FPC-heap stomp (the host is freed to the FPC heap, not the GC heap) + species-fallback exploits for both host-dereferencing callbacks ([Symbol.matchAll]and[Symbol.split]) — it faults before the fix and passes after. ADR 0084 records the finding. (A second code-review pass then hardened this gate — split coverage + documented stomp magnitudes.)🤖 Generated with Claude Code