Repository navigation
[Unsafe][Java.Interop] Retain JNI callback registration owners - #13156
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
Safety-critical callback teardown needs human review, particularly disposal with live registrations and unregistration-failure coverage.
1 open finding
What changed in this PR
Strengthens Java.Interop callback ownership as preparation for the memory-safety migration, without changing public API signatures or compiler settings.
Changes:
- Retains registration snapshots and owners before JNI registration.
- Reorders teardown to preserve roots when unregistration fails.
- Documents callback obligations and adds four regression cases.
| File | Description |
|---|---|
| tests/Mono.Android-Tests/Java.Interop-Tests/Java.Interop/JniNativeRegistrationTests.cs | Adds callback retention regressions. |
| external/Java.Interop/src/Java.Interop/Java.Interop/JniType.cs | Retains registration attempts and reorders disposal. |
| external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.cs | Tracks individual registration owners. |
| external/Java.Interop/src/Java.Interop/Java.Interop/JniNativeMethodRegistration.cs | Documents raw callback lifetime requirements. |
| external/Java.Interop/src/Java.Interop/Java.Interop/JniEnvironment.Types.cs | Preserves delegates through cleanup and documents caller obligations. |
| external/Java.Interop/Documentation/Architecture.md | Explains callback ownership and teardown contracts. |
🧠 Review effort: Balanced
simonrozsival
changed the title
JNI may publish callbacks before registration fails. Root each owner\nand snapshot every attempted registration before entering JNI so input\narray mutation, repeated registration, and partial failure cannot drop\ncallable delegates.\n\nTrack owners by identity and unregister before releasing roots or JNI\nreferences. Document native-call quiescence and in-flight completion\nrequirements for managed registrations and raw callback storage.\n\nAdd four captured-delegate regression cases covering mutation, repeated\nregistrations, collected owners, and partial failure. This preparation\nlayer preserves API signatures and does not enable compiler safety rules. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
3d3ae5a to
31001bb
Compare
Take ownership of the supplied native registration array instead of cloning it. Document that callers must not modify the transferred array through any alias while JNI can use its callbacks, including after a partial registration failure. Reject another registration attempt with InvalidOperationException until successful explicit unregistration. Keep the first array and delegates rooted, and do not transfer ownership or enter JNI for rejected attempts. Cover original-array lifetime, failed registration, rejected attempts, and release on unregistration or disposal. Simplify lifetime wording and explain why runtime cleanup must not pre-clear tracked owners. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
Verdict: Needs changes
Found 1 error and 1 warning, both called out inline:
- A validation failure before JNI is entered leaves the
JniTypepermanently registered in managed state and forces a potentially destructive class-wide unregister before retrying. - Rejecting subsequent
RegisterNativeMethods()calls breaks the existing public behavior even though retaining all transferred arrays would preserve it safely.
CI build #1640579 is still in progress: both completed Android Tools test lanes passed, while the Linux, Windows, and macOS builds remain running and the aggregate check is queued.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
azcliprod.blob.core.windows.net
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
Generated by Android PR Reviewer for #13156 · copilot · gpt56 · 137.9 AIC · ⌖ 8.64 AIC · ⊞ 26.9K
Comment /review to run again
Keep the original single-array, success-only registration ownership. Document caller obligations for repeated calls, transferred-array aliases, and callbacks that JNI might install before a registration failure. Remove the duplicate-registration guard and defer partial registration recovery instead of adding callback or phase-tracking machinery. Mark caller-obligation registration APIs unsafe, retain explicit unsafe blocks with SAFETY explanations, and keep raw address storage documented as registration-time requirements rather than pointer dereferences. Unregister before dropping runtime roots or the JNI reference, avoid pre-clearing tracked owners, and keep delegates alive through exceptional marshalling cleanup. Cover successful ownership, direct disposal, preparation failures, and existing JNI exception propagation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
simonrozsival
added
the
ready-to-review
|
@dalexsoto review |
|
@simonrozsival Not reviewing this request in this iteration: GitHub returned an HTTP 403 secondary-rate-limit error during discovery. The read-only retry recovered, but I am deferring the code review until a clean API cycle. No code review has been performed and no review decision has been submitted. |
|
@dalexsoto review |
|
@simonrozsival Not reviewing this request in this iteration: the PR was merged before this request could be completed. No GitHub review was submitted by this monitor. Any read-only inspection is retained as historical, unpublished evidence only. |
Audit JNI callback safety contracts and fix the teardown bugs found during that audit, without redesigning registration ownership. Published revision:
0074a9bb763f0cad7a9cf1ac88c18c9436cba5d9; six files, +384/-19.Safety contracts and retained behavior
Add caller-facing
unsafemodifiers to the managed-array registration APIs, explicit implementationunsafe { }blocks with concrete// SAFETY:comments, and documentation for callback ABI, array aliases, native storage lifetime, and in-flight calls. The raw descriptor constructor only stores addresses; its remarks describe requirements when registering those addresses. Compiler opt-in remains in the final activation layer.JniType.RegisterNativeMethods()still calls JNI before retaining the original array and tracking the owner, and does so only on success. Only the last successful array is remembered. There is no clone, history collection, or duplicate-registration guard. The caller must not register again on the same managed owner before successful unregistration, or mutate the transferred array/entries through any alias while callbacks can run. These are safety obligations, not runtime checks.Partial-registration ownership/recovery is explicitly deferred by maintainer decision. Existing JNI exceptions propagate unchanged. Failed attempts do not transfer array ownership, even if JNI installed some callbacks; callers must keep those delegates rooted until successful unregistration and callback completion. Managed preparation failure leaves previous successful state unchanged. No callback plumbing, phase flag, automatic rollback, retry, or new exception policy is added.
Targeted cleanup fixes
GC.KeepAlive(methods)infinally, including exceptional marshalling exits.Callers must prevent new calls and wait for running callbacks before unregistration/disposal. Raw callback storage must remain callable through successful unregistration and completion of all in-flight calls.
JNI handles versus object identity
The existing
Dictionary<IntPtr, IDisposable>remains. Its key,JniType.PeerReference.Handle, is an opaque JNI global-reference handle—not a canonical Java-class identifier. Different global references to the same class can have different handle values; JNI'sIsSameObjectestablishes underlying identity. The dictionary therefore does not deduplicate wrappers by Java-class identity. NoHashSetconversion or class-wide registration-owner registry is introduced.Separate wrappers must coordinate class-wide JNI operations. The single-registration obligation applies to this managed delegate-array API, not the separate raw-span/UCO path used by the production typemap.
Validation and remaining limits
The eight current regression cases are compiled, not run on Android. They cover successful array/delegate/owner retention and release, real disposal with native unregistration checked through an independent class reference, owner collection, preparation-failure retry and preservation of existing registrations, independent-owner disposal safety, and existing Java exception propagation. The failure test keeps caller-owned array/delegate roots through explicit cleanup; it does not promise partial-failure ownership or recovery. Tests remain
NativeAOTIgnorebecause they require dynamic callbacks. Controlled unregistration-failure injection coverage is explicitly blocked by the maintainer's decision not to add a production test seam.Current-revision host compilation used
DOTNET_PROCESSOR_COUNT=2andCMAKE_BUILD_PARALLEL_LEVEL=2:No fresh full SDK/device validation or integrated opt-in metadata proof validates this revision. The host project remains
Features='',LangVersion=14.0. Earlier four-case callback device results at3d3ae5ad5d/31001bbbfcand the four runtime configurations recorded for final stack snapshotcd9e12f2d4are historical evidence for the earlier implementation, not passes for0074a9bb76. The intermediateee4e65ae4cduplicate-rejection policy was host-compiled, not device-tested, and is removed here.The latest audit revisions for the nine downstream layers are local and unpushed, paused for churn reassessment. Their published branches are not synchronized with these revisions; integrated runtime validation remains pending. This description update changes no source, PR base, branch, or native stack metadata.
This is the callback preparation layer extracted from #13070, not the final compiler activation. The extraction used baseline
888dfb3f87eb0f34c42075e0a3d8e78db4c3d54dand source951fa3a2dc588e15ff63eea4565d1dd14a30f036; original local validation usedca6c659c138f160104ab4ade135a01dc8c113530, and the callback commit was later prepared onf021275077331b36d936ca7f63753b24daeba21a. No issue is closed by this preparatory PR.