Skip to content

Commit e3062db

Browse files
jonpryorjonpryor
committed
Fix GREF leak
A "funny" thing happened after 628aa39: a teardown assertion fired! % dotnet test bin/TestRelease-net8.0/Java.Interop-Tests.dll … # jonp: LoadJvmLibrary(/Users/runner/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.15-6/x64/Contents/Home/lib/libjli.dylib)=140707427245328 # jonp: JNI_CreateJavaVM=7113469040; JNI_GetCreatedJavaVMs=7113469120 # jonp: executing JNI_CreateJavaVM=1a7feec70 # jonp: r=0 javavm=1a9dc1830 jnienv=7fe37d0422b0 TearDown failed for test fixture Java.InteropTests.JavaObjectArray_object_ContractTest JNI global references: grefStartCount=324; gref=340 Expected: True But was: False TearDown : NUnit.Framework.AssertionException : JNI global references: grefStartCount=324; gref=340 Expected: True But was: False StackTrace: at Java.InteropTests.JavaObjectArray_object_ContractTest.EndCheckGlobalRefCount() in /Users/runner/work/1/s/tests/Java.Interop-Tests/Java.Interop/JavaObjectArrayTest.cs:line 158 --TearDown at NUnit.Framework.Assert.ReportFailure(String message) at NUnit.Framework.Assert.ReportFailure(ConstraintResult result, String message, Object[] args) at NUnit.Framework.Assert.That[TActual](TActual actual, IResolveConstraint expression, String message, Object[] args) at NUnit.Framework.Assert.IsTrue(Boolean condition, String message, Object[] args) at Java.InteropTests.JavaObjectArray_object_ContractTest.EndCheckGlobalRefCount() in /Users/runner/work/1/s/tests/Java.Interop-Tests/Java.Interop/JavaObjectArrayTest.cs:line 158 at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor) at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr) Skipped References_CreatedReference_GlobalRef [3 ms] Skipped References_CreatedReference_LocalRef [< 1 ms] Skipped DoesTheJmethodNeedToMatchDeclaringType [5 ms] # jonp: LoadJvmLibrary(/Users/runner/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.15-6/x64/Contents/Home/lib/libjli.dylib)=140707427245328 # jonp: JNI_CreateJavaVM=7113469040; JNI_GetCreatedJavaVMs=7113469120 # jonp: executing JNI_CreateJavaVM=1a7feec70 # jonp: r=-5 javavm=0 jnienv=0 Unfortunately, teardown assertions "don't count", i.e. CI was green, even though the above is copied from the CI run. The cause of the problem? Having `JavaObject.Dispose()` call `JniObjectReferenceControlBlock.Free()`. The underlying problem is that `JniRuntime.JniValueManager.DisposePeer()` uses `JavaObject.PeerReference` *after* calling `JavaObject.Disposed()` (which calls `JavaObject.Dispose(bool)`), yet `JavaObject.Dispose(bool)` released the `JniObjectReferenceControlBlock` that backs `PeerReference`, meaning we "lost" (leaked!) the GREF! There are two possible solutions: 1. Update `JniRuntime.JniValueManager.DisposePeer()` to use `JavaObject.PeerReference` *before* calling `JavaObject.Disposed()` 2. Update `JavaObject.Dispose(bool)` to not release the `JniObjectReferenceControlBlock` that backs `PeerReference`. I avoided (1) because I didn't want to have to audit and update dotnet/android and all other potential callsites. Which brings us to (2): if not in `Dispose(bool)`, then where? The last thing that `JniRuntime.JniValueManager.DisposePeer()` and `JniRuntime.JniValueManager.FinalizePeer()` do is call `JavaObject.SetPeerReference(default)`. *This*, then, is where cleanup needs to happen. Update `JavaObject` and `JavaException` to use the `JniManagedPeerStates` field to track "have I been disposed?", and then update `SetPeerReference()` to call `JniObjectReferenceControlBlock.Free()` if the state is "disposed". This fixes the leak.
1 parent 174fd85 commit e3062db

File tree

4 files changed

+49
-8
lines changed

4 files changed

+49
-8
lines changed

‎src/Java.Interop/Java.Interop/JavaException.cs‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
namespace Java.Interop
66
{
77
[JniTypeSignature (JniTypeName, GenerateJavaPeer=false)]
8-
unsafe public class JavaException : Exception, IJavaPeerable
8+
unsafe public partial class JavaException : Exception, IJavaPeerable
99
{
1010
internal const string JniTypeName = "java/lang/Throwable";
1111
readonly static JniPeerMembers _members = new JniPeerMembers (JniTypeName, typeof (JavaException));
@@ -170,9 +170,6 @@ protected virtual void Dispose (bool disposing)
170170
if (inner != null) {
171171
inner.Dispose ();
172172
}
173-
#if FEATURE_JNIOBJECTREFERENCE_INTPTRS
174-
Java.Interop.JniObjectReferenceControlBlock.Free (ref jniObjectReferenceControlBlock);
175-
#endif // FEATURE_JNIOBJECTREFERENCE_INTPTRS
176173
}
177174

178175
public override bool Equals (object? obj)
@@ -266,11 +263,13 @@ protected void SetJavaStackTrace (JniObjectReference peerReferenceOverride = def
266263

267264
void IJavaPeerable.Disposed ()
268265
{
266+
JniManagedPeerState |= Disposed;
269267
Dispose (disposing: true);
270268
}
271269

272270
void IJavaPeerable.Finalized ()
273271
{
272+
JniManagedPeerState |= Disposed;
274273
Dispose (disposing: false);
275274
}
276275

@@ -287,6 +286,11 @@ void IJavaPeerable.SetJniManagedPeerState (JniManagedPeerStates value)
287286
void IJavaPeerable.SetPeerReference (JniObjectReference reference)
288287
{
289288
SetPeerReference (ref reference, JniObjectReferenceOptions.Copy);
289+
#if FEATURE_JNIOBJECTREFERENCE_INTPTRS
290+
if (!reference.IsValid && JniManagedPeerState.HasFlag (Disposed)) {
291+
Java.Interop.JniObjectReferenceControlBlock.Free (ref jniObjectReferenceControlBlock);
292+
}
293+
#endif // FEATURE_JNIOBJECTREFERENCE_INTPTRS
290294
}
291295

292296
IntPtr IJavaPeerable.JniObjectReferenceControlBlock =>

‎src/Java.Interop/Java.Interop/JavaObject.cs‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ namespace Java.Interop
88
{
99
[JniTypeSignature ("java/lang/Object", GenerateJavaPeer=false)]
1010
[Serializable]
11-
unsafe public class JavaObject : IJavaPeerable
11+
unsafe public partial class JavaObject : IJavaPeerable
1212
{
1313
internal const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;
1414

@@ -120,9 +120,6 @@ public virtual void DisposeUnlessReferenced ()
120120

121121
protected virtual void Dispose (bool disposing)
122122
{
123-
#if FEATURE_JNIOBJECTREFERENCE_INTPTRS
124-
Java.Interop.JniObjectReferenceControlBlock.Free (ref jniObjectReferenceControlBlock);
125-
#endif // FEATURE_JNIOBJECTREFERENCE_INTPTRS
126123
}
127124

128125
public override bool Equals (object? obj)
@@ -153,11 +150,13 @@ public override unsafe int GetHashCode ()
153150

154151
void IJavaPeerable.Disposed ()
155152
{
153+
managedPeerState |= Disposed;
156154
Dispose (disposing: true);
157155
}
158156

159157
void IJavaPeerable.Finalized ()
160158
{
159+
managedPeerState |= Disposed;
161160
Dispose (disposing: false);
162161
}
163162

@@ -174,6 +173,12 @@ void IJavaPeerable.SetJniManagedPeerState (JniManagedPeerStates value)
174173
void IJavaPeerable.SetPeerReference (JniObjectReference reference)
175174
{
176175
SetPeerReference (ref reference, JniObjectReferenceOptions.Copy);
176+
177+
#if FEATURE_JNIOBJECTREFERENCE_INTPTRS
178+
if (!reference.IsValid && managedPeerState.HasFlag (Disposed)) {
179+
Java.Interop.JniObjectReferenceControlBlock.Free (ref jniObjectReferenceControlBlock);
180+
}
181+
#endif // FEATURE_JNIOBJECTREFERENCE_INTPTRS
177182
}
178183

179184
IntPtr IJavaPeerable.JniObjectReferenceControlBlock =>

‎src/Java.Interop/Java.Interop/JniManagedPeerStates.cs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,4 +11,12 @@ public enum JniManagedPeerStates {
1111
/// <include file="../Documentation/Java.Interop/JniManagedPeerStates.xml" path="/docs/member[@name='F:Replaceable']/*" />
1212
Replaceable = (1 << 1),
1313
}
14+
15+
partial class JavaObject {
16+
const JniManagedPeerStates Disposed = (JniManagedPeerStates) (1 << 2);
17+
}
18+
19+
partial class JavaException {
20+
const JniManagedPeerStates Disposed = (JniManagedPeerStates) (1 << 2);
21+
}
1422
}

‎tests/Java.Interop-Tests/Java.Interop/JavaObjectArrayTest.cs‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,30 @@ protected override bool SequenceEqual (IEnumerable<T> a, IEnumerable<T> b)
3939
{
4040
return JniMarshal.RecursiveEquals (a, b);
4141
}
42+
43+
int beforeTestGrefCount;
44+
45+
[SetUp]
46+
public void LogBeginCurrentTestGrefCount ()
47+
{
48+
beforeTestGrefCount = JniEnvironment.Runtime.GlobalReferenceCount;
49+
JniEnvironment.Runtime.ObjectReferenceManager.WriteGlobalReferenceLine(
50+
"{0}",
51+
$"Begin {TestContext.CurrentContext.Test.FullName}; " +
52+
$"GREFs={beforeTestGrefCount}; "
53+
);
54+
}
55+
56+
[TearDown]
57+
public void LogEndCurrentTestToGrefCount ()
58+
{
59+
int afterTestGrefCount = JniEnvironment.Runtime.GlobalReferenceCount;
60+
JniEnvironment.Runtime.ObjectReferenceManager.WriteGlobalReferenceLine(
61+
"{0}",
62+
$"End {TestContext.CurrentContext.Test.FullName}; " +
63+
$"GREFs={afterTestGrefCount}; diff={afterTestGrefCount - beforeTestGrefCount}"
64+
);
65+
}
4266
}
4367

4468
[TestFixture]

0 commit comments

Comments
 (0)