[r8] Retain field names for safe inherited JNI lookup

[simonrozsival]

Summary

This PR does not relax field-name preservation for either CoreCLR or NativeAOT. Alex's original review identified an inherited JNI lookup that could silently select a renamed subclass field. The NativeAOT exception was withdrawn, along with its runtime task input, target wiring, and rename claims.

The remaining R8 change replaces the obsolete JavaInterop1 rationale with the actual inherited-field restriction and strengthens existing preservation diagnostics. The user-requested Throwable CI cleanup is now ownership-aware following Alex's subsequent review: it preserves the actual exception/cause peer escaping failed initialization while releasing an unpublished failed wrapper's independent reference.

No GC bridge test or instrumentation changes are included. The unmatched GCBridgeRoundEmission failure remains a separate investigation; the automatically triggered CI run will provide another observation, not proof by itself that it is a flake.

Why field names still need preservation

The existing runtime-remapping rule is:

-keepclassmembernames class * { <fields>; }

The original comment cited experimental JavaInterop1 field accessors without registration metadata. JavaInterop1 was retired in #13030, but that does not make inherited-field renaming safe.

With public Java Derived extends Base, R8 can assign:

Base.b:int        -> a
Derived.value:int -> b

A handwritten derived accessor requesting b.I through its own Java.Interop.JniPeerMembers intends to access Base.b. Field remaps are indexed under the declaring owner, so the exact Derived.b:I remap misses. Instance and static lookup then try the original name on the derived class before consulting base remaps. That succeeds against residual Derived.b, formerly Derived.value, and the wrong field ID is cached even if the correct Base.b -> a entry survives.

Residual hierarchy names a and b do not collide; it is reuse of an ancestor's former name that creates the ambiguity. Earlier passing generated/handwritten/inherited examples did not cover this case and were insufficient proof of safe relaxation. CoreCLR additionally has the previously observed declaring-base registration retention limitation.

Field names therefore remain preserved for both runtimes. Safe future relaxation needs a separately scoped declaring-owner/member-identity solution; blindly prioritizing all base remaps would change legitimate Java shadowing semantics. The original bootstrap/raw-JNI protections and class/method policies remain unchanged.

Throwable CI failure and latest review correction

Build 1630732 failed the stage-4 runtime exception lookup cases because the partially constructed Throwable's own copied GREF was not released. The initial cleanup fixed that failure, but Alex's latest review identified a necessary ownership distinction:

• A Java RuntimeException subclass can succeed on the first printStackTrace(PrintWriter) call, then throw this on the second.
• The borrowed JavaException initialization performs the first extraction before registration. Throwable.SetHandle() then constructs/registers the managed wrapper before performing the second extraction.
• Pending-throwable conversion can return that exact managed wrapper. A different Java exception with this as its cause can similarly expose the wrapper through InnerException.
• Unconditionally disposing the failed wrapper also disposes the exception/cause that is being rethrown. Its PeerReference becomes invalid and it can no longer perform JNI calls or be raised back to Java.

The fix uses one outer initialization handler, replacing both the branch's inner cleanup and the broader unconditional cleanup integrated on current main. It walks the escaping exception's InnerException chain using managed reference identity, with cycle detection. If the actual managed wrapper is the exception or a cause, it remains alive for the caller; otherwise its independent constructor-owned peer is disposed. A different wrapper for the same native Java object is not ownership of this wrapper: the unpublished copy must still be released. There are no reflection or JNI calls in the guard. Cleanup errors are logged without replacing the original initialization exception.

The existing transferred-input finally is unchanged. The integrated JNIEnv.ToJniObjectReferenceOptions(transfer) behavior from #13052, including DoNotRegister, is preserved.

Current main snapshot dc31a58cf94c5a1fc82f69a94d958ce74786bdc3 was merged in feda74258549a9edcc6f3846aa932ca7bc6719a5 (other parent: reviewed head b5559148722b4474c7b559e885c2f7f6f956de96) so local validation exercises the actual outer cleanup and registration contract, not the older copy-only implementation. No sibling branch was cherry-picked and no dependency/framework pin was changed.

Fixes #13066.

Focused regression coverage

The existing Java failure fixture gains only two second-extraction modes: throw itself, or throw an independent exception whose cause is itself. One parameterized test in the existing ownership suite exercises eight combinations: those two modes with borrowed input, transferred local input, transferred global input, and borrowed DoNotRegister input.

The test verifies:

• the escaping peer is valid and represents the original Java object;
• registry lookup returns that same managed peer;
• JNI access through Class remains usable;
• raising the peer back to Java and converting it again preserves managed identity;
• exactly one source GREF remains while the escaping peer is live, including the unregistered-alias case;
• the existing ownership observer verifies exact-once input release, validity of borrowed input, and no owned copies after caller disposal.

This covers self/cause escapes, reentrant registration, repeated extraction, and independently owned aliases without adding a new fixture class or changing runtime dispatch. Existing independent-exception leak cases and successful/registration ownership tests remain in the same run.

Validation

All builds and tests use this branch's rebuilt SDK and explicit API 36 arm64 emulator emulator-5554; the attached physical phone was not touched. Current product/test TFMs and SDK pins are unchanged.

Executed negative reproduction

With the main-integrated unconditional cleanup, before applying the identity guard:

• ./dotnet-local.sh build Microsoft.Android.slnx -c Debug -v quiet -m:2 -bl:bin/BuildDebug/throwable-escape-baseline-sdk-restored.binlog: passed, 30 existing warnings, 0 errors. A preceding --no-restore attempt failed on missing restored ELFSharp references after main moved tasks into a different project; normal restore resolved it without source changes.
• ANDROID_SERIAL=emulator-5554 ./dotnet-local.sh build -t:Install -c Debug tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj -p:PublishAot=false -p:RuntimeIdentifier=android-arm64 '-p:AdbTarget=-s emulator-5554' -p:IncludeCategories=TransferredReferences -m:2 -v quiet -bl:bin/BuildDebug/throwable-escape-baseline-install.binlog: passed.
• From tests/Mono.Android-Tests/Mono.Android-Tests: ANDROID_SERIAL=emulator-5554 ../../../dotnet-local.sh test Mono.Android.NET-Tests.csproj --no-build -c Debug -p:PublishAot=false -p:RuntimeIdentifier=android-arm64 '-p:AdbTarget=-s emulator-5554' -p:IncludeCategories=TransferredReferences --device emulator-5554 --report-trx --results-directory ../../../bin/TestDebug/TestResults/ThrowableEscapeBaseline: 68 existing cases passed; all 8 escape cases failed at Constructor cleanup must not dispose an escaping peer.

This is now an executed reproduction of the review finding, not only a source-level counterexample.

Corrected implementation

• ./dotnet-local.sh build Microsoft.Android.slnx --no-restore -c Debug -v quiet -m:2 -bl:bin/BuildDebug/throwable-escape-fixed-sdk.binlog: passed, 29 existing warnings, 0 errors.

Final build/install commands, each with ANDROID_SERIAL=emulator-5554:

• ./dotnet-local.sh build -t:Install -c Debug tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj -p:PublishAot=false -p:RuntimeIdentifier=android-arm64 '-p:AdbTarget=-s emulator-5554' -p:IncludeCategories=TransferredReferences -m:2 -v quiet -bl:bin/BuildDebug/throwable-escape-final-CoreClrDebug-install.binlog: passed.
• ./dotnet-local.sh build -t:Install -c Release tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj -p:PublishAot=false -p:RuntimeIdentifier=android-arm64 '-p:AdbTarget=-s emulator-5554' -p:IncludeCategories=TransferredReferences -m:2 -v quiet -bl:bin/BuildDebug/throwable-escape-final-CoreClrRelease-install.binlog: passed with existing trim warnings, 0 errors.
• ./dotnet-local.sh build -t:Install -c Release tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj -p:PublishAot=true -p:RuntimeIdentifier=android-arm64 '-p:AdbTarget=-s emulator-5554' -p:IncludeCategories=TransferredReferences -m:2 -v quiet -bl:bin/BuildDebug/throwable-escape-final-NativeAotRelease-install.binlog: passed with existing AOT/trim warnings, 0 errors.

Final tests run from tests/Mono.Android-Tests/Mono.Android-Tests, each with ANDROID_SERIAL=emulator-5554:

• ../../../dotnet-local.sh test Mono.Android.NET-Tests.csproj --no-build -c Debug -p:PublishAot=false -p:RuntimeIdentifier=android-arm64 '-p:AdbTarget=-s emulator-5554' -p:IncludeCategories=TransferredReferences --device emulator-5554 --report-trx --results-directory ../../../bin/TestDebug/TestResults/ThrowableEscapeFinalCoreClrDebug: 76 passed, 0 skipped/failed.
• ../../../dotnet-local.sh test Mono.Android.NET-Tests.csproj --no-build -c Debug -p:PublishAot=false -p:RuntimeIdentifier=android-arm64 '-p:AdbTarget=-s emulator-5554' -p:IncludeCategories=TransferredReferences --device emulator-5554 --report-trx --results-directory ../../../bin/TestDebug/TestResults/ThrowableEscapeFinalCoreClrDebugRepeat: 76 passed, 0 skipped/failed.
• ../../../dotnet-local.sh test Mono.Android.NET-Tests.csproj --no-build -c Release -p:PublishAot=false -p:RuntimeIdentifier=android-arm64 '-p:AdbTarget=-s emulator-5554' -p:IncludeCategories=TransferredReferences --device emulator-5554 --report-trx --results-directory ../../../bin/TestDebug/TestResults/ThrowableEscapeFinalCoreClrRelease: 76 passed, 0 skipped/failed.
• ../../../dotnet-local.sh test Mono.Android.NET-Tests.csproj --no-build -c Release -p:PublishAot=false -p:RuntimeIdentifier=android-arm64 '-p:AdbTarget=-s emulator-5554' -p:IncludeCategories=TransferredReferences --device emulator-5554 --report-trx --results-directory ../../../bin/TestDebug/TestResults/ThrowableEscapeFinalCoreClrReleaseRepeat: 76 passed, 0 skipped/failed.
• ../../../dotnet-local.sh test Mono.Android.NET-Tests.csproj --no-build -c Release -p:PublishAot=true -p:RuntimeIdentifier=android-arm64 '-p:AdbTarget=-s emulator-5554' -p:IncludeCategories=TransferredReferences --device emulator-5554 --report-trx --results-directory ../../../bin/TestDebug/TestResults/ThrowableEscapeFinalNativeAotRelease: 76 passed, 0 skipped/failed.
• ../../../dotnet-local.sh test Mono.Android.NET-Tests.csproj --no-build -c Release -p:PublishAot=true -p:RuntimeIdentifier=android-arm64 '-p:AdbTarget=-s emulator-5554' -p:IncludeCategories=TransferredReferences --device emulator-5554 --report-trx --results-directory ../../../bin/TestDebug/TestResults/ThrowableEscapeFinalNativeAotReleaseRepeat: 76 passed, 0 skipped/failed.

All six final TRX files verify the eight escape variants and the stronger single-owner GREF assertion: 456 successful case executions. During fixture development, the raw ExceptionOccurred() result initially caused a local-reference accounting assertion; the test now registers that result with the existing public References.CreatedReference() helper before ownership transfer. The final repeated runs pass without suppressing diagnostics or changing runtime accounting.

The earlier conservative-field revision also passed its SDK rebuild, 144 R8/JNI host tests (one optional real-ILLink skip), and both existing CoreCLR/NativeAOT runtime-remapping device cases. Those results predate this latest main merge/ownership correction; the 76-case ownership results above are the current validation. Full host/device suites were not run. Test apps were uninstalled and all shared validation leases released before commit/push. New CI is expected to run automatically; no manual retry or GC event test change was made.

---

Pull Request
title and
description
should follow the
commit-messages.md workflow documentation, and in particular should include:

• Useful description of why the change is necessary.
• Links to issues fixed
• Unit tests

[Copilot]

🟢 Approval recommended

The runtime gate is conservative and covered by focused host and device tests; only a minor test-diagnostic improvement remains.

1 open finding

• Add a descriptive failure message to the policy assertion · New

What changed in this PR

Enables R8 field-name remapping for NativeAOT while preserving CoreCLR compatibility.

Changes:

• Passes the selected runtime into R8 policy generation.
• Retains the NativeAOT bootstrap field explicitly.
• Updates host/device tests and documentation.

File	Description
R8RuntimeRemappingTests.cs	Verifies runtime-specific field renaming.
Xamarin.Android.D8.targets	Passes the runtime to R8.
R8Tests.cs	Tests generated keep rules.
R8.cs	Implements runtime-specific field policy.
build-properties.md	Documents behavior and limitations.

🧠 Review effort: Balanced

[simonrozsival]

@dalexsoto review

[dalexsoto]

The complete five-file correctness and integration review leaves one NativeAOT correctness blocker. The conservative CoreCLR gate, optional-input fallback, bootstrap field keep and intentional existing-fixture scope are accepted; this does not ask to redesign CoreCLR retention.

Inherited member access can silently select a different renamed field (R8.cs:285-287). Consider public Java Derived extends Base, with a live base JNI class literal (for example through its generated constructor), and this supported R8 assignment:

Base.b:int        -> a
Derived.value:int -> b

A handwritten derived accessor requesting "b.I" through its own JniPeerMembers intends to read/write inherited Base.b. The generated mapping is indexed under the declaring owner, so the exact Derived.b:I remap misses. Instance lookup then returns the original-name JNI field before consulting base remaps. That lookup succeeds against Derived's residual b, formerly value, and the wrong field ID is cached. Static field lookup has the same order. Reads and writes therefore affect the wrong field rather than throwing.

This still occurs when NativeAOT retained the correct Base.b -> a entry: a live generated constructor references base peer members, and the inherited accessor supplies b/I literals for declaring-base field selection. It is independent of the accepted CoreCLR metadata omission, and uses the supported member-remapping path rather than raw JNI or reflection.

Assignment reachability was checked against the exact shipped R8 9.4.28 source, revision c753a1b8c6a6e612d2779cfa3b9d662df97403da, recovered from the published jar whose SHA-1 36cc798105d4d91dd95bd5f0c514216d052addbc matches Maven. Its field strategy reserves only fixed/non-minifiable original names, so a renameable original b is not reserved. Top-down naming clones the parent state, and the counter starts at 1, yielding a then b for ordinary one-field Base/Derived classes. These are distinct residual names; hierarchy collision protection does not prevent reuse of an ancestor's former name.

Keep field-name preservation for the affected path, or resolve the requested declaring-owner/member identity without accepting a residual subclass field as an original-name match. Preserve legitimate unchanged/shadowing semantics: unconditionally moving every inherited remap before original lookup is not sufficient.

The separate completeness pass and final independent search found no additional high-confidence blocker. This is an exact-head static source counterexample, not an executed reproduction or a finding based on pending CI.

[simonrozsival]

@dalexsoto Addressed in b555914 by withdrawing the NativeAOT exception and preserving field names for both runtimes. The runtime task input/wiring and rename documentation/assertions are removed. The comment now describes the original-name/subclass collision; I did not reorder base remaps ahead of legitimate original/shadowing lookup.

Your Base.b -> a, Derived.value -> b example is independent of retention: the derived original-name lookup can return the wrong residual field even with the correct base remap retained. My earlier five-field successful runs did not cover that case and were insufficient proof of safe relaxation. The PR title/body now explicitly say field-name relaxation is no longer implemented, include your example, and identify declaring-owner/member-identity resolution as separately scoped work.

Validation against the rebuilt SDK: R8/JNI host checks 144 passed, 1 optional ILLink skip; existing CoreCLR/NativeAOT runtime fixture 2 passed, with field preservation and initial/changed-method-rule/disabled app runs. The prior validated Throwable CI cleanup remains unchanged. No new fixture or runtime/scanner redesign was added.

[dalexsoto]

The inherited-field blocker is fixed: withdrawing the NativeAOT exception and preserving field names for both runtimes addresses the original-before-base lookup collision without changing legitimate shadowing semantics.

[P2] Preserve the Java exception escaping failed initialization (src/Mono.Android/Java.Lang/Throwable.cs:120-123). The new unconditional Dispose() assumes the partially constructed peer cannot escape, but pending-throwable conversion can return that exact registered managed exception.

A source-level counterexample is a Java RuntimeException subclass whose printStackTrace(PrintWriter) succeeds on its first invocation and throw this on its second, wrapped directly with new Java.Lang.Throwable(handle, JniHandleOwnership.DoNotTransfer). The borrowed JavaException constructor performs the first extraction before registration; SetHandle() then constructs/registers the wrapper before the second extraction. AndroidExceptionManager.GetExceptionForThrowable() returns that registered exception, and the generated CallVoidMethod wrapper rethrows it through ExceptionDispatchInfo without cloning it. This catch consequently disposes the same object it rethrows, leaving the escaping exception with an invalid PeerReference. JNI operations fail as disposed, and raising it back to Java reaches JniEnvironment.Exceptions.Throw() with an invalid reference and throws ArgumentException. Throwing a different exception with this as its cause similarly leaves an invalidated peer in InnerException.

Please preserve the escaping throwable/causal-chain peers while releasing only exclusively constructor-owned references, retaining the input-transfer finally. The current stage-4 fixture throws an independent IllegalStateException, so its successful cleanup cases do not cover this identity/ownership case. Apply the protection consistently to the broader outer cleanup already present in the actual CI integration base as well.

This counterexample is source-proven, not an executed reproduction. The current build's separate GCBridgeRoundEmission failure remains unmatched; I am not attributing it to this finding.

[simonrozsival]

@dalexsoto Addressed the latest escaping-exception finding in 5193b62, after merging current main snapshot dc31a58 in feda742 to preserve the integrated ToJniObjectReferenceOptions(transfer)/DoNotRegister contract and broader outer cleanup.

The nested unconditional handlers are replaced with one outer guard. If the actual managed wrapper is the escaping exception or appears in its InnerException chain, cleanup leaves it alive and rethrows the original failure. The traversal uses reference identity with cycle detection, without reflection/JNI calls. A separate wrapper for the same Java object does not own this wrapper’s GREF, so an unpublished DoNotRegister copy is still disposed. The original input-transfer finally remains unchanged; disposal errors are reported without substituting a cleanup exception for the original failure.

Extended the existing Java failure fixture with second-extraction throw this and throw new IllegalStateException(..., this) modes. One parameterized test checks both modes with borrowed/local/global input and an unregistered-alias variant: native identity/liveness, registry identity, usable JNI Class, Java rethrow round-trip, and exactly one source GREF while the exception is live. The existing observer still checks input ownership and no remaining copied references after caller disposal.

Executed baseline against unconditional cleanup: 68 existing cases passed, all 8 escape cases failed because the escaping peer was disposed. Corrected final suite: 76/76 passed twice in CoreCLR Debug, CoreCLR Release, and NativeAOT Release (456 successful case executions), including the original independent-failure leak tests and main’s registration/reentrancy tests. PR description contains the exact commands and distinguishes these results from older copy-only validation.

GCBridgeRoundEmission and bridge instrumentation are unchanged. The automatic CI run will provide another data point for its separate unmatched failure; no manual retry or flake classification is being assumed.

[dalexsoto]

Both prior correctness concerns are resolved on this main-integrated revision. Field names remain preserved for CoreCLR and NativeAOT, without changing legitimate inherited/shadowing lookup order. The single outer managed-identity/cycle guard preserves an escaping exception/cause peer while releasing an independently owned unpublished alias; the current DoNotRegister/copy contract and exact-once incoming-reference finally remain intact. The targeted existing fixture/suite assertions cover self/cause escape, JNI/registry/rethrow identity and exactly one source GREF while live.

The fresh primary and independent specialist local, end-to-end integration and completeness passes found no remaining high-confidence blocker in the complete six-file scope. The separately scoped declaring-owner/scanner redesign and unchanged GC bridge investigation remain deferred.

The completed CI build is not green: its APKs2 failure is a prerequisite Gradle build unable to provision Android API 35 because SDK manifest retrieval fails DNS resolution for dl.google.com, before the JCW leak tests start; the existing nested binlog provides the failure evidence. The other 40 checks passed, with the aggregate failure representing this same failed leg. This approval is the code-review verdict, not a flake, CI-green or retry claim.