[Java.Interop] Do not dispose live references on return to JNI

[radekdoulik]

Fixes #374

Also refactor the code generation to not create _handle variable
when returning value to JNI

Example generated marshal method:

using Android.OS;
using Android.Views;
using Java.Interop;
using System;

public static IntPtr n_onCreateView_Landroid_view_LayoutInflater_Landroid_view_ViewGroup_Landroid_os_Bundle_ (IntPtr __jnienv, IntPtr __this, IntPtr inflater, IntPtr container, IntPtr savedInstanceState)
{
	JniTransition jniTransition = new JniTransition (__jnienv);
	JniRuntime runtime = default(JniRuntime);
	global::Android.Views.View view = default(global::Android.Views.View);
	try {
		runtime = JniEnvironment.Runtime;
		JniRuntime.JniValueManager valueManager = runtime.ValueManager;
		valueManager.WaitForGCBridgeProcessing ();
		FragmentContainer value = valueManager.GetValue<FragmentContainer> (__this);
		LayoutInflater value2 = valueManager.GetValue<LayoutInflater> (inflater);
		ViewGroup value3 = valueManager.GetValue<ViewGroup> (container);
		Bundle value4 = valueManager.GetValue<Bundle> (savedInstanceState);
		view = value.OnCreateView (value2, value3, value4);
		JniObjectReference value5 = (view != null) ? view.PeerReference : default(JniObjectReference);
		return JniEnvironment.References.NewReturnToJniRef (value5);
	} catch (Exception ex) when (runtime.ExceptionShouldTransitionToJni (ex)) {
		jniTransition.SetPendingException (ex);
		return default(IntPtr);
	} finally {
		if (view != null) {
			((IJavaPeerable)view).DisposeUnlessReferenced ();
		}
		jniTransition.Dispose ();
	}
	IntPtr intPtr = default(IntPtr);
	return intPtr;
}

[jonpryor]

This condition should always be true: this is within JavaPeerableValueMarshaler -- a JniValueMarshaler<IJavaPeerable> -- so sourceValue had better be an IJavaPeerable or something has gotten very horribly confused. (Probably me!)

[radekdoulik]

Yeah, that just me being too careful :-) I guess we can drop the test as it will fail in the next line pretty clearly in case something got broken horribly.

[jonpryor]

From the "wait, what was I thinking" department... Is sourceValue + "_handle" really what we want?

Or should this instead be sourceValue.Name + "_handle"?

My guess is that sourceValue + "_handle" is a bug, and it "just happened" to work for us so far.

[radekdoulik]

Looks like ParameterExpression:ToString () returns the sourceValue.Name, so it worked so far. I will change it to sourceValue.Name + "_handle" to be on the safe side. Which also means we would not need to update the tests hopefully :-)

Just to note, the documentation for the ParameterExpression:ToString () states just: "Returns a textual representation of the Expression."

[jonpryor]

In another "What was I thinking?" moment... Is CreateParameterFromManagedExpression() even used from anywhere?

Should it even exist?

This method would be called when creating an Expression to convert a managed (C#) parameter to a Java parameter.

What does that even mean? At least in the context of jnimarshalmethod-gen.exe, we never convert parameters from C# to Java. We convert Java parameters to C#. We convert C# return values to Java (hence CreateReturnValueFromManagedExpression()). But parameters?

Which brings us to this line: As per our earlier offline discussion, we should never call JniObjectReference.Dispose(ref JniObjectReference). Meaning this line -- the line I'm commenting on -- should not exist at all; it's presence is a bug:

context.CleanupStatements.Add (DisposeObjectReference (r));

That said...note that we don't see any use of JniObjectReference.Dispose() left in this PR. Meaning this CreateParameterFromManagedExpression() is not actually used, at least not in any of our existing unit tests!

Is this method (now) dead code, as you've changed CreateReturnValueFromManagedExpression() to no longer call CreateParameterFromManagedExpression()?

I suspect you could replace this CreateParameterFromManagedExpression() method with throw new NotImplementedException() and nothing would break.

---

In any event, we shouldn't be calling DisposeObjectReference() here. (There probably shouldn't even be a "here" here...)

[jonpryor]

Further "interesting" is commit ff4053c:

• CreateParameterFromManagedExpression():
  Managed > Java marshaler.
  Convert the managed parameter sourceValue to MarshalType.
  ...

Do in conjunction w/ Java.Interop.Dynamic

While Java.Interop.Dynamic can (should!) use
JniValueManager.CreateParameterFromManagedExpression(), it doesn't
currently because it doesn't even perform the JNI invocation via
expression trees; it uses expression trees to invoke the intermediate
helper method JavaClassInfo.TryInvokeMember().

Looks like the idea of marshaling C# parameter types to Java types was to eventually support Java.Interop.Dynamic, so maybe it does make sense in the abstract, but it won't be used from jnimarshalmethod-gen.exe.

[radekdoulik]

Looks like we should be all set with this PR, so I merged it to get dotnet/android#2153 going and let it build while the build bots are not busy in the morning.