Skip to content
This repository has been archived by the owner on May 15, 2024. It is now read-only.

Allow JNI activation of EssentialsNetworkCallback to fix #1996 #1997

Closed
wants to merge 2 commits into from

Conversation

cpraehaus
Copy link
Contributor

Description of Change

This avoids app crash with "NotSupportedException: Unable to activate instance of type Xamarin.Essentials.Connectivity+EssentialsNetworkCallback from native handle" in case callback invocation is scheduled but managed wrapper EssentialsNetworkCallback has already been disposed.

This change allows JNI to create managed wrapper in case original EssentialsNetworkCallback is already disposed. In such a case EssentialsNetworkCallback is created, the callback is delivered and ignored and then the callback object disposes itself to avoid mem-leak.

Bugs Fixed

API Changes

None

Behavioral Changes

None

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard
  • Updated documentation (see walkthrough)

This avoids app crash with "NotSupportedException: Unable to activate
instance of type Xamarin.Essentials.Connectivity+EssentialsNetworkCallback from
native handle" in case callback invocation is scheduled but managed wrapper
EssentialsNetworkCallback has already been disposed.
@kobynz
Copy link

kobynz commented Aug 10, 2022

Bump - Can we get this reviewed?

This issue is causing crashes in production apps...

@PTancek
Copy link

PTancek commented Jan 13, 2023

Could we please finish this issue? This is still causing crashes in our production

@epsmae
Copy link

epsmae commented Jan 24, 2023

Any news on that?

@vividos
Copy link
Contributor

vividos commented Feb 19, 2023

@jfversluis I'm also gettting some crashes in AppCenter for one of my apps. Is it already fixed in MAUI Essentials?

@PTancek
Copy link

PTancek commented Feb 21, 2023

To anyone wtill facing this issue. We went back to using https://www.nuget.org/packages/Xam.Plugin.Connectivity and the issue went away.
Even though that packages is not maintained any more, it is still compatible with the latest Xamarin.Forms version (we are currently on 5.0.0.2545)

Really dissapointed on this issue with xamarin essentials

@jonpryor
Copy link
Member

I don't like this change, adding (IntPtr, JniHandlerOwnership) constructors often hides problems. The NotSupportedException + MissingMethodException is thrown because Java code is attempting to invoke a method on a C# class, and the association between the Java-side instance and the C#-side instance has been broken.

Why has it been broken? Unless you don't care, and you'll never car, adding a (IntPtr, JniHandleOwnership) constructor "fix the bug" by hiding the issue. It's not a fix itself.

Enter a quick investigation: #1996 (comment)

I think this is actually a multithreading issue between Java code invoking methods on EssentialsNetworkCallback and the unregistration code of Connectivity.UnregisterNetworkCallback(). If this assumption is correct, the actual course of action is to remove this .Dispose() call:

The GC can cleanup the instance "later", once Java code is no longer referencing the EssentialsNetworkCallback instance.

@vividos
Copy link
Contributor

vividos commented May 17, 2023

@jonpryor I also got the sporadic errors in AppCenter for some time, and I had the impression that it's only happening when the C# part of the app is long gone, but the Java part tries to receive a broadcast and had to "re-create" the C# class, since it was already GCd. At some point in time I added removing the event handler again:
https://github.com/vividos/WhereToFly/blob/c1fcc549047e3af21b9dd47a980a5cafa4dbbb02/src/App/Core/Views/MapPage.cs#L851

I got no recent errors in AppCenter anymore, although I can't say that this was the fix, as not many people use my app.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants