fix(gc): re-read js_put_value_set operands after the key coercion (#8495) - #8499
Merged
Conversation
The function rooted target/key/value/receiver in a RuntimeHandleScope and then bound all four to plain f64 locals BEFORE calling js_to_property_key. That call runs user code (toString / Symbol.toPrimitive) which allocates and can evacuate: the handles keep the objects ALIVE, but the locals hold pre-collection addresses, so every path below the coercion operated on stale pointers. Rooting the caller could not help — this is the same shape as #8427/#8428, where a correctly-rooted value is defeated by binding the raw pointer too early. Symptom: o[heavyKey("k")] = payload() reads back undefined one collection later — exactly the #6935 contract, re-broken from the other side. Isolated to a 16-line reproduction: a static key survives, the same write with an allocating key coercion loses the value. gc_property_key_operand_rooting_6935 goes 1/3 -> 2/3: proxy_target_set_survives_forced_evacuation ......... now ok property_key_receivers_survive_forced_evacuation .... still ok property_key_stored_values_survive_forced_evacuation dyn-index-set now passes; its Symbol.toPrimitive flavour still fails and routes through js_object_set_index_polymorphic instead — tracked in #8495. perry-runtime proxy:: (17) and object:: (201) unit tests pass. Refs #8495
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesPutValue stale operand fix
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
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.
Refs #8495 — a real GC correctness bug, isolated to a 16-line reproduction.
js_put_value_setrooted target/key/value/receiver in aRuntimeHandleScopeand then immediately bound all four to plainf64locals — before callingjs_to_property_key:ToPropertyKeyinvokes usertoString/Symbol.toPrimitive, which allocates and can move objects. The handles keep them alive, but the locals captured pre-collection addresses, so every path below the coercion operated on stale pointers. This is the same shape as #8427/#8428: correct rooting defeated by binding the raw pointer too early.Symptom:
o[heavyKey("k")] = payload()reads backundefinedone collection later — the #6935 contract, re-broken from the other side.Isolation (both under
PERRY_GC_FORCE_EVACUATE=1):Result on
gc_property_key_operand_rooting_6935: 1/3 → 2/3.proxy_target_set_survives_forced_evacuation— now okproperty_key_receivers_survive_forced_evacuation— still okproperty_key_stored_values_survive_forced_evacuation— itsdyn-index-setcase now passes; theSymbol.toPrimitiveflavour still fails and routes throughjs_object_set_index_polymorphicrather than this function, so it needs the same audit there. Tracked in GC:thisholds a stale from-space pointer after a moving collection inside a method body #8495 — this PR does not claim to close it.Checks:
perry-runtimeproxy::(17) andobject::(201) unit tests pass;cargo fmtclean.Worth noting for reviewers: this bug was invisible for months because
cargo-teststops at its first failing suite and thebun_ffi_stage1abort (#8479) sat ahead of it.full-suite-gatehas been red since June.Summary by CodeRabbit