-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Android] Don't dispose connectivity listeners #15145
Merged
PureWeen
merged 1 commit into
dotnet:main
from
jonpryor:jonp-fix-Essentials-missing-Android-activation-ctor
May 18, 2023
Merged
[Android] Don't dispose connectivity listeners #15145
PureWeen
merged 1 commit into
dotnet:main
from
jonpryor:jonp-fix-Essentials-missing-Android-activation-ctor
May 18, 2023
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
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
PureWeen
approved these changes
May 18, 2023
mattleibow
added
the
backport/suggested
The PR author or issue review has suggested that the change should be backported.
label
May 22, 2023
rmarinho
pushed a commit
that referenced
this pull request
May 30, 2023
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
hartez
added
the
backport/approved
After some discussion or review, this PR or change was approved to be backported.
label
May 31, 2023
/backport to net7.0 |
Started backporting to net7.0: https://github.com/dotnet/maui/actions/runs/5138843200 |
samhouts
added
platform/android 🤖
area-essentials
Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info
labels
Jul 20, 2023
samhouts
added
the
fixed-in-8.0.0-preview.5.8529
Look for this fix in 8.0.0-preview.5.8529!
label
Aug 2, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
area-essentials
Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info
backport/approved
After some discussion or review, this PR or change was approved to be backported.
backport/suggested
The PR author or issue review has suggested that the change should be backported.
fixed-in-8.0.0-preview.5.8529
Look for this fix in 8.0.0-preview.5.8529!
platform/android 🤖
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.
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:
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: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#:
so where did the instance go?
There are generally two ways that the mapping between a Java instance and C# instance are lost:
.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, which allows you to see where e.g.
key_handle 0x522746d
(which comes from the exception message) was disposed:If it's a GC bug, the
-g-
message is from threadfinalizer
. 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 aDispose()
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 subsequentNotSupportedException
+MissingMethodException
?Enter multithreading…
Turns Out™ that
ConnectivityManager
appears to make use of multiple threads, which provides this possible chain of events:manager.RegisterNetworkCallback(request, networkCallback)
networkCallback
, which we'll refer to asjavaCB
:ConnectivityManager.NetworkCallback javaCB = …
manager.UnregisterNetworkCallback(networkCallback)
networkCallback.Dispose()
, which severs the mapping betweenjavaCB
andnetworkCallback
.javaCB.onCapabilitiesChanged()
ConnectivityManager.NetworkCallback.OnCapabilitiesChanged()
, which needs to get an instance upon which to invoke.OnCapabilitiesChanged()
. This promptly blows up with theNotSupportedException
.The fix, in this case, is to not do step (4): avoiding the
.Dispose()
invocation allowsjavaCB
to remain valid, and will preventjavaCB.onCapabilitiesChanged(…)
from throwing. This does mean that thenetworkCallback
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.Description of Change
Issues Fixed
Fixes #