Skip to content

Commit 384bbfd

Browse files
jonpryorjonathanpeppers
jonpryor
authored and
jonathanpeppers
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 a29fe5e commit 384bbfd

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));
@@ -171,9 +171,6 @@ protected virtual void Dispose (bool disposing)
171171
if (inner != null) {
172172
inner.Dispose ();
173173
}
174-
#if FEATURE_JNIOBJECTREFERENCE_INTPTRS
175-
Java.Interop.JniObjectReferenceControlBlock.Free (ref jniObjectReferenceControlBlock);
176-
#endif // FEATURE_JNIOBJECTREFERENCE_INTPTRS
177174
}
178175

179176
public override bool Equals (object? obj)
@@ -267,11 +264,13 @@ protected void SetJavaStackTrace (JniObjectReference peerReferenceOverride = def
267264

268265
void IJavaPeerable.Disposed ()
269266
{
267+
JniManagedPeerState |= Disposed;
270268
Dispose (disposing: true);
271269
}
272270

273271
void IJavaPeerable.Finalized ()
274272
{
273+
JniManagedPeerState |= Disposed;
275274
Dispose (disposing: false);
276275
}
277276

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

293297
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

@@ -122,9 +122,6 @@ public virtual void DisposeUnlessReferenced ()
122122

123123
protected virtual void Dispose (bool disposing)
124124
{
125-
#if FEATURE_JNIOBJECTREFERENCE_INTPTRS
126-
Java.Interop.JniObjectReferenceControlBlock.Free (ref jniObjectReferenceControlBlock);
127-
#endif // FEATURE_JNIOBJECTREFERENCE_INTPTRS
128125
}
129126

130127
public override bool Equals (object? obj)
@@ -155,11 +152,13 @@ public override unsafe int GetHashCode ()
155152

156153
void IJavaPeerable.Disposed ()
157154
{
155+
managedPeerState |= Disposed;
158156
Dispose (disposing: true);
159157
}
160158

161159
void IJavaPeerable.Finalized ()
162160
{
161+
managedPeerState |= Disposed;
163162
Dispose (disposing: false);
164163
}
165164

@@ -176,6 +175,12 @@ void IJavaPeerable.SetJniManagedPeerState (JniManagedPeerStates value)
176175
void IJavaPeerable.SetPeerReference (JniObjectReference reference)
177176
{
178177
SetPeerReference (ref reference, JniObjectReferenceOptions.Copy);
178+
179+
#if FEATURE_JNIOBJECTREFERENCE_INTPTRS
180+
if (!reference.IsValid && managedPeerState.HasFlag (Disposed)) {
181+
Java.Interop.JniObjectReferenceControlBlock.Free (ref jniObjectReferenceControlBlock);
182+
}
183+
#endif // FEATURE_JNIOBJECTREFERENCE_INTPTRS
179184
}
180185

181186
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)