From 830f56562f37ca097f58d66e932be923cf5afe93 Mon Sep 17 00:00:00 2001 From: Radek Doulik Date: Wed, 3 Oct 2018 14:15:14 +0200 Subject: [PATCH 1/4] [Java.Interop] Do not dispose live references on return to JNI Fixes https://github.com/xamarin/java.interop/issues/374 Also refactor the code generation to not create `_handle` variable when returning value to JNI. Updated the tests and their expected code strings. 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 (__this); LayoutInflater value2 = valueManager.GetValue (inflater); ViewGroup value3 = valueManager.GetValue (container); Bundle value4 = valueManager.GetValue (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; } --- .../Java.Interop/MarshalMemberBuilderTest.cs | 11 +++++--- .../JniRuntime.JniValueManager.cs | 27 +++++++++++++------ .../JniValueMarshalerContractTests.cs | 11 +++++--- 3 files changed, 35 insertions(+), 14 deletions(-) diff --git a/src/Java.Interop.Export/Tests/Java.Interop/MarshalMemberBuilderTest.cs b/src/Java.Interop.Export/Tests/Java.Interop/MarshalMemberBuilderTest.cs index a09456a53..4a0c71e18 100644 --- a/src/Java.Interop.Export/Tests/Java.Interop/MarshalMemberBuilderTest.cs +++ b/src/Java.Interop.Export/Tests/Java.Interop/MarshalMemberBuilderTest.cs @@ -449,7 +449,6 @@ public void CreateMarshalFromJniMethodExpression_FuncIJavaObject () JavaObject __mret; ExportTest __this_val; JniObjectReference __mret_ref; - IntPtr __mret_handle; IntPtr __mret_rtn; __envp = new JniTransition(__jnienv); @@ -468,7 +467,6 @@ public void CreateMarshalFromJniMethodExpression_FuncIJavaObject () { return __mret_ref = __mret.PeerReference; } - __mret_handle = __mret_ref.Handle; __mret_rtn = References.NewReturnToJniRef(__mret_ref); return __mret_rtn; } @@ -479,7 +477,14 @@ public void CreateMarshalFromJniMethodExpression_FuncIJavaObject () } finally { - JniObjectReference.Dispose(__mret_ref); + if (null != __mret) + { + __mret.DisposeUnlessReferenced(); + } + else + { + default(void); + } __envp.Dispose(); } }"); diff --git a/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs b/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs index 6de565a39..8e19a404c 100644 --- a/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs +++ b/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs @@ -590,6 +590,17 @@ public override void DestroyGenericArgumentState (IJavaPeerable value, ref JniVa } public override Expression CreateParameterFromManagedExpression (JniValueMarshalerContext context, ParameterExpression sourceValue, ParameterAttributes synchronize) + { + var r = CreateIntermediaryExpressionFromManagedExpression (context, sourceValue); + + var h = Expression.Variable (typeof (IntPtr), sourceValue + "_handle"); + context.LocalVariables.Add (h); + context.CreationStatements.Add (Expression.Assign (h, Expression.Property (r, "Handle"))); + + return h; + } + + Expression CreateIntermediaryExpressionFromManagedExpression (JniValueMarshalerContext context, ParameterExpression sourceValue) { var r = Expression.Variable (typeof (JniObjectReference), sourceValue.Name + "_ref"); context.LocalVariables.Add (r); @@ -598,19 +609,19 @@ public override Expression CreateParameterFromManagedExpression (JniValueMarshal test: Expression.Equal (Expression.Constant (null), sourceValue), ifTrue: Expression.Assign (r, Expression.New (typeof (JniObjectReference))), ifFalse: Expression.Assign (r, Expression.Property (sourceValue, "PeerReference")))); - context.CleanupStatements.Add (DisposeObjectReference (r)); - var h = Expression.Variable (typeof (IntPtr), sourceValue + "_handle"); - context.LocalVariables.Add (h); - context.CreationStatements.Add (Expression.Assign (h, Expression.Property (r, "Handle"))); - return h; + if (typeof (IJavaPeerable).GetTypeInfo ().IsAssignableFrom (sourceValue.Type.GetTypeInfo ())) { + context.CleanupStatements.Add (Expression.IfThen ( + test: Expression.NotEqual (Expression.Constant (null), sourceValue), + ifTrue: Expression.Call (sourceValue, typeof (IJavaPeerable).GetTypeInfo ().GetDeclaredMethod ("DisposeUnlessReferenced")))); + } + + return r; } public override Expression CreateReturnValueFromManagedExpression (JniValueMarshalerContext context, ParameterExpression sourceValue) { - CreateParameterFromManagedExpression (context, sourceValue, 0); - var r = context.LocalVariables [sourceValue + "_ref"]; - return ReturnObjectReferenceToJni (context, sourceValue.Name, r); + return ReturnObjectReferenceToJni (context, sourceValue.Name, CreateIntermediaryExpressionFromManagedExpression (context, sourceValue)); } public override Expression CreateParameterToManagedExpression (JniValueMarshalerContext context, ParameterExpression sourceValue, ParameterAttributes synchronize, Type targetType) diff --git a/src/Java.Interop/Tests/Java.Interop/JniValueMarshalerContractTests.cs b/src/Java.Interop/Tests/Java.Interop/JniValueMarshalerContractTests.cs index 9e266c2a9..47d524ebc 100644 --- a/src/Java.Interop/Tests/Java.Interop/JniValueMarshalerContractTests.cs +++ b/src/Java.Interop/Tests/Java.Interop/JniValueMarshalerContractTests.cs @@ -591,7 +591,6 @@ protected override string GetExpectedReturnValueFromManagedExpression (string jv JniRuntime {jvm}; IJavaPeerable {value}; JniObjectReference {value}_ref; - IntPtr {value}_handle; IntPtr {value}_rtn; try @@ -604,13 +603,19 @@ protected override string GetExpectedReturnValueFromManagedExpression (string jv {{ return {value}_ref = {value}.PeerReference; }} - {value}_handle = {value}_ref.Handle; {value}_rtn = References.NewReturnToJniRef({value}_ref); return {pret.Name}; }} finally {{ - JniObjectReference.Dispose({value}_ref); + if (null != __value) + {{ + __value.DisposeUnlessReferenced(); + }} + else + {{ + default(void); + }} }} }}"; } From 7fdcffb88081179cebc861d8504b0fecee9efccb Mon Sep 17 00:00:00 2001 From: Radek Doulik Date: Thu, 4 Oct 2018 22:12:06 +0200 Subject: [PATCH 2/4] [Java.Interop] Remove unnecessary test, fix varible names The test is not really needed, because the sourceValue.Type in the `JavaPeerableValueMarshaler` should always implement `IJavaPeerable` interface. Use `sourceValue.Name` when constructing variable names. `sourceValue.ToString ()` is equal (by coincidence). To be on the safe side use the `Name` property directly. --- .../Java.Interop/JniRuntime.JniValueManager.cs | 13 +++++-------- src/Java.Interop/Java.Interop/JniValueMarshaler.cs | 2 +- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs b/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs index 8e19a404c..97bcbe6ab 100644 --- a/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs +++ b/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs @@ -593,7 +593,7 @@ public override Expression CreateParameterFromManagedExpression (JniValueMarshal { var r = CreateIntermediaryExpressionFromManagedExpression (context, sourceValue); - var h = Expression.Variable (typeof (IntPtr), sourceValue + "_handle"); + var h = Expression.Variable (typeof (IntPtr), sourceValue.Name + "_handle"); context.LocalVariables.Add (h); context.CreationStatements.Add (Expression.Assign (h, Expression.Property (r, "Handle"))); @@ -609,12 +609,9 @@ Expression CreateIntermediaryExpressionFromManagedExpression (JniValueMarshalerC test: Expression.Equal (Expression.Constant (null), sourceValue), ifTrue: Expression.Assign (r, Expression.New (typeof (JniObjectReference))), ifFalse: Expression.Assign (r, Expression.Property (sourceValue, "PeerReference")))); - - if (typeof (IJavaPeerable).GetTypeInfo ().IsAssignableFrom (sourceValue.Type.GetTypeInfo ())) { - context.CleanupStatements.Add (Expression.IfThen ( - test: Expression.NotEqual (Expression.Constant (null), sourceValue), - ifTrue: Expression.Call (sourceValue, typeof (IJavaPeerable).GetTypeInfo ().GetDeclaredMethod ("DisposeUnlessReferenced")))); - } + context.CleanupStatements.Add (Expression.IfThen ( + test: Expression.NotEqual (Expression.Constant (null), sourceValue), + ifTrue: Expression.Call (sourceValue, typeof (IJavaPeerable).GetTypeInfo ().GetDeclaredMethod ("DisposeUnlessReferenced")))); return r; } @@ -626,7 +623,7 @@ public override Expression CreateReturnValueFromManagedExpression (JniValueMarsh public override Expression CreateParameterToManagedExpression (JniValueMarshalerContext context, ParameterExpression sourceValue, ParameterAttributes synchronize, Type targetType) { - var r = Expression.Variable (targetType, sourceValue + "_val"); + var r = Expression.Variable (targetType, sourceValue.Name + "_val"); context.LocalVariables.Add (r); context.CreationStatements.Add ( Expression.Assign (r, diff --git a/src/Java.Interop/Java.Interop/JniValueMarshaler.cs b/src/Java.Interop/Java.Interop/JniValueMarshaler.cs index 4c4a2e40b..b70213e80 100644 --- a/src/Java.Interop/Java.Interop/JniValueMarshaler.cs +++ b/src/Java.Interop/Java.Interop/JniValueMarshaler.cs @@ -162,7 +162,7 @@ Expression CreateSelf (JniValueMarshalerContext context, ParameterExpression sou public virtual Expression CreateReturnValueFromManagedExpression (JniValueMarshalerContext context, ParameterExpression sourceValue) { CreateParameterFromManagedExpression (context, sourceValue, 0); - var s = context.LocalVariables [sourceValue + "_state"]; + var s = context.LocalVariables [sourceValue.Name + "_state"]; return ReturnObjectReferenceToJni (context, sourceValue.Name, Expression.Property (s, "ReferenceValue")); } From 54fa780226a78e8a3894c2117bd5fce29a6e8b38 Mon Sep 17 00:00:00 2001 From: Radek Doulik Date: Fri, 5 Oct 2018 18:54:18 +0200 Subject: [PATCH 3/4] [Java.Interop] Do not dispose the return value Do not dispose the return valueat at all. Not even using `DisposeUnlessReferenced`, becuse that would dispose the IJavaPeerable object and not just the reference. --- .../Tests/Java.Interop/MarshalMemberBuilderTest.cs | 8 -------- .../Java.Interop/JniRuntime.JniValueManager.cs | 4 +--- .../Tests/Java.Interop/JniValueMarshalerContractTests.cs | 9 +-------- 3 files changed, 2 insertions(+), 19 deletions(-) diff --git a/src/Java.Interop.Export/Tests/Java.Interop/MarshalMemberBuilderTest.cs b/src/Java.Interop.Export/Tests/Java.Interop/MarshalMemberBuilderTest.cs index 4a0c71e18..1491a6cf3 100644 --- a/src/Java.Interop.Export/Tests/Java.Interop/MarshalMemberBuilderTest.cs +++ b/src/Java.Interop.Export/Tests/Java.Interop/MarshalMemberBuilderTest.cs @@ -477,14 +477,6 @@ public void CreateMarshalFromJniMethodExpression_FuncIJavaObject () } finally { - if (null != __mret) - { - __mret.DisposeUnlessReferenced(); - } - else - { - default(void); - } __envp.Dispose(); } }"); diff --git a/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs b/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs index 97bcbe6ab..b2cadd092 100644 --- a/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs +++ b/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs @@ -592,6 +592,7 @@ public override void DestroyGenericArgumentState (IJavaPeerable value, ref JniVa public override Expression CreateParameterFromManagedExpression (JniValueMarshalerContext context, ParameterExpression sourceValue, ParameterAttributes synchronize) { var r = CreateIntermediaryExpressionFromManagedExpression (context, sourceValue); + context.CleanupStatements.Add (DisposeObjectReference (r)); var h = Expression.Variable (typeof (IntPtr), sourceValue.Name + "_handle"); context.LocalVariables.Add (h); @@ -609,9 +610,6 @@ Expression CreateIntermediaryExpressionFromManagedExpression (JniValueMarshalerC test: Expression.Equal (Expression.Constant (null), sourceValue), ifTrue: Expression.Assign (r, Expression.New (typeof (JniObjectReference))), ifFalse: Expression.Assign (r, Expression.Property (sourceValue, "PeerReference")))); - context.CleanupStatements.Add (Expression.IfThen ( - test: Expression.NotEqual (Expression.Constant (null), sourceValue), - ifTrue: Expression.Call (sourceValue, typeof (IJavaPeerable).GetTypeInfo ().GetDeclaredMethod ("DisposeUnlessReferenced")))); return r; } diff --git a/src/Java.Interop/Tests/Java.Interop/JniValueMarshalerContractTests.cs b/src/Java.Interop/Tests/Java.Interop/JniValueMarshalerContractTests.cs index 47d524ebc..e239ecf68 100644 --- a/src/Java.Interop/Tests/Java.Interop/JniValueMarshalerContractTests.cs +++ b/src/Java.Interop/Tests/Java.Interop/JniValueMarshalerContractTests.cs @@ -608,14 +608,7 @@ protected override string GetExpectedReturnValueFromManagedExpression (string jv }} finally {{ - if (null != __value) - {{ - __value.DisposeUnlessReferenced(); - }} - else - {{ - default(void); - }} + default(void); }} }}"; } From 84ec07fdbea385a98304603bdd9241760cd66484 Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Mon, 8 Oct 2018 16:27:22 -0400 Subject: [PATCH 4/4] Don't dispose managed parameters. --- src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs b/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs index b2cadd092..8c4b27c28 100644 --- a/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs +++ b/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs @@ -592,8 +592,6 @@ public override void DestroyGenericArgumentState (IJavaPeerable value, ref JniVa public override Expression CreateParameterFromManagedExpression (JniValueMarshalerContext context, ParameterExpression sourceValue, ParameterAttributes synchronize) { var r = CreateIntermediaryExpressionFromManagedExpression (context, sourceValue); - context.CleanupStatements.Add (DisposeObjectReference (r)); - var h = Expression.Variable (typeof (IntPtr), sourceValue.Name + "_handle"); context.LocalVariables.Add (h); context.CreationStatements.Add (Expression.Assign (h, Expression.Property (r, "Handle")));