Fix completion value clobbered by re-entrant Evaluate() during module execution#2493
Merged
lahma merged 2 commits intoJun 3, 2026
Merged
Conversation
… module execution ScriptEvaluation stored the script's completion value in the engine-level _completionValue field and then drained the shared event loop via RunAvailableContinuations(). When a microtask (e.g. an awaited dynamic import whose module body calls a CLR function that itself calls Evaluate()) re-entered the engine, the deeper ScriptEvaluation overwrote _completionValue before the outer Evaluate() read it back, so the outer call returned the wrong value (manifesting as "Cannot read property '...' of undefined"). Carry the completion value out as a per-frame local instead: ScriptEvaluation now returns the JsValue, Evaluate()/Execute() funnel through a shared ExecuteForCompletion() helper, ExecuteAsync() reads via Evaluate(), and the _completionValue field is removed. A local survives re-entrancy, so this class of bug cannot recur. The invariant for the remaining ambient fields (_error, _lastSyntaxElement) is documented; they are safe because they are produced and consumed synchronously with no re-entrant drain in between. No struct grew (a field was removed); MinimalScriptBenchmark shows flat time and allocations base vs fix. Fixes sebastienros#2492. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 15, 2026
This was referenced Jun 24, 2026
This was referenced Jul 6, 2026
This was referenced Jul 13, 2026
This was referenced Jul 20, 2026
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.
Summary
Fixes #2492 — a regression where a script's completion value could be clobbered by a re-entrant
Evaluate()triggered from module execution, so a previous evaluation returned the wrong object (reported asCannot read property 'generateRandomText' of undefined).Root cause
Engine.ScriptEvaluationstored the script's completion value in the engine-level_completionValuefield and then drained the shared event loop:RunAvailableContinuations()can re-enter the engine: an awaited dynamicimport()runs a module whose body calls a CLR function that itself callsEvaluate(). Because the event loop has an_isProcessingre-entrancy guard, the middleEvaluate()is the active drainer; the deeperEvaluate()writes_completionValuebefore the middle frame reads it back — so the middleEvaluate()returns the deeper frame's value. (Surfaces only when the imported module has its ownimport, which shifts microtask timing.)Fix
Carry the completion value out as a per-frame local instead of a shared field — a local is immune to re-entrancy by construction:
ScriptEvaluationnow returns theJsValue(captured before the drain, returned after).Evaluate(in Prepared<Script>)andExecute(in Prepared<Script>)funnel through a sharedExecuteForCompletion(...)helper.ExecuteAsync(string,...)reads viaEvaluate(...)._completionValuefield is removed._error,_lastSyntaxElement) is documented; they are safe because they're produced and consumed synchronously with no re-entrant drain in between.No public API signatures change.
Tests
Two regression tests, both verified red on
main, green after:EngineTests.NestedEvaluateDuringContinuationDrainDoesNotClobberOuterResult— minimal deterministic repro (Promise.resolve().thenqueues a microtask drained by a nestedEvaluate()); no modules needed.ModuleTests.NestedEvaluateFromModuleExecutionDoesNotClobberCompletionValue— faithful repro of the issue (dynamicimport+import * as+ animportLibraryCLR callback).Full
Jint.Testspasses (3030); Test262 unchanged (99,206 passed; only the pre-existing timing-flakyAtomics.waitAsyncdiffers, confirmed flaky onmaintoo).Performance
The change removes a field rather than growing any hot struct (e.g.
ExecutionContext, which is full-copied on every push / scope change), so no regression is expected.MinimalScriptBenchmark(BenchmarkDotNet ShortRun, net10.0), base vs fix:Means are within the measurement error; allocations are flat-to-marginally-lower.
🤖 Generated with Claude Code