[java-interop] Require dynamic JVM loading (#338)

There is a (*yugely* important) "philosophical" problem with using
`Java.Runtime.Environment.dll.config` to specify the location of the
`jvm.dll` to load, as originated in commit caca88d:

It cannot possibly work *reliably* on any machine *other than* the
build machine.

This is the case because the
`/configuration/dllmap[@dll='jvm.dll']/@target` value comes from the
value of `$(JI_JVM_PATH)`, which is a `make` variable generated during
`make prepare` (see also 4bd9297).  As such, the only way this can
work on an end-users machine is if the end user has the same JDK
version installed as the machine which generated
`Java.Runtime.Environment.dll.config`, which is *highly* unlikely.
(Not everyone on the Xamarin.Android team has the same JDK version!)

The only sane course of action, then, is to *not* use
`Java.Runtime.Environment.dll.config` to specify the location of the
`jvm.dll` to load.  Commit 34129b6 started implementing this; let's
finish the effort, and require it everywhere.

*A* reason that commit 34129b6 kept
`Java.Runtime.Environment.dll.config` use was so that unit tests would
continue to execute, as the `TestJVM` type uses
`Java.Runtime.Environment.dll`, which uses
`Java.Runtime.Environment.dll.config`.

To support this scenario, update the `<JdkInfo/>` task so that the
generated `JdkInfo.mk` file *`export`*s the `$(JI_JVM_PATH)` variable.
This allows unit tests executed from e.g. `make run-all-tests` to
inherit the `$JI_JVM_PATH` environment variable, allowing them to
"implicitly explicitly" specify the location of the JVM to use.

With unit test support in place, we can rip out all use of `jvm.dll`,
requiring that we instead use e.g.
`java-interop!java_interop_jvm_load()` to load the JVM.

Additionally, reduce the "public API surface" within
`java-interop-jvm.h` by moving the declaration of `DylibJVM` into
`java-interop-jvm.c`.  The `DylibJVM` type doesn't need to be public,
and making it private will allow us to more easily change it.

Author: jonpryor

Makefile

@@ -178,10 +178,8 @@ $(JRE_DLL_CONFIG): src/Java.Runtime.Environment/Java.Runtime.Environment.csproj
 	$(MSBUILD) $(MSBUILD_FLAGS) $<
 
 run-test-jnimarshal: bin/Test$(CONFIGURATION)/Java.Interop.Export-Tests.dll bin/Test$(CONFIGURATION)/$(JAVA_INTEROP_LIB) $(JRE_DLL_CONFIG)
-	mv -fv "$(JRE_DLL_CONFIG)" "$(JRE_DLL_CONFIG).bak"
 	MONO_TRACE_LISTENER=Console.Out \
 	$(RUNTIME) bin/$(CONFIGURATION)/jnimarshalmethod-gen.exe -v --jvm "$(JI_JVM_PATH)" -L "$(JI_MONO_LIB_PATH)mono/4.5" -L "$(JI_MONO_LIB_PATH)mono/4.5/Facades" "$<"
-	mv -fv "$(JRE_DLL_CONFIG).bak" "$(JRE_DLL_CONFIG)"
 	$(call RUN_TEST,$<)
 
 # $(call GEN_CORE_OUTPUT, outdir, suffix, extra)

gendarme-ignore.txt

@@ -565,7 +565,6 @@ M: System.IntPtr Java.Interop.NativeMethods::java_interop_jnienv_get_direct_buff
 M: System.Int64 Java.Interop.NativeMethods::java_interop_jnienv_get_direct_buffer_capacity(System.IntPtr,System.IntPtr)
 M: Java.Interop.JniObjectReferenceType Java.Interop.NativeMethods::java_interop_jnienv_get_object_ref_type(System.IntPtr,System.IntPtr)
 M: System.Int32 Java.Interop.NativeMethods::java_interop_jvm_list(System.IntPtr[],System.Int32,System.Int32&)
-M: System.Int32 Java.Interop.NativeMethods::java_interop_jvm_load(System.String)
 
 
 R: Gendarme.Rules.Naming.UseCorrectSuffixRule

src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkInfo.cs

@@ -147,10 +147,10 @@ void WritePropertyFile (string jarPath, string javacPath, string jdkJvmPath, IEn
 		void WriteMakeFragmentFile (string jarPath, string javacPath, string jdkJvmPath, IEnumerable<string> includes)
 		{
 			using (var o = new StreamWriter (MakeFragmentFile.ItemSpec)) {
-				o.WriteLine ($"JI_JAR_PATH          := {jarPath}");
-				o.WriteLine ($"JI_JAVAC_PATH        := {javacPath}");
-				o.WriteLine ($"JI_JDK_INCLUDE_PATHS := {string.Join (" ", includes)}");
-				o.WriteLine ($"JI_JVM_PATH          := {jdkJvmPath}");
+				o.WriteLine ($"export  JI_JAR_PATH          := {jarPath}");
+				o.WriteLine ($"export  JI_JAVAC_PATH        := {javacPath}");
+				o.WriteLine ($"export  JI_JDK_INCLUDE_PATHS := {string.Join (" ", includes)}");
+				o.WriteLine ($"export  JI_JVM_PATH          := {jdkJvmPath}");
 			}
 		}
 

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

@@ -60,7 +60,7 @@ public partial class CreationOptions {
 
 			public  JniObjectReferenceManager   ObjectReferenceManager      {get; set;}
 			public  JniTypeManager              TypeManager                 {get; set;}
-			public  string                      JvmDllPath                  {get; set;}
+			public  string                      JvmLibraryPath              {get; set;}
 
 			public CreationOptions ()
 			{
@@ -77,17 +77,10 @@ interface ISetRuntime {
 	}
 
 	partial class NativeMethods {
-		const string JvmLibrary = "jvm.dll";
 		const string JavaInteropLibrary = "java-interop";
 
-		[DllImport (JvmLibrary)]
-		internal static extern int JNI_GetCreatedJavaVMs ([Out] IntPtr[] handles, int bufLen, out int nVMs);
-
 		[DllImport (JavaInteropLibrary)]
 		internal static extern int java_interop_jvm_list ([Out] IntPtr[] handles, int bufLen, out int nVMs);
-
-		[DllImport (JavaInteropLibrary, CharSet=CharSet.Ansi)]
-		internal static extern int java_interop_jvm_load (string path);
 	}
 
 	public partial class JniRuntime : IDisposable
@@ -96,8 +89,6 @@ public partial class JniRuntime : IDisposable
 		const   int     JNI_EDETACHED   = -2;
 		const   int     JNI_EVERSION    = -3;
 
-		static public bool JvmDllLoaded  { get; private set; }
-
 		static ConcurrentDictionary<IntPtr, JniRuntime>     Runtimes = new ConcurrentDictionary<IntPtr, JniRuntime> ();
 
 		public static IEnumerable<JniRuntime> GetRegisteredRuntimes ()
@@ -115,10 +106,7 @@ public static JniRuntime GetRegisteredRuntime (IntPtr invocationPointer)
 
 		internal static int GetCreatedJavaVMs (IntPtr[] handles, int bufLen, out int nVMs)
 		{
-			if (JvmDllLoaded)
-				return NativeMethods.java_interop_jvm_list (handles, bufLen, out nVMs);
-
-			return NativeMethods.JNI_GetCreatedJavaVMs (handles, bufLen, out nVMs);
+			return NativeMethods.java_interop_jvm_list (handles, bufLen, out nVMs);
 		}
 
 		public static IEnumerable<IntPtr> GetAvailableInvocationPointers ()
@@ -134,16 +122,6 @@ public static IEnumerable<IntPtr> GetAvailableInvocationPointers ()
 			return handles;
 		}
 
-		protected static bool LoadJvmDll (string path)
-		{
-			if (JvmDllLoaded)
-				return true;
-
-			JvmDllLoaded = NativeMethods.java_interop_jvm_load (path) != 0;
-
-			return JvmDllLoaded;
-		}
-
 		static JniRuntime current;
 		public static JniRuntime CurrentRuntime {
 			get {

src/Java.Runtime.Environment/Java.Interop/JreRuntime.cs

@@ -26,6 +26,8 @@ public class JreRuntimeOptions : JniRuntime.CreationOptions {
 
 		internal    List<string>    Options = new List<string> ();
 
+		public  string      JvmLibraryPath              {get; set;}
+
 		public  JniVersion  JniVersion                  {get; set;}
 		public  bool        IgnoreUnrecognizedOptions   {get; set;}
 
@@ -73,21 +75,9 @@ public JreRuntime CreateJreVM ()
 
 	public class JreRuntime : JniRuntime
 	{
-		const string JvmLibrary = "jvm.dll";
-		const string JavaInteropLibrary = "java-interop";
-
-		[DllImport (JvmLibrary)]
-		static extern int JNI_CreateJavaVM (out IntPtr javavm, out IntPtr jnienv, ref JavaVMInitArgs args);
-
-		[DllImport (JavaInteropLibrary)]
-		static extern int java_interop_jvm_create (out IntPtr javavm, out IntPtr jnienv, ref JavaVMInitArgs args);
-
 		static int CreateJavaVM (out IntPtr javavm, out IntPtr jnienv, ref JavaVMInitArgs args)
 		{
-			if (JvmDllLoaded)
-				return java_interop_jvm_create (out javavm, out jnienv, ref args);
-
-			return JNI_CreateJavaVM (out javavm, out jnienv, ref args);
+			return NativeMethods.java_interop_jvm_create (out javavm, out jnienv, ref args);
 		}
 
 		static unsafe JreRuntimeOptions CreateJreVM (JreRuntimeOptions builder)
@@ -98,8 +88,12 @@ static unsafe JreRuntimeOptions CreateJreVM (JreRuntimeOptions builder)
 			if (builder.InvocationPointer != IntPtr.Zero)
 				return builder;
 
-			if (builder.JvmDllPath != null && !LoadJvmDll (builder.JvmDllPath))
-				throw new Exception ($"Unable to load JVM library: {builder.JvmDllPath}");
+			if (!string.IsNullOrEmpty (builder.JvmLibraryPath)) {
+				int r = NativeMethods.java_interop_jvm_load (builder.JvmLibraryPath);
+				if (r != 0) {
+					throw new Exception ($"Could not load JVM path `{builder.JvmLibraryPath}` ({r})!");
+				}
+			}
 
 			var args = new JavaVMInitArgs () {
 				version             = builder.JniVersion,
@@ -160,5 +154,13 @@ protected override void Dispose (bool disposing)
 			base.Dispose (disposing);
 		}
 	}
+
+	partial class NativeMethods {
+		[DllImport (JavaInteropLib, CharSet=CharSet.Ansi)]
+		internal static extern int java_interop_jvm_load (string path);
+
+		[DllImport (JavaInteropLib, CharSet=CharSet.Ansi)]
+		internal static extern int java_interop_jvm_create (out IntPtr javavm, out IntPtr jnienv, ref JavaVMInitArgs args);
+	}
 }
 

src/Java.Runtime.Environment/Java.Runtime.Environment.dll.config.in

@@ -1,4 +1,3 @@
 <configuration>
-  <dllmap dll="jvm.dll" os="@OS_NAME@" target="@JI_JVM_PATH@"/>
 @JAVA_RUNTIME_ENVIRONMENT_DLLMAP@
 </configuration>

src/java-interop/java-interop-jvm.c

@@ -5,56 +5,73 @@
 #include "java-interop-logger.h"
 #include "java-interop-util.h"
 
-static struct DylibJVM *jvm = NULL;
 
-int java_interop_jvm_load (const char *path)
+typedef int (*java_interop_JNI_CreateJavaVM_fptr) (JavaVM **p_vm, void **p_env, void *vm_args);
+typedef int (*java_interop_JNI_GetCreatedJavaVMs_fptr) (JavaVM **vmBuf, int bufLen, int *nVMs);
+
+struct DylibJVM {
+	void                                       *dl_handle;
+	java_interop_JNI_CreateJavaVM_fptr          JNI_CreateJavaVM;
+	java_interop_JNI_GetCreatedJavaVMs_fptr     JNI_GetCreatedJavaVMs;
+};
+
+static struct DylibJVM *jvm;
+
+int
+java_interop_jvm_load (const char *path)
 {
+	if (jvm != NULL) {
+		return JAVA_INTEROP_JVM_FAILED_ALREADY_LOADED;
+	}
+
 	jvm = calloc (1, sizeof (struct DylibJVM));
+	if (!jvm) {
+		return JAVA_INTEROP_JVM_FAILED_OOM;
+	}
 
 	jvm->dl_handle = dlopen (path, RTLD_LAZY);
-
-	if (!jvm->dl_handle)
-		return 0;
+	if (!jvm->dl_handle) {
+		free (jvm);
+		jvm = NULL;
+		return JAVA_INTEROP_JVM_FAILED_NOT_LOADED;
+	}
 
 	int symbols_missing = 0;
 
-#define LOAD_SYMBOL(symbol) \
-	jvm->symbol = dlsym (jvm->dl_handle, #symbol); \
-	if (!jvm->symbol) { \
-		log_error (LOG_DEFAULT, "Failed to load JVM symbol: %s", #symbol); \
-		symbols_missing = 1; \
-	}
+#define LOAD_SYMBOL(symbol) do { \
+		jvm->symbol = dlsym (jvm->dl_handle, #symbol); \
+		if (!jvm->symbol) { \
+			log_error (LOG_DEFAULT, "Failed to load JVM symbol: %s", #symbol); \
+			symbols_missing = 1; \
+		} \
+	} while (0)
+
+	LOAD_SYMBOL(JNI_CreateJavaVM);
+	LOAD_SYMBOL(JNI_GetCreatedJavaVMs);
 
-	LOAD_SYMBOL(JNI_CreateJavaVM)
-	LOAD_SYMBOL(JNI_GetCreatedJavaVMs)
+#undef LOAD_SYMBOL
 
 	if (symbols_missing) {
-		log_fatal (LOG_DEFAULT, "Failed to load some Mono symbols, aborting...");
-		exit (FATAL_EXIT_JVM_MISSING_SYMBOLS);
+		free (jvm);
+		jvm = NULL;
+		return JAVA_INTEROP_JVM_FAILED_SYMBOL_MISSING;
 	}
 
-	return 1;
+	return 0;
 }
 
-static inline void
-_assert_dl_handle ()
-{
-	if (!jvm || !jvm->dl_handle) {
-		log_fatal (LOG_DEFAULT, "Missing JVM symbols!");
-		exit (FATAL_EXIT_JVM_MISSING_SYMBOLS);
-	}
-}
+#define ji_return_val_if_fail(expr, val) do { if (!(expr)) return (val); } while (0)
 
 int java_interop_jvm_create (JavaVM **p_vm, void **p_env, void *vm_args)
 {
-	_assert_dl_handle ();
+	ji_return_val_if_fail (jvm != NULL, JAVA_INTEROP_JVM_FAILED_NOT_LOADED);
 
 	return (*jvm->JNI_CreateJavaVM) (p_vm, p_env, vm_args);
 }
 
 int java_interop_jvm_list (JavaVM **vmBuf, int bufLen, int *nVMs)
 {
-	_assert_dl_handle ();
+	ji_return_val_if_fail (jvm != NULL, JAVA_INTEROP_JVM_FAILED_NOT_LOADED);
 
 	return (*jvm->JNI_GetCreatedJavaVMs) (vmBuf, bufLen, nVMs);
 }

src/java-interop/java-interop-jvm.h

@@ -5,18 +5,14 @@
 
 typedef void JavaVM;
 
-typedef int (*java_interop_JNI_CreateJavaVM_fptr) (JavaVM **p_vm, void **p_env, void *vm_args);
-typedef int (*java_interop_JNI_GetCreatedJavaVMs_fptr) (JavaVM **vmBuf, int bufLen, int *nVMs);
-
-/* NOTE: structure members MUST NOT CHANGE ORDER. */
-struct DylibJVM {
-	void						       *dl_handle;
-	java_interop_JNI_CreateJavaVM_fptr			JNI_CreateJavaVM;
-	java_interop_JNI_GetCreatedJavaVMs_fptr			JNI_GetCreatedJavaVMs;
-};
-
 JAVA_INTEROP_BEGIN_DECLS
 
+#define JAVA_INTEROP_JVM_FAILED                 (-1000)
+#define JAVA_INTEROP_JVM_FAILED_ALREADY_LOADED  (JAVA_INTEROP_JVM_FAILED-1)
+#define JAVA_INTEROP_JVM_FAILED_NOT_LOADED      (JAVA_INTEROP_JVM_FAILED-2)
+#define JAVA_INTEROP_JVM_FAILED_OOM             (JAVA_INTEROP_JVM_FAILED-3)
+#define JAVA_INTEROP_JVM_FAILED_SYMBOL_MISSING  (JAVA_INTEROP_JVM_FAILED-4)
+
 MONO_API    int java_interop_jvm_load (const char *path);
 MONO_API    int java_interop_jvm_create (JavaVM **p_vm, void **p_env, void *vm_args);
 MONO_API    int java_interop_jvm_list (JavaVM **vmBuf, int bufLen, int *nVMs);

tests/TestJVM/TestJVM.cs

@@ -14,7 +14,9 @@ public class TestJVM : JreRuntime {
 		static JreRuntimeOptions CreateBuilder (string[] jars)
 		{
 			var dir = Path.GetDirectoryName (typeof (TestJVM).Assembly.Location);
-			var builder = new JreRuntimeOptions ();
+			var builder = new JreRuntimeOptions () {
+				JvmLibraryPath              = Environment.GetEnvironmentVariable ("JI_JVM_PATH"),
+			};
 			if (jars != null) {
 				foreach (var jar in jars)
 					builder.ClassPath.Add (Path.Combine (dir, jar));

tools/jnimarshalmethod-gen/App.cs

@@ -160,7 +160,9 @@ void ProcessAssemblies (List<string> assemblies)
 
 		void CreateJavaVM (string jvmDllPath)
 		{
-			var builder = new JreRuntimeOptions { JvmDllPath = jvmDllPath };
+			var builder = new JreRuntimeOptions {
+				JvmLibraryPath  = jvmDllPath,
+			};
 
 			try {
 				builder.CreateJreVM ();