From e6468d2900b63066c683a7478e22847e72dc7632 Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Tue, 10 Nov 2015 14:56:56 -0500 Subject: [PATCH] [Java.Interop] Fix JNI reference count assertions The JNI reference count assertions in JniRuntime.JniObjectReferenceManager were "off," in that the values they were asserting didn't actually make sense. Previous logic for JNI local reference create + destroy: // Assume localReferenceCount starts at 0, because of course it does Assert (localReferenceCount >= 0); // True; 0 >= 0 localReferenceCount++; // localReferenceCount == 1 ... Assert (localReferenceCount >= 0); // True; 1 >= 0 localReferenceCount--; // localReferenceCount == 0 The problem with this logic is that it doesn't actually make sense; when localReferenceCount is incremented to 1, there is one reference in existence; conceptually, then, the created reference *is* #1. Meanwhile, at *cleanup*, we first check that localReferenceCount is valid, *before* we decrement it. We're not validating that e.g. reference "#1" has been destroyed, or that the number of outstanding references *after* cleanup is identical to what existed *before* it was created. In short, the "dispose" check is in the wrong place. It should be done *after* decrementing the count, not before: Assert (localReferenceCount >= 0); // True; 0 >= 0 localReferenceCount++; // localReferenceCount == 1 ... localReferenceCount--; // localReferenceCount == 0 Assert (localReferenceCount >= 0); // True; 0 >= 0 This dovetails nicely with LoggingJniObjectReferenceManagerDecorator behavior, in that the logging should follow the same pattern as the count updating: log after create, before delete. If/when reference lifetimes are entirely nested and not overlapping, this allows for lrefc value "1" on create to have the same lrefc value "1" on destroy. --- src/Java.Interop/Java.Interop/JavaException.cs | 4 ++++ .../Java.Interop/JniRuntime.JniObjectReferenceManager.cs | 8 ++++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/Java.Interop/Java.Interop/JavaException.cs b/src/Java.Interop/Java.Interop/JavaException.cs index a99d01f180a..b25d16d444c 100644 --- a/src/Java.Interop/Java.Interop/JavaException.cs +++ b/src/Java.Interop/Java.Interop/JavaException.cs @@ -157,6 +157,10 @@ public void DisposeUnlessRegistered () protected virtual void Dispose (bool disposing) { + var inner = InnerException as JavaException; + if (inner != null) { + inner.Dispose (); + } } public override bool Equals (object obj) diff --git a/src/Java.Interop/Java.Interop/JniRuntime.JniObjectReferenceManager.cs b/src/Java.Interop/Java.Interop/JniRuntime.JniObjectReferenceManager.cs index 939580404f9..c800dde8e46 100644 --- a/src/Java.Interop/Java.Interop/JniRuntime.JniObjectReferenceManager.cs +++ b/src/Java.Interop/Java.Interop/JniRuntime.JniObjectReferenceManager.cs @@ -63,8 +63,8 @@ public virtual void DeleteLocalReference (ref JniObjectReference reference, ref if (!reference.IsValid) return; AssertReferenceType (ref reference, JniObjectReferenceType.Local); - AssertCount (localReferenceCount, "LREF", reference.ToString ()); localReferenceCount--; + AssertCount (localReferenceCount, "LREF", reference.ToString ()); JniEnvironment.References.DeleteLocalRef (reference.Handle); reference.Invalidate (); } @@ -96,8 +96,8 @@ public virtual IntPtr ReleaseLocalReference (ref JniObjectReference reference, r { if (!reference.IsValid) return IntPtr.Zero; - AssertCount (localReferenceCount, "LREF", reference.ToString ()); localReferenceCount--; + AssertCount (localReferenceCount, "LREF", reference.ToString ()); var h = reference.Handle; reference.Invalidate (); return h; @@ -121,8 +121,8 @@ public virtual void DeleteGlobalReference (ref JniObjectReference reference) if (!reference.IsValid) return; AssertReferenceType (ref reference, JniObjectReferenceType.Global); - AssertCount (grefc, "GREF", reference.ToString ()); Interlocked.Decrement (ref grefc); + AssertCount (grefc, "GREF", reference.ToString ()); JniEnvironment.References.DeleteGlobalRef (reference.Handle); reference.Invalidate (); } @@ -141,8 +141,8 @@ public virtual void DeleteWeakGlobalReference (ref JniObjectReference reference) if (!reference.IsValid) return; AssertReferenceType (ref reference, JniObjectReferenceType.WeakGlobal); - AssertCount (wgrefc, "WGREF", reference.ToString ()); Interlocked.Decrement (ref wgrefc); + AssertCount (wgrefc, "WGREF", reference.ToString ()); JniEnvironment.References.DeleteWeakGlobalRef (reference.Handle); reference.Invalidate (); }