[net7.0] [Android] Don't dispose connectivity listeners #15379
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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
rmarinho
approved these changes
Jun 1, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport of #15145 to net7.0
/cc @hartez @jonpryor