Sporadic crash in `AvTrace.IsWpfTracingEnabledInRegistry()` when calling `PresentationTraceSources.Refresh()` from non-UI thread

ssrmm commented 1 week ago

Description

Calling PresentationTraceSources.Refresh() from a thread other than the UI thread may sporadically lead to System.InvalidOperationException: Nullable object must have a value. in AvTrace.IsWpfTracingEnabledInRegistry() (see two possible stack traces below).

The documentation of PresentationTraceSources.Refresh() does not mention anything about having to be called on a specific thread. And looking at the source code implicated by the stack traces, it seems like the class is intended to be thread-safe, in particular because of the comment in IsWpfTracingEnabledInRegistry():

if( _enabledInRegistry == null )
{
    [...]

    // Update the static.  Doing this last protects us from threading problems; worse case, multiple
    // threads will set the same value into it.
    _enabledInRegistry = enabled;
}

return (bool) _enabledInRegistry;

However turns out this comment isn't quiet correct: Doing it twice isn't the worst case. A thread other than the UI thread may call PresentationTraceSources.Refresh(), which in turn calls AvTrace.Refresh(). If this happens between the assignment (_enabledInRegistry = enabled) and the return statement then the Nullable<bool> _enabledInRegistry will not have a value and the cast to bool throws an exception.

public void Refresh()
{
    // Cause the registry to be re-read in case it's changed.
    _enabledInRegistry = null;

    // Re-initialize everything
    Initialize();
}

Stack traces

``` Unhandled exception. System.Windows.Markup.XamlParseException: 'Initialization of 'System.Windows.Controls.TextBlock' threw an exception.' Line number '9' and line position '76'. ---> System.Xaml.XamlObjectWriterException: Set property 'System.Windows.ResourceDictionary.DeferrableContent' threw an exception. ---> System.TypeInitializationException: The type initializer for 'MS.Internal.TraceResourceDictionary' threw an exception. ---> System.InvalidOperationException: Nullable object must have a value. at System.Nullable`1.get_Value() at MS.Internal.AvTrace.IsWpfTracingEnabledInRegistry() at MS.Internal.AvTrace.ShouldCreateTraceSources() at MS.Internal.AvTrace.Initialize() at MS.Internal.TraceResourceDictionary..cctor() --- End of inner exception stack trace --- at MS.Internal.TraceResourceDictionary.get_IsEnabled() at System.Windows.ResourceDictionary.SetKeys(IList`1 keyCollection, IServiceProvider serviceProvider) at MS.Internal.Xaml.Runtime.ClrObjectRuntime.SetValue(Object inst, XamlMember property, Object value) --- End of inner exception stack trace --- at MS.Internal.Xaml.Runtime.ClrObjectRuntime.SetValue(Object inst, XamlMember property, Object value) at MS.Internal.Xaml.Runtime.PartialTrustTolerantRuntime.SetValue(Object obj, XamlMember property, Object value) at System.Xaml.XamlObjectWriter.Logic_ApplyPropertyValue(ObjectWriterContext ctx, XamlMember prop, Object value, Boolean onParent) at System.Xaml.XamlObjectWriter.Logic_DoAssignmentToParentProperty(ObjectWriterContext ctx) at System.Xaml.XamlObjectWriter.WriteEndMember() at System.Xaml.XamlServices.Transform(XamlReader xamlReader, XamlWriter xamlWriter, Boolean closeWriter) at System.Windows.SystemResources.ResourceDictionaries.LoadDictionary(Assembly assembly, String assemblyName, String resourceName, Boolean isTraceEnabled, Uri& dictionarySourceUri) at System.Windows.SystemResources.ResourceDictionaries.LoadThemedDictionary(Boolean isTraceEnabled) at System.Windows.SystemResources.FindDictionaryResource(Object key, Type typeKey, ResourceKey resourceKey, Boolean isTraceEnabled, Boolean allowDeferredResourceReference, Boolean mustReturnDeferredResourceReference, Boolean& canCache) at System.Windows.SystemResources.FindResourceInternal(Object key, Boolean allowDeferredResourceReference, Boolean mustReturnDeferredResourceReference) at System.Windows.StyleHelper.GetThemeStyle(FrameworkElement fe, FrameworkContentElement fce) at System.Windows.FrameworkElement.UpdateThemeStyleProperty() at System.Windows.FrameworkElement.OnInitialized(EventArgs e) at MS.Internal.Xaml.Runtime.ClrObjectRuntime.InitializationGuard(XamlType xamlType, Object obj, Boolean begin) --- End of inner exception stack trace --- at System.Windows.Markup.XamlReader.RewrapException(Exception e, IXamlLineInfo lineInfo, Uri baseUri) at System.Windows.Markup.WpfXamlLoader.Load(XamlReader xamlReader, IXamlObjectWriterFactory writerFactory, Boolean skipJournaledProperties, Object rootObject, XamlObjectWriterSettings settings, Uri baseUri) at System.Windows.Markup.WpfXamlLoader.LoadBaml(XamlReader xamlReader, Boolean skipJournaledProperties, Object rootObject, XamlAccessLevel accessLevel, Uri baseUri) at System.Windows.Markup.XamlReader.LoadBaml(Stream stream, ParserContext parserContext, Object parent, Boolean closeStream) at System.Windows.Application.LoadBamlStreamWithSyncInfo(Stream stream, ParserContext pc) at System.Windows.Application.DoStartup() at System.Windows.Application.<.ctor>b__1_0(Object unused) at System.Windows.Threading.ExceptionWrapper.InternalRealCall(Delegate callback, Object args, Int32 numArgs) at System.Windows.Threading.ExceptionWrapper.TryCatchWhen(Object source, Delegate callback, Object args, Int32 numArgs, Delegate catchHandler) at System.Windows.Threading.DispatcherOperation.InvokeImpl() at MS.Internal.CulturePreservingExecutionContext.CallbackWrapper(Object obj) at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state) --- End of stack trace from previous location --- at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state) at MS.Internal.CulturePreservingExecutionContext.Run(CulturePreservingExecutionContext executionContext, ContextCallback callback, Object state) at System.Windows.Threading.DispatcherOperation.Invoke() at System.Windows.Threading.Dispatcher.ProcessQueue() at System.Windows.Threading.Dispatcher.WndProcHook(IntPtr hwnd, Int32 msg, IntPtr wParam, IntPtr lParam, Boolean& handled) at MS.Win32.HwndWrapper.WndProc(IntPtr hwnd, Int32 msg, IntPtr wParam, IntPtr lParam, Boolean& handled) at MS.Win32.HwndSubclass.DispatcherCallbackOperation(Object o) at System.Windows.Threading.ExceptionWrapper.InternalRealCall(Delegate callback, Object args, Int32 numArgs) at System.Windows.Threading.ExceptionWrapper.TryCatchWhen(Object source, Delegate callback, Object args, Int32 numArgs, Delegate catchHandler) at System.Windows.Threading.Dispatcher.LegacyInvokeImpl(DispatcherPriority priority, TimeSpan timeout, Delegate method, Object args, Int32 numArgs) at MS.Win32.HwndSubclass.SubclassWndProc(IntPtr hwnd, Int32 msg, IntPtr wParam, IntPtr lParam) at MS.Win32.UnsafeNativeMethods.DispatchMessage(MSG& msg) at System.Windows.Threading.Dispatcher.PushFrameImpl(DispatcherFrame frame) at System.Windows.Application.RunDispatcher(Object ignore) at System.Windows.Application.RunInternal(Window window) at WpfApp1.Program.Main() ``` ``` Unhandled exception. System.TypeInitializationException: The type initializer for 'MS.Internal.TraceRoutedEvent' threw an exception. ---> System.InvalidOperationException: Nullable object must have a value. at System.Nullable`1.get_Value() at MS.Internal.AvTrace.IsWpfTracingEnabledInRegistry() at MS.Internal.AvTrace.ShouldCreateTraceSources() at MS.Internal.AvTrace.Initialize() at MS.Internal.TraceRoutedEvent..cctor() --- End of inner exception stack trace --- at MS.Internal.TraceRoutedEvent.get_IsEnabled() at System.Windows.UIElement.RaiseEventImpl(DependencyObject sender, RoutedEventArgs args) at System.Windows.FrameworkElement.OnRenderSizeChanged(SizeChangedInfo sizeInfo) at System.Windows.ContextLayoutManager.fireSizeChangedEvents() at System.Windows.ContextLayoutManager.UpdateLayout() at System.Windows.Interop.HwndSource.SetLayoutSize() at System.Windows.Interop.HwndSource.set_RootVisualInternal(Visual value) at System.Windows.Window.SetRootVisualAndUpdateSTC() at System.Windows.Window.SetupInitialState(Double requestedTop, Double requestedLeft, Double requestedWidth, Double requestedHeight) at System.Windows.Window.CreateSourceWindow(Boolean duringShow) at System.Windows.Window.ShowHelper(Object booleanBox) at System.Windows.Threading.ExceptionWrapper.InternalRealCall(Delegate callback, Object args, Int32 numArgs) at System.Windows.Threading.ExceptionWrapper.TryCatchWhen(Object source, Delegate callback, Object args, Int32 numArgs, Delegate catchHandler) at System.Windows.Threading.DispatcherOperation.InvokeImpl() at MS.Internal.CulturePreservingExecutionContext.CallbackWrapper(Object obj) at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state) --- End of stack trace from previous location --- at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state) at MS.Internal.CulturePreservingExecutionContext.Run(CulturePreservingExecutionContext executionContext, ContextCallback callback, Object state) at System.Windows.Threading.DispatcherOperation.Invoke() at System.Windows.Threading.Dispatcher.ProcessQueue() at System.Windows.Threading.Dispatcher.WndProcHook(IntPtr hwnd, Int32 msg, IntPtr wParam, IntPtr lParam, Boolean& handled) at MS.Win32.HwndWrapper.WndProc(IntPtr hwnd, Int32 msg, IntPtr wParam, IntPtr lParam, Boolean& handled) at MS.Win32.HwndSubclass.DispatcherCallbackOperation(Object o) at System.Windows.Threading.ExceptionWrapper.InternalRealCall(Delegate callback, Object args, Int32 numArgs) at System.Windows.Threading.ExceptionWrapper.TryCatchWhen(Object source, Delegate callback, Object args, Int32 numArgs, Delegate catchHandler) at System.Windows.Threading.Dispatcher.LegacyInvokeImpl(DispatcherPriority priority, TimeSpan timeout, Delegate method, Object args, Int32 numArgs) at MS.Win32.HwndSubclass.SubclassWndProc(IntPtr hwnd, Int32 msg, IntPtr wParam, IntPtr lParam) at MS.Win32.UnsafeNativeMethods.DispatchMessage(MSG& msg) at System.Windows.Threading.Dispatcher.PushFrameImpl(DispatcherFrame frame) at System.Windows.Application.RunDispatcher(Object ignore) at System.Windows.Application.RunInternal(Window window) at WpfApp1.Program.Main() ```

Reproduction Steps

We are experiencing this as a very rare occurrence on some machines. I have created a minimal reproduction case that forces the issue to happen more frequently. The important parts are in Program.cs.

Simply starting this application a few times produces the crash pretty easily on my machine.

Expected behavior

Preferred: Crash is avoided by making AvTrace properly thread-safe.

Alternatively: Requirement of calling PresentationTraceSources.Refresh() on the UI thread is documented and ideally enforced by always throwing an exception when called on the wrong thread.

Actual behavior

A crash occurs very rarely and only on specific systems, making this problem hard to find during testing.

Regression?

Looking at the Git history, a regression seems unlikely. The code in question seems to have been unchanged for a long time.

Known Workarounds

Perform calls to PresentationTraceSources.Refresh() on the UI thread only.

Impact

Rare application startup crash that may only happen on customer machines and is difficult to find during development/testing.

Configuration

.NET 6/.NET 8 x64 Windows 10/Windows 11

Other information

No response

h3xds1nz commented 1 week ago

This is indeed a bad design if the intention was to make it TS, a static 'cache' variable that can get 'refreshed' without any locking, made nullable for 'state' and casted to regular bool.

lindexi commented 1 week ago

Thank you for your detailed investigation.

Although there are possible thread-safety issues here, I think the cost of the lock is relatively large. I think it's a good idea to keep the current code and update the documentation.

ssrmm commented 1 week ago

I haven't looked at the remainder of the class, but at least the accesses to _enabledInRegistry seem like they could be converted to use the Interlocked class. So that might be an option if the performance impact of using a full lock would be too large.

h3xds1nz commented 1 week ago

@lindexi I agree the lock here would be unnecessary overhead, the tracing is already slow. Making it an atomic operation would a performant solution that would also fix a problem, something I had in mind as I personally think that AvTrace should be thread-safe; and the intention appears to have been there as well.

In general, all classes should be considered not thread-safe in .NET unless it is specified otherwise in the documentation.

dotnet / wpf