[Java.Interop] Avoid JNIEnv::NewObject().

(This unexpectedly gigantic 2653 line patch brought to you by Isanity,
Experience, and YOU DON'T KNOW, MAN! YOU WEREN'T THERE!)

JNI, in its infinite wisdom, has two different mechanisms to create an
instance of a Java object: the easy way, and the correct way.

 1. The Easy Way: JNIEnv::NewObject()
 2. The Correct Way: JNIEnv::AllocObject() +
    JNIEnv::CallNonvirtualVoidMethod()

The easy way is JNIEnv::NewObject(class, constructor, args): give it
the type, the constructor, and (optional) arguments, and you get back
an object.

The correct way is JNIEnv::AllocObject(), which _creates_ the object
but DOES NOT RUN THE CONSTRUCTOR, followed by
JNIEnv::CallNonvirtualVoidMethod(), which executes the constructor.

Why does it matter? Leaky abstractions, and virtual method invocation
from the constructor.

Just as with C#, and unlike C++, in Java virtual methods always
resolve to the most derived implementation:

	// C#
	class B
		{public B()
			{Console.WriteLine("B..ctor");
			CalledFromConstructor();}
		public virtual void CalledFromConstructor()
			{}}
	class D : B
		{public D()
			{Console.WriteLine ("d..ctor");}
		public override void CalledFromConstructor()
			{Console.WriteLine("D.CalledFromConstructor");}}

	// ...
	new D();

The above prints:

	B..ctor
	D.CalledFromConstructor
	d..ctor

Note that D.M()is invoked _before_ the D constructor executes!
(In actuality _part_ of the D constructor executes _first_, but that's
the part that ~immediately calls the B constructor. The only way to
intervene in this chain is with member initializers, and "hacks" with
constructor parameters.)

The "problem" is that the same thing happens in Java, and if `D` has a
native method implemented in C#, and JNIEnv::NewObject() is used, then
things go all "funky," because the JNI method marshaler could be
called before there's a C# instance to invoke the method upon:

	// Hypothetical D.CalledFromConstructorHandler impl
	partial class D {
		static void CalledFromConstructorHandler (IntPtr jnienv, IntPtr self)
		{
			var self = JavaVM.Current.GetObject<D>(n_self);
			self.CalledFromConstructor ();
		}
	}

So our runtime behavior would be:

	  C#: D..ctor(), eventually calls JNIEnv::NewObject()
	Java: B.<init>()
	Java: B.calledFromConstructor()
	Java: D.n_calledFromConstructor();
	  C#: D.CalledFromConstructorHandler()
	  C#: JavaVM.GetObject(): no object registered

At this point one of two things will happen:

Option 1: D has a (JniReferenceSafeHandle, JniHandleOwnership)
constructor:

	  C#: JavaVM.GetObject() creates a new "dummy" D
	  C#: D.CalledFromConstructorHandler() calls
	      D.CalledFromConstructor().

The problem is that at the end of this, the "original" execution path
is referencing _one_ 'D' instance, which is unrelated to the 'D'
instance that D.CalledFromConstructorHandler() created. There are
_two_ 'D' instances!

Insanity quickly follows.

Option 2: D doesn't have a (JniReferenceSafeHandle, JniHandleOwnership)
constructor.

	  C#: JavaVM.GetObject() throws an exception
	**BOOM**

Becuase we don't have correct exception propogation implemented yet,
this results in corrupting OpenJDK (we unwound native Java stack
frames!), and the process later aborts:

	malloc: *** error for object 0x79d8c248: Non-aligned pointer being freed
	*** set a breakpoint in malloc_error_break to debug
	Stacktrace:

	  at <unknown> <0xffffffff>
	  at (wrapper managed-to-native) object.wrapper_native_0x3a47b05 (Java.Interop.JniEnvironmentSafeHandle,Java.Interop.JniReferenceSafeHandle) <IL 0x00092, 0xffffffff>
	  at Java.Interop.JniEnvironment/Handles.NewLocalRef (Java.Interop.JniReferenceSafeHandle) [0x0001b] in /Users/Shared/Dropbox/Developer/Java.Interop/src/Java.Interop/Java.Interop/JniEnvironment.g.cs:881
	  at Java.Interop.JniReferenceSafeHandle.NewLocalRef () [0x00002] in /Users/Shared/Dropbox/Developer/Java.Interop/src/Java.Interop/Java.Interop/JniReferenceSafeHandle.cs:40
	  at Java.Interop.JavaVM.SetObjectSafeHandle<T> (T,Java.Interop.JniReferenceSafeHandle,Java.Interop.JniHandleOwnership) [0x00030] in /Users/Shared/Dropbox

The above stacktrace is meaningless; the corruption happened long ago,
and things are blowing up.

So the problem with JNIEnv::NewObject() is that it implicitly requires
creating "throwaway" instances, and very often you DO NOT WANT
"throwaway" instances to be created!

What's the fix then? JNIEnv::AllocObject(), which does NOT invoke the
Java constructor. This allows us to (temporarily!) register the
instance _during constructor execution_ so that JNI method marshalers
can lookup the already created instance. This results in the saner
control flow:

	  C#: D..ctor(), eventually calls JNIEnv::AllocObject()
	  C#: D instance is registered with AllocObject()'d handle.
	  C#: JNIEnv::CallNonvirtualVoidMethod() invoked on B.<init>()
	Java: B.<init>()
	Java: B.calledFromConstructor()
	Java: D.n_calledFromConstructor();
	  C#: D.CalledFromConstructorHandler()
	  C#: JavaVM.GetObject(): registered instance found, used.
	- execution returns back through B.<init>() to D..ctor()

No hair is lost in the process.

There is one "minor" problem here, though: Android.

Specifically, Android prior to v3.0 Honeycomb doesn't properly support
JNIEnv::AllocObject() + JNIEnv::CallNonvirtualVoidMethod(); it throws
CloneNotSupportedException:

	https://code.google.com/p/android/issues/detail?id=13832

Using JNIEnv::NewObject() leads to insanity. Requiring
JNIEnv::AllocObject() means things CANNOT work on Android < v3.0.

The fix? Options! (Or don't support older Android versions...)

Add a new JavaVMOptions.NewObjectRequired property: if True, then
JNIEnv::NewObject() is used. If False (the default!), then
JNIEnv::AllocObject() is used, sanity is retained, and everyone
rejoices. (Yay.)

(~150 lines to describe WHAT's being addressed!)

How's this implemented? Through three sets of methods:

 1. JniPeerInstanceMethods.StartCreateInstance()
 2. JniPeerInstanceMethods.FinishCreateInstance()
 3. JavaObject.SetSafeHandle(), JavaException.SetSafeHandle().

JniPeerInstanceMethods.StartCreateInstance() kicks things off: if
JavaVMOptions.NewObjectRequired is True, then it's
JNIEnv::NewObject(); if False, it's JNIEnv::AllocObject().
StartCreateInstance() returns the JNI handle.

JniPeerInstanceMethods.FinishCreateInstance() does nothing if
JavaVMOptions.NewObjectRequired is True; if false, then it calls
JNIEnv::CallNonvirtualVoidMethod().

JavaObject.SetSafeHandle() and JavaException.SetSafeHandle() ties them
all together: it takes the handle returned from
JniPeerInstanceMethods.StartCreateInstance(), (optionally) registers
the instance, and then (later) will unregister the instance.

	class SomeClass : JavaObject {

		public SomeClass(Args...)
		{
			using (SetSafeHandle (
						JniPeerMembers.InstanceMethods.StartCreateInstance (JNI_SIGNATURE, GetType (), args...),
						JniHandleOwnership.Transfer)) {
				JniPeerMembers.InstanceMethods.FinishCreateInstance (JNI_SIGNATURE, this, args...);
			}
	}

Flow of control (ideal case):

 1. StartCreateInstance() calls JNIEnv::AllocObject()
 2. SetSafeHandle() registers the handle::instance mapping, returns
    "Cleanup" instance.
 3. FinishCreateInstance() calls JNIEnv::CallNonvirtualVoidMethod() to
    invoke the constructor.
 4. "Cleanup".Dispose() unregisters the instance mapping from (2).

Just as with JniPeerInstanceMethods.Call*Method(),
JniPeerInstanceMethods.StartCreateInstance() and
JniPeerInstanceMethods.FinishCreateInstance() are overloaded to
generically marshal up to 16 parameters.

Finally, CACHING: we need the appropriate jclass and jmethodID for the
most derived subclass that we're creating. In Xamarin.Android these
WERE NOT CACHED: they were looked up (and destroyed) when creating
each instance of a type with an "Android Callable Wrapper" (generated
Java "stub" type). Only jclass and constructor jmethodID instances for
"normal" Java types were cached.

JniPeerInstanceMethods DOES cache: it maintains a
JniPeerInstanceMethods.SubclassConstructors mapping that tracks all
encountered subclasses and their constructors, so that future instance
creation doesn't need to relookup the jclass and jmethodID values.

Author: jonpryor

src/Java.Interop/Java.Interop.csproj

@@ -36,6 +36,7 @@
   <ItemGroup>
     <Compile Include="Properties\AssemblyInfo.cs" />
     <Compile Include="Java.Interop\JavaVM.cs" />
+    <Compile Include="Java.Interop\JniAllocObjectRef.cs" />
     <Compile Include="Java.Interop\JniReferenceSafeHandle.cs" />
     <Compile Include="Java.Interop\JniInstanceMethodID.cs" />
     <Compile Include="Java.Interop\JniEnvironment.cs" />

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

@@ -14,28 +14,45 @@ public class JavaException : Exception, IJavaObject, IJavaObjectEx
 
 		public JavaException ()
 		{
-			JavaVM.SetObjectSafeHandle (this, _NewObject (GetType (), JniPeerMembers), JniHandleOwnership.Transfer);
+			using (SetSafeHandle (
+						JniPeerMembers.InstanceMethods.StartCreateInstance ("()V", GetType ()),
+						JniHandleOwnership.Transfer)) {
+				JniPeerMembers.InstanceMethods.FinishCreateInstance ("()V", this);
+			}
 			javaStackTrace    = _GetJavaStack (SafeHandle);
 		}
 
 		public JavaException (string message)
 			: base (message)
 		{
-			JavaVM.SetObjectSafeHandle (this, _NewObject (GetType (), JniPeerMembers), JniHandleOwnership.Transfer);
+			const string signature  = "(Ljava/lang/String;)V";
+			using (SetSafeHandle (
+						JniPeerMembers.InstanceMethods.StartCreateInstance (signature, GetType (), message),
+						JniHandleOwnership.Transfer)) {
+				JniPeerMembers.InstanceMethods.FinishCreateInstance (signature, this, message);
+			}
 			javaStackTrace    = _GetJavaStack (SafeHandle);
 		}
 
 		public JavaException (string message, Exception innerException)
 			: base (message, innerException)
 		{
-			JavaVM.SetObjectSafeHandle (this, _NewObject (GetType (), JniPeerMembers), JniHandleOwnership.Transfer);
+			const string signature  = "(Ljava/lang/String;)V";
+			using (SetSafeHandle (
+						JniPeerMembers.InstanceMethods.StartCreateInstance (signature, GetType (), message),
+						JniHandleOwnership.Transfer)) {
+				JniPeerMembers.InstanceMethods.FinishCreateInstance (signature, this, message);
+			}
 			javaStackTrace    = _GetJavaStack (SafeHandle);
 		}
 
 		public JavaException (JniReferenceSafeHandle handle, JniHandleOwnership transfer)
 			: base (_GetMessage (handle), _GetCause (handle))
 		{
-			JavaVM.SetObjectSafeHandle (this, handle, transfer);
+			if (handle == null)
+				return;
+			using (SetSafeHandle (handle, transfer)) {
+			}
 			javaStackTrace    = _GetJavaStack (SafeHandle);
 		}
 
@@ -65,6 +82,15 @@ public override string StackTrace {
 			}
 		}
 
+		protected SetSafeHandleCompletion SetSafeHandle (JniReferenceSafeHandle handle, JniHandleOwnership transfer)
+		{
+			return JniEnvironment.Current.JavaVM.SetObjectSafeHandle (
+					this,
+					handle,
+					transfer,
+					a => new SetSafeHandleCompletion (a));
+		}
+
 		public void RegisterWithVM ()
 		{
 			JniEnvironment.Current.JavaVM.RegisterObject (this);
@@ -138,24 +164,6 @@ string _GetJavaStack (JniReferenceSafeHandle handle)
 			}
 		}
 
-		static JniLocalReference _NewObject (Type type, JniPeerMembers peerMembers)
-		{
-			var info    = JniEnvironment.Current.JavaVM.GetJniTypeInfoForType (type);
-			if (info.JniTypeName == null)
-				throw new NotSupportedException (
-					string.Format ("Cannot create instance of type '{0}': no Java peer type found.",
-						type.FullName));
-
-			if (type == peerMembers.ManagedPeerType) {
-				var c = peerMembers.InstanceMethods.GetConstructor ("()V");
-				return peerMembers.JniPeerType.NewObject (c);
-			}
-			using (var t = new JniType (info.ToString ()))
-			using (var c = t.GetConstructor ("()V")) {
-				return t.NewObject (c);
-			}
-		}
-
 		int IJavaObjectEx.IdentityHashCode {
 			get {return identity;}
 			set {identity = value;}
@@ -175,6 +183,22 @@ void IJavaObjectEx.SetSafeHandle (JniReferenceSafeHandle handle)
 		{
 			SafeHandle = handle;
 		}
+
+		protected struct SetSafeHandleCompletion : IDisposable {
+
+			readonly    Action  action;
+
+			public SetSafeHandleCompletion (Action action)
+			{
+				this.action = action;
+			}
+
+			public void Dispose ()
+			{
+				if (action != null)
+					action ();
+			}
+		}
 	}
 }
 

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

@@ -26,30 +26,28 @@ public  virtual JniPeerMembers              JniPeerMembers {
 
 		public JavaObject (JniReferenceSafeHandle handle, JniHandleOwnership transfer)
 		{
-			JavaVM.SetObjectSafeHandle (this, handle, transfer);
+			if (handle == null)
+				return;
+			using (SetSafeHandle (handle, transfer)) {
+			}
 		}
 
-		static JniLocalReference _NewObject (Type type, JniPeerMembers peerMembers)
+		public JavaObject ()
 		{
-			var info    = JniEnvironment.Current.JavaVM.GetJniTypeInfoForType (type);
-			if (info.JniTypeName == null)
-				throw new NotSupportedException (
-						string.Format ("Cannot create instance of type '{0}': no Java peer type found.",
-							type.FullName));
-
-			if (type == peerMembers.ManagedPeerType) {
-				var c = peerMembers.InstanceMethods.GetConstructor ("()V");
-				return peerMembers.JniPeerType.NewObject (c);
-			}
-			using (var t = new JniType (info.ToString ()))
-			using (var c = t.GetConstructor ("()V")) {
-				return t.NewObject (c);
+			using (SetSafeHandle (
+						JniPeerMembers.InstanceMethods.StartCreateInstance ("()V", GetType ()),
+						JniHandleOwnership.Transfer)) {
+				JniPeerMembers.InstanceMethods.FinishCreateInstance ("()V", this);
 			}
 		}
 
-		public JavaObject ()
+		protected SetSafeHandleCompletion SetSafeHandle (JniReferenceSafeHandle handle, JniHandleOwnership transfer)
 		{
-			JavaVM.SetObjectSafeHandle (this, _NewObject (GetType (), JniPeerMembers), JniHandleOwnership.Transfer);
+			return JniEnvironment.Current.JavaVM.SetObjectSafeHandle (
+					this,
+					handle,
+					transfer,
+					a => new SetSafeHandleCompletion (a));
 		}
 
 		public void RegisterWithVM ()
@@ -117,6 +115,22 @@ void IJavaObjectEx.SetSafeHandle (JniReferenceSafeHandle handle)
 		{
 			SafeHandle = handle;
 		}
+
+		protected struct SetSafeHandleCompletion : IDisposable {
+
+			readonly    Action  action;
+
+			public SetSafeHandleCompletion (Action action)
+			{
+				this.action = action;
+			}
+
+			public void Dispose ()
+			{
+				if (action != null)
+					action ();
+			}
+		}
 	}
 }
 

src/Java.Interop/Java.Interop/JavaVM.cs

@@ -83,6 +83,9 @@ public class JavaVMOptions {
 		public  bool        TrackIDs                    {get; set;}
 		public  bool        DestroyVMOnDispose          {get; set;}
 
+		// Prefer JNIEnv::NewObject() over JNIEnv::AllocObject() + JNIEnv::CallNonvirtualVoidMethod()
+		public  bool        NewObjectRequired           {get; set;}
+
 		public  JavaVMSafeHandle            VMHandle            {get; set;}
 		public  JniEnvironmentSafeHandle    EnvironmentHandle   {get; set;}
 
@@ -146,6 +149,8 @@ public static void SetCurrent (JavaVM newCurrent)
 
 		public  JavaVMSafeHandle                        SafeHandle      {get; private set;}
 
+		public  bool                                    NewObjectRequired   {get; private set;}
+
 		protected JavaVM (JavaVMOptions options)
 		{
 			if (options == null)
@@ -158,6 +163,8 @@ protected JavaVM (JavaVMOptions options)
 			TrackIDs     = options.TrackIDs;
 			DestroyVM    = options.DestroyVMOnDispose;
 
+			NewObjectRequired   = options.NewObjectRequired;
+
 			SafeHandle  = options.VMHandle;
 			Invoker     = SafeHandle.CreateInvoker ();
 
@@ -378,21 +385,36 @@ internal void UnRegisterObject (IJavaObjectEx value)
 						(t = (IJavaObject) wv.Target) != null &&
 						object.ReferenceEquals (value, t))
 					RegisteredInstances.Remove (key);
+				value.Registered = false;
 			}
 		}
 
-		internal static void SetObjectSafeHandle<T> (T value, JniReferenceSafeHandle handle, JniHandleOwnership transfer)
+		internal TCleanup SetObjectSafeHandle<T, TCleanup> (T value, JniReferenceSafeHandle handle, JniHandleOwnership transfer, Func<Action, TCleanup> createCleanup)
 			where T : IJavaObject, IJavaObjectEx
+			where TCleanup : IDisposable
 		{
 			if (handle == null)
 				throw new ArgumentNullException ("handle");
 			if (handle.IsInvalid)
 				throw new ArgumentException ("handle is invalid.", "handle");
 
+			bool register   = handle is JniAllocObjectRef;
+
 			value.SetSafeHandle (handle.NewLocalRef ());
 			JniEnvironment.Handles.Dispose (handle, transfer);
 
 			value.IdentityHashCode = JniSystem.IdentityHashCode (value.SafeHandle);
+
+			if (register) {
+				RegisterObject (value);
+				Action unregister = () => {
+					UnRegisterObject (value);
+					using (var g = value.SafeHandle)
+						value.SetSafeHandle (g.NewLocalRef ());
+				};
+				return createCleanup (unregister);
+			}
+			return createCleanup (null);
 		}
 
 		internal void DisposeObject<T> (T value)

src/Java.Interop/Java.Interop/JniAllocObjectRef.cs

@@ -0,0 +1,13 @@
+using System;
+
+namespace Java.Interop {
+
+	class JniAllocObjectRef : JniLocalReference
+	{
+		public JniAllocObjectRef (IntPtr handle)
+		{
+			SetHandle (handle);
+		}
+	}
+}
+

src/Java.Interop/Java.Interop/JniLocalReference.cs

@@ -27,5 +27,12 @@ internal IntPtr ReturnToJniRef ()
 			JniEnvironment.Current.LogDestroyLocalRef (h);
 			return h;
 		}
+
+		internal JniAllocObjectRef ToAllocObjectRef ()
+		{
+			var h   = handle;
+			handle  = IntPtr.Zero;
+			return new JniAllocObjectRef (h);
+		}
 	}
 }

src/Java.Interop/Java.Interop/JniPeerInstanceMethods.cs

@@ -7,38 +7,99 @@ public sealed partial class JniPeerInstanceMethods
 	{
 		internal JniPeerInstanceMethods (JniPeerMembers members)
 		{
-			Members = members;
+			DeclaringType   = members.ManagedPeerType;
+			JniPeerType     = members.JniPeerType;
 		}
 
-		readonly JniPeerMembers                             Members;
+		JniPeerInstanceMethods (Type declaringType)
+		{
+			var jvm     = JniEnvironment.Current.JavaVM;
+			var info    = jvm.GetJniTypeInfoForType (declaringType);
+			if (info.JniTypeName == null)
+				throw new NotSupportedException (
+						string.Format ("Cannot create instance of type '{0}': no Java peer type found.",
+							declaringType.FullName));
+
+			DeclaringType   = declaringType;
+			JniPeerType     = new JniType (info.ToString ());
+			JniPeerType.RegisterWithVM ();
+		}
+
+		readonly Type                                       DeclaringType;
+		readonly JniType                                    JniPeerType;
 		readonly Dictionary<string, JniInstanceMethodID>    InstanceMethods = new Dictionary<string, JniInstanceMethodID>();
+		readonly Dictionary<Type, JniPeerInstanceMethods>   SubclassConstructors = new Dictionary<Type, JniPeerInstanceMethods> ();
 
 		public JniInstanceMethodID GetConstructor (string signature)
 		{
-			string method   = "<init>";
+			if (signature == null)
+				throw new ArgumentNullException ("signature");
 			lock (InstanceMethods) {
 				JniInstanceMethodID m;
 				if (!InstanceMethods.TryGetValue (signature, out m)) {
-					m = Members.JniPeerType.GetInstanceMethod (method, signature);
+					m = JniPeerType.GetConstructor (signature);
 					InstanceMethods.Add (signature, m);
 				}
 				return m;
 			}
 		}
 
+		JniPeerInstanceMethods GetConstructorsForType (Type declaringType)
+		{
+			if (declaringType == DeclaringType)
+				return this;
+
+			JniPeerInstanceMethods methods;
+			lock (SubclassConstructors) {
+				if (!SubclassConstructors.TryGetValue (declaringType, out methods)) {
+					methods = new JniPeerInstanceMethods (declaringType);
+					SubclassConstructors.Add (declaringType, methods);
+				}
+			}
+			return methods;
+		}
+
 		public JniInstanceMethodID GetMethodID (string encodedMember)
 		{
 			lock (InstanceMethods) {
 				JniInstanceMethodID m;
 				if (!InstanceMethods.TryGetValue (encodedMember, out m)) {
 					string method, signature;
 					JniPeerMembers.GetNameAndSignature (encodedMember, out method, out signature);
-					m = Members.JniPeerType.GetInstanceMethod (method, signature);
+					m = JniPeerType.GetInstanceMethod (method, signature);
 					InstanceMethods.Add (encodedMember, m);
 				}
 				return m;
 			}
 		}
+
+		public JniLocalReference StartCreateInstance (string constructorSignature, Type declaringType, params JValue[] arguments)
+		{
+			if (JniEnvironment.Current.JavaVM.NewObjectRequired) {
+				return NewObject (constructorSignature, declaringType, arguments);
+			}
+			using (var lref = GetConstructorsForType (declaringType)
+					.JniPeerType
+					.AllocObject ())
+				return lref.ToAllocObjectRef ();
+		}
+
+		JniLocalReference NewObject (string constructorSignature, Type declaringType, JValue[] arguments)
+		{
+			var methods = GetConstructorsForType (declaringType);
+			var ctor    = methods.GetConstructor (constructorSignature);
+			return methods.JniPeerType.NewObject (ctor, arguments);
+		}
+
+		public void FinishCreateInstance (string constructorSignature, IJavaObject self, params JValue[] arguments)
+		{
+			if (JniEnvironment.Current.JavaVM.NewObjectRequired) {
+				return;
+			}
+			var methods = GetConstructorsForType (self.GetType ());
+			var ctor    = methods.GetConstructor (constructorSignature);
+			ctor.CallNonvirtualVoidMethod (self.SafeHandle, methods.JniPeerType.SafeHandle, arguments);
+		}
 	}
 
 	struct JniArgumentMarshalInfo<T> {