[Mono.Android] Return registered peers after activation - #12772
simonrozsival wants to merge 3 commits 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. |
![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
A competing registered peer can replace the result of GetObject(), leaving the newly constructed alias holding a global JNI reference until finalization. Release the distinct unreturned alias with DisposeUnlessReferenced() before returning the compatible winner. Retain created peers in the reentrant and concurrent regressions and assert that only the returned winner keeps a valid JNI reference. Cover normal registration, requested-type aliases, and low-level GetPeer() results to preserve the lifetime of peers still returned. Addresses both discarded-alias review threads on #12772. 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.
|
There was a problem hiding this comment.
⚠️ Needs Changes
Findings: 0 errors · 1 warning · 0 suggestions
The post-creation recheck now preserves managed identity and promptly releases a discarded alias, and the regression coverage exercises reentrant, concurrent, normal, and incompatible-type paths. One behavioral regression remains: the new compatible-peer fast path bypasses the value manager's disposed-state guard.
CI is green: all reported dotnet-android jobs and license/cla completed successfully.
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 #12772 · copilot · gpt56 · 112.2 AIC · ⌖ 11.2 AIC · ⊞ 21K
Comment /review to run again
| return null; | ||
|
|
||
| var r = JniEnvironment.Runtime.ValueManager.GetPeer (new JniObjectReference (handle), type); | ||
| var peer = PeekObject (handle, type); |
There was a problem hiding this comment.
🤖 JniValueManager.GetPeer() with PeekObject() removes the EnsureNotDisposed() check that GetPeer() performed before every lookup. On the compatible-peer fast path, GetObject() can now return a peer after the value manager/runtime has been disposed, while the miss path still throws from CreatePeer(). Please preserve the disposed-state guard before this first peek without adding a second registry lookup to the fast path.
Rule: Preserve disposed-state checks
GetObject() now looks up peers directly instead of going through the GetPeer() disposal check. Guard PeekPeer() in all three Android value managers so the fast path still rejects a disposed manager before accessing its registry, without adding another registry lookup. Use the runtime factory to create isolated managers for cache-hit, cache-miss, and invalid-reference disposal coverage. Keep the active runtime untouched and verify that its registered peer remains usable. The two disposal cases fail before this change and pass with the fix. Focused peer and constructor coverage passes on CoreCLR llvm-ir, CoreCLR trimmable, Mono, and NativeAOT. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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. When a distinct registered peer wins, release the unreturned alias's JNI reference with
DisposeUnlessReferenced()before returning the winner. The already-registered fast path still performs one lookup, and the sharedJniValueManager.GetPeer()/CreatePeer()implementations and their alias contracts remain unchanged. A registered peer incompatible with the requested managed type is not substituted, and returned typed aliases remain usable.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 normal peer registration, incompatible registered types, and concurrent array marshaling returning the registered winner. Retain both constructed peers in the reentrant and concurrent regressions and assert that only the returned winner has a valid JNI reference before test cleanup. The low-level concurrent
GetPeer()regression verifies that both returned aliases remain usable.Validation
Source-built Release SDK, API 35 arm64-v8a emulator:
llvm-ir): 10 passed, 3 trimmable-only tests skipped.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.