[Java.Interop] JavaObject: GREFs by default, not LREFs.

Fixes: #6
Context: https://trello.com/c/EXxOMZOD/397-add-java-lang-object-unregisterfromvm

After presenting parts of the Java.Interop design to various other
Xamarin employees at the 2014 Xummit, I got lots of pushback about the
idea of not registering JavaObject instances by default and using
JNI Local References by default.

The problem/fear: it would be a usability nightmare. Single-threaded
use kinda/sorta works...if you never leave the method. If another
thread tries to access your JavaObject instance, you blow up. If you
call Java code which calls back into managed code tries to grab that
instance, you may blow up, as the value wasn't registered.

It's not good.

Then there's the future Android side of things: Android only permits
512 JNI local references; allocate more, and you abort.

That's *really* not good.

Partially revert commit 1caf077: don't use JNI Local References, use
JNI Global References, *always*. (This fixes Android.)

Additionally, register JavaObject and JavaException instances by
default. Allow them to be *unregistered* via the new
IJavaPeerable.UnregisterWithRuntime() method, *or* the new
JniObjectReferenceOptions.DoNotRegisterWithRuntime enum value.
The former will remove an existing registration; the latter will
prevent it from being registered AT ALL [0].

[0]:  This is a lie: we'll register the instance for the duration of
      the constructor, the unregister it automatically [1].

[1]:  I'm not sure if this is a good idea or not. On the one time, we
      *need* the registration within the constructor to support
      Java-side invocation of virtual members: the instance MUST be
      registered so that we lookup the appropriate instance for
      virtual method dispatch at a later point in time.

      However, it also means that it *is* being registered, if only
      for a short period, and thus when sharing an instance from
      multiple threads, each trying to register/unregister, things may
      fail badly; see e.g. [2, 3]

      This idea requires further consideration; yet another reason
      JavaObject isn't ready for Xamarin.Android integration.

[2]: https://bugzilla.xamarin.com/show_bug.cgi?id=14999
[3]: xamarin/monodroid@8138649

Author: jonpryor

src/Java.Interop.Dynamic/Tests/Java.Interop.Dynamic/DynamicJavaInstanceTests.cs

@@ -26,8 +26,8 @@ public void Constructor ()
 		public void DisposeWithJavaObjectDisposesObject ([Values (true, false)] bool register)
 		{
 			var native      = new JavaObject ();
-			if (register) {
-				native.RegisterWithVM ();
+			if (!register) {
+				native.UnregisterFromRuntime ();
 			}
 
 			var instance    = new DynamicJavaInstance (native);

src/Java.Interop.Export/Tests/Java.Interop/ExportedMemberBuilderTest.cs

@@ -39,7 +39,6 @@ public void AddExportMethods ()
 				Assert.IsTrue (ExportTest.StaticActionInt32StringCalled);
 
 				using (var o = CreateExportTest (t)) {
-					o.RegisterWithVM ();
 					t.GetInstanceMethod ("testMethods", "()V").InvokeVirtualVoidMethod (o.PeerReference);
 					Assert.IsTrue (o.HelloCalled);
 					o.Dispose ();

src/Java.Interop/Java.Interop/IJavaPeerable.cs

@@ -7,7 +7,7 @@ public interface IJavaPeerable : IDisposable
 		JniObjectReference      PeerReference  {get;}
 		JniPeerMembers          JniPeerMembers {get;}
 
-		void    RegisterWithVM ();
+		void    UnregisterFromRuntime ();
 		void    DisposeUnlessRegistered ();
 	}
 }

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

@@ -130,9 +130,11 @@ protected SetSafeHandleCompletion SetPeerReference (ref JniObjectReference handl
 					a => new SetSafeHandleCompletion (a));
 		}
 
-		public void RegisterWithVM ()
+		public void UnregisterFromRuntime ()
 		{
-			JniEnvironment.Runtime.RegisterObject (this);
+			if (!PeerReference.IsValid)
+				throw new ObjectDisposedException (GetType ().FullName);
+			JniEnvironment.Runtime.UnRegisterObject (this);
 		}
 
 		public void Dispose ()

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

@@ -71,9 +71,11 @@ protected SetPeerReferenceCompletion SetPeerReference (ref JniObjectReference ha
 					a => new SetPeerReferenceCompletion (a));
 		}
 
-		public void RegisterWithVM ()
+		public void UnregisterFromRuntime ()
 		{
-			JniEnvironment.Runtime.RegisterObject (this);
+			if (!PeerReference.IsValid)
+				throw new ObjectDisposedException (GetType ().FullName);
+			JniEnvironment.Runtime.UnRegisterObject (this);
 		}
 
 		public void Dispose ()

src/Java.Interop/Java.Interop/JavaProxyObject.cs

@@ -62,7 +62,6 @@ public static JavaProxyObject GetProxy (object value)
 				if (CachedValues.TryGetValue (value, out proxy))
 					return proxy;
 				proxy = new JavaProxyObject (value);
-				proxy.RegisterWithVM ();
 				CachedValues.Add (value, proxy);
 				return proxy;
 			}

src/Java.Interop/Java.Interop/JniEnvironment.Errors.cs

@@ -12,9 +12,6 @@ public static void Throw (Exception e)
 				var je = e as JavaException;
 				if (je == null) {
 					je  = new JavaProxyThrowable (e);
-					// because `je` may cross thread boundaries;
-					// We'll need to rely on the GC to cleanup
-					je.RegisterWithVM ();
 				}
 				Throw (je.PeerReference);
 			}

src/Java.Interop/Java.Interop/JniEnvironment.References.cs

@@ -38,8 +38,6 @@ public static void Dispose (ref JniObjectReference reference, JniObjectReference
 					}
 					reference.Invalidate ();
 					break;
-				default:
-					throw new NotImplementedException ("Do not know how to transfer: " + transfer);
 				}
 			}
 

src/Java.Interop/Java.Interop/JniObjectReferenceOptions.cs

@@ -8,10 +8,7 @@ public enum JniObjectReferenceOptions
 		Invalid                     = 0,
 		CreateNewReference          = 1 << 0,   // DoNotTransfer
 		DisposeSourceReference      = 1 << 1,   // Transfer
-
-		/*
-		Unregistered                = 4,
-		 */
+		DoNotRegisterWithRuntime    = 1 << 2,
 	}
 }
 

src/Java.Interop/Java.Interop/JniRuntime.cs

@@ -327,7 +327,8 @@ internal void RegisterObject<T> (T value)
 			value.Registered = true;
 		}
 
-		internal void UnRegisterObject (IJavaPeerableEx value)
+		internal void UnRegisterObject<T> (T value)
+			where T : IJavaPeerable, IJavaPeerableEx
 		{
 			int key = value.IdentityHashCode;
 			lock (RegisteredInstances) {
@@ -341,27 +342,23 @@ internal void UnRegisterObject (IJavaPeerableEx value)
 			}
 		}
 
-		internal TCleanup SetObjectPeerReference<T, TCleanup> (T value, ref JniObjectReference reference, JniObjectReferenceOptions transfer, Func<Action, TCleanup> createCleanup)
+		internal TCleanup SetObjectPeerReference<T, TCleanup> (T value, ref JniObjectReference reference, JniObjectReferenceOptions options, Func<Action, TCleanup> createCleanup)
 			where T : IJavaPeerable, IJavaPeerableEx
 			where TCleanup : IDisposable
 		{
 			if (!reference.IsValid)
 				throw new ArgumentException ("handle is invalid.", nameof (reference));
 
-			bool register   = reference.Flags == JniObjectReferenceFlags.Alloc;
-
-			value.SetPeerReference (reference.NewLocalRef ());
-			JniEnvironment.References.Dispose (ref reference, transfer);
+			var newRef      = reference.NewGlobalRef ();
+			value.SetPeerReference (newRef);
+			JniEnvironment.References.Dispose (ref reference, options);
 
-			value.IdentityHashCode = JniSystem.IdentityHashCode (value.PeerReference);
+			value.IdentityHashCode = JniSystem.IdentityHashCode (newRef);
 
-			if (register) {
-				RegisterObject (value);
+			RegisterObject (value);
+			if ((options & JniObjectReferenceOptions.DoNotRegisterWithRuntime) == JniObjectReferenceOptions.DoNotRegisterWithRuntime) {
 				Action unregister = () => {
 					UnRegisterObject (value);
-					var o = value.PeerReference;
-					value.SetPeerReference (o.NewLocalRef ());
-					JniEnvironment.References.Dispose (ref o);
 				};
 				return createCleanup (unregister);
 			}
@@ -461,7 +458,7 @@ public IJavaPeerable GetObject (ref JniObjectReference reference, JniObjectRefer
 				return null;
 
 			var existing = PeekObject (reference);
-			if (existing != null && targetType != null && targetType.IsInstanceOfType (existing)) {
+			if (existing != null && (targetType == null || targetType.IsInstanceOfType (existing))) {
 				JniEnvironment.References.Dispose (ref reference, transfer);
 				return existing;
 			}
@@ -742,9 +739,6 @@ public virtual void RaisePendingException (Exception pendingException)
 			var je  = pendingException as JavaException;
 			if (je == null) {
 				je  = new JavaProxyThrowable (pendingException);
-				// because `je` may cross thread boundaries;
-				// We'll need to rely on the GC to cleanup
-				je.RegisterWithVM ();
 			}
 			JniEnvironment.Exceptions.Throw (je.PeerReference);
 #endif  // !XA_INTEGRATION

src/Java.Interop/Tests/Java.Interop/InvokeVirtualFromConstructorTests.cs

@@ -14,7 +14,7 @@ public void InvokeVirtualFromConstructor ()
 		{
 			using (var t = new CallVirtualFromConstructorDerived (42)) {
 				Assert.IsTrue (t.Called);
-				Assert.IsNull (JniRuntime.Current.PeekObject (t.PeerReference));
+				Assert.IsNotNull (JniRuntime.Current.PeekObject (t.PeerReference));
 			}
 		}
 	}

src/Java.Interop/Tests/Java.Interop/JavaExceptionTests.cs

@@ -93,7 +93,6 @@ static JavaException CreateJavaProxyThrowable (Exception value)
 				.Assembly
 				.GetType ("Java.Interop.JavaProxyThrowable", throwOnError :true);
 			var proxy   = (JavaException) Activator.CreateInstance (JavaProxyThrowable_type, value);
-			proxy.RegisterWithVM ();
 			return proxy;
 		}
 	}

src/Java.Interop/Tests/Java.Interop/JavaObjectTest.cs

@@ -18,11 +18,9 @@ public void JavaReferencedInstanceSurvivesCollection ()
 			using (var t = new JniType ("java/lang/Object")) {
 				var oldHandle = IntPtr.Zero;
 				var array     = new JavaObjectArray<JavaObject> (1);
-				array.RegisterWithVM ();
 				var w = new Thread (() => {
 						var v       = new JavaObject ();
 						oldHandle   = v.PeerReference.Handle;
-						v.RegisterWithVM ();
 						array [0] = v;
 				});
 				w.Start ();
@@ -34,6 +32,7 @@ public void JavaReferencedInstanceSurvivesCollection ()
 				Assert.IsNotNull (JniRuntime.Current.PeekObject (first.PeerReference));
 				var f = first.PeerReference;
 				var o = (JavaObject) JniRuntime.Current.GetObject (ref f, JniObjectReferenceOptions.CreateNewReference);
+				Assert.AreSame (first, o);
 				if (oldHandle != o.PeerReference.Handle) {
 					Console.WriteLine ("Yay, object handle changed; value survived a GC!");
 				} else {
@@ -45,39 +44,31 @@ public void JavaReferencedInstanceSurvivesCollection ()
 		}
 
 		[Test]
-		public void RegisterWithVM ()
+		public void UnregisterFromRuntime ()
 		{
 			int registeredCount = JniRuntime.Current.GetSurfacedObjects ().Count;
 			JniObjectReference l;
 			JavaObject o;
 			using (o = new JavaObject ()) {
 				l   = o.PeerReference.NewLocalRef ();
-				Assert.AreEqual (JniObjectReferenceType.Local, o.PeerReference.Type);
-				Assert.AreEqual (registeredCount, JniRuntime.Current.GetSurfacedObjects ().Count);
-				Assert.IsNull (JniRuntime.Current.PeekObject (l));
-				o.RegisterWithVM ();
-				Assert.AreNotSame (l, o.PeerReference);
 				Assert.AreEqual (JniObjectReferenceType.Global, o.PeerReference.Type);
-				JniEnvironment.References.Dispose (ref l);
-				l = o.PeerReference.NewLocalRef ();
-				Assert.AreEqual (registeredCount + 1, JniRuntime.Current.GetSurfacedObjects ().Count);
+				Assert.AreEqual (registeredCount+1, JniRuntime.Current.GetSurfacedObjects ().Count);
+				Assert.IsNotNull (JniRuntime.Current.PeekObject (l));
+				Assert.AreNotSame (l, o.PeerReference);
 				Assert.AreSame (o, JniRuntime.Current.PeekObject (l));
 			}
 			Assert.AreEqual (registeredCount, JniRuntime.Current.GetSurfacedObjects ().Count);
 			Assert.IsNull (JniRuntime.Current.PeekObject (l));
 			JniEnvironment.References.Dispose (ref l);
-			Assert.Throws<ObjectDisposedException> (() => o.RegisterWithVM ());
+			Assert.Throws<ObjectDisposedException> (() => o.UnregisterFromRuntime ());
 		}
 
 		[Test]
 		public void RegisterWithVM_ThrowsOnDuplicateEntry ()
 		{
 			using (var original = new JavaObject ()) {
-				original.RegisterWithVM ();
 				var p       = original.PeerReference;
-				var alias   = new JavaObject (ref p, JniObjectReferenceOptions.CreateNewReference);
-				Assert.Throws<NotSupportedException> (() => alias.RegisterWithVM ());
-				alias.Dispose ();
+				Assert.Throws<NotSupportedException> (() => new JavaObject (ref p, JniObjectReferenceOptions.CreateNewReference));
 			}
 		}
 
@@ -90,7 +81,6 @@ public void UnreferencedInstanceIsCollected ()
 					var v     = new JavaObject ();
 					oldHandle = v.PeerReference.NewWeakGlobalRef ();
 					r         = new WeakReference (v);
-					v.RegisterWithVM ();
 			});
 			t.Start ();
 			t.Join ();
@@ -127,6 +117,7 @@ public void Dispose_Finalized ()
 			t.Join ();
 			GC.Collect ();
 			GC.WaitForPendingFinalizers ();
+			GC.Collect ();
 			GC.WaitForPendingFinalizers ();
 			Assert.IsFalse (d);
 			Assert.IsTrue (f);
@@ -144,7 +135,7 @@ public void ObjectDisposed ()
 
 			// These should throw
 			Assert.Throws<ObjectDisposedException> (() => o.GetHashCode ());
-			Assert.Throws<ObjectDisposedException> (() => o.RegisterWithVM ());
+			Assert.Throws<ObjectDisposedException> (() => o.UnregisterFromRuntime ());
 			Assert.Throws<ObjectDisposedException> (() => o.ToString ());
 			Assert.Throws<ObjectDisposedException> (() => o.Equals (o));
 		}
@@ -184,29 +175,12 @@ public void CrossThreadSharingRequresRegistration ()
 			JavaObject o = null;
 			var t = new Thread (() => {
 					o = new JavaObject ();
-					o.RegisterWithVM ();
 			});
 			t.Start ();
 			t.Join ();
 			o.ToString ();
 			o.Dispose ();
 		}
-
-		[Test]
-		public void CrossThreadSharingNotSupported ()
-		{
-			if (!HaveSafeHandles) {
-				Assert.Ignore ("SafeHandles not used. Cross-thread sharing can't be checked.");
-				return;
-			}
-
-			JavaObject o = null;
-			var t = new Thread (() => o = new JavaObject ());
-			t.Start ();
-			t.Join ();
-			Assert.Throws<NotSupportedException> (() => o.ToString ());
-			o.Dispose ();
-		}
 	}
 
 	class JavaObjectWithNoJavaPeer : JavaObject {

src/Java.Interop/Tests/Java.Interop/JavaVMTest.cs

@@ -61,10 +61,11 @@ public void GetRegisteredJavaVM_ExistingInstance ()
 		public void GetObject_ReturnsAlias ()
 		{
 			var local   = new JavaObject ();
+			local.UnregisterFromRuntime ();
 			Assert.IsNull (JniRuntime.Current.PeekObject (local.PeerReference));
 			// GetObject must always return a value (unless handle is null, etc.).
-			// However, since we didn't call local.RegisterWithVM(),
-			// JavaVM.PeekObject() is null (asserted above), but GetObject() must
+			// However, since we called local.UnregisterFromRuntime(),
+			// JniRuntime.PeekObject() is null (asserted above), but GetObject() must
 			// **still** return _something_.
 			// In this case, it returns an _alias_.
 			// TODO: "most derived type" alias generation. (Not relevant here, but...)
@@ -88,8 +89,6 @@ public void GetObject_ReturnsRegisteredInstance ()
 			JniObjectReference lref;
 			using (var o = new JavaObject ()) {
 				lref = o.PeerReference.NewLocalRef ();
-				Assert.IsNull (JniRuntime.Current.PeekObject (lref));
-				o.RegisterWithVM ();
 				Assert.AreSame (o, JniRuntime.Current.PeekObject (lref));
 			}
 			// At this point, the Java-side object is kept alive by `lref`,

src/Java.Interop/Tests/Java.Interop/JniTransitionTest.cs

@@ -19,7 +19,7 @@ public void Dispose_ClearsLocalReferences ()
 			}
 			JniObjectReference lref;
 			using (var envp = new JniTransition (JniEnvironment.EnvironmentPointer)) {
-				lref    = new JavaObject ().PeerReference;
+				lref    = new JavaObject ().PeerReference.NewLocalRef ();
 				Assert.IsTrue (lref.IsValid);
 			}
 			Assert.IsFalse (lref.IsValid);

src/Java.Interop/Tests/Java.Interop/TestType.cs

@@ -86,8 +86,11 @@ static Delegate GetEqualsThisHandler ()
 		{
 			Func<IntPtr, IntPtr, IntPtr, bool> h = (jnienv, n_self, n_value) => {
 				var jvm     = JniEnvironment.Runtime;
-				var self    = jvm.GetObject<TestType>(n_self);
-				var value   = jvm.GetObject (n_value);
+				var r_self  = new JniObjectReference (n_self);
+				var p = JniEnvironment.Runtime.PeekObject (r_self);
+				var self    = jvm.GetObject<TestType>(ref r_self, JniObjectReferenceOptions.DoNotRegisterWithRuntime);
+				var r_value = new JniObjectReference (n_self);
+				var value   = jvm.GetObject (ref r_value, JniObjectReferenceOptions.DoNotRegisterWithRuntime);
 
 				try {
 					return self.EqualsThis (value);
@@ -102,7 +105,8 @@ static Delegate GetEqualsThisHandler ()
 		static Delegate GetInt32ValueHandler ()
 		{
 			Func<IntPtr, IntPtr, int> h = (jnienv, n_self) => {
-				var self = JniEnvironment.Runtime.GetObject<TestType>(n_self);
+				var r_self  = new JniObjectReference (n_self);
+				var self    = JniEnvironment.Runtime.GetObject<TestType>(ref r_self, JniObjectReferenceOptions.DoNotRegisterWithRuntime);
 				try {
 					return self.GetInt32Value ();
 				} finally {
@@ -120,7 +124,8 @@ static Delegate _GetStringValueHandler ()
 
 		static IntPtr GetStringValueHandler (IntPtr jnienv, IntPtr n_self, int value)
 		{
-			var self = JniEnvironment.Runtime.GetObject<TestType>(n_self);
+			var r_self  = new JniObjectReference (n_self);
+			var self    = JniEnvironment.Runtime.GetObject<TestType>(ref r_self, JniObjectReferenceOptions.DoNotRegisterWithRuntime);
 			try {
 				var s = self.GetStringValue (value);
 				var r = JniEnvironment.Strings.NewString (s);
@@ -136,7 +141,8 @@ static IntPtr GetStringValueHandler (IntPtr jnienv, IntPtr n_self, int value)
 
 		static void MethodThrowsHandler (IntPtr jnienv, IntPtr n_self)
 		{
-			var self = JniEnvironment.Runtime.GetObject<TestType> (n_self);
+			var r_self  = new JniObjectReference (n_self);
+			var self    = JniEnvironment.Runtime.GetObject<TestType>(ref r_self, JniObjectReferenceOptions.DoNotRegisterWithRuntime);
 			try {
 				self.MethodThrows ();
 			} finally {

src/Java.Interop/Tests/Java.Interop/TestTypeTests.cs

@@ -29,6 +29,7 @@ public void EndArrayTests ()
 		public void TestCase ()
 		{
 			using (var t = new TestType ()) {
+				t.UnregisterFromRuntime ();
 				t.RunTests ();
 			}
 		}