[Java.Interop] fix global ref leak in ConstructPeer#1403
[Java.Interop] fix global ref leak in ConstructPeer#1403jonathanpeppers merged 3 commits intomainfrom
Conversation
Fixes: dotnet/android#11101 Fixes: dotnet/android#10989 When ConstructPeer is called on a peer that already has a valid PeerReference (but without Activatable set), lines 107-108 had a bug: the second JniObjectReference.Dispose call was a duplicate of line 100, and NewGlobalRef created a new global ref without disposing the old one. This caused a global ref leak every time ConstructPeer was called multiple times during constructor chains. Changes: - Fix ConstructPeer lines 107-108 to properly dispose the old PeerReference before replacing it with a new global ref. - Make JniObjectReference.Dispose a no-op for Invalid type refs, since activation code stores raw jobject handles with Invalid type that cannot be deleted via JNI. - Add regression test that calls ConstructPeer twice on the same peer and asserts the global ref count does not grow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes a JNI global reference leak when ConstructPeer is invoked multiple times for the same managed peer (common in constructor chains during activation).
Changes:
- Update
JniRuntime.JniValueManager.ConstructPeerto dispose the previousPeerReferencewhen replacing it with a new global ref. - Make
JniObjectReference.Dispose (ref ...)safely handleInvalidreference types (skip JNI delete, but still invalidate). - Add a regression test asserting global reference count does not increase across repeated
ConstructPeercalls.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/Java.Interop-Tests/Java.Interop/JniRuntimeJniValueManagerContract.cs | Adds regression test covering repeated ConstructPeer calls and global ref accounting. |
| src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs | Fixes the leak by disposing the old peer global ref when re-globalizing an existing PeerReference. |
| src/Java.Interop/Java.Interop/JniObjectReference.cs | Allows disposing Invalid-typed references without throwing. |
| var orig = newRef; | ||
| newRef = orig.NewGlobalRef (); | ||
| JniObjectReference.Dispose (ref orig); |
There was a problem hiding this comment.
Maybe I'm missing something, but I wonder why we even create a new global ref to the same object if we then release the old handle? This means that we own the gref and we could maybe just keep using it without the need to create a new one? I must be missing something 🤔
There was a problem hiding this comment.
The peer's current PeerReference (orig) might not be a global ref — it could be Invalid type (from SetPeerReference with a raw jobject), a local ref, or a weak global ref. NewGlobalRef() ensures we get a proper global ref regardless of the input type. We can't just keep using the existing ref because:
- If it's
Invalidtype, JNI operations on it may fail or behave unpredictably - If it's a local ref, it becomes stale when the JNI frame ends
- Only global refs provide the stable, long-lived reference the peer table needs
So NewGlobalRef + Dispose(old) normalizes any ref type into a proper global ref.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes: dotnet/android#11101 Fixes: dotnet/android#10989 When ConstructPeer is called on a peer that already has a valid PeerReference (but without Activatable set), lines 107-108 had a bug: the second JniObjectReference.Dispose call was a duplicate of line 100, and NewGlobalRef created a new global ref without disposing the old one. This caused a global ref leak every time ConstructPeer was called multiple times during constructor chains. Changes: - Fix ConstructPeer lines 107-108 to properly dispose the old PeerReference before replacing it with a new global ref. - Make JniObjectReference.Dispose a no-op for Invalid type refs, since activation code stores raw jobject handles with Invalid type that cannot be deleted via JNI. - Add regression test that calls ConstructPeer twice on the same peer and asserts the global ref count does not grow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…kiness fixed (#11202) Fixes: #11201 The test is failing across multiple CI configurations. Multiple fixes have been merged (#11112, dotnet/java-interop#1403, dotnet/java-interop#1410) but the leak is not fully resolved yet. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Fixes: dotnet/android#11101
Fixes: dotnet/android#10989
When ConstructPeer is called on a peer that already has a valid PeerReference (but without Activatable set), lines 107-108 had a bug: the second JniObjectReference.Dispose call was a duplicate of line 100, and NewGlobalRef created a new global ref without disposing the old one. This caused a global ref leak every time ConstructPeer was called multiple times during constructor chains.
Changes:
Fix ConstructPeer lines 107-108 to properly dispose the old PeerReference before replacing it with a new global ref.
Make JniObjectReference.Dispose a no-op for Invalid type refs, since activation code stores raw jobject handles with Invalid type that cannot be deleted via JNI.
Add regression test that calls ConstructPeer twice on the same peer and asserts the global ref count does not grow.