Skip to content

Commit

Permalink
[Android] Don't dispose connectivity listeners
Browse files Browse the repository at this point in the history
Context (Fixes?): xamarin/Essentials#1996
Context: https://android.googlesource.com/platform/frameworks/base/+/a12044215b1148826ea9a88d5d1102378b13922f/core/java/android/net/ConnectivityManager.java#2412
Context: https://github.com/xamarin/xamarin-android/blob/ff5455ca95fc83c788e957353114578abf3b4f54/Documentation/guides/internals/debug-jni-objrefs.md#crash-via-unhandled-exception

In xamarin/Essentials#1996, the customer reports an app crash:

	AndroidRuntime: FATAL EXCEPTION: ConnectivityThread
	AndroidRuntime: Process: ***, PID: 31179
	AndroidRuntime: android.runtime.JavaProxyThrowable: System.NotSupportedException: Unable to activate instance of type Xamarin.Essentials.Connectivity+EssentialsNetworkCallback from native handle 0x780d4cef34 (key_handle 0x522746d). ---> System.MissingMethodException: No constructor found for Xamarin.Essentials.Connectivity+EssentialsNetworkCallback::.ctor(System.IntPtr, Android.Runtime.JniHandleOwnership) ---> Java.Interop.JavaLocationException: Exception of type 'Java.Interop.JavaLocationException' was thrown.
	AndroidRuntime:    --- End of inner exception stack trace ---
	AndroidRuntime:   at Java.Interop.TypeManager.CreateProxy (System.Type type, System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) [0x000b5] in <e41c0215a1b34d5f990de0d09dbe0e84>:0
	AndroidRuntime:   at Java.Interop.TypeManager.CreateInstance (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type targetType) [0x00111] in <e41c0215a1b34d5f990de0d09dbe0e84>:0
	AndroidRuntime:    --- End of inner exception stack trace ---
	AndroidRuntime:   at Java.Interop.TypeManager.CreateInstance (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type targetType) [0x0017e] in <e41c0215a1b34d5f990de0d09dbe0e84>:0
	AndroidRuntime:   at Java.Lang.Object.GetObject (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type type) [0x00023] in <e41c0215a1b34d5f990de0d09dbe0e84>:0
	AndroidRuntime:   at Java.Lang.Object._GetObject[T] (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) [0x00017] in <e41c0215a1b34d5f990de0d09dbe0e84>:0
	AndroidRuntime:   at Java.Lang.Object.GetObject[T] (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) [0x00000] in <e41c0215a1b34d5f990de0d09dbe0e84>:0
	AndroidRuntime:   at Java.Lang.Object.GetObject[T] (System.IntPtr jnienv, System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) [0x00006] in <e41c0215a1b34d5f990de0d09dbe0e84>:0
	AndroidRuntime:   at Android.Net.ConnectivityManager+NetworkCallback.n_OnCapabilitiesChanged_Landroid_net_Network_Landroid_net_NetworkCapabilities_ (System.IntPtr jnienv, System.IntPtr native__this, System.IntPtr native_network, System.IntPtr native_networkCapabilities) [0x00000] in <e41c0215a1b34d5f990de0d09dbe0e84>:0
	AndroidRuntime:   at (wrapper dynamic-method) Android.Runtime.DynamicMethodNameCounter.42(intptr,intptr,intptr,intptr)
	AndroidRuntime: 	at crc64a0e0a82d0db9a07d.Connectivity_EssentialsNetworkCallback.n_onCapabilitiesChanged(Native Method)
	AndroidRuntime: 	at crc64a0e0a82d0db9a07d.Connectivity_EssentialsNetworkCallback.onCapabilitiesChanged(Connectivity_EssentialsNetworkCallback.java:50)
	AndroidRuntime: 	at android.net.ConnectivityManager$NetworkCallback.onAvailable(ConnectivityManager.java:3580)
	AndroidRuntime: 	at android.net.ConnectivityManager$CallbackHandler.handleMessage(ConnectivityManager.java:3793)
	AndroidRuntime: 	at android.os.Handler.dispatchMessage(Handler.java:107)
	AndroidRuntime: 	at android.os.Looper.loop(Looper.java:237)
	AndroidRuntime: 	at android.os.HandlerThread.run(HandlerThread.java:67)

When there is an exception "chain" of `NotSupportedException` >
`MissingMethodException` mentioning a "missing constructor" with a
`(System.IntPtr, Android.Runtime.JniHandleOwnership)` signature, it
means that Java is calling a method on a C# class:

	// Java
	ConnectivityManager.NetworkCallback javaCB = …
	javaCB.onCapabilitiesChanged(…);

and .NET Android could not find an existing instance associated with
the Java instance `javaCB`, and is attempting to create a new C#
instance to subsequently invoke a method on it.

Usually, Java has this instance because it was created in C#:

	var networkCallback = new EssentialsNetworkCallback();
	manager.RegisterNetworkCallback(request, networkCallback);

so where did the instance go?

There are generally two ways that the mapping between a Java instance
and C# instance are lost:

 1. Horrible terrible no good very bad GC bug, or
 2. Someone called `.Dispose()` when they shouldn't have.

(1), while a possibility, is rarely the case. (2) is far more common.

To track down such things, you [capture a GREF log][0], which allows
you to see where e.g. `key_handle 0x522746d` (which comes from the
exception message) was disposed:

	+g+ grefc 217 gwrefc 0 obj-handle 0x9/I -> new-handle 0x25f6/G from thread '(null)'(20)
	…
	handle 0x25f6; key_handle 0xf3ac36b: Java Type: `crc64a0e0a82d0db9a07d/Connectivity_EssentialsNetworkCallback`; MCW type: `Xamarin.Essentials.Connectivity+EssentialsNetworkCallback`
	…
	-g- grefc 216 gwrefc 0 handle 0x25f6/G from thread '(null)'(20)
	…

If it's a GC bug, the `-g-` message is from thread `finalizer`.
If it's a "premature `.Dispose()`" bug, the `-g-` message will *not*
be from the finalizer thread, and the associated stack trace (if
present) will include a `Dispose()` method invocation.

In the absence of a complete GREF log, we have to use our imagination
a bit: what would cause `.Dispose()` to be invoked, and then a
subsequent `NotSupportedException`+`MissingMethodException`?

***Enter multithreading…***

Turns Out™ that `ConnectivityManager` appears to [make use of][1]
multiple threads, which provides this possible chain of events:

 1. Thread 1 (C#) calls
    `manager.RegisterNetworkCallback(request, networkCallback)`
 2. Thread 2 (Java) obtains a Java-side reference to `networkCallback`,
    which we'll refer to as `javaCB`:
    `ConnectivityManager.NetworkCallback javaCB = …`
 3. Thread 1 (C#) later calls
    `manager.UnregisterNetworkCallback(networkCallback)`
 4. Thread 1 (C#) calls
    `networkCallback.Dispose()`, which severs the mapping between
    `javaCB` and `networkCallback`.
 5. Thread 2 (Java) calls `javaCB.onCapabilitiesChanged()`
 6. This hits the marshal method for
    `ConnectivityManager.NetworkCallback.OnCapabilitiesChanged()`,
    which needs to get an instance upon which to invoke
    `.OnCapabilitiesChanged()`.
    This promptly blows up with the `NotSupportedException`.

The fix, in this case, is to *not* do step (4): avoiding the
`.Dispose()` invocation allows `javaCB` to remain valid, and will
prevent `javaCB.onCapabilitiesChanged(…)` from throwing.
This *does* mean that the `networkCallback` instance will live longer,
as we'll need to wait for a full cross-VM GC to occur before it is
collected, but this is "safest" and prevents the crash.

*In general*, if another Java-side thread can potentially invoke
methods on a C# subclass, you *should not* call `.Dispose()` on
instances of that type.

[0]: https://github.com/xamarin/xamarin-android/blob/ff5455ca95fc83c788e957353114578abf3b4f54/Documentation/guides/internals/debug-jni-objrefs.md#collect-complete-jni-object-reference-logs
[1]: https://android.googlesource.com/platform/frameworks/base/+/a12044215b1148826ea9a88d5d1102378b13922f/core/java/android/net/ConnectivityManager.java#2248
  • Loading branch information
jonpryor authored and github-actions committed Jun 1, 2023
1 parent a860fa9 commit a6d1fc5
Showing 1 changed file with 0 additions and 2 deletions.
2 changes: 0 additions & 2 deletions src/Essentials/src/Connectivity/Connectivity.android.cs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ void StopListeners()
Debug.WriteLine("Connectivity receiver already unregistered. Disposing of it.");
}

conectivityReceiver.Dispose();
conectivityReceiver = null;
}

Expand Down Expand Up @@ -100,7 +99,6 @@ void UnregisterNetworkCallback()

manager.UnregisterNetworkCallback(networkCallback);

networkCallback?.Dispose();
networkCallback = null;
}

Expand Down

0 comments on commit a6d1fc5

Please sign in to comment.