Stop a property lookup after a static-table PropertyCallback builder throws - #475
Stop a property lookup after a static-table PropertyCallback builder throws#475robobun wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughChangesStatic Property Exception Propagation
Merge Risk: ⚪ Minimal · up to The change stops property lookup after a static-property builder throws and preserves the pending exception across affected lookup paths; reported verification passes, and no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and directly related to the change. It explains the problem, fix, affected code paths, relationship to related issues, and verification results. It does not include the template's Bugzilla URL, reviewer line, or path/function summary, but the core required information is present. Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it touches the core property-lookup hot paths (getPropertySlot, getNonIndexPropertySlot) and four megamorphic JIT slow paths, and the author notes local verification against a Bun build is still in progress, a human look would still be worthwhile.
Checked: the new checks are gated on hasStaticPropertyTable so objects without a static table take the same path as before; getPropertySlot uses exceptionForInspection() (no scope) consistently with its existing contract; the JITOperations checks sit after the structure reload and before initAsMiss, so they can't poison the megamorphic cache on the throw path.
Extended reasoning...
Overview
The PR adds pending-exception checks after a property-lookup miss on an object whose type has a static property table, in six locations: JSObject::getPropertySlot (JSObject.h), JSObject::getNonIndexPropertySlot (JSObjectInlines.h), JSObject::getOwnStaticPropertySlot (JSObject.cpp), and the four megamorphic slow paths getByIdMegamorphic / getByValMegamorphic / inByIdMegamorphic / inByValMegamorphic (JITOperations.cpp). This is a Bun-fork-specific fix: upstream, static-table PropertyCallback builders don't throw, but in this fork they can (Lookup.cpp:73 already handles this by returning false), and callers were treating that false as an ordinary miss.
Security risks
None identified. The change is purely additive — it inserts early-return-on-exception checks. It does not remove any check, change any cache-write condition on the success path, or alter object layout. The failure mode without this fix (stale megamorphic-cache miss) is a correctness bug, not a memory-safety issue.
Level of scrutiny
High. getPropertySlot and getNonIndexPropertySlot are ALWAYS_INLINE and sit on the hottest path in the engine; the megamorphic slow paths feed the megamorphic cache that JIT-compiled code reads directly. The change is small and defensive, and the hasStaticPropertyTable gate keeps the common path unchanged, but this is exactly the kind of code where a maintainer should confirm the placement (e.g., that exceptionForInspection() is the right primitive in getPropertySlot given it has no throw scope, and that no caller relies on the loop reaching the end-of-chain parseIndex fallback after a static-table miss).
Other factors
- The PR description is thorough and the reasoning matches the code in
setUpStaticFunctionSlot. - The author explicitly states verification against a local Bun build is still in progress and a Bun-side test PR is forthcoming.
- No prior human review on the timeline.
- The pattern mirrors the existing
RETURN_IF_EXCEPTIONafter the overriding-getOwnPropertySlotbranch ingetNonIndexPropertySlot, which gives some confidence in correctness.
96fb521 to
c697cfb
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it adds exception checks to the core property-lookup hot paths (getPropertySlot, getNonIndexPropertySlot, getOwnStaticPropertySlot) and the four megamorphic JIT slow paths — code where correctness and performance both matter — a human look would still be worthwhile.
Checked that the new checks are gated on hasStaticPropertyTable so ordinary objects take the same path as before.
Checked that RETURN_IF_EXCEPTION(scope, { }) in the megamorphic helpers returns before the miss is recorded in MegamorphicCache and before the fallback getPropertySlot call.
Checked that getPropertySlot uses exceptionForInspection() (it has no ThrowScope) and only on a miss, matching the existing contract with its callers.
Checked that getNonIndexPropertySlot now observes the builder's scope on a hit as well, covering the validateExceptionChecks abort described in the PR.
Extended reasoning...
Overview
This PR adds pending-exception checks after the own-property step in six property-lookup paths, all conditioned on the object's type having a static property table:
JSObject::getPropertySlot(JSObject.h) — the general prototype-chain walkJSObject::getNonIndexPropertySlot(JSObjectInlines.h) — the non-index variantJSObject::getOwnStaticPropertySlot(JSObject.cpp) — the class-info table walkgetByIdMegamorphic,getByValMegamorphic,inByIdMegamorphic,inByValMegamorphic(JITOperations.cpp) — the megamorphic-cache slow paths
The motivation is a Bun-fork-specific behavior: static-table PropertyCallback builders can run JS and throw, and setUpStaticFunctionSlot reports that as a miss. Without these checks, the miss is treated as ordinary and the lookup continues with an exception pending — which trips validateExceptionChecks, and in the megamorphic paths poisons the cache for the base structure so later reads permanently return undefined.
Security risks
None identified. The change adds early-return guards; it does not introduce new data flows, allocation, or user-controlled input handling. The only observable effect is that a lookup stops sooner when an exception is already pending.
Level of scrutiny
High. getPropertySlot and getNonIndexPropertySlot are among the hottest functions in the engine, inlined into essentially every property access. The megamorphic helpers directly manage the MegamorphicCache. A subtle mistake here could break property-lookup semantics for all JS execution, and the added branch (even gated on hasStaticPropertyTable) sits in an ALWAYS_INLINE function replicated across many call sites. Someone familiar with JSC's exception-scope discipline and this fork's builder semantics should confirm the placement of each check and that the exceptionForInspection() use in getPropertySlot is the right choice for a scope-less function.
Other factors
The PR description is thorough, references the related #390 structure-reload change, and reports verification against a debug ASAN build with targeted Bun tests exercising both walk loops and all four megamorphic ops. The change follows the existing RETURN_IF_EXCEPTION pattern already used after the overriding-getOwnPropertySlot branch in the same loops. Still, the combination of hot-path placement, subtle exception-scope semantics, and megamorphic-cache interaction makes this a change that benefits from a maintainer's eyes rather than automated approval.
c697cfb to
43f67cb
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it modifies core property-lookup hot paths (getPropertySlot, getNonIndexPropertySlot, the four megamorphic JIT slow paths) and changes exception-scope handling in reifyAllStaticProperties, a human look would still be worthwhile.
What was reviewed:
- The
hasStaticPropertyTableguard on each new check — confirmedgetOwnNonIndexPropertySlotonly reaches a builder viagetOwnStaticPropertySlotunder that flag, so plain objects skip the check. - The four megamorphic helpers — each already owns a
ThrowScope, andRETURN_IF_EXCEPTION(scope, { })matches the existing return-value convention in each. getPropertySlot's use ofexceptionForInspection()without a scope — consistent with its existing scope-less design; callers (e.g.JSObject::get) alreadyRETURN_IF_EXCEPTIONafter it.
Extended reasoning...
Overview
This PR adds pending-exception checks after the own-property step of property lookup on objects whose type carries a static property table, so that a PropertyCallback builder that throws stops the lookup instead of being treated as an ordinary miss. It touches six lookup loops: JSObject::getPropertySlot (JSObject.h), JSObject::getNonIndexPropertySlot (JSObjectInlines.h), JSObject::getOwnStaticPropertySlot and reifyAllStaticProperties (JSObject.cpp), and the four megamorphic get_by_id/get_by_val/in_by_id/in_by_val slow paths (JITOperations.cpp). It also swaps vm.exceptionForInspection() for a TopExceptionScope in reifyAllStaticProperties so each builder's scope is observed before the next runs.
Security risks
None identified. The change adds early-outs on pending exceptions and does not introduce new data flows, allocations, or user-controlled inputs. The megamorphic-cache fix prevents recording a stale miss, which is a correctness improvement rather than a new surface.
Level of scrutiny
High. getPropertySlot and getNonIndexPropertySlot are ALWAYS_INLINE and sit on the hottest property-access path in the engine; the megamorphic helpers feed the JIT's MegamorphicCache. The new branches are gated on TypeInfo::hasStaticPropertyTable(inlineTypeFlags()), which keeps ordinary objects on the old path, but the perf and exception-scope-validation implications of touching these loops warrant a maintainer's eye. The TopExceptionScope change in reifyAllStaticProperties also alters scope-simulation behaviour under validateExceptionChecks=1 and deserves confirmation from someone familiar with that machinery.
Other factors
The PR description reports verification against a debug ASAN build with validateExceptionChecks=1 and Bun test suites, and the reasoning for each guard is well-documented in comments. The change is internally consistent across all six sites and mirrors the existing RETURN_IF_EXCEPTION pattern already used after overridden getOwnPropertySlot calls. No prior human reviews or outstanding comments on the PR.
Preview Builds
|
43f67cb to
3438def
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it touches core JSC property-lookup hot paths (getPropertySlot, getNonIndexPropertySlot, the four megamorphic JIT slow paths) and exception-scope semantics, and there's an open sequencing decision with #390, a human look would still be worthwhile.
What was reviewed:
- The new exception checks are gated on
hasStaticPropertyTable, so plain-object lookups take the same path as before. RETURN_IF_EXCEPTION(scope, { })in the megamorphic helpers returns an emptyJSValue, matching the existing overriding-branch checks in each helper.TopExceptionScopeinreifyAllStaticPropertiesavoids simulating a throw todeleteProperty, consistent with the reasoning in the description and the prior #306 change.
Extended reasoning...
Overview
The PR adds pending-exception checks after the own-property step of property lookup on objects with static property tables, in six places: JSObject::getPropertySlot (JSObject.h), JSObject::getNonIndexPropertySlot (JSObjectInlines.h), JSObject::getOwnStaticPropertySlot and reifyAllStaticProperties (JSObject.cpp), and the four megamorphic get_by_id/get_by_val/in_by_id/in_by_val slow paths (JITOperations.cpp). It also swaps the raw vm.exceptionForInspection() in reifyAllStaticProperties for a TopExceptionScope so each builder's scope is observed before the next runs.
Security risks
No new attack surface. The change adds early-exit checks on paths that already run arbitrary JS via PropertyCallback builders; it does not introduce new JS entry points, new cache writes, or new memory access. The megamorphic-cache fix prevents recording a spurious miss, which is a correctness improvement rather than a risk.
Level of scrutiny
High. These are among the hottest code paths in the engine — ALWAYS_INLINE prototype-walk loops and JIT slow-path helpers that feed the megamorphic cache. The diff is small and each hunk is a guarded early return, but the surrounding code is subtle (structure reloads after builder-driven transitions, ThrowScope vs TopExceptionScope validator semantics), and the PR description explicitly asks for a sequencing decision relative to #390. That combination should get a human maintainer's sign-off.
Other factors
The description is thorough with concrete verification against preview builds on Linux ASAN and Windows debug, and a companion Bun test PR (oven-sh/bun#39703). The bug-hunting system found nothing. I checked that the new hasStaticPropertyTable gate keeps the added branch off the common plain-object path, that the { } return in the megamorphic helpers matches what those functions already return on exception in the overriding branch, and that the getNonIndexPropertySlot change also runs on a hit (covering the succeeding-builder validator case the description mentions). Nothing looked wrong, but given the criticality of these paths and the open #390 interaction, deferring to a human is the right call.
…a lazy property whose builder threw A read of an unreified static-table property whose PropertyCallback builder throws is reported as a miss by setUpStaticFunctionSlot. The prototype walk loops in JSObject::getPropertySlot and JSObject::getNonIndexPropertySlot went on to the prototype with the exception pending (and getPropertySlot used the structure from before the builder ran, which asserts when the builder transitioned the object), and the megamorphic get_by_id, get_by_val, in_by_id and in_by_val slow paths recorded the miss for the object's structure, so every later megamorphic access of that property returned undefined (false for `in`) without running the builder again. reifyAllStaticProperties ran builders back to back without checking between them, which made spreading Bun abort under BUN_JSC_validateExceptionChecks=1. The WebKit change checks for the exception after the own-property step on static-table objects and after each builder in reifyAllStaticProperties. The tests read Bun.sql with the sql builder made to throw: through Bun and through a function that inherits from Bun, behind a Proxy prototype and under BUN_JSC_validateExceptionChecks=1; with a builder that reifies another property of Bun first; and from megamorphic get_by_id, get_by_val, in_by_id and in_by_val sites, where the second access has to throw again. The process.env pre-read in the test above them worked around the structure assertion on Windows and is removed, and the file leaves test/no-validate-exceptions.txt now that spreading Bun is clean under the validator.
…a lazy property whose builder threw A read of an unreified static-table property whose PropertyCallback builder throws is reported as a miss by setUpStaticFunctionSlot. The prototype walk loops in JSObject::getPropertySlot and JSObject::getNonIndexPropertySlot went on to the prototype with the exception pending (and getPropertySlot used the structure from before the builder ran, which asserts when the builder transitioned the object), and the megamorphic get_by_id, get_by_val, in_by_id and in_by_val slow paths recorded the miss for the object's structure, so every later megamorphic access of that property returned undefined (false for `in`) without running the builder again. reifyAllStaticProperties ran builders back to back without checking between them, which made spreading Bun abort under the exception check validator of debug builds. The WebKit change checks for the exception after the own-property step on static-table objects and after each builder in reifyAllStaticProperties. The tests read Bun.sql with the sql builder made to throw. One reads it through Bun and one through a function that inherits from Bun, both behind a Proxy prototype and with the validator enabled. One makes the builder reify another property of Bun first. One reads from megamorphic get_by_id, get_by_val, in_by_id and in_by_val sites, where the second access has to throw again. The process.env pre-read in the test above them worked around the structure assertion on Windows and is removed, and the file leaves test/no-validate-exceptions.txt now that spreading Bun is clean under the validator.
3438def to
cb22487
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it touches the hottest property-lookup paths in JSC (getPropertySlot, getNonIndexPropertySlot, four megamorphic JIT slow paths) and exception-scope semantics, a human look would still be worthwhile.
What was reviewed:
- The
hasStaticPropertyTableguard is a cheap inline-flag test, so objects without static tables take the same path as before. RETURN_IF_EXCEPTIONplacement in the megamorphic helpers is before the cache write, so a throwing builder no longer poisons theMegamorphicCache.TopExceptionScopeinreifyAllStaticPropertiesmatches theJSBoundFunction::nameSlowpattern and avoids simulating a throw todeleteProperty(the #306 concern).getPropertySlotusesexceptionForInspection()(no scope of its own) only on the miss path, consistent with how its callers already check afterwards.
Extended reasoning...
Overview
The PR adds exception checks after the own-property step in six property-lookup call sites when the object's type carries a static property table, so that a PropertyCallback builder that throws stops the lookup instead of continuing up the prototype chain or being recorded as a miss in the megamorphic cache. It also swaps vm.exceptionForInspection() for a TopExceptionScope in reifyAllStaticProperties so each builder's scope is observed before the next one runs. Files touched: JSObject.h, JSObjectInlines.h, JSObject.cpp, JITOperations.cpp.
Security risks
None identified. The change tightens exception propagation; it does not relax any check, expose data, or alter permissions/auth. The added branches are guarded by an inline type-flag bit and only fire on objects with static tables.
Level of scrutiny
High. JSObject::getPropertySlot and getNonIndexPropertySlot are ALWAYS_INLINE and sit on essentially every property access in the engine; the four *Megamorphic helpers are JIT slow paths whose caching behavior affects release-build correctness. Exception-scope discipline (ThrowScope vs TopExceptionScope vs exceptionForInspection) is subtle and interacts with validateExceptionChecks. This is core-engine, performance-sensitive code where a maintainer familiar with JSC's exception validator and the history around #306/#390 should sign off.
Other factors
The PR description is thorough and documents extensive verification against Bun debug/ASAN builds on Linux and Windows, plus targeted tests in oven-sh/bun#39703. The added checks follow existing patterns already used in the same functions (e.g. the RETURN_IF_EXCEPTION after the overriding-getOwnPropertySlot branch in getNonIndexPropertySlot). The interaction with #390 (whether to land it first, rebase over it, or close it as superseded) is a coordination decision the author explicitly left to a maintainer. No prior human or bot review comments to address.
…a lazy property whose builder threw A read of an unreified static-table property whose PropertyCallback builder throws is reported as a miss by setUpStaticFunctionSlot. The prototype walk loops in JSObject::getPropertySlot and JSObject::getNonIndexPropertySlot went on to the prototype with the exception pending (and getPropertySlot used the structure from before the builder ran, which asserts when the builder transitioned the object), and the megamorphic get_by_id, get_by_val, in_by_id and in_by_val slow paths recorded the miss for the object's structure, so every later megamorphic access of that property returned undefined (false for `in`) without running the builder again. reifyAllStaticProperties ran builders back to back without checking between them, which made spreading Bun abort under the exception check validator of debug builds. The WebKit change checks for the exception after the own-property step on static-table objects and after each builder in reifyAllStaticProperties. The tests read Bun.sql with the sql builder made to throw. One reads it through Bun and one through a function that inherits from Bun, both behind a Proxy prototype and with the validator enabled. One makes the builder reify another property of Bun first. One reads from megamorphic get_by_id, get_by_val, in_by_id and in_by_val sites, where the second access has to throw again. The process.env pre-read in the test above them worked around the structure assertion on Windows and is removed, and the file leaves test/no-validate-exceptions.txt now that spreading Bun is clean under the validator.
cb22487 to
594c682
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it modifies the core property-lookup hot paths (getPropertySlot, getNonIndexPropertySlot, getOwnStaticPropertySlot) and the four megamorphic JIT slow paths, and involves subtle exception-scope semantics, a human look would still be worthwhile — including the open question of whether #390 should land first or be superseded.
What was reviewed:
- Each new check is gated on
hasStaticPropertyTable, so objects without a static table follow the same code path as before. - The megamorphic helpers bail with
RETURN_IF_EXCEPTIONbefore the miss can be recorded in the cache, and after the structure reload so no stale state is used. getNonIndexPropertySlotnow checks the scope on both hit and miss, matching the overriding-getOwnPropertySlotbranch and covering the succeeding-builder validator case.reifyAllStaticPropertiesusesTopExceptionScope(notThrowScope) sodeletePropertycallers that don't check afterwards keep the same behaviour as the priorexceptionForInspection()early-return.
Extended reasoning...
Overview
The PR adds pending-exception checks after the own-property step in every prototype-walk loop that can reach a static-table PropertyCallback builder: JSObject::getPropertySlot (JSObject.h), JSObject::getNonIndexPropertySlot (JSObjectInlines.h), JSObject::getOwnStaticPropertySlot (JSObject.cpp), and the four megamorphic get_by_id / get_by_val / in_by_id / in_by_val slow paths in JITOperations.cpp. It also swaps the raw vm.exceptionForInspection() check in reifyAllStaticProperties for a TopExceptionScope so the validator sees each builder's scope as observed before the next one runs.
Security risks
None identified. The change adds early-exit paths on pending exceptions; it does not remove any check, does not change caching behaviour when no exception is pending, and does not touch auth, permissions, or memory layout. The megamorphic-cache poisoning it fixes is a correctness issue (stale undefined returned) rather than an exploitable disclosure.
Level of scrutiny
High. getPropertySlot and getNonIndexPropertySlot are on the critical path for essentially every non-indexed property access in the engine, and the megamorphic helpers back the JIT's property-access ICs. Exception-scope discipline in JSC (ThrowScope vs CatchScope vs TopExceptionScope, exceptionForInspection() vs RETURN_IF_EXCEPTION) is subtle enough that a reviewer familiar with the validator's rules should confirm the choices — especially the deliberate use of exceptionForInspection() in getPropertySlot (which owns no scope) versus RETURN_IF_EXCEPTION(scope, ...) in the loops that do. The new checks are guarded by TypeInfo::hasStaticPropertyTable(object->inlineTypeFlags()), so the common no-static-table fast path is a single flag test with no additional VM access; that should keep the hot-path cost negligible, but it's worth a human eye given how hot these functions are.
Other factors
The description is unusually thorough, includes cross-platform (Linux ASAN, Windows debug) verification against preview builds, and references companion Bun tests (oven-sh/bun#39703) covering each failure mode. The author also flags an open coordination question with #390 — whether to land that first and rebase, or close it as superseded — which is a decision a maintainer should make. No prior human review is on the thread; coderabbit was rate-limited and did not review.
…a lazy property whose builder threw A read of an unreified static-table property whose PropertyCallback builder throws is reported as a miss by setUpStaticFunctionSlot. The prototype walk loops in JSObject::getPropertySlot and JSObject::getNonIndexPropertySlot went on to the prototype with the exception pending (and getPropertySlot used the structure from before the builder ran, which asserts when the builder transitioned the object), and the megamorphic get_by_id, get_by_val, in_by_id and in_by_val slow paths recorded the miss for the object's structure, so every later megamorphic access of that property returned undefined (false for `in`) without running the builder again. reifyAllStaticProperties ran builders back to back without checking between them, which made spreading Bun abort under the exception check validator of debug builds. The WebKit change checks for the exception after the own-property step on static-table objects and after each builder in reifyAllStaticProperties. The tests read Bun.sql with the sql builder made to throw. One reads it through Bun and one through a function that inherits from Bun, both behind a Proxy prototype and with the validator enabled. One makes the builder reify another property of Bun first. One reads from megamorphic get_by_id, get_by_val, in_by_id and in_by_val sites, where the second access has to throw again. The process.env pre-read in the test above them worked around the structure assertion on Windows and is removed, and the file leaves test/no-validate-exceptions.txt now that spreading Bun is clean under the validator.
594c682 to
7d1bf31
Compare
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunt found no issues. Because it touches the hot getPropertySlot/getNonIndexPropertySlot walks and four megamorphic JIT slow paths with subtle exception-scope semantics, a human look is still worthwhile.
What was reviewed:
- The new checks are gated on
hasStaticPropertyTable, so plain-object lookups skip the exception load. getNonIndexPropertySlotchecks on hit as well (matches the overriding branch) for the succeeding-builder validator case;getPropertySlotusesexceptionForInspection()on miss only, since it has no scope.- The megamorphic paths bail before the miss reaches the
MegamorphicCache, andreifyAllStaticPropertiesusesTopExceptionScoperather thanThrowScopesodeletePropertycallers still don't need to check.
Extended reasoning...
Overview
This PR changes exception handling in JSC's property-lookup fast paths so that when a Bun static-table PropertyCallback builder throws (which setUpStaticFunctionSlot reports as a miss), the caller stops the lookup instead of continuing up the prototype chain or recording a spurious miss. It touches JSObject::getPropertySlot (JSObject.h), JSObject::getNonIndexPropertySlot (JSObjectInlines.h), getOwnStaticPropertySlot and reifyAllStaticProperties (JSObject.cpp), and the four megamorphic get_by_id/get_by_val/in_by_id/in_by_val helpers in JITOperations.cpp.
Security risks
None identified. The change adds early-outs on pending exceptions and does not introduce new data flow, allocation, or caching. The megamorphic-cache change strictly removes a case where a stale miss was recorded, which is a correctness fix rather than a new attack surface.
Level of scrutiny
High. getPropertySlot is ALWAYS_INLINE and explicitly documented as very hot in variable lookup; adding a branch there has performance implications even though it is gated on hasStaticPropertyTable and marked [[unlikely]]. The distinction between ThrowScope, TopExceptionScope, and raw vm.exceptionForInspection() is subtle and load-bearing (the description references #306 for why reifyAllStaticProperties must not use a ThrowScope). The four JITOperations hunks are mechanically identical but sit on the boundary between runtime state and the JIT's megamorphic cache. This is core-VM logic where a maintainer familiar with the fork's builder semantics and the exception validator should sign off.
Other factors
The description is thorough and cites concrete validator failures, a release-mode reproduction of the megamorphic-cache poisoning, and cross-platform test runs against preview builds. It also notes an interaction with #390 that a maintainer should decide on (rebase vs. supersede). No prior human or bot review is on the thread yet.
…a lazy property whose builder threw A read of an unreified static-table property whose PropertyCallback builder throws is reported as a miss by setUpStaticFunctionSlot. The prototype walk loops in JSObject::getPropertySlot and JSObject::getNonIndexPropertySlot went on to the prototype with the exception pending (and getPropertySlot used the structure from before the builder ran, which asserts when the builder transitioned the object), and the megamorphic get_by_id, get_by_val, in_by_id and in_by_val slow paths recorded the miss for the object's structure, so every later megamorphic access of that property returned undefined (false for `in`) without running the builder again. reifyAllStaticProperties ran builders back to back without checking between them, which made spreading Bun abort under the exception check validator of debug builds. The WebKit change checks for the exception after the own-property step on static-table objects and after each builder in reifyAllStaticProperties. The tests read Bun.sql with the sql builder made to throw. One reads it through Bun and one through a function that inherits from Bun, both behind a Proxy prototype and with the validator enabled. One makes the builder reify another property of Bun first. One reads from megamorphic get_by_id, get_by_val, in_by_id and in_by_val sites, where the second access has to throw again. The process.env pre-read in the test above them worked around the structure assertion on Windows and is removed, and the file leaves test/no-validate-exceptions.txt now that spreading Bun is clean under the validator.
7d1bf31 to
2501e6c
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it touches the core property-lookup hot paths (getPropertySlot, getNonIndexPropertySlot, reifyAllStaticProperties) and the four megamorphic JIT slow paths, and involves subtle exception-scope semantics plus an open coordination point with #390, a human look would still be worthwhile.
What was reviewed
- The
hasStaticPropertyTableguard on each new check — objects without a static table take the unchanged fast path, so no new branch on the common case. RETURN_IF_EXCEPTIONplacement in the megamorphic helpers — it fires before the miss is written toMegamorphicCacheand beforehasPropertyis consumed, so a throwing builder can't poison the cache or short-circuit as a hit.getNonIndexPropertySlotchecking on hit as well as miss, which the PR body says is needed for the succeeding-builder validator case;getPropertySlotonly checks on miss (no scope of its own), which matches the description.DECLARE_TOP_EXCEPTION_SCOPEinreifyAllStaticProperties— matches theJSBoundFunction::nameSlowpattern and preserves the #306 property thatdeletePropertycallers don't get a simulated throw.
Extended reasoning...
Overview
The PR adds pending-exception checks after the own-property step in six property-lookup call sites (JSObject::getPropertySlot, JSObject::getNonIndexPropertySlot, JSObject::getOwnStaticPropertySlot, and the four megamorphic get_by_id/get_by_val/in_by_id/in_by_val slow paths in JITOperations.cpp), and switches reifyAllStaticProperties from a raw vm.exceptionForInspection() check to a TopExceptionScope so each builder's scope is observed before the next one runs. All checks are gated on TypeInfo::hasStaticPropertyTable(inlineTypeFlags()), which is the only condition under which getOwnNonIndexPropertySlot can invoke a PropertyCallback builder, so plain objects and arrays are unaffected.
Security risks
None identified. The change tightens exception handling — it stops a lookup earlier when an exception is already pending, rather than continuing into user-observable operations (Proxy traps, host getOwnPropertySlot) with a pending exception. It also prevents a poisoned-cache correctness bug where a throwing builder would cause later megamorphic reads to permanently return undefined. No new attack surface.
Level of scrutiny
High. getPropertySlot and getNonIndexPropertySlot are among the hottest inline paths in the VM, and the megamorphic helpers back the JIT's polymorphic property-access slow paths. Any mistake here affects semantics or performance of essentially all JS property reads. The change is small and each hunk follows an existing pattern (the same RETURN_IF_EXCEPTION the overriding-getOwnPropertySlot branch already uses), but the interaction between ThrowScope, CatchScope, TopExceptionScope, and exceptionForInspection() under validateExceptionChecks=1 is subtle enough that a maintainer familiar with the fork's exception-scope conventions and with #306/#390 should confirm the choices — particularly using exceptionForInspection() (rather than a scope) in getPropertySlot and getOwnStaticPropertySlot, and the deliberate asymmetry where getNonIndexPropertySlot checks on hit but getPropertySlot only checks on miss.
Other factors
The description is unusually thorough and cites concrete verification against Bun debug/ASAN builds on Linux and Windows, under the exception validator, with a linked Bun-side test PR. All 42 preview-build lanes are green. There is an explicit open question about ordering relative to #390 (rebase vs. supersede) that a human should decide. No prior reviewer comments to address. Given the criticality of the touched paths and the coordination point, deferring to human review is the right call even though no defects were found.
…a lazy property whose builder threw A read of an unreified static-table property whose PropertyCallback builder throws is reported as a miss by setUpStaticFunctionSlot. The prototype walk loops in JSObject::getPropertySlot and JSObject::getNonIndexPropertySlot went on to the prototype with the exception pending (and getPropertySlot used the structure from before the builder ran, which asserts when the builder transitioned the object), and the megamorphic get_by_id, get_by_val, in_by_id and in_by_val slow paths recorded the miss for the object's structure, so every later megamorphic access of that property returned undefined (false for `in`) without running the builder again. reifyAllStaticProperties ran builders back to back without checking between them, which made spreading Bun abort under the exception check validator of debug builds. The WebKit change checks for the exception after the own-property step on static-table objects and after each builder in reifyAllStaticProperties. The tests read Bun.sql with the sql builder made to throw. One reads it through Bun and one through a function that inherits from Bun, both behind a Proxy prototype and with the validator enabled. One makes the builder reify another property of Bun first. One reads from megamorphic get_by_id, get_by_val, in_by_id and in_by_val sites, where the second access has to throw again. The process.env pre-read in the test above them worked around the structure assertion on Windows and is removed, and the file leaves test/no-validate-exceptions.txt now that spreading Bun is clean under the validator.
2501e6c to
d92bce5
Compare
…a lazy property whose builder threw A read of an unreified static-table property whose PropertyCallback builder throws is reported as a miss by setUpStaticFunctionSlot. The prototype walk loops in JSObject::getPropertySlot and JSObject::getNonIndexPropertySlot went on to the prototype with the exception pending (and getPropertySlot used the structure from before the builder ran, which asserts when the builder transitioned the object), and the megamorphic get_by_id, get_by_val, in_by_id and in_by_val slow paths recorded the miss for the object's structure, so every later megamorphic access of that property returned undefined (false for `in`) without running the builder again. reifyAllStaticProperties ran builders back to back without checking between them, which made spreading Bun abort under the exception check validator of debug builds. The WebKit change checks for the exception after the own-property step on static-table objects and after each builder in reifyAllStaticProperties. The tests read Bun.sql with the sql builder made to throw. One reads it through Bun and one through a function that inherits from Bun, both behind a Proxy prototype and with the validator enabled. One makes the builder reify another property of Bun first. One reads from megamorphic get_by_id, get_by_val, in_by_id and in_by_val sites, where the second access has to throw again. The process.env pre-read in the test above them worked around the structure assertion on Windows and is removed, and the file leaves test/no-validate-exceptions.txt now that spreading Bun is clean under the validator.
d92bce5 to
693e78d
Compare
…a lazy property whose builder threw A read of an unreified static-table property whose PropertyCallback builder throws is reported as a miss by setUpStaticFunctionSlot. The prototype walk loops in JSObject::getPropertySlot and JSObject::getNonIndexPropertySlot went on to the prototype with the exception pending (and getPropertySlot used the structure from before the builder ran, which asserts when the builder transitioned the object), and the megamorphic get_by_id, get_by_val, in_by_id and in_by_val slow paths recorded the miss for the object's structure, so every later megamorphic access of that property returned undefined (false for `in`) without running the builder again. reifyAllStaticProperties ran builders back to back without checking between them, which made spreading Bun abort under the exception check validator of debug builds. The WebKit change checks for the exception after the own-property step on static-table objects and after each builder in reifyAllStaticProperties. The tests read Bun.sql with the sql builder made to throw. One reads it through Bun and one through a function that inherits from Bun, both behind a Proxy prototype and with the validator enabled. One makes the builder reify another property of Bun first. One reads from megamorphic get_by_id, get_by_val, in_by_id and in_by_val sites, where the second access has to throw again. The process.env pre-read in the test above them worked around the structure assertion on Windows and is removed, and the file leaves test/no-validate-exceptions.txt now that spreading Bun is clean under the validator.
693e78d to
898102d
Compare
|
Preview builds are currently blocked for every PR in this repository: the Seen on this PR's run 33060595263 (attempt 1 and a re-run of the failed job, both 7 seconds) and on runs for unrelated branches started since about 09:49Z today: 33060284775, 33060269980, 33060589499 and 33060629908 all fail the same lane at the same step. The other 42 lanes of this PR's run are green and their artifacts are on the run, so once that step works again a |
|
Follow-up on the preview blocker: the Once either lands on |
…a lazy property whose builder threw A read of an unreified static-table property whose PropertyCallback builder throws is reported as a miss by setUpStaticFunctionSlot. The prototype walk loops in JSObject::getPropertySlot and JSObject::getNonIndexPropertySlot went on to the prototype with the exception pending (and getPropertySlot used the structure from before the builder ran, which asserts when the builder transitioned the object), and the megamorphic get_by_id, get_by_val, in_by_id and in_by_val slow paths recorded the miss for the object's structure, so every later megamorphic access of that property returned undefined (false for `in`) without running the builder again. reifyAllStaticProperties ran builders back to back without checking between them, which made spreading Bun abort under the exception check validator of debug builds. The WebKit change checks for the exception after the own-property step on static-table objects and after each builder in reifyAllStaticProperties. The tests read Bun.sql with the sql builder made to throw. One reads it through Bun and one through a function that inherits from Bun, both behind a Proxy prototype and with the validator enabled. One makes the builder reify another property of Bun first. One reads from megamorphic get_by_id, get_by_val, in_by_id and in_by_val sites, where the second access has to throw again. The process.env pre-read in the test above them worked around the structure assertion on Windows and is removed, and the file leaves test/no-validate-exceptions.txt now that spreading Bun is clean under the validator.
898102d to
1e46058
Compare
…a lazy property whose builder threw A read of an unreified static-table property whose PropertyCallback builder throws is reported as a miss by setUpStaticFunctionSlot. The prototype walk loops in JSObject::getPropertySlot and JSObject::getNonIndexPropertySlot went on to the prototype with the exception pending (and getPropertySlot used the structure from before the builder ran, which asserts when the builder transitioned the object), and the megamorphic get_by_id, get_by_val, in_by_id and in_by_val slow paths recorded the miss for the object's structure, so every later megamorphic access of that property returned undefined (false for `in`) without running the builder again. reifyAllStaticProperties ran builders back to back without checking between them, which made spreading Bun abort under the exception check validator of debug builds. The WebKit change checks for the exception after the own-property step on static-table objects and after each builder in reifyAllStaticProperties. The tests read Bun.sql with the sql builder made to throw. One reads it through Bun and one through a function that inherits from Bun, both behind a Proxy prototype and with the validator enabled. One makes the builder reify another property of Bun first. One reads from megamorphic get_by_id, get_by_val, in_by_id and in_by_val sites, where the second access has to throw again. The process.env pre-read in the test above them worked around the structure assertion on Windows and is removed, and the file leaves test/no-validate-exceptions.txt now that spreading Bun is clean under the validator.
1e46058 to
49296ec
Compare
…a lazy property whose builder threw A read of an unreified static-table property whose PropertyCallback builder throws is reported as a miss by setUpStaticFunctionSlot. The prototype walk loops in JSObject::getPropertySlot and JSObject::getNonIndexPropertySlot went on to the prototype with the exception pending (and getPropertySlot used the structure from before the builder ran, which asserts when the builder transitioned the object), and the megamorphic get_by_id, get_by_val, in_by_id and in_by_val slow paths recorded the miss for the object's structure, so every later megamorphic access of that property returned undefined (false for `in`) without running the builder again. reifyAllStaticProperties ran builders back to back without checking between them, which made spreading Bun abort under the exception check validator of debug builds. The WebKit change checks for the exception after the own-property step on static-table objects and after each builder in reifyAllStaticProperties. The tests read Bun.sql with the sql builder made to throw. One reads it through Bun and one through a function that inherits from Bun, both behind a Proxy prototype and with the validator enabled. One makes the builder reify another property of Bun first. One reads from megamorphic get_by_id, get_by_val, in_by_id and in_by_val sites, where the second access has to throw again. The process.env pre-read in the test above them worked around the structure assertion on Windows and is removed, and the file leaves test/no-validate-exceptions.txt now that spreading Bun is clean under the validator.
49296ec to
960b835
Compare
…a lazy property whose builder threw A read of an unreified static-table property whose PropertyCallback builder throws is reported as a miss by setUpStaticFunctionSlot. The prototype walk loops in JSObject::getPropertySlot and JSObject::getNonIndexPropertySlot went on to the prototype with the exception pending (and getPropertySlot used the structure from before the builder ran, which asserts when the builder transitioned the object), and the megamorphic get_by_id, get_by_val, in_by_id and in_by_val slow paths recorded the miss for the object's structure, so every later megamorphic access of that property returned undefined (false for `in`) without running the builder again. reifyAllStaticProperties ran builders back to back without checking between them, which made spreading Bun abort under the exception check validator of debug builds. The WebKit change checks for the exception after the own-property step on static-table objects and after each builder in reifyAllStaticProperties. The tests read Bun.sql with the sql builder made to throw. One reads it through Bun and one through a function that inherits from Bun, both behind a Proxy prototype and with the validator enabled. One makes the builder reify another property of Bun first. One reads from megamorphic get_by_id, get_by_val, in_by_id and in_by_val sites, where the second access has to throw again. The process.env pre-read in the test above them worked around the structure assertion on Windows and is removed, and the file leaves test/no-validate-exceptions.txt now that spreading Bun is clean under the validator.
960b835 to
3945a1f
Compare
…a lazy property whose builder threw A read of an unreified static-table property whose PropertyCallback builder throws is reported as a miss by setUpStaticFunctionSlot. The prototype walk loops in JSObject::getPropertySlot and JSObject::getNonIndexPropertySlot went on to the prototype with the exception pending (and getPropertySlot used the structure from before the builder ran, which asserts when the builder transitioned the object), and the megamorphic get_by_id, get_by_val, in_by_id and in_by_val slow paths recorded the miss for the object's structure, so every later megamorphic access of that property returned undefined (false for `in`) without running the builder again. reifyAllStaticProperties ran builders back to back without checking between them, which made spreading Bun abort under the exception check validator of debug builds. The WebKit change checks for the exception after the own-property step on static-table objects and after each builder in reifyAllStaticProperties. The tests read Bun.sql with the sql builder made to throw. One reads it through Bun and one through a function that inherits from Bun, both behind a Proxy prototype and with the validator enabled. One makes the builder reify another property of Bun first. One reads from megamorphic get_by_id, get_by_val, in_by_id and in_by_val sites, where the second access has to throw again. The process.env pre-read in the test above them worked around the structure assertion on Windows and is removed, and the file leaves test/no-validate-exceptions.txt now that spreading Bun is clean under the validator.
…throws In this fork a static-table PropertyCallback builder can run JS and throw. setUpStaticFunctionSlot then reports the slot as not found so that the caller observes the exception. Every caller treated that miss like an ordinary miss and kept going with the exception pending: - JSObject::getPropertySlot and JSObject::getNonIndexPropertySlot walked on to the prototype. A prototype that overrides getOwnPropertySlot (a Proxy, a host object) then ran with the exception pending. Exception scope verification reports this as an unchecked exception. getPropertySlot also read the prototype off the Structure* it had loaded before the builder ran, which asserts in Structure::storedPrototype when the builder transitioned the object before it threw. - The megamorphic get_by_id, get_by_val, in_by_id and in_by_val slow paths walked to the end of the chain and recorded the miss in the MegamorphicCache for the base object's structure. The builder stores nothing when it throws, so the structure does not change, and every later megamorphic access of that property on that object returned undefined (false for `in`) without running the builder again. - JSObject::getOwnStaticPropertySlot went on to the parent class tables. Check for a pending exception after the own-property step on an object that has a static property table, which is the only case in which getOwnNonIndexPropertySlot can run a builder, and stop the lookup there. Objects without a static table take the same path as before. In the loops that own a ThrowScope the check is the same RETURN_IF_EXCEPTION they already make after an overridden getOwnPropertySlot, and it is made on a hit as well: under exception scope verification, getNonIndexPropertySlot's own scope otherwise reports the scope of a builder that succeeded as unchecked. getPropertySlot has no scope of its own and checks with exceptionForInspection() on a miss, leaving the check to its caller as the plain-prototype case does today. reifyAllStaticProperties runs builders back to back and has the same verification problem between two of them: the first builder's scope is reported as unchecked when the second one declares its scope, so spreading or Object.assign'ing such an object aborts under validateExceptionChecks even though nothing threw. Give it a TopExceptionScope (not a ThrowScope, which would simulate a throw to callers such as JSObject::deleteProperty that do not check, see #306) and check it after each builder. A real exception still stays pending for the caller as before.
3945a1f to
94c5a2d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…a lazy property whose builder threw A read of an unreified static-table property whose PropertyCallback builder throws is reported as a miss by setUpStaticFunctionSlot. The prototype walk loops in JSObject::getPropertySlot and JSObject::getNonIndexPropertySlot went on to the prototype with the exception pending (and getPropertySlot used the structure from before the builder ran, which asserts when the builder transitioned the object), and the megamorphic get_by_id, get_by_val, in_by_id and in_by_val slow paths recorded the miss for the object's structure, so every later megamorphic access of that property returned undefined (false for `in`) without running the builder again. reifyAllStaticProperties ran builders back to back without checking between them, which made spreading Bun abort under the exception check validator of debug builds. The WebKit change checks for the exception after the own-property step on static-table objects and after each builder in reifyAllStaticProperties. The tests read Bun.sql with the sql builder made to throw. One reads it through Bun and one through a function that inherits from Bun, both behind a Proxy prototype and with the validator enabled. One makes the builder reify another property of Bun first. One reads from megamorphic get_by_id, get_by_val, in_by_id and in_by_val sites, where the second access has to throw again. The process.env pre-read in the test above them worked around the structure assertion on Windows and is removed, and the file leaves test/no-validate-exceptions.txt now that spreading Bun is clean under the validator.
1 participant
Problem
In this fork a static-table
PropertyCallbackbuilder can run JS and throw.setUpStaticFunctionSlot(Lookup.cpp) then reports the slot as not found so that the caller observes the exception. Every caller treats that miss like an ordinary miss and continues the lookup with the exception pending:JSObject::getPropertySlot(JSObject.h) andJSObject::getNonIndexPropertySlot(JSObjectInlines.h) walk on to the prototype. If the prototype overridesgetOwnPropertySlot(a Proxy, a host object), itsgetOwnPropertySlotruns with the exception pending. WithvalidateExceptionChecks=1this aborts: "Unchecked JS exception ... unchecked as of this scope: getNonIndexPropertySlot @ JSObjectInlines.h:286" (orgetOwnPropertySlotCommon @ ProxyObject.cppwhen the walk started ingetNonIndexPropertySlot).getPropertySlotalso takes the prototype from theStructure*it loaded before the builder ran. If the builder transitioned the object before it threw, that asserts inStructure::storedPrototype(StructureInlinesLight.h:56), which is what JSObject::getPropertySlot: reload the structure before the prototype step #390 is about.get_by_id,get_by_val,in_by_idandin_by_valslow paths (JITOperations.cpp) walk to the end of the chain and record the miss in theMegamorphicCachefor the base object's structure. A builder that throws stores nothing, so the structure does not change. Every later megamorphic access of that property on that object then returnsundefined(falseforin) and never runs the builder again. This reproduces on release builds of Bun 1.4.0: after one read ofBun.sqlthat throws, every megamorphico.sqlsite returnsundefinedforBun, while a fresh site returns the function.JSObject::getOwnStaticPropertySlotgoes on to the parent class tables.getNonIndexPropertySlotowns aThrowScopebut never checks after the own-property step, so a builder that succeeds on that path (first read ofBun.sqlthrough a function whose prototype isBun) aborts undervalidateExceptionChecks=1when the loop's scope is destroyed.JSObject::reifyAllStaticPropertiesruns builders back to back, so the first builder's scope is reported as unchecked when the second builder declares its own:({...Bun})aborts under the validator even though nothing throws. That is why Bun keeps the test file for itsBunobject out of its validator lane.Fix
After the own-property step on an object whose type has a static property table, check for a pending exception and stop the lookup. That is the only case in which
getOwnNonIndexPropertySlotcan run a builder, so objects without a static table take the same path as before. In the loops that own a scope (getNonIndexPropertySlotand the four megamorphic helpers) the check is the sameRETURN_IF_EXCEPTIONthey already make after an overriddengetOwnPropertySlot, made on a hit as well, which covers the succeeding-builder case.getPropertySlothas no scope of its own, so it checks withexceptionForInspection()on a miss and leaves the check to its caller, as the plain-prototype case does today.reifyAllStaticPropertiesgets aTopExceptionScopeand checks it after each builder. Not aThrowScope: that would simulate a throw to callers such asJSObject::deletePropertythat do not check, which is what #306 removed. ATopExceptionScopeis whatJSBoundFunction::nameSlowuses in the same situation. A real exception still stays pending for the caller exactly as before.Relation to #390: that PR reloads the structure before the prototype step of
getPropertySlot. With this change a throwing builder never reaches that step, so the case #390 fixes becomes unreachable and its test passes with this change alone. The reload is still correct as hardening. Either land #390 first and I rebase this (the two hunks are a few lines apart), or close it as superseded by this.Verification
Rebased onto
b7f217b4(#477),aea1f010(#330),c148a12d(#494), the8c4fd56347upstream merge (cb61607f),1cb96a7b(#513),76882271,2da33d53(#519),72597399(#521),0bb01ed5,f5deafe0,1817c3c3(#528, the6b879687eeupstream merge),c4ddc0cf(#527) andceb9f90f(#530, #531) as Bun moved its pin; the diff is unchanged and applied without edits each time. The preview built from the current head (autobuild-preview-pr-475-94c5a2d5, all lanes green on the first attempt) passes the Bun test file below on Linux x64 debug ASAN, plain and under the validator, plus Bun'sffi-ptr-non-view-cell-argfixture for #477. Bun debug builds against this PR's preview release (autobuild-preview-pr-475-43f67cb4) pass the Bun test file below on Linux x64 debug ASAN and on Windows x64 debug, plain and under the validator. The same Windows build against0f966e81fails five of its tests, including the one that exercised the transition-then-throw case through a$builder that reifies another property first. Before the release existed, the same was checked against a debug ASAN JSC built locally from this branch and linked into a Bun debug build. Bun tests (oven-sh/bun#39703): a Proxy prototype behindBunwithvalidateExceptionChecks=1through both walk loops, the transition-then-throw case from #390, megamorphicget_by_id,get_by_val,in_by_idandin_by_valsites, and the whole file, which spreadsBun, under the validator. All of these fail on a Bun build against the current pin0f966e81with the symptoms above (the megamorphic one on release Bun too) and pass against this branch.test/js/bun/util,test/js/bun/jsc,globals(which deletes globals, thedeletePropertyroute intoreifyAllStaticProperties),namespace-prototype-pollutionand the sql adapter tests pass against this branch, the first four also under the validator. Spreadingprocess,globalThisandimport.metaruns clean under the validator as well.