[Mono.Android] Return registered peers after activation - #12772
simonrozsival wants to merge 1 commit into
Conversation
Java.Lang.Object.GetObject() can return an unregistered alias when activation reenters lookup and registers another compatible peer first. A caller can cache that alias while array marshaling returns the registered peer, breaking managed reference identity. Recheck the compatible registered peer after creation without adding a lookup to the already-registered fast path. Preserve the low-level GetPeer()/CreatePeer() alias contracts and type-specific aliases. Add a deterministic, single-threaded regression and incompatible-type coverage. Update concurrent array coverage to require the registered winner and retain both created peers for deterministic cleanup. Related to #10973. The precise x64 CI startup interleaving still needs confirmation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved peer disposal and disposed-state handling issues remain.
Get a fresh assessment by requesting another Copilot review.
Review tier: Lite
Findings: 1
Open findings (1)
What changed in this PR
This PR fixes JNI peer identity mismatches caused by reentrant activation during object lookup.
Changes:
- Rechecks registered peers after peer creation.
- Adds reentrant and incompatible-type regression tests.
- Tightens concurrent marshaling assertions and cleanup.
Review findings:
Object.cs— moderate, 2 votes: dispose the losing alias before replacement.Object.cs— moderate, 1 vote: preserve the disposed-state guard.PeerIdentityTests.cs— nit, 1 vote: reorder assertions for better diagnostics.
| File | Summary |
|---|---|
tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj |
Includes the new test source. |
tests/Mono.Android-Tests/Mono.Android-Tests/java/net/dot/android/test/ReentrantLookupPeer.java |
Adds the Java reentrant-lookup fixture. |
tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/TrimmableTypeMapRuntimeCoverageTests.cs |
Tracks constructed peers for cleanup. |
tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/PeerIdentityTests.cs |
Adds peer identity and incompatible-type coverage. |
tests/Mono.Android-Tests/Mono.Android-Tests/Android.Runtime/JnienvArrayMarshaling.cs |
Tightens concurrent identity assertions and cleanup. |
src/Mono.Android/Java.Lang/Object.cs |
Rechecks registered peers after activation. |
| peer = JniEnvironment.Runtime.ValueManager.CreatePeer (ref reference, JniObjectReferenceOptions.Copy, type); | ||
| if (peer != null) { | ||
| // Activation can register a competing peer before this one is constructed. | ||
| peer = PeekObject (handle, type) ?? peer; |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Needs Changes
Findings: 0 errors · 1 warning · 0 suggestions
The re-peek correctly preserves managed identity for both re-entrant and concurrent activation, and the incompatible-type regression coverage protects the intended type-filtering behavior. The newly created losing alias still owns a global JNI reference when it is discarded, however, so its cleanup should be explicit rather than left to finalization.
CI build 1594059 is still running; all completed checks reported success at review time.
Generated by Android PR Reviewer for #12772 · copilot · gpt56 · 110 AIC · ⌖ 7.12 AIC · ⊞ 22.6K
Comment /review to run again
| peer = JniEnvironment.Runtime.ValueManager.CreatePeer (ref reference, JniObjectReferenceOptions.Copy, type); | ||
| if (peer != null) { | ||
| // Activation can register a competing peer before this one is constructed. | ||
| peer = PeekObject (handle, type) ?? peer; |
There was a problem hiding this comment.
🤖 peer drops the only managed reference to the freshly created alias. ConstructPeer has already promoted that alias to a global JNI reference, while registration deliberately rejected it, so the global remains alive until finalization. Re-entrant or concurrent activation can therefore retain Java object graphs and consume global references nondeterministically. Please keep the created peer in a separate local and call DisposeUnlessReferenced() before returning the distinct registered winner, then assert that cleanup in the regression test.
Rule: JNI reference lifecycle
Related to #10973; follows the deterministic reproduction and investigation.
Java.Lang.Object.GetObject()can return an unregistered alias when activation reenters lookup and registers another compatible peer first. A caller can cache that alias while subsequent array marshaling returns the registered peer: the Java object is the same, but managed reference identity differs.Recheck for a compatible registered peer after creation. The already-registered fast path still performs one lookup, and the shared
JniValueManager.GetPeer()/CreatePeer()implementations and their alias contracts remain unchanged. A registered peer incompatible with the requested managed type is not substituted.Regression coverage
The new
GetObject_ReentrantActivation_PreservesRoundtripIdentitytest uses one plain Java object and a synchronous, test-owned activation callback to force the ordering. It needs no worker threads, sleeps, forced GC, or application startup. The existingGetObjectArrayidentity assertion is not weakened.Also cover incompatible registered types and update concurrent array marshaling to require the registered winner. Retain both constructed peers in that test so finalization of the unreturned alias cannot race its disposal assertion.
Validation
Source-built Release SDK, API 35 arm64-v8a emulator:
llvm-ir): 9 passed, 3 trimmable-only tests skipped.JNIObjectArraytests passed on both trimmable CoreCLR and NativeAOT.This fixes a demonstrated mechanism producing the reported symptom. Keep #10973 open: the exact startup interleaving in CI build 1589170 has not been established, and that failure was x64 rather than the locally tested arm64.