xamarin / XamarinCommunityToolkit

The Xamarin Community Toolkit is a collection of Animations, Behaviors, Converters, and Effects for mobile development with Xamarin.Forms. It simplifies and demonstrates common developer tasks building iOS, Android, and UWP apps with Xamarin.Forms.
MIT License
1.58k stars 471 forks source link

[Bug] Fatal NullReferenceException while disposing ImageReader #1413

Open NuAoA opened 3 years ago

NuAoA commented 3 years ago

Description

I've run into problems on a android application I am developing where it appears a low level NullReferenceException is being thrown while disposing the camera view from XamarainCommunityToolkit. This exception will crash the whole application and I can't reliably reproduce the issue.

Looking at the source code for this it appears the call is already has a null check, so i'm not sure how the error brings down the application (Xamarin.CommunityToolkit.UI.Views.CameraFragment.DisposeImageReader()). Maybe there is a race condition where the code can pass the null check and another thread has set the value to null?

https://github.com/xamarin/XamarinCommunityToolkit/blob/main/src/CommunityToolkit/Xamarin.CommunityToolkit/Views/CameraView/Android/CameraFragment.android.cs#L890

Stack Trace

Time    Device Name Type    PID Tag Message
06-17 10:02:22.842  Samsung SM-G930W8 (ce011711241f10660c)  Info    25327   MonoDroid   android.runtime.JavaProxyThrowable: System.NullReferenceException: Object reference not set to an instance of an object
  at Xamarin.CommunityToolkit.UI.Views.CameraFragment.DisposeImageReader () [0x00013] in <5265966be8d2407fb7a4c3b38c6441d7>:0 
  at Xamarin.CommunityToolkit.UI.Views.CameraFragment.CloseDevice () [0x00051] in <5265966be8d2407fb7a4c3b38c6441d7>:0 
  at Xamarin.CommunityToolkit.UI.Views.CameraFragment.Android.Views.TextureView.ISurfaceTextureListener.OnSurfaceTextureDestroyed (Android.Graphics.SurfaceTexture surface) [0x00000] in <5265966be8d2407fb7a4c3b38c6441d7>:0 
  at Android.Views.TextureView+ISurfaceTextureListenerInvoker.n_OnSurfaceTextureDestroyed_Landroid_graphics_SurfaceTexture_ (System.IntPtr jnienv, System.IntPtr native__this, System.IntPtr native_surface) [0x0000f] in <fb24f5518a9b4a54850cdd0dc561edd9>:0 
  at (wrapper dynamic-method) Android.Runtime.DynamicMethodNameCounter.96(intptr,intptr,intptr)
Time    Device Name Type    PID Tag Message
06-17 10:02:22.842  Samsung SM-G930W8 (ce011711241f10660c)  Info    25327   MonoDroid   
  at Java.Interop.JniEnvironment+InstanceMethods.CallNonvirtualVoidMethod (Java.Interop.JniObjectReference instance, Java.Interop.JniObjectReference type, Java.Interop.JniMethodInfo method, Java.Interop.JniArgumentValue* args) [0x0008e] in <1959115d56f8444789986cf39185638c>:0 
  at Java.Interop.JniPeerMembers+JniInstanceMethods.InvokeVirtualVoidMethod (System.String encodedMember, Java.Interop.IJavaPeerable self, Java.Interop.JniArgumentValue* parameters) [0x00063] in <1959115d56f8444789986cf39185638c>:0 
  at Android.Views.ViewGroup.RemoveView (Android.Views.View view) [0x00031] in <fb24f5518a9b4a54850cdd0dc561edd9>:0 
  at Xamarin.Forms.Platform.Android.ViewExtensions.RemoveFromParent (Android.Views.View view) [0x00013] in <25214d643f8b4b97a15bcdb62359e8e5>:0 
  at Xamarin.Forms.Platform.Android.AppCompat.Platform+<>c__DisplayClass30_0.<Xamarin.Forms.INavigation.PopModalAsync>b__0 (Android.Animation.Animator a) [0x00000] in <25214d643f8b4b97a15bcdb62359e8e5>:0 
  at Xamarin.Forms.Platform.Android.GenericAnimatorListener.OnAnimationEnd (Android.Animation.Animator animation) [0x0000e] in <25214d643f8b4b97a15bcdb62359e8e5>:0 
  at Android.Animation.AnimatorListenerAdapter.n_OnAnimationEnd_Landroid_animation_Animator_ (System.IntPtr jnienv, System.IntPtr native__this, System.IntPtr native_animation) [0x0000f] in <fb24f5518a9b4a54850cdd0dc561edd9>:0 
  at (wrapper dynamic-method) Android.Runtime.DynamicMethodNameCounter.90(intptr,intptr,intptr)
    at crc643f46942d9dd1fff9.GenericAnimatorListener.n_onAnimationEnd(Native Method)
    at crc643f46942d9dd1fff9.GenericAnimatorListener.onAnimationEnd(GenericAnimatorListener.java:40)
    at android.view.ViewPropertyAnimator$AnimatorEventListener.onAnimationEnd(ViewPropertyAnimator.java:1122)
    at android.animation.Animator$AnimatorListener.onAnimationEnd(Animator.java:552)
    at android.animation.ValueAnimator.endAnimation(ValueAnimator.java:1209)
    at android.animation.ValueAnimator.doAnimationFrame(ValueAnimator.java:1449)
    at android.animation.AnimationHandler.doAnimationFrame(AnimationHandler.java:146)
    at android.animation.AnimationHandler.-wrap2(Unknown Source:0)
    at android.animation.AnimationHandler$1.doFrame(AnimationHandler.java:54)
    at android.view.Choreographer$CallbackRecord.run(Choreographer.java:909)
    at android.view.Choreographer.doCallbacks(Choreographer.java:723)
    at android.view.Choreographer.doFrame(Choreographer.java:655)
    at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:897)
    at android.os.Handler.handleCallback(Handler.java:789)
    at android.os.Handler.dispatchMessage(Handler.java:98)
    at android.os.Looper.loop(Looper.java:164)
    at android.app.ActivityThread.main(ActivityThread.java:6944)
    at java.lang.reflect.Method.invoke(Native Method)
    at com.android.internal.os.Zygote$MethodAndArgsCaller.run(Zygote.java:327)
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1374)

Steps to Reproduce

I can't reliably recreate this issue, it appears to happen randomly. I have setup automated testing on my application that will auto navigate through a few screens, acquire a image using the CameraView, navigate back to the landing page and repeat. Once every 100-200 or so cycles the app will hard crash. The stack trace was pulled from logcat.

Basic Information

jfversluis commented 3 years ago

Thanks for the detailed report!

NuAoA commented 3 years ago

After attempting to clean up some of my test code i'm still running into this exception. It must be some kind of race happening where the previous camera view is closing when the server pushes a command to open the view again.

Looking at the XCT source, it appears it is setup to catch Java.Lang.Exception and not a generic System.Exception. That is why this error is crashing the entire application. I am far from a xamarin expert but is there a reason that a System.Exception should be bubbled up from the camera view and only Java.Lang.Exception should be caught?

    try
    {
        DisposeMediaRecorder();
    }
    catch (Java.Lang.Exception e)
    {
        LogError("Error close device", e);
    }
    CloseSession();
    try
    {
        if (sessionBuilder != null)
        {
            sessionBuilder.Dispose();
            sessionBuilder = null;
        }

        if (device != null)
        {
            device.Close();
            device = null;
        }

        DisposeImageReader();
    }
    catch (Java.Lang.Exception e)
    {
        LogError("Error close device", e);
    }
NuAoA commented 3 years ago

I've had some more time to dig into this, and i believe i have confirmed my suspicions that this is a race between threads.

I connected the XCT source to my project to debug things and updated CameraGragment.android.cs to log the ID of the current thread that is disposing it:

void DisposeImageReader()
{
    if (photoReader != null)
    {
        System.Diagnostics.Debug.WriteLine($"Disposing Image Reader on thread ID {Thread.CurrentThread().Id}");
        photoReader.Close();
        photoReader.Dispose();
        photoReader = null;
    }
}

I managed to catch the race happening:

Console Log:

[0:] Disposing Image Reader on thread ID 6191
[0:] Disposing Image Reader on thread ID 2

image

Exception:

Object reference not set to an instance of an object.
  at Xamarin.CommunityToolkit.UI.Views.CameraFragment.DisposeImageReader () [0x0002e] in C:\repos\XamarinCommunityToolkit\src\CommunityToolkit\Xamarin.CommunityToolkit\Views\CameraView\Android\CameraFragment.android.cs:895 
  at Xamarin.CommunityToolkit.UI.Views.CameraFragment.CloseDevice () [0x0006a] in C:\repos\XamarinCommunityToolkit\src\CommunityToolkit\Xamarin.CommunityToolkit\Views\CameraView\Android\CameraFragment.android.cs:716 
  at Xamarin.CommunityToolkit.UI.Views.CameraFragment.Android.Views.TextureView.ISurfaceTextureListener.OnSurfaceTextureDestroyed (Android.Graphics.SurfaceTexture surface) [0x00001] in C:\repos\XamarinCommunityToolkit\src\CommunityToolkit\Xamarin.CommunityToolkit\Views\CameraView\Android\CameraFragment.android.cs:799 
  at Android.Views.TextureView+ISurfaceTextureListenerInvoker.n_OnSurfaceTextureDestroyed_Landroid_graphics_SurfaceTexture_ (System.IntPtr jnienv, System.IntPtr native__this, System.IntPtr native_surface) [0x00010] in /Users/builder/azdo/_work/2/s/xamarin-android/src/Mono.Android/obj/Release/monoandroid10/android-29/mcw/Android.Views.TextureView.cs:129 
  at (wrapper dynamic-method) Android.Runtime.DynamicMethodNameCounter.84(intptr,intptr,intptr)

When I placed breakpoints to see what is calling into this method there are two sources, when the object is disposed and also the TextureView.ISurfaceTextureListener.OnSurfaceTextureDestroyed handler.

I think my next steps will be adding a lock around the DisposeImageReader logic to prevent this race condition.