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.88k stars 1.38k forks source link

ToastContentBuilder().Show() throw unhandled exception System.AccessViolationException #4794

Open frostshoxx opened 1 year ago

frostshoxx commented 1 year ago

Describe the bug

Hi there I have a background task that will read .NET Channels message and generate ToastContentBuilder() based on that.

However, I notice some random occurrence of System.AccessViolationException after several .Show() is called. It tends to happen on the 3rd call to the .Show() command if that helps with some clues.

Here is my example code.

       while (await _progressMqSvc.WaitForMessageAsync(cancellationToken))
            {
                if (_progressMqSvc.TryGetMessage(out var progressMsg) && progressMsg is not null)
                {
                    try
                    {
                        var title = progressMsg.Title;
                        var description = progressMsg.Description;
                        new ToastContentBuilder()
                            .AddArgument("conversationId", 9813)
                            .AddText($"{title}")
                            .AddText($"{description}")
                            .Show();
                    }
                    catch (AccessViolationException e)
                    {
                        await _logger.Log(new LogMessage("Unable to raise a toast notification", LogLevel.Error,
                            exception: e));
                    }

                    await _logger.Log(new LogMessage($"{progressMsg.Title} (TaskID #{progressMsg.TaskId}):{progressMsg.Description}"));
                }
            }
  at ABI.Windows.UI.Notifications.IToastNotifierMethods.Show(WinRT.IObjectReference, Windows.UI.Notifications.ToastNotification)
   at CommunityToolkit.WinUI.Notifications.ToastContentBuilder.Show(CommunityToolkit.WinUI.Notifications.CustomizeToast)
   at PASApp.DataBroker.TestConsoleApp.ToasterNotificationProgressChannelReaderConsumer+<ExecuteAsync>d__3.MoveNext()
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1+AsyncStateMachineBox`1[[System.Threading.Tasks.VoidTaskResult, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].ExecutionContextCallback(System.Object)
   at System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1+AsyncStateMachineBox`1[[System.Threading.Tasks.VoidTaskResult, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].MoveNext(System.Threading.Thread)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1+AsyncStateMachineBox`1[[System.Threading.Tasks.VoidTaskResult, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].MoveNext()
   at System.Runtime.CompilerServices.TaskAwaiter+<>c.<OutputWaitEtwEvents>b__12_0(System.Action, System.Threading.Tasks.Task)
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore+ContinuationWrapper.Invoke()
   at System.Threading.Tasks.AwaitTaskContinuation.RunOrScheduleAction(System.Action, Boolean)
   at System.Threading.Tasks.Task.RunContinuations(System.Object)
   at System.Threading.Tasks.Task.FinishContinuations()

Expected behavior

Toaster notification should keep showing as there are notificaitons to be pushed out

Screenshot

image

Target OS Version

Windows 10.0.22621.0

Visual Studio Version

2022

ghost commented 1 year ago

Hello frostshoxx, 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 🙌

michael-hawker commented 1 year ago

@frostshoxx is this in a UWP or WinUI/WindowsAppSDK project? Which version of the NuGet package are you using?

FYI @AdamBarlow @vaheeshta

frostshoxx commented 1 year ago

@frostshoxx is this in a UWP or WinUI/WindowsAppSDK project? Which version of the NuGet package are you using?

FYI @adambarlow @vaheeshta

I tried separately and had problem on both of them (CommunityToolkit.WinUI.Notifications and Microsoft.Toolkit.Uwp.Notifications version 7.1.2). I first thought it was a WinUI library issue, but once I switched to the Uwp one, it still throws the same error. Note that the issues have not happened on the first call to .Show() from what I tried to reproduce so far. Sometimes the .Show() were called several minutes apart.

michael-hawker commented 1 year ago

@frostshoxx we just released 7.1.3 (UWP package, but should work for WinUI as well); can you try that one and see if it exhibits the same behavior?

frostshoxx commented 1 year ago

Sounds good, I'll give that a try. Thanks!

frostshoxx commented 1 year ago

@michael-hawker,

I ran the code for about half an hour and had not seen the issue coming back so far. I'll try to run it overnight before calling a day and see if the issue comes back.

Do you have any plan on publishing the WinUI package version?

Thanks again!

frostshoxx commented 1 year ago

Unfortunately, the same error still occurs . The only thing I did was to change the code branch and rerun it (also made sure that the active branch is using latest UWP 7.1.3).

Could this have anything to do with "conversationId" or some attribute that ties to an old toaster notification from previous runs?

alexbereznikov commented 2 months ago

@michael-hawker Any news on this? We're experiencing this issue, too. For us, notification is displayed and then access violation happens. Just a 2 lines of text, no buttons etc, nothing. Happens rarely but the more users we have the more often we face it.

Also consider this related Mozilla bug - https://bugzilla.mozilla.org/show_bug.cgi?id=1828073

I did some investigation. Unfortunately I have no access to MS sources but it narrows down to the following.

Access Violation happens here (in StringReference::_ConstructorHelper): image

Here the code seems to be scanning a string until it faces 0 terminator. So either rdx is invalid or there is no 0.

This function is called here (in ToastNotifierImpl::LogShowTelemetry): image

So we do pass via rdx qword contained at address rsi+48h, let's look what we have there image

There we have 8 zero bytes. Hence, when iterating in constructor helper, code tries to access address 0 and fails with access violation.

This rsi comes from rcx which seems to be the first method parameter, let's look where it called (in ToastNotifierImpl::Show): image

So it seems like here rcx is set to rdi before the call, and rdi is set to rcx at the method beginning (i e it seems to be the first parameter).

This method is called in IToastNotifierMethods.Show and what happens there is absolute madness, but my best guess is that value being passed is thisPtr.

This did not helped me to fix the issue but maybe might give you some clues. We're going to implement a custom VEH handler because it is weird when your app crashes because of notification.

alexbereznikov commented 2 months ago

Also, offloading notifications display logic to a separate STA thread seemed to help us