Skip to content

Commit 5ef79ab

Browse files
brianrobCopilot
brianrob
and
Copilot
authored
Fix XamlMessageBox STA Threading Crash from Background Threads (#2400)
* Fix XamlMessageBox STA threading crash from background threads XamlMessageBox.Show() now auto-dispatches to the UI thread when called from a background thread, matching the old System.Windows.MessageBox behavior. This fixes the SecurityCheck delegate crash during symbol resolution, where the delegate is invoked on Task.Run threads. The fix checks Application.Current.Dispatcher.CheckAccess() and uses synchronous Dispatcher.Invoke to marshal the call, preserving the return value for callers. Existing UI-thread call sites are unaffected (CheckAccess returns true, no dispatch needed). Fixes #2300 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * review-loop iteration 1: prefer owner dispatcher, guard Application singleton, guard RegisterClassHandler - XamlMessageBox.cs: Use owner?.Dispatcher ?? Application.Current?.Dispatcher so the dispatch targets the correct UI thread when an owner window is provided. Simplify with Dispatcher.Invoke(Func<T>) to return result directly. - XamlMessageBoxTests.cs: Guard Application creation with Application.Current ?? to prevent InvalidOperationException in shared AppDomains. - XamlMessageBoxTests.cs: Add static guard for RegisterClassHandler (permanent, AppDomain-wide registration) and use XamlMBTest_ prefix for unique captions. - XamlMessageBoxTests.cs: Use indexer assignment for theme resources (idempotent). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * review-loop iteration 2: use current STA dispatcher instead of app.Dispatcher in test Replace app.Dispatcher.BeginInvoke with Dispatcher.CurrentDispatcher.BeginInvoke to ensure work is queued to the dispatcher being pumped by Dispatcher.Run(). If Application.Current was reused from a prior test on a different thread, app.Dispatcher would target that thread's dispatcher, causing the test to hang. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 83c8085 commit 5ef79ab

2 files changed

Lines changed: 173 additions & 0 deletions

File tree

‎src/PerfView.Tests/Dialogs/XamlMessageBoxTests.cs‎

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
using System;
2+
using System.Threading;
3+
using System.Threading.Tasks;
4+
using System.Windows;
5+
using System.Windows.Media;
6+
using System.Windows.Threading;
7+
using PerfView.Dialogs;
8+
using Xunit;
9+
10+
#pragma warning disable VSTHRD001 // Use JoinableTaskFactory — we're explicitly testing WPF Dispatcher threading
11+
#pragma warning disable VSTHRD110 // Observe awaitable — fire-and-forget Task.Run is intentional in these tests
12+
13+
namespace PerfViewTests.Dialogs
14+
{
15+
/// <summary>
16+
/// Regression tests for <see cref="XamlMessageBox"/> threading behavior.
17+
/// See https://github.com/microsoft/perfview/issues/2300
18+
/// </summary>
19+
public class XamlMessageBoxTests
20+
{
21+
/// <summary>
22+
/// Verifies that <see cref="XamlMessageBox.Show(string, string, MessageBoxButton)"/> auto-dispatches
23+
/// to the UI thread when called from a background thread, rather than throwing
24+
/// "The calling thread must be STA, because many UI components require this."
25+
/// Also verifies that calling from the UI thread directly still works (no-op dispatch).
26+
/// This is the core regression test for issue #2300.
27+
/// </summary>
28+
[Fact]
29+
public void Show_AutoDispatchesToUIThreadFromBackgroundThread()
30+
{
31+
Exception exception = null;
32+
MessageBoxResult uiResult = MessageBoxResult.None;
33+
MessageBoxResult bgResult = MessageBoxResult.None;
34+
35+
var staThread = new Thread(() =>
36+
{
37+
try
38+
{
39+
var app = Application.Current ?? new Application();
40+
app.ShutdownMode = ShutdownMode.OnExplicitShutdown;
41+
RegisterMinimalThemeResources(app);
42+
43+
// Catch unhandled dispatcher exceptions so they don't silently hang.
44+
app.DispatcherUnhandledException += (s, args) =>
45+
{
46+
exception = args.Exception;
47+
args.Handled = true;
48+
Dispatcher.CurrentDispatcher.InvokeShutdown();
49+
};
50+
51+
// Auto-close any XamlMessageBox dialogs as soon as they load.
52+
RegisterAutoCloseHandler();
53+
54+
// Safety timeout: force shutdown if the test hangs.
55+
var safetyTimer = new DispatcherTimer { Interval = TimeSpan.FromSeconds(8) };
56+
safetyTimer.Tick += (s, e) =>
57+
{
58+
safetyTimer.Stop();
59+
if (exception == null)
60+
{
61+
exception = new TimeoutException("Safety timer fired — dialog was not auto-closed");
62+
}
63+
Dispatcher.CurrentDispatcher.InvokeShutdown();
64+
};
65+
safetyTimer.Start();
66+
67+
// Use Dispatcher.CurrentDispatcher (the STA thread's dispatcher being
68+
// pumped by Dispatcher.Run) rather than app.Dispatcher, because if
69+
// Application.Current was reused from a prior test, app.Dispatcher may
70+
// belong to a different thread.
71+
var currentDispatcher = Dispatcher.CurrentDispatcher;
72+
currentDispatcher.BeginInvoke((Action)(() =>
73+
{
74+
try
75+
{
76+
// Part 1: Call directly from the UI thread (dispatch is a no-op).
77+
uiResult = XamlMessageBox.Show("Test message", "XamlMBTest_UI", MessageBoxButton.OK);
78+
79+
// Part 2: Call from a background thread — before the fix for issue #2300,
80+
// this would throw InvalidOperationException ("The calling thread must be STA")
81+
// because XamlMessageBox creates a WPF Window requiring the UI thread.
82+
Task.Run(() =>
83+
{
84+
try
85+
{
86+
bgResult = XamlMessageBox.Show("Test message", "XamlMBTest_BG", MessageBoxButton.YesNo);
87+
}
88+
catch (Exception ex)
89+
{
90+
exception = ex;
91+
}
92+
finally
93+
{
94+
currentDispatcher.BeginInvoke(
95+
(Action)(() => currentDispatcher.InvokeShutdown()));
96+
}
97+
});
98+
}
99+
catch (Exception ex)
100+
{
101+
exception = ex;
102+
Dispatcher.CurrentDispatcher.InvokeShutdown();
103+
}
104+
}));
105+
106+
Dispatcher.Run();
107+
}
108+
catch (Exception ex)
109+
{
110+
exception = ex;
111+
}
112+
});
113+
114+
staThread.SetApartmentState(ApartmentState.STA);
115+
staThread.Start();
116+
Assert.True(staThread.Join(TimeSpan.FromSeconds(10)), "Test timed out — dialog may not have been auto-closed");
117+
118+
Assert.Null(exception);
119+
// Both dialogs were auto-closed without clicking a button, so Result is None.
120+
Assert.Equal(MessageBoxResult.None, uiResult);
121+
Assert.Equal(MessageBoxResult.None, bgResult);
122+
}
123+
124+
private static bool s_autoCloseHandlerRegistered;
125+
126+
/// <summary>
127+
/// Registers a class-level handler that auto-closes any <see cref="Window"/> with
128+
/// a test caption as soon as it finishes loading. The handler fires inside
129+
/// <see cref="Window.ShowDialog"/>'s nested message loop.
130+
/// </summary>
131+
private static void RegisterAutoCloseHandler()
132+
{
133+
if (s_autoCloseHandlerRegistered)
134+
{
135+
return;
136+
}
137+
138+
s_autoCloseHandlerRegistered = true;
139+
EventManager.RegisterClassHandler(
140+
typeof(Window),
141+
FrameworkElement.LoadedEvent,
142+
new RoutedEventHandler((sender, args) =>
143+
{
144+
Window w = sender as Window;
145+
if (w != null && w.Title != null && w.Title.StartsWith("XamlMBTest_"))
146+
{
147+
w.Dispatcher.BeginInvoke((Action)(() => w.Close()));
148+
}
149+
}));
150+
}
151+
152+
/// <summary>
153+
/// Registers the minimal resources needed by <c>MessageBoxWindow.xaml</c> so it
154+
/// can be created without loading the full PerfView theme.
155+
/// </summary>
156+
private static void RegisterMinimalThemeResources(Application app)
157+
{
158+
app.Resources["CustomToolWindowStyle"] = new Style(typeof(Window));
159+
app.Resources["ControlDarkerBackground"] = new SolidColorBrush(Colors.LightGray);
160+
app.Resources["ControlDefaultBorderBrush"] = new SolidColorBrush(Colors.Gray);
161+
}
162+
}
163+
}

‎src/PerfView/Dialogs/XamlMessageBox.cs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,16 @@ public static MessageBoxResult Show(Window owner, string message, string caption
4646
/// <inheritdoc cref="MessageBox.Show(Window, string, string, MessageBoxButton, MessageBoxImage, MessageBoxResult)"/>
4747
public static MessageBoxResult Show(Window owner, string message, string caption, MessageBoxButton buttons, MessageBoxImage icon, MessageBoxResult defaultResult)
4848
{
49+
// XamlMessageBox uses a WPF window that must be created and shown on the UI thread.
50+
// Auto-dispatch to match the old System.Windows.MessageBox behavior of working from
51+
// any thread. This fixes callers like the SecurityCheck delegate which is invoked from
52+
// background threads during symbol resolution (see issue #2300).
53+
var dispatcher = owner?.Dispatcher ?? Application.Current?.Dispatcher;
54+
if (dispatcher is not null && !dispatcher.CheckAccess())
55+
{
56+
return dispatcher.Invoke(() => Show(owner, message, caption, buttons, icon, defaultResult));
57+
}
58+
4959
MessageBoxWindow window = new(message, caption, buttons, icon, defaultResult);
5060
if (owner is not null)
5161
{

0 commit comments

Comments
 (0)