Backport #4165 to 4.x: Walk bound-function chains in IsConstructor and GetFunctionRealm instead of recursing - #4169
Merged
Conversation
…nstructor and GetFunctionRealm instead of recursing Backport of PR sebastienros#4165 (commit 8a1aad7) from main. BindFunction.IsConstructor asked BoundTargetFunction.IsConstructor, and GetFunctionRealm called itself for every [[BoundTargetFunction]] and [[ProxyTarget]] link - one native frame per link with no stack probe - so on net8.0/net10.0, where tier-0 code is not tail-called, a chain of ~16,000 (IsConstructor) or ~12,800 (GetFunctionRealm) links on a 1 MB thread ended the process through `new f()`, `Reflect.construct`, `class extends`, `super()`, `new Proxy(f, h)`, an array species constructor or a ShadowRealm call, before any probed [[Call]]/[[Construct]] ran. Neither question observes anything on the way, so both are loops now. The StackOverflowGuard probes sebastienros#4007 brought to 4.x sit in BindFunction's and JsProxy's [[Call]] and [[Construct]], not on these two walks, so an engine that opted into the guard was not protected either. Adapted for 4.x: - Jint/Native/Function/BindFunction.cs: main's walk, verbatim. The doc comment drops main's "(sebastienros#4130 made building one linear)", which is not true on 4.x. - Jint/Native/Function/Function.cs: the same loop, keeping 4.x's test order (Function, then BindFunction, then JsProxy). Main tests BindFunction first because sebastienros#3658 made BindFunction derive from Function there; on 4.x BindFunction and JsProxy both derive from ObjectInstance, so the order is free and main's "Step 2 before step 3" comment is replaced by one that says so. - Jint.Tests/Runtime/FunctionTests.cs: the three cases, [Test] -> [Fact]. TheRealmOfAChainOfBoundFunctionsAndProxiesIsItsInnermostTargets creates the other realm with engine._host.CreateRealm(), the pattern this file already uses, in place of main's Test262Object/$262.createRealm(), which Jint.Tests on 4.x does not have. - Jint.Tests.PublicInterface/BoundFunctionChainWalkTests.cs: new, [Test] -> [Fact]; routes, depths (200,000 bound links, 100,000 proxy+bind pairs), the 1 MB thread and the expected outcomes are main's. Each chain step also runs `delete f.name`: 4.x lacks sebastienros#4130, so bind writes "bound " + the target's name eagerly and an undeleted chain retains names quadratic in its depth (measured: 370 MB of managed heap at 8,000 links; the ~16,000 links a 1 MB thread needs is past a 1 GB heap). With no own name the next bind reads Function.prototype's empty one, so the build is linear (200,000 links in ~0.5 s, ~50 MB) and the chain is the same chain of BindFunctions. Main's test has no NETFRAMEWORK carve-out and none is needed: on .NET Framework x64 the JIT tail-calls both recursions, so the cases pass there before and after the fix. Evidence (-c Release, freshly built): - Unfixed 4.x, net10.0: each BoundFunctionChainWalkTests case, run alone, kills the test host - "Stack overflow." with BindFunction.get_IsConstructor() repeated 15,998 / 16,003 times (the guarded and unguarded bound-chain cases) and Function.GetFunctionRealm repeated 12,807 times (the mixed chain); under dotnet test that surfaces as "[FATAL ERROR] Xunit.Sdk.TestPipelineException" and no results. net472: 3/3 pass (tail-called). The three FunctionTests cases pass on both TFMs before the fix (72/72 FunctionTests): they pin that the walk kept the answer. - Unfixed 4.x is reachable without an expensive chain: with StackOverflowGuard on, a script that recurses until the guard's RangeError and, in the catch at that depth, evaluates `new Proxy(f, {})` over a plain 2,000-link bind chain (24 MB, built in ~0.25 s) ends the process (get_IsConstructor repeated ~1,800 times); a plain 20,000-link proxy chain (4 MB) as an array's constructor ends it in GetFunctionRealm (12,794 frames). With the fix both answer. - With the fix: BoundFunctionChainWalkTests 3/3 and FunctionTests 72/72 on net10.0 and net472. - Full solution dotnet test -c Release: 0 failures. Jint.Tests 7616/0/4 (net10.0), 7531/0/4 (net472); Jint.Tests.PublicInterface 1877/0/9 (net10.0), 1869/0/9 (net472), public-API Verify snapshots unchanged; Jint.Tests.CommonScripts 28/0/0 on both; Jint.Tests.SourceGenerators 52/0/0. (passed/failed/skipped) - test262 (4.x pin), from that same run: 102,501 passed / 0 failed / 183 skipped, identical to the 4.x control; no load flakes in this run. - JINT_HOST_CONTRACT_VERIFICATION=1: Jint.Tests 7616/0/4 (net10.0), 7531/0/4 (net472); Jint.Tests.PublicInterface 1881/0/5 (net10.0), 1873/0/5 (net472). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PanPJbBD7pQC9fRiTpHxvs
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.
Backport of #4165 (commit 8a1aad7) from main.
BindFunction.IsConstructoraskedBoundTargetFunction.IsConstructor, andGetFunctionRealmcalled itself for every[[BoundTargetFunction]]and[[ProxyTarget]]link. That is one native frame per link, with no stack probe between them. So on net8.0/net10.0, where tier-0 code is not tail-called, a chain of about 16,000 links (IsConstructor) or about 12,800 links (GetFunctionRealm) on a 1 MB thread ended the process. Any of these routes reached it:new f(),Reflect.construct,class extends,super(),new Proxy(f, h), an array species constructor, a ShadowRealm call. Each of them asks one of the two questions before any probed[[Call]]/[[Construct]]runs. Neither question observes anything on the way, so both are loops now.4.x has the
StackOverflowGuardprobes from #4007 inBindFunction's andJsProxy's[[Call]]/[[Construct]], but not on these two walks. So an engine that opted into the guard was not protected here either.Adapted for 4.x
Jint/Native/Function/BindFunction.cs: main's walk, unchanged. The doc comment drops main's "(Stop a chain of binds copying the whole bound name at every level #4130 made building one linear)" because that is not true on 4.x.Jint/Native/Function/Function.cs: the same loop, but it keeps 4.x's test order:Function, thenBindFunction, thenJsProxy. Main testsBindFunctionfirst because Function: a bound function is a Function, so everything that asks by type gets the right answer #3658 madeBindFunctionderive fromFunctionthere. On 4.x bothBindFunctionandJsProxyderive fromObjectInstance, so the order does not matter. Main's "Step 2 before step 3" comment is replaced by one that says so.Jint.Tests/Runtime/FunctionTests.cs: the three cases, with[Test]changed to[Fact]. The realm case creates the other realm withengine._host.CreateRealm(), the pattern this file already uses. Main usesTest262Object/$262.createRealm(), whichJint.Testson 4.x does not have.Jint.Tests.PublicInterface/BoundFunctionChainWalkTests.cs: new file, with[Test]changed to[Fact]. The routes, the depths (200,000 bound links; 100,000 proxy+bind pairs), the 1 MB thread and the expected outcomes are all main's. The one change: each chain step also runsdelete f.name.bindbuilds"bound " + nameeagerly. An undeleted chain keeps a name as long as its depth on every link, so memory grows quadratically. Measured: 370 MB of managed heap at 8,000 links. A 1 MB thread needs about 16,000 links, which is more than a 1 GB heap holds.bindreadsFunction.prototype's empty name. The build becomes linear (200,000 links in about 0.5 s, about 50 MB) and it is still the same chain ofBindFunctions.NETFRAMEWORKcarve-out, and none is needed: on .NET Framework x64 the JIT tail-calls both recursions, so the cases pass there before and after the fix.Evidence (
-c Release, freshly built)Unfixed 4.x, net10.0: each
BoundFunctionChainWalkTestscase, run alone, kills the test host withStack overflow.:BindFunction.get_IsConstructor()repeated 15,998 times;Function.GetFunctionRealmrepeated 12,807 times.Under
dotnet testthis shows up as[FATAL ERROR] Xunit.Sdk.TestPipelineExceptionand no results.Unfixed 4.x, net472: 3/3 pass, because the JIT tail-calls both recursions.
FunctionTests before the fix: the three new cases pass on both TFMs (72/72 FunctionTests). They pin that the walk kept the answer.
The crash needs no expensive chain on unfixed 4.x. Measured with a throwaway console on net10.0, a 1 MB thread and a 1 GB heap cap:
RangeError, then in thecatchat that depth evaluatesnew Proxy(f, {})over a 2,000-link chain (24 MB, built in about 0.25 s). The process ends, withget_IsConstructorrepeated about 1,800 times.constructorends the process inGetFunctionRealm(12,794 frames), with or without the guard.With the fix:
BoundFunctionChainWalkTests3/3 andFunctionTests72/72 on net10.0 and net472.Full solution
dotnet test -c Release: 0 failures. Results are passed/failed/skipped.The public-API Verify snapshots are unchanged.
test262 (4.x pin), from the same run: 102,501 passed / 0 failed / 183 skipped. This matches the 4.x control, with no load flakes in this run.
JINT_HOST_CONTRACT_VERIFICATION=1: Jint.Tests 7616/0/4 (net10.0) and 7531/0/4 (net472); Jint.Tests.PublicInterface 1881/0/5 (net10.0) and 1873/0/5 (net472).Not covered here (same on main)
A pure proxy chain of about 7,100 or more links still ends the process on net10.0 after this change. The crash moves to
JsProxy.Getforwarding: for example the@@speciesor"prototype"lookup that follows the realm walk.JsProxy's[[Get]]/[[GetOwnProperty]]forwarding has no probe on 4.x or on main; that is the #4076 work. Main's tests interleave binds so that no route forwards through a proxy chain, and this backport keeps that shape.🤖 Generated with Claude Code
https://claude.ai/code/session_01PanPJbBD7pQC9fRiTpHxvs