Repository navigation
[Mono.Android] Fix .NET 10 peer control-block lifetime leaks - #13293
Conversation
Context: #13290 Context: dotnet/java-interop#1502 Changes: dotnet/java-interop@8e8291c...f79a53b Update the Java.Interop submodule to the canonical servicing PR commit, which frees still-invalid control blocks after finalization callbacks and routes unregistered disposal through the normal disposed-state/callback path. Preserve callback reference reconstruction, exception cleanup, and Mono bridge ordering rather than swapping Finalized and reference clearing. Add 36 on-device regression cases for both JavaObject and JavaException, covering natural finalization, invalid and absent blocks, throwing constructors/callbacks, reference reconstruction, registered and unregistered disposal, aliases, idempotency, and JNI reference deletion. Preserve Mono's re-registration of peers whose Java references are still valid. The in-tree SDK build and all 36 focused cases passed. The source-built public Rect reproducer on Mono 10.0.13 remained flat over 75,000 additional wrappers: native deltas were -208 bytes for natural finalization and -400 bytes for unregistered disposal, with no JNI global-reference growth. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3ac73074-6c43-4940-8cde-8925203f171d
There was a problem hiding this comment.
🔵 Needs a closer look
The fix depends on the still-open dotnet/java-interop#1502, so a maintainer must verify it lands first and that the Java.Interop pin matches the landed servicing commit.
No open issues 🎉
What changed in this PR
Adds Android regression coverage for the .NET 10 peer control-block lifetime fix in upstream Java.Interop, addressing #13290. The production fix is supplied through the submodule dependency; this PR must wait for upstream PR dotnet/java-interop#1502 to be accepted.
Changes:
- Adds 36 on-device cases covering
JavaObjectandJavaExceptionfinalization, disposal, exception paths, and callback reference reconstruction. - Verifies registered-peer and alias preservation, disposal retries and idempotency, native control-block release, and JNI global-reference deletion.
| File | Description |
|---|---|
| tests/Mono.Android-Tests/Java.Interop-Tests/Java.Interop/JavaPeerControlBlockTests.cs | Adds peer-lifetime regression tests and observation helpers. |
Review effort: Balanced
Update Java.Interop to the release/10.0.1xx tip after dotnet/java-interop#1502 was merged. The landed commit has the same source tree as the previously pinned PR commit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@dalexsoto review |
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
|
/azp run |
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Needs Changes — CI validation pending
Findings: 0 errors, 0 warnings, 1 non-blocking suggestion posted inline. No production correctness defect found in the reviewed changes.
The pinned Java.Interop change releases invalid control blocks in finally without moving the finalization callback ahead of reference-tracking teardown, and preserves references reconstructed by the callback. Routing unregistered disposal through the normal disposal path also preserves valid-reference callback entry and JNI-reference deletion. The 36-case fixture covers both peer types, callback exceptions, reconstruction, aliases, idempotency, and actual finalization. The upstream dependency, dotnet/java-interop#1502, is merged, and the gitlink points to its landed commit 567e6b66cd5cef134e5f3607887d950a4c2b74b0.
CI: On head 9ef07e6ff1fe2bd8409384dbb677cf4a9dea738d, only license/cla is reported and passing; there are no dotnet-android checks or Azure DevOps build links, and combined status is pending. Build/device-test validation therefore cannot yet be assessed as green. The Mono ARM64 results in the PR description are author-reported; I did not independently rerun them.
Generated by Android PR Reviewer for #13293 · copilot · gpt61 · 58.9 AIC · ⌖ 7.95 AIC · ⊞ 19.3K
Comment /review to run again
| var observation = new FinalizationObservation (); | ||
| FinalizerHelpers.PerformNoPinAction (() => CreateCollectiblePeer (exception, invalid, throwingConstructor, observation)); | ||
| try { | ||
| var stopwatch = Stopwatch.StartNew (); |
There was a problem hiding this comment.
🤖 💡 Testing — Consider transferring worker-thread exceptions back to the NUnit thread here. FinalizerHelpers.PerformNoPinAction starts a raw Thread and only joins it; it does not propagate exceptions. CreateCollectiblePeer now executes Assert.Throws<InvalidOperationException> on that worker, so a failed assertion (for example, if construction throws a different exception) escapes the thread and can terminate the instrumentation process instead of producing an ordinary failed test case. Keep the no-pin worker, but capture failures inside its action and rethrow/assert on the test thread after the join.
{Rule: Report test failures on the test runner thread}
|
Verified this PR with the actual Source and test settings
Both source SDK/framework builds succeeded. The sample used the source-built local workload through The comparison used the same Android 10/API 29 x86_64 emulator, Mono/.NET 10.0.11, private .NET SDK 10.0.400, and JDK 21. The SDK/framework build configuration was Debug; the sample was Release, with trimming, AOT, the interpreter, and marshal methods disabled. The sample's SDK pin was changed from 10.0.402 to the branch's prepared 10.0.400 SDK. MeasurementsOne finalization run and one explicit All changes below are in bytes. Batch 0 is after warm-up; batches 5-20 cover 75,000 new wrappers.
The unfixed finalization case added about 160,000 native bytes per 5,000-wrapper batch. The median batch increase over batches 6-20 was 31.9808 bytes per wrapper, while managed and Java live memory stayed almost unchanged. Explicit disposal did not show that continuing increase. The fixed finalization result includes a one-time 93,200-byte increase at batch 6, followed by only 752 bytes of total change from batch 6 to batch 20. Its median batch increase was 0.0064 bytes per wrapper. The later 10-20 window changed by only 832 bytes across 50,000 new wrappers. This separates the remaining background changes from the previous increase with each allocation. Framework identity checks and limitsThe source pack DLL bytes matched the decoded framework DLL payloads in each signed APK. Their module identifiers matched the APK's native type maps. Runtime One version-label detail: the initial preparation generated Mono.Android's informational version at PR HEAD. That generated label remained during the incremental baseline build, so both This verifies the reported defect with this sample on x86_64. ARM64 hardware was not tested, and this does not establish that all native memory growth in a larger app is fixed. Exact commands, build logs/binlogs, signed APKs, DLL hashes, and all four measurement captures were retained locally. |
|
@jonathanpeppers Not reviewing this request in this iteration: The read-only review worker stopped with an authorization error: "401 You do not have access to the organization tied to the API key." It did not produce a complete review. GitHub access is working, and the request and partial source evidence are preserved; I am withholding a formal decision rather than treating the failed attempt as a clean review. |
|
@dalexsoto stamp |
dalexsoto
left a comment
dalexsoto
left a comment
There was a problem hiding this comment.
Stamping as requested by @jonathanpeppers; no code review performed.
|
Checked the failed tasks and test-result XML files in build 15844117. None of the failures appear related to this PR.
The new The NativeAOT failure is directly confirmed on the unfixed baseline. The network, local-port, and debugger timing failures appear unrelated based on their errors; they were not reproduced separately in this investigation. |
Dependency
Depends on dotnet/java-interop#1502. Do not merge this Android PR before the upstream servicing fix is accepted.
The gitlink points to
f79a53b01a33739eb738a2ed951dcc2335bce3f5, published in the canonicaldotnet/java-interoprepository, not a local or fork-only object. After opening the upstream PR, that exact SHA was successfully fetched into a fresh object database fromhttps://github.com/dotnet/java-interop.git, and the canonical feature branch was verified to advertise the same SHA. If the upstream merge rewrites the commit, update the gitlink to its landed servicing SHA before merging this PR.Summary
Address the .NET 10 portion of #13290, based on
release/10.0.1xxat49516ae50dffb0421b7faf5f60f9df9cc220743d.8e8291c9ef45b4bbc28b09456dc5888eb80af77bto the canonical upstream servicing fix.Dispose(false), including exception paths, while preserving valid JNI references reconstructed by the callback.DisposeUnlessReferenced()through the normal disposed-state/Dispose(true)path, preserving valid-reference callback entry, JNI deletion, and idempotency.JavaObjectandJavaException: real natural finalization, invalid/absent blocks, throwing constructors and callbacks, callback reference reconstruction, Mono live-reference re-registration, registered/unregistered peers, aliases, retry after a throwing disposal callback, explicit/idempotent disposal, and JNI global-reference deletion.The production changes are in the upstream Java.Interop PR; this PR carries its pin and the Android regression fixture. The existing finalization order and bridge/reference-tracking teardown order are unchanged. This is not the unsafe two-call finalizer reorder.
Related main-only coverage is in #13292, where the production corrections already existed. This PR does not include the deferred
release/11.0.1xxbackport.Validation
Built the changed in-tree SDK using the repository preparation/bootstrap/
PrepareJavaInteropworkflow and:make leeroycompleted native runtime builds, API 36/36.1 bindings, andConfigureLocalWorkload. All builds completed with zero errors; the existing instrumentation project emits platform-analysis and other warnings.On an owned Android 15 ARM64 emulator, repository
RunTestApksinstrumentation targets passed 32/32 focused lifecycle cases selected withIncludeCategories=PeerControlBlockandExcludeCategories=JniReferenceLeak, then 4/4 separately selectedJniReferenceLeakcases. Actual NUnit XML case identities/counts were checked. These runs used the source SDK's Mono 10.0.11 runtime.Rebuilt the public Rect reproducer through
./dotnet-local.shwith the changed in-tree framework andRuntimeFrameworkVersion="10.0.13", and verified the loaded framework identities. No stock framework DLLs were rewritten. Between batches 5 and 20, representing 75,000 additional wrappers:DisposeUnlessReferenced()Median adjacent-batch native growth was 0 bytes per wrapper in all modes. Whole-process allocation counters include background noise; the pointer-level regression assertions independently verify block release.
An additional run intended to select existing peer fixtures ignored the legacy suite selector and ran 945 tests: 934 passed, 5 ignored, and 6 HTTP redirect/certificate tests returned HTTP 403 errors. Those HTTP errors were not baselined and are not claimed to be pre-existing. All 34 cases in the intended existing
JavaObjectTest,JavaExceptionTests, andAndroidValueManagerContractTestsfixtures passed.