Skip to content

Exceptions: JavaScriptException.Location returns by value, so a net472 failure reports instead of crashing the host - #3564

Merged
lahma merged 1 commit into
sebastienros:mainfrom
lahma:fix/3549-netfx-byref-location-reflection
Sep 1, 2026
Merged

Exceptions: JavaScriptException.Location returns by value, so a net472 failure reports instead of crashing the host#3564
lahma merged 1 commit into
sebastienros:mainfrom
lahma:fix/3549-netfx-byref-location-reflection

Conversation

@lahma

@lahma lahma commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Closes #3549.

On net472, a test that fails by letting a JavaScriptException escape does not report a failure — it takes
the test host down, and every test still queued in that process goes with it.

Repro, before the change

Four [TestCase] rows that let a JavaScriptException out of the body, run on net472:

Failed ASignedDecimalStringIsTheBigIntItNames("BigInt('+12')","12") [230 ms]
Error Message:
 System.NotSupportedException : ByRef return value not supported in reflection invocation.
Stack Trace:
   at System.Reflection.RuntimePropertyInfo.GetValue(Object obj, Object[] index)
   at NUnit.Framework.Internal.ExceptionHelper.AppendExceptionProperties(Exception ex, StringBuilder sb)
   at NUnit.Framework.Internal.ExceptionHelper.BuildMessage(Exception exception, Boolean excludeExceptionNames)
   at NUnit.Framework.Internal.TestResult.RecordException(Exception ex, FailureSite site)
   ...
The active test run was aborted. Reason: Test host process crashed

Failed!  - Failed: 1, Passed: 1, Skipped: 0, Total: 2, ...
Test Run Aborted.

Two of five cases reported, three vanished, and dotnet test itself never returned — the first attempt was
killed at a ten-minute timeout, which is what a red net472 run looks like from the outside: a hang, not a
failure. It cost an earlier session about forty minutes.

Same four rows, after:

Failed!  - Failed: 4, Passed: 1, Skipped: 0, Total: 5, Duration: 214 ms

with the message a reader actually wants:

Jint.Runtime.JavaScriptException : Unexpected string (<anonymous>:1:35)
  JavaScriptStackTrace:
  Location: [1,34): <anonymous>
  Error: SyntaxError: Unexpected string (<anonymous>:1:35)

Why it happens, and where the fix belongs

JavaScriptException.Location has been ref readonly SourceLocation since #1270. Nothing that calls it by
name
notices. But a public property of an exception is read reflectively by everything that renders a
failed run — a test runner, a structured logger, an error page — and .NET Framework's
RuntimePropertyInfo.GetValue answers a by-ref-returning property with
NotSupportedException: ByRef return value not supported in reflection invocation instead of dereferencing
it the way .NET Core does. That is why only the net472 leg dies while net8.0 and net10.0 report
normally.

NUnit's ExceptionHelper.AppendExceptionProperties does guard the getter — with
catch (TargetInvocationException). This exception is raised by GetValue before any invocation, so it
is not a TargetInvocationException and it escapes; the worker thread unwinds and the run aborts. NUnit
should not be crashable by a getter's shape, whatever the library, and that half is filed upstream at
nunit/nunit#5401 — but waiting for it would leave every Jint
embedder on net472 with the same defect in their own logs, so the fix is Jint's.

The change

// before
public ref readonly SourceLocation Location => ref _jsErrorException.Location;

// after
public SourceLocation Location => _jsErrorException.Location;

SourceLocation is 24 bytes and the path is a thrown exception, so the reference bought nothing anything can
measure. Reading the location is unchanged — ex.Location, ex.Location.Start.Line and
ex.Location != default all keep compiling; only ref readonly var l = ref ex.Location stops, which is
docs/v5-migration.md §3.18.

ExceptionHelper also walks InnerException, so the private nested JavaScriptErrorWrapperException is
enumerated too. Its Location keeps the by-ref accessor and turns internal, which is all it takes to drop
out of BindingFlags.Public.

Location gained the <summary> it never had, so UndocumentedPublicApi.txt shrinks by one.

Public API impact

One line in each of the five baselines, and nothing else:

-        public Acornima.SourceLocation& Location { get; }
+        public Acornima.SourceLocation Location { get; }

What holds it

Jint.Tests.PublicInterface/ExceptionPropertyReflectionTests.cs, two tests:

  • structural — no Exception in the assembly, public or not, exposes a by-ref-returning public property.
    It fails on net8.0 and net10.0 as well, so a future one is caught on every leg rather than only on the
    one that would crash. Against unfixed code it names Jint.Runtime.JavaScriptException.Location.
  • behavioural — replays what a renderer does (enumerate public instance properties of the exception and
    of every inner exception, read each one) over a real thrown JavaScriptException, and asserts the location
    survives the by-value read rather than merely not throwing. Against unfixed code it fails on net472 with
    the issue's exact NotSupportedException.

The four remaining by-ref public properties are on non-exception types — Completion, CallFrame,
DebugInformation, ExceptionThrownEventArgs — and stay as they are; Completion.Location is genuinely
hot. The rule the test enforces is scoped to exceptions for that reason, and Jint/AGENTS.md says so.

Verification

  • dotnet build -c Release: clean.
  • Jint.Tests: 11,601 passed on net10.0 and net8.0, 8,221 on net472, 0 failed.
  • Jint.Tests.PublicInterface: 3,352 / 3,342 / 2,719 passed on net10.0 / net8.0 / net472, 0 failed.
  • Jint.Tests.CommonScripts (28 × 2) and Jint.Tests.SourceGenerators (71): 0 failed.
  • test262: 102,537 passed / 0 failed / 151 skipped of 102,688 — the control, unmoved.

…2 failure reports instead of crashing the host

`JavaScriptException.Location` has been `ref readonly` since sebastienros#1270. Nothing calls it by name in a way that
notices, but a public property of an exception is read *reflectively* by everything that renders a failed
run - a test runner, a structured logger, an error page - and .NET Framework answers
`PropertyInfo.GetValue` on a by-ref-returning property with
`NotSupportedException: ByRef return value not supported in reflection invocation` rather than
dereferencing it the way .NET Core does.

Under NUnit that is not a degraded message, it is a lost run. `ExceptionHelper.AppendExceptionProperties`
guards the getter with `catch (TargetInvocationException)`, and this exception is raised by `GetValue`
itself before any invocation, so it escapes: the failure message names the reflection limitation instead
of the JavaScript error, the worker thread dies, and the run ends with
"The active test run was aborted. Reason: Test host process crashed" - every test still queued behind the
red one gone with it. Reproduced on this tree before the change with four failing test cases: 2 of 5
reported, host crashed, and `dotnet test` never returned. After: 4 failed, 1 passed, 5 of 5, exit 1.

`Location` therefore returns the value. Reading it is unchanged (`ex.Location.Start.Line`,
`ex.Location != default`); only `ref readonly var l = ref ex.Location` stops compiling, which is
`docs/v5-migration.md` 3.18. The private inner exception a renderer reaches through `InnerException` keeps
its by-ref accessor and turns it `internal`, which is all it takes to hide it from `BindingFlags.Public`.

`Jint.Tests.PublicInterface/ExceptionPropertyReflectionTests.cs` holds every `Exception` in the assembly
to the rule structurally - so a future one fails on net8.0 and net10.0 too, not only on the leg that would
crash - and replays what a renderer actually does over a real thrown `JavaScriptException`.

The four remaining by-ref public properties are on non-exception types (`Completion`, `CallFrame`,
`DebugInformation`, `ExceptionThrownEventArgs`) and stay as they are; `Completion.Location` in particular
is genuinely hot.

NUnit's half of it is reported upstream: a renderer should not be able to take the host down over a getter
shape, whatever the library.

Closes sebastienros#3549

Claude-Session: https://claude.ai/code/session_014W5mbjGhyvgAS4pivXoc4S

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@lahma
lahma merged commit 55a45ce into sebastienros:main Sep 1, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A failing test on net472 crashes the test host, because JavaScriptException.Location is ref readonly

1 participant