From 0b48892f77c6f36c738f3da65989779b3af67fa3 Mon Sep 17 00:00:00 2001
From: Jonathan Pryor <jonpryor@vt.edu>
Date: Thu, 14 Dec 2017 16:20:30 -0500
Subject: [PATCH] [Mono.Android] Don't dispose of event handler implementors
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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
---
 src/Mono.Android/Java.Interop/AndroidEventHelper.cs | 1 -
 src/Mono.Android/Java.Interop/EventHelper.cs        | 1 -
 2 files changed, 2 deletions(-)

diff --git a/src/Mono.Android/Java.Interop/AndroidEventHelper.cs b/src/Mono.Android/Java.Interop/AndroidEventHelper.cs
index df85fab98cc..d39f2f5c9fe 100644
--- a/src/Mono.Android/Java.Interop/AndroidEventHelper.cs
+++ b/src/Mono.Android/Java.Interop/AndroidEventHelper.cs
@@ -33,7 +33,6 @@ public static void RemoveEventHandler<TInterface, TImplementor> (
 				return;
 			remove (impl);
 			if (empty (impl)) {
-				impl.Dispose ();
 				impl = null;
 				implementor = null;
 				setListener (impl);
diff --git a/src/Mono.Android/Java.Interop/EventHelper.cs b/src/Mono.Android/Java.Interop/EventHelper.cs
index d46a42eb702..ba1752a3d16 100644
--- a/src/Mono.Android/Java.Interop/EventHelper.cs
+++ b/src/Mono.Android/Java.Interop/EventHelper.cs
@@ -33,7 +33,6 @@ public static void RemoveEventHandler<TInterface, TImplementor> (
 			remove (impl);
 			if (empty (impl)) {
 				unsetListener (impl);
-				impl.Dispose ();
 				impl = null;
 				implementor.Target = null;
 				implementor = null;