Skip to content

Commit 4269528

Browse files
authored
[Mono.Android] Don't dispose of event handler implementors (#1111)
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=59804 Context: https://gist.github.com/jonpryor/d0e7bda3107913d0ab8ad46e8ae26419 Context: http://download.oracle.com/otndocs/jcp/7224-javabeans-1.01-fr-spec-oth-JSpec/ (Would you believe a 5.5+ year old bug? Of course you would!) Assume a *multithreaded* event registration and dispatch Java app, such as that in the JavaBeans 1.0.1 spec §6.5.1, in which one thread (un-)subscribes to events via `Model.addModelChangedListener()` and `Model.removeModelChangedListener()`, while *another thread* is responsible for calling `Model.notifyModelChanged()`. This is fine, *so long as* a Listener doesn't "invalidate" itself while still accessible from `Model.listeners`: // This is fine and normal MyModelChangedListener l = new MyModelChangedListener(); model.addModelChangedListener(l); // ... model.removeModelChangedListener(l); // Doing this *immediately* after removeModelChangedListener() is BAD l.dispose(); To see *why* `l.dispose()` is bad -- assuming `dispose()` semantics in which `l.modelChanged()` starts throwing an exception after `l.dispose()` is invoked -- one must better understand the multithreaded nature of the app: the "notification thread" *cannot* know that the instance is disposed. Consider this serialized execution: // Thread 1: model.addModelChangedListener(l); // Thread 2: Vector listeners; synchronized (this) {listeners = (Vector) this.listeners.clone();} // Thread 1: model.removeModelChangedListener(l); l.dispose(); // Here, `l` is still in Thread 2's `listeners`, but is invalid! // Thread 2: for (ModelChangedListener l : listeners) { l.modelChanged(); // will fail } The Xamarin.Android event implementation would *cause* the same scenario: partial class /* Android.App. */ Dialog { public event EventHandler CancelEvent { add { global::Java.Interop.EventHelper.AddEventHandler<Android.Content.IDialogInterfaceOnCancelListener, Android.Content.IDialogInterfaceOnCancelListenerImplementor>( ref weak_implementor_SetOnCancelListener, __CreateIDialogInterfaceOnCancelListenerImplementor, SetOnCancelListener, __h => __h.Handler += value); } remove { global::Java.Interop.EventHelper.RemoveEventHandler<Android.Content.IDialogInterfaceOnCancelListener, Android.Content.IDialogInterfaceOnCancelListenerImplementor>( ref weak_implementor_SetOnCancelListener, Android.Content.IDialogInterfaceOnCancelListenerImplementor.__IsEmpty, __v => SetOnCancelListener (null), __h => __h.Handler -= value); } } } The key problem was within `EventHelper.RemoveEventHandler()`, where it would *dispose of the listener* after unregistering it: // Within EventHelper.RemoveEventHandler(): if (empty (impl)) { unsetListener (impl); impl.Dispose (); // ... } The `unsetListener()` invocation is analogous to `model.removeModelChangedListener()`, causing the `impl` instance to be removed from the Java event. The `impl.Dispose()` is the problem, as if `impl` is later used from another thread, things will fail: System.NotSupportedException: Unable to activate instance of type IWhateverListenerImplementor from native handle 0x1d200001 (key_handle 0x41b92598). ---> System.MissingMethodException: No constructor found for IWhateverListenerImplementor::.ctor(System.IntPtr, Android.Runtime.JniHandleOwnership) ---> ... Update `AndroidEventHelper.RemoveEventHandler()` and `EventHelper.RemoveEventHandler()` so that they no longer `Dispose()` of the `TImplementor` instance. This will prevent the `NotSupportedException` from being raised.# Please enter the commit message for your changes. Lines starting
1 parent 4ac085a commit 4269528

File tree

2 files changed

+0
-2
lines changed

2 files changed

+0
-2
lines changed

src/Mono.Android/Java.Interop/AndroidEventHelper.cs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@ public static void RemoveEventHandler<TInterface, TImplementor> (
3333
return;
3434
remove (impl);
3535
if (empty (impl)) {
36-
impl.Dispose ();
3736
impl = null;
3837
implementor = null;
3938
setListener (impl);

src/Mono.Android/Java.Interop/EventHelper.cs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@ public static void RemoveEventHandler<TInterface, TImplementor> (
3333
remove (impl);
3434
if (empty (impl)) {
3535
unsetListener (impl);
36-
impl.Dispose ();
3736
impl = null;
3837
implementor.Target = null;
3938
implementor = null;

0 commit comments

Comments
 (0)