Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[Android] Don't dispose connectivity listeners (#15379)
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 Co-authored-by: Jonathan Pryor <jonpryor@vt.edu>
- Loading branch information