CommunityToolkit / WindowsCommunityToolkit

The Windows Community Toolkit is a collection of helpers, extensions, and custom controls. It simplifies and demonstrates common developer tasks building .NET apps with UWP and the Windows App SDK / WinUI 3 for Windows 10 and Windows 11. The toolkit is part of the .NET Foundation.
https://docs.microsoft.com/windows/communitytoolkit/
Other
5.86k stars 1.37k forks source link

Null reference error from PrintHelper #3465

Open MartinRichards23 opened 4 years ago

MartinRichards23 commented 4 years ago

Describe the bug

We are occasionally getting null reference errors from users in our logging, originating from the PrintHelper.

I haven't been able to reproduce the issue, only seen in the logs, I have provided the stack trace.

Is the fix as simple as adding a null ref check in the PrintHelper class?

    System.NullReferenceException: Object reference not set to an instance of an object.

    Microsoft.Toolkit.Uwp.Helpers    PrintHelper.<DetachCanvas>b__39_0 () /_/Microsoft.Toolkit.Uwp/Helpers/PrintHelper/PrintHelper.cs at 227:21
    System    Action.Invoke ()
    Microsoft.Toolkit.Uwp.Helpers.DispatcherHelper    <>c__DisplayClass11_0.<AwaitableRunAsync>b__0 () /_/Microsoft.Toolkit.Uwp/Helpers/DispatcherHelper.cs at 258:21
    Microsoft.Toolkit.Uwp.Helpers.DispatcherHelper.<>c__DisplayClass10_0$1<System    __Canon>.<AwaitableRunAsync>b__0 () /_/Microsoft.Toolkit.Uwp/Helpers/DispatcherHelper.cs at 235:21
    System.Runtime.ExceptionServices    ExceptionDispatchInfo.Throw ()
    System.Runtime.CompilerServices    TaskAwaiter.ThrowForNonSuccess (Task)
    System.Runtime.CompilerServices    TaskAwaiter.HandleNonSuccessAndDebuggerNotification (Task)
    System.Runtime.CompilerServices    TaskAwaiter.ValidateEnd (Task)
    System.Runtime.CompilerServices    TaskAwaiter.GetResult ()
    Microsoft.Toolkit.Uwp.Helpers.PrintHelper    <DetachCanvas>d__39.MoveNext () /_/Microsoft.Toolkit.Uwp/Helpers/PrintHelper/PrintHelper.cs at 238:13
    System.Runtime.ExceptionServices    ExceptionDispatchInfo.Throw ()
    System.Runtime.CompilerServices    TaskAwaiter.ThrowForNonSuccess (Task)
    System.Runtime.CompilerServices    TaskAwaiter.HandleNonSuccessAndDebuggerNotification (Task)
    System.Runtime.CompilerServices    TaskAwaiter.ValidateEnd (Task)
    System.Runtime.CompilerServices    TaskAwaiter.GetResult ()
    Microsoft.Toolkit.Uwp.Helpers.PrintHelper.<>c__DisplayClass40_1    <<PrintTaskRequested>b__2>d.MoveNext () at 16707566
    System.Runtime.ExceptionServices    ExceptionDispatchInfo.Throw ()
    System.Runtime.CompilerServices.AsyncMethodBuilderCore    <>c.<ThrowAsync>b__7_0 (Object)
    System    Action`1.Invoke (T)    
    System.Threading.WinRTSynchronizationContext
    Invoker.InvokeCore ()    System.Runtime.InteropServices    McgMarshal.ThrowOnExternalCallFailed (Int32, RuntimeTypeHandle)
    __Interop    ComCallHelpers.Call ($__ComObject __this, RuntimeTypeHandle __typeHnd, Int32 __targetIndex) Call at 15732480
    __Interop.ForwardComStubs    Stub_11<System.__Canon> (Void* InstParam, $__ComObject __this, Int32 __targetIndex) Stub_11 at 16707566
    Microsoft.AppCenter.Utils    ApplicationLifecycleHelper.<ctor>b__17_1 (Object sender, $UnhandledErrorDetectedEventArgs eventArgs)

Environment

It doesn't seem to be specific to any device, Toolkit or Windows verison.

ghost commented 4 years ago

Hello MartinRichards23, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

Kyaa-dost commented 4 years ago

@MartinRichards23 can you please provide the steps you are taking from the beginning and what windows build number and target version being used?

MartinRichards23 commented 4 years ago

Hi Kyaa-dost, we are targeting version 1903, build 18362, minimum version Fall Creators update, build 16299.

Our code is essentially the same as the demo provided here https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/master/Microsoft.Toolkit.Uwp.SampleApp/SamplePages/PrintHelper/PrintHelperPage.xaml.cs

We are using App Center for logging, this issue seems rare so its not something I've been able to reproduce.

alvinashcraft commented 3 years ago

I'm investigating this issue to see if I can repro it in the sample app or my own projects.

alvinashcraft commented 3 years ago

Given the stack trace from @MartinRichards23, it seems like either _canvasContainer or _printCanvas is null when this code is executed in DetachCanvas():

                await DispatcherQueue.EnqueueAsync(() =>
                {
                    _canvasContainer.Children.Remove(_printCanvas);
                    _printCanvas.Children.Clear();
                });

I could add null checks for both, but I can't repro the original issue. I hate to create a PR for something without reproducing the issue and showing that it's resolved with the change.

alvinashcraft commented 3 years ago

_printCanvas seems more likely. The parameter that sets _canvasContainer is checked for null, and the value is not changed/removed later.

Kyaa-dost commented 3 years ago

@MartinRichards23 ⬆️

MartinRichards23 commented 3 years ago

I can't add any more insight as to the cause of the issue, as I only have some error logs to go on, and haven't reproduced the issue myself.

Isn't it sensible to always check for null if a null is possible?

michael-hawker commented 3 years ago

Took a quick look at this. Feel like if _printCanvas is null then it means the PrintHelper has already been disposed, as that's the only time it's cleared from its initialization. But then this is only called after a Print has been completed (either successfully or not), so it'd be weird for it to be cleaned-up in-between. Could this be a case of a print being initiated and then the app closed?

The _canvasContainer is an external reference, so maybe we should have a WeakReference or something to not hold the parent? However in any of these cases, I'm not sure what the expected result/outcome would want to be?

Would be good to have repro steps to see if we can catch this some how. Also just wondering if we want to clean-up how the object is disposed in general since we don't have unmanaged resources and check in other spots instead if its already been disposed? (https://vkontech.com/the-dispose-pattern-step-by-step/ https://docs.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose)