Skip to content

[Java.Interop] Release native peer control blocks on finalization - #13016

Merged
simonrozsival merged 7 commits into
mainfrom
simonrozsival-peer-finalization-lifetime
Oct 9, 2026
Merged

simonrozsival merged 7 commits into
mainfrom
simonrozsival-peer-finalization-lifetime

Conversation

@simonrozsival

@simonrozsival simonrozsival commented Oct 6, 2026 •

Copy link
Copy Markdown
Member

Refs #13010, finding 1. Independent PR against main; this does not close the multi-finding tracker.

FinalizePeer() clears the JNI reference before calling Finalized(). JavaObject and JavaException previously freed their native control block only when clearing a reference after the internal disposed state had already been set. Actual finalization therefore retained the native allocation; initially invalid peers could even allocate a block during clearing and retain it.

Release the cleared control block in a finally after Dispose(false) in both peer implementations, using the shared FreeIfInvalid helper. Preserve value-manager/context detachment ordering, the invalid-reference callback contract, and explicit-disposal behavior. Retain valid references reconstructed by derived callbacks, including when the callback throws and while reference-tracking contexts borrow the block. No native layout, bridge queue, or production synchronization changes.

Add 26 Android lifetime cases covering global/local/invalid/no-block finalization, actual finalizers, throwing constructors/callbacks, managed resurrection, valid reconstruction/registration, and explicit disposal followed by finalization. Four additional global-reference deletion cases run under the existing dedicated, warmed-up JniReferenceLeak category. Ordinary-suite native-pointer/callback checks do not depend on VM-wide GREF totals. Document native-block ownership separately from JNI-reference deletion.

Before/after evidence: Before changing production code, 18 of the original 22 host cases failed with nonzero control-block pointers after finalization. The original 22 passed after the fix; expanded coverage subsequently passed on host, CoreCLR, and NativeAOT. The initial focused host GREF assertions passed, but VM-wide deltas are not per-reference ownership evidence in an ordinary shared suite. This proves a native allocation leak, not the cause of intermittent GREF-count increases.

Review corrections

  • dd047b07af: centralize invalid-block cleanup without changing behavior.
  • 9596f9c6af: address Alex's first blocker by moving both exact VM-total assertions out of ordinary lifetime tests. Azure build 1624644 had three ExplicitDispose(False) assertion failures (235 expected/245 actual, 193/246, 193/245), verified against all 87 run summaries, failed results, and both stage-attempt logs. This was a PR test regression, not attribution of the VM-wide increases. Collection precedes cache warm-up in the dedicated category.
  • d25b714447 merges current main (53c12fa441); 909b509e8d addresses Alex's second blocker. Main's [Java.Interop] Remove reflection-backed JNI managers #13018 removed JavaVMFixture, reflection-backed JNI managers, and the standalone host-JVM test project. The actual CI merge in 1625456 consequently failed with CS0246 in Debug, Release, NoAab, NativeAOT, and JniReferenceLeaks before execution. Adopt the supported no-base Android fixture convention and remove the obsolete host-manager conditional. Do not restore the retired harness.
  • 094ed915a1: address the local review suggestion by combining valid reconstruction with a throwing finalization callback for both peer types. Assert exception propagation, exactly one callback, retained JNI identity/native block/registry entry, then explicit disposal invalidates the reference, frees the block, and removes registration. Production code and callback/context ordering are unchanged. All current GitHub feedback was reread; there are no new unresolved inline threads or actionable comments beyond the already addressed blockers.

Validation

All commands ran in this isolated macOS arm64 worktree using its source-built SDK and owned API29 arm64 emulator. ROOT=$PWD; OUT denotes the session artifact directory containing build logs and TRX files. No other checkout's output was reused; no physical device or sibling emulator was modified.

Source SDK preparation and builds:

export RuntimeFrameworkVersion=10.0.0
export AndroidToolchainDirectory="$ROOT/bin/android-toolchain"
export ANDROID_SDK_ROOT= ANDROID_HOME=
export PATH="$ROOT/bin/Release/dotnet:$PATH"
export DOTNET_ROLL_FORWARD=Major
make prepare CONFIGURATION=Release
./dotnet-local.sh build Microsoft.Android.slnx -c Release \
  -t:BuildExtraApiLevels -m:4 -nr:false -v:minimal
./dotnet-local.sh build build-tools/create-packs/Microsoft.Android.Sdk.proj \
  -c Release -t:ConfigureLocalWorkload -m:4 -nr:false -v:minimal
make all CONFIGURATION=Release MSBUILD_ARGS='-m:4 -nr:false'

Initial prepare passed with the validation-only environment override; without it, restore failed for unavailable Microsoft.NETCore.App.Ref 10.0.13. Extra API reference builds passed with 78 existing warnings/0 errors and resolved missing workload configuration inputs; workload configuration passed. No product/package pins changed. make all passed initially and after each review correction, including the current-main merge and 094ed915a1 test update. Latest source SDK build passed. Provisioned SDK/runtime is 12.0 alpha and Android target is net11.0-android, per repository configuration.

Latest device validation — 094ed915a1: Check live ports before boot; use only the worktree-owned copilot-peer-control-block AVD on emulator-5586, with private adb5057 restricted via --one-device emulator-5586 to avoid claiming physical USB devices. Current-main core JNI tests run on Android; standalone host-JVM validation is no longer supported.

unset RuntimeFrameworkVersion
export AndroidToolchainDirectory="$ROOT/bin/android-toolchain"
export ANDROID_HOME="$ROOT/bin/android-toolchain/sdk"
export ANDROID_USER_HOME="$ROOT/bin/android-user"
export ANDROID_ADB_SERVER_PORT=5057 DOTNET_ROLL_FORWARD=Major

# Run once with AOT=false, then with AOT=true.
./dotnet-local.sh build -t:Clean,Install -c Release \
  tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj \
  -p:AndroidTypeMapImplementation=trimmable \
  -p:IncludeCategories=PeerControlBlock -p:RuntimeIdentifier=android-arm64 \
  -p:PublishAot="$AOT" -p:AdbTarget='-s emulator-5586' \
  -m:4 -nr:false -v:minimal
(cd tests/Mono.Android-Tests/Mono.Android-Tests && \
  ../../../dotnet-local.sh test Mono.Android.NET-Tests.csproj --no-build \
  -c Release -p:AndroidTypeMapImplementation=trimmable \
  -p:IncludeCategories=PeerControlBlock -p:RuntimeIdentifier=android-arm64 \
  -p:PublishAot="$AOT" -p:AdbTarget='-s emulator-5586' \
  --device emulator-5586 --report-trx --results-directory "$OUT/$RUN")

Both clean build/install commands passed: CoreCLR 38 existing warnings/0 errors; NativeAOT 11 existing warnings/0 errors. CoreCLR 26/26 passed; NativeAOT 26/26 passed, with zero failures/skips and actual selected counts verified. Includes both new throwing-reconstruction cases. Artifacts: review-reattach-coreclr-results and review-reattach-nativeaot-results. Diff checks passed; owned emulator/private adb stopped afterward.

Previous current-main validation — 909b509e8d: Same device command pair, with IncludeCategories="$CATEGORY" on both commands, run for all four pairs: AOT=false CATEGORY=PeerControlBlock, AOT=false CATEGORY=JniReferenceLeak, AOT=true CATEGORY=PeerControlBlock, AOT=true CATEGORY=JniReferenceLeak. All four clean build/install commands passed. CoreCLR lifetime 24/24, CoreCLR isolated 7/7, NativeAOT lifetime 24/24, NativeAOT isolated 7/7. Isolated selections include all four new deletion cases plus three existing cases; not rerun for the latest test-only addition, which changes neither isolated selection nor production code.

Full ordinary CoreCLR suite at 909b509e8d: same clean build/install and test pair with PublishAot=false and without IncludeCategories, results directory $OUT/alex-fixture-coreclr-shared-results. Build/install passed; 893 passed, 7 existing skipped, 0 failed (900 total). This result predates the two new test cases and is not claimed as a latest-head full-suite run.

Historical host evidence, before main retired the harness:

DOTNET_ROLL_FORWARD=Major ./dotnet-local.sh build \
  external/Java.Interop/tests/Java.Interop-Tests/Java.Interop-Tests.csproj \
  -c Release -p:RuntimeFrameworkVersion=10.0.0 \
  -p:UtilityOutputFullPath="$ROOT/external/Java.Interop/bin/HostValidationTools/" \
  -p:JavaCPath="$ROOT/bin/android-toolchain/jdk-25/bin/javac" \
  -p:JarPath="$ROOT/bin/android-toolchain/jdk-25/bin/jar" \
  -p:DotnetToolPath="$ROOT/bin/Release/dotnet/dotnet" \
  -m:4 -nr:false -v:minimal
DOTNET_ROLL_FORWARD=Major ./dotnet-local.sh test \
  external/Java.Interop/bin/TestRelease-net10.0/Java.Interop-Tests.dll \
  --filter 'FullyQualifiedName~JavaPeerControlBlockTests|FullyQualifiedName~JavaObjectTest|FullyQualifiedName~JniValueManagerTests'
DOTNET_ROLL_FORWARD=Major ./dotnet-local.sh test \
  external/Java.Interop/bin/TestRelease-net10.0/Java.Interop-Tests.dll \
  --filter 'TestCategory!=JniReferenceLeak'
DOTNET_ROLL_FORWARD=Major ./dotnet-local.sh test \
  external/Java.Interop/bin/TestRelease-net10.0/Java.Interop-Tests.dll \
  --filter 'TestCategory=JniReferenceLeak'
DOTNET_ROLL_FORWARD=Major ./dotnet-local.sh test \
  external/Java.Interop/bin/TestRelease-net10.0/Java.Interop-Tests.dll

Historical results: build passed; original focused selection 37 passed/1 existing ignored; after first review correction ordinary host 757 passed/5 skipped, isolated host 7/7, default unfiltered host 764 passed/5 skipped. These are evidence for earlier commits, not current-main host coverage. Earlier device lifetime selections passed 24/24 on both runtimes; full ordinary CoreCLR passed 835/8 skipped and isolated 7/7 on both. Intermediate NativeAOT category switches exposed stale incremental APK selection; clean packaging is now used.

Limits

Other ABIs/API levels and fresh Debug/NoAab device runs were not exercised locally. An earlier additional combined PeerControlBlock%2CGCBridge run passed 28 cases/1 existing ignored on CoreCLR, including the rooted managed-peer cycle; NativeAOT selected only 24 lifetime cases, so the extra NativeAOT cycle remains unselected and is not claimed. No intermittent production GREF root-cause attribution is made. New-head Azure CI has not been verified green; no manual retries, merge, or approval performed.


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

Preserve the value-manager callback ordering and release cleared JavaObject and JavaException control blocks after Dispose(false), including exceptional callbacks. Keep references reconstructed by derived callbacks alive. Cover normal, invalid, partial-construction, resurrection, and explicit-disposal lifetimes separately from JNI reference counts.

Refs #13010

Co-authored-by: Copilot App <[email protected]>
Copilot AI balanced review requested due to automatic review settings October 6, 2026 09:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

It changes unsafe unmanaged-memory ownership in a core finalization path shared by all Java.Interop consumers.

Review effort: Balanced
Findings: 1 Low severity

Open (1)
What changed in this PR

Fixes native peer control-block leaks during JavaObject and JavaException finalization while preserving reconstructed references.

Changes:

  • Releases invalid control blocks after Dispose(false), including exception paths.
  • Documents control-block versus JNI-reference ownership.
  • Adds 24 host/Android lifecycle regression cases.
File Description
JavaObject.cs Releases invalid control blocks after finalization.
JavaException.cs Applies equivalent exception-peer cleanup.
IJavaPeerable.xml Documents finalization ownership and ordering.
JavaPeerControlBlockTests.cs Covers disposal, finalization, resurrection, and failure paths.

Comment thread external/Java.Interop/src/Java.Interop/Java.Interop/JavaException.cs Outdated
Share the finalization validity guard between JavaObject and JavaException without changing callback or reference-tracking context ordering.

Co-authored-by: Copilot App <[email protected]>
@simonrozsival

Copy link
Copy Markdown
Member Author

@dalexsoto review

@dalexsoto dalexsoto left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The complete five-file ownership, native/bridge-consumer and test-integration review found one blocker. The shared invalid-block cleanup preserves callback ordering, reconstructed valid references and the existing native ABI; the previous centralization suggestion is resolved.

Isolate or replace the exact VM-wide GREF assertions in JavaPeerControlBlockTests.cs:167-182 and the analogous assertions at lines 51-52. This fixture runs in the ordinary shared-VM suite, but disposing one owned reference does not guarantee that the entire VM counter changes by exactly one: other peers, cached class references and asynchronous bridge/finalizer activity share that counter. Sequential NUnit execution is not equivalent to the dedicated isolation used by reference-count tests. The new assertion can fail before the callback/native-pointer checks complete. Exact-source build 1624644 has three failures of ExplicitDispose_PreservesCallbackAndReleasesControlBlock(False) (235 expected/245 actual, 193/246, and 193/245), independently verified from all 87 run summaries, the three failed results and Debug/Release logs. Keep native-pointer/callback checks in the normal suite; verify the particular owned reference's deletion, or put VM-total verification in a dedicated warmed-up isolated run. Address both assertion sites without changing production disposal ordering or bridge synchronization. This establishes a current test blocker, not the cause of those VM-wide increases or proof of a production native/GREF leak.

Keep native allocation and callback checks in the ordinary shared-VM suite. Move exact GREF-count assertions to the dedicated JniReferenceLeak selection and warm peer caches after collection, so unrelated runtime activity does not mask the lifetime regression checks.

Refs #13010

Co-authored-by: Copilot App <[email protected]>
@simonrozsival

Copy link
Copy Markdown
Member Author

@dalexsoto Addressed in 9596f9c6af, pushed to this independent branch.

Both ordinary-suite VM-total assertion sites are replaced with peer-reference invalidity checks; all native-pointer, callback, throwing-finalization, reconstruction/registration, and double-free checks remain. Exact GREF deletion now has four cases under the existing dedicated JniReferenceLeak selection, covering both peer types and explicit disposal/finalization. Collection precedes cache warm-up, so collection cannot undo the warm-up before measurement. No production lifetime ordering, bridge synchronization, or instrumentation filtering changed.

Confirmed all three failing results from build 1624644, including both stage attempts, are this new shared-VM count assertion. This is a test regression, not evidence that the native allocation fix causes those GREF increases.

Final local validation: full ordinary host suite 757 passed/5 existing skipped; isolated host 7/7. Source-built CoreCLR full ordinary device suite 835 passed/8 existing skipped, lifetime selection 24/24, isolated GREF selection 7/7. Source-built NativeAOT lifetime 24/24 and isolated GREF selection 7/7, verified after clean packaging to avoid a stale category-selection APK. All device runs used my owned arm64 API29 emulator. The earlier extra NativeAOT rooted-cycle case remains unselected and is not claimed. Exact commands, results, and intermediate validation corrections are recorded in the PR body. New-head CI is not yet claimed green.

@dalexsoto dalexsoto left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prior ordinary-suite VM-total assertions are fixed: native-pointer, callback and reconstruction checks remain, and the four exact GREF-deletion cases use the dedicated category with collection before warm-up. The complete five-file lifetime/bridge/test-integration review, separate completeness pass and final sweep leave one current integration blocker; no additional defect was found in the native cleanup.

Make the new shared fixture compatible with current main. JavaPeerControlBlockTests.cs:16 still inherits JavaVMFixture. The frozen head/base retains the partial fixture, but the actual CI merge f06ea5b incorporates main's removal of both fixture definitions. The shared Android project includes this unchanged new test file but has no remaining declaration of its base class. Exact-source build 1625456 consequently aborts Debug, Release, NoAab, NativeAOT and JniReferenceLeaks with CS0246 at line 16, column 43, before test execution. Zero published test failures does not mean those five runs passed.

Align the fixture with main's supported no-base Android fixture convention, preserving the lifetime/deletion coverage rather than restoring the retired reflection harness, and validate both ordinary and dedicated-category consumers. The inheritance predates your assertion correction; this is newly available current-main integration evidence, not a claim that 9596f9c introduced the inheritance or that native cleanup causes GREF increases.

simonrozsival and others added 2 commits October 7, 2026 10:02
Remove the retired JavaVMFixture inheritance and host-only registered-peer cleanup. Current main removed the reflection-backed JVM harness; the shared fixture now follows its no-base Android convention and exercises the actual bridge finalizer path.

Refs #13010

Co-authored-by: Copilot App <[email protected]>
@simonrozsival

Copy link
Copy Markdown
Member Author

@dalexsoto Addressed the current-main fixture blocker in 909b509, after merging current main (53c12fa) via d25b714. Pushed both new commits; no history rewrite.

Confirmed CS0246 at the shared fixture base in exact-build 1625456 Debug, NativeAOT and JniReferenceLeaks logs. Main #13018 retired JavaVMFixture and the standalone reflection-backed host-JVM harness. Removed the new fixture inheritance and obsolete NO_GC_BRIDGE_SUPPORT manager-removal path, following main’s no-base Android convention. No retired harness restored and no production cleanup/ordering changes.

Rebuilt the source Release SDK against integrated main. On my owned API29 arm64 emulator: CoreCLR lifetime 24/24 and dedicated JniReferenceLeak 7/7; NativeAOT lifetime 24/24 and dedicated JniReferenceLeak 7/7; full ordinary CoreCLR suite 893 passed, 7 existing skipped, zero failures (900 total). All builds/installations passed. Used clean packaging and verified actual selected counts. Current-main host-JVM execution is no longer supported; previous host evidence remains historical, not claimed for the merged code. Exact commands/results and limits are in the updated template-preserving PR body. Fresh Debug/NoAab device execution and the additional NativeAOT cycle are not claimed. No physical phone or sibling emulator was used; my emulator/private adb were stopped.

Exercise successful and throwing finalization callbacks for both peer types after valid reconstruction. Verify retained JNI identity and registration, then explicit disposal releases the native block and removes registration.

Co-authored-by: Copilot App <[email protected]>
simonrozsival added a commit that referenced this pull request Oct 7, 2026
Core JNI interop tests now run exclusively in the Android device suite. Keeping their sources in the former standalone test directory leaves misleading ownership, external source globs, and separate desktop/Android Java fixture selection. This Android-only relocation puts the sources beside their actual test project and simplifies fixture compilation without changing supported Android test behavior.

Follow-up to the merged #13018. This PR now targets `main`; its diff contains only the Android test relocation and cleanup. It preserves the removal of the desktop harness, reflection-backed JNI managers, and obsolete `JavaVMFixture` scaffolding.

- Move core C# sources and Java fixtures to `tests/Mono.Android-Tests/Java.Interop-Tests/`; use the project's default `Compile` glob and a local `TestJarEntry` glob. Preserve the existing exclusion for tests of the internal `JNIEnv` layout.
- Remove `__ANDROID__` branches while retaining their Android assertions. Remove unsupported desktop-only custom-type-manager remapping tests and unused helper fixtures. Preserve `NO_MARSHAL_MEMBER_BUILDER_SUPPORT`, `NO_GC_BRIDGE_SUPPORT`, and applicable runtime/AOT exclusions.
- Consolidate `GetThis.java` onto the Android-safe implementation for every typemap. Remove duplicate `java-trimmable` selection and unused desktop test targets, and prevent implicit `AndroidJavaSource` compilation from duplicating fixture classes in the JAR.
- Update directly related documentation, instructions, and test catalog/skill guidance. Unrelated Java.Interop tooling suites are unchanged.

**Migration guidance:** Core JNI test additions belong in the new Android directory, including C# fixtures such as `ThrowableInputCleanupTests.cs` and `JavaPeerControlBlockTests.cs`, and Java fixtures under `java/net/dot/jni/test/`. Preserve coverage introduced by #13014 and #13015 when moving or updating these files. Other branches touching the old directory, including #13016, should rebase into the new location. Do not recreate the removed standalone desktop test project.

**Validation**

The following results were recorded during implementation. The installed-SDK build passed, but repository-SDK/device coverage remains blocked; marking this PR ready for review does not imply that coverage passed.

| Command / check | Outcome |
|---|---|
| `dotnet build tests/Mono.Android-Tests/Java.Interop-Tests/Java.Interop-Tests.NET.csproj -c Release` | Passed using the installed Android SDK, including the local Java fixture JAR and AAR; 0 warnings, 0 errors. Repeated after rebasing onto the parent. This is compile coverage, not validation against a newly built repository SDK. |
| `dotnet msbuild tests/Mono.Android-Tests/Java.Interop-Tests/Java.Interop-Tests.NET.csproj -p:Configuration=Release -getItem:Compile,TestJarEntry,AndroidJavaSource` | Passed during implementation: verified 40 local Compile items, all 12 Java fixtures, and no duplicate AndroidJavaSource items. |
| `javac --release 17 -d <session-artifacts>/java-fixture-classes tests/Mono.Android-Tests/Java.Interop-Tests/java/net/dot/jni/test/*.java external/Java.Interop/src/Java.Interop/java/net/dot/jni/GCUserPeerable.java external/Java.Interop/src/Java.Interop/java/net/dot/jni/ManagedPeer.java` | Passed for all 12 fixtures at the configured Java target version. |
| Android-preprocessed source comparison against parent | Passed for 39 migrated C# files during implementation; the remaining file intentionally removes desktop-only remapping coverage and its unused helpers. Capability guards are unchanged. |
| `jar tf tests/Mono.Android-Tests/Java.Interop-Tests/Jars/Mono.Android-Test-classes-trimmable.jar` and `javap -c -classpath tests/Mono.Android-Tests/Java.Interop-Tests/Jars/Mono.Android-Test-classes-trimmable.jar net.dot.jni.test.GetThis` | Passed: one GetThis class, implements GCUserPeerable, no desktop native registration/constructor calls. |
| `git diff HEAD^ HEAD --check` | Passed during implementation. |
| `make prepare CONFIGURATION=Release` | Blocked by NU1102: `Microsoft.NETCore.App.Ref` version `10.0.13` was unavailable in the configured feeds. The corresponding nuget.org package URL also returned HTTP 404. |
| `make all CONFIGURATION=Release` | Blocked by MSB4062: missing `bin/BuildRelease/net10.0/xa-prep-tasks.dll` after preparation failed. |
| `./dotnet-local.sh build -t:Install -c Release tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj -p:AndroidTypeMapImplementation=llvm-ir` | Blocked by NETSDK1147: local SDK had no Android workload because repository SDK preparation/build could not complete. |
| `./dotnet-local.sh build -t:Install -c Release tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj -p:AndroidTypeMapImplementation=trimmable` | Blocked by the same NETSDK1147 prerequisite. |

No on-device tests were run because the required local SDK/install builds failed, despite a connected arm64 emulator. Mono/CoreCLR/NativeAOT runtime behavior and both typemap device lanes remain unvalidated. No desktop JVM validation is required.

----

Pull Request
[title](https://github.com/dotnet/android/blob/main/Documentation/workflow/commit-messages.md#commit-summary) and
[description](https://github.com/dotnet/android/blob/main/Documentation/workflow/commit-messages.md#commit-body)
should follow the
[`commit-messages.md` workflow documentation](https://github.com/dotnet/android/blob/main/Documentation/workflow/commit-messages.md), and in particular should include:

- [x] Useful description of *why the change is necessary*.
- [ ] Links to issues fixed
- [ ] Unit tests

No issue is closed by this relocation. Existing Android tests compile; on-device test execution is blocked by the local SDK prerequisites described above.
@dalexsoto

Copy link
Copy Markdown
Member

@simonrozsival I haven't submitted a new GitHub review for this context: I verified that the prior ordinary GREF assertions and removed JavaVMFixture dependency are fixed, and completed the five-file native/JNI lifetime audit. I cannot yet verify placement and Android compilation/discovery of the new 26 ordinary plus four dedicated lifetime cases after current main's JNI-test directory relocation. The published GitHub/CI merge still incorporates older main; a default directory-rename-aware local Git reconstruction against current main produced an unresolved file-location conflict for JavaPeerControlBlockTests.cs. That conflicted local tree is not an authoritative GitHub result, so I am not claiming that GitHub omitted the tests or that a source regression is proven. I need a resolved current-main integration with the fixture in the Android project's compile selection before completing this review. This is not a deferral for pending CI or optional local device execution.

Resolve the JNI test directory relocation by retaining JavaPeerControlBlockTests unchanged under tests/Mono.Android-Tests/Java.Interop-Tests. Preserve the lifetime fix and all regression coverage.

Co-authored-by: Copilot App <[email protected]>
@simonrozsival

Copy link
Copy Markdown
Member Author

@dalexsoto review

@simonrozsival

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

✅ Android PR Reviewer completed successfully!

Generated by Android PR Reviewer for #13016

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review summary

Verdict: Looks good; no actionable issues found.

The finalization cleanup is correctly centralized, runs from finally for both JavaObject and JavaException, and preserves a JNI reference reconstructed by Dispose(false). The tests cover explicit disposal, callback exceptions, reconstruction, resurrection, constructor failure, actual finalization, and GREF deletion. The updated ownership documentation also matches the runtime ordering: reference-tracking contexts are detached before the peer callback and the cleared native control block is released afterward.

Findings: 0 blocking, 0 non-blocking. CI is green.

Generated by Android PR Reviewer for #13016 · copilot · gpt56 · 118.1 AIC · ⌖ 11.1 AIC · ⊞ 26.4K
Comment /review to run again

@simonrozsival

Copy link
Copy Markdown
Member Author

@dalexsoto stamp

@simonrozsival simonrozsival added the ready-to-review This PR is ready to review/merge, I think any CI failures are just flaky (ignorable). label Oct 9, 2026

@dalexsoto dalexsoto left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stamping as requested by @simonrozsival; no code review performed.

@simonrozsival
simonrozsival merged commit e5b44a4 into main Oct 9, 2026
42 checks passed
@simonrozsival
simonrozsival deleted the simonrozsival-peer-finalization-lifetime branch October 9, 2026 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-review This PR is ready to review/merge, I think any CI failures are just flaky (ignorable).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants