fix: make UiApplication instance thread-static instead of static

[chucker]

Pull request type

Please check the type of change your PR introduces:

• Update
• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no api changes)
• Build related changes
• Documentation content changes

What is the current behavior?

Various controls don't safely handle a multi-threaded UI. See also #594.

An example stack trace would be:

System.Windows.Markup.XamlParseException : Initialization of 'Wpf.Ui.Controls.Button' threw an exception.
---- System.InvalidOperationException : Cannot access Freezable 'System.Windows.Media.SolidColorBrush' across threads because it cannot be frozen.
Stack Trace: 

XamlReader.RewrapException(Exception e, IXamlLineInfo lineInfo, Uri baseUri)

FrameworkTemplate.LoadTemplateXaml(XamlReader templateReader, XamlObjectWriter currentWriter)

FrameworkTemplate.LoadTemplateXaml(XamlObjectWriter objectWriter)

FrameworkTemplate.LoadOptimizedTemplateContent(DependencyObject container, IComponentConnector componentConnector, IStyleConnector styleConnector, List1 affectedChildren, UncommonField1 templatedNonFeChildrenField)

FrameworkTemplate.LoadContent(DependencyObject container, List1 affectedChildren) StyleHelper.ApplyTemplateContent(UncommonField1 dataField, DependencyObject container, FrameworkElementFactory templateRoot, Int32 lastChildIndex, HybridDictionary childIndexFromChildID, FrameworkTemplate frameworkTemplate)

FrameworkTemplate.ApplyTemplateContent(UncommonField`1 templateDataField, FrameworkElement container)

FrameworkElement.ApplyTemplate()

FrameworkElement.MeasureCore(Size availableSize)

UIElement.Measure(Size availableSize)

<48 more frames...>

FrameworkElement.OnStyleChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)

DependencyObject.OnPropertyChanged(DependencyPropertyChangedEventArgs e)

FrameworkElement.OnPropertyChanged(DependencyPropertyChangedEventArgs e)

DependencyObject.NotifyPropertyChange(DependencyPropertyChangedEventArgs args)

DependencyObject.UpdateEffectiveValue(EntryIndex entryIndex, DependencyProperty dp, PropertyMetadata metadata, EffectiveValueEntry oldEntry, EffectiveValueEntry& newEntry, Boolean coerceWithDeferredReference, Boolean coerceWithCurrentValue, OperationType operationType)

DependencyObject.InvalidateProperty(DependencyProperty dp, Boolean preserveCurrentValue)

FrameworkElement.UpdateStyleProperty()

FrameworkElement.OnInitialized(EventArgs e)

FrameworkElement.TryFireInitialized()

ClrObjectRuntime.InitializationGuard(XamlType xamlType, Object obj, Boolean begin)

What is the new behavior?

The root cause appears to be the static UiApplication instance. By marking it with [ThreadStatic], we instead make the runtime create one instance per thread.

Other information

[luca-domenichini]

Sorry @pomianowski but I don't fully understand this PR.
Isn't WPF supposed to work on a single UI thread? By merging this PR, the UIApplication is going to be instantiated multiple times, but with the same underlying WPF Application.Current.

What is the use case of having an application-per-thread?
From what I understood so far about UI programming, and WPF is no different in that, the UI components/app need to be created/managed on the single-apartment-thread used to render the "UI loop".

In the example provided by @chucker, I think the problem is that he is trying to mess with the color of a button inside the wrong thread! Just use Dispatcher.BeginInvoke() to get the color changed properly.
I don't think having a ThreadLocal makes really sense..

Could you elaborate on that? Maybe I am wroing, but currently I don't see a valid reason for this change.

[chucker]

Isn't WPF supposed to work on a single UI thread?

Generally speaking, WPF assumes that all UI takes place on a single thread, yes.

By merging this PR, the UIApplication is going to be instantiated multiple times

I don't think that's true. Can you provide an example of why that would be? It'll be instantiated again if accessed from a different thread, which should only be the case when it's needed in a different thread.

My personal use case for using WPF controls (or brushes, or other resources) from multiple threads (but never one and the same object from different threads) is actually unit testing. Making NUnit or xUnit.net perform all tests on a single thread is tricky, so libraries like StaFact merely ensure one test is run in one and the same thread, but multiple tests can still run on different threads. See also this earlier comment of mine.

[luca-domenichini]

It'll be instantiated again if accessed from a different thread, which should only be the case when it's needed in a different thread.

Yes, sorry, I meant that.

Since System.Windows.Application.Current returns the singleton application for current AppDomain, we are practically wrapping with a thread-local var that singleton in WpfUI UiApplication.Current.
While the previous behavior was to throw an exception when accessing that Application from a different thread -- and this usually helps developers identifying bugs -- with this PR we are shadowing that and allowing different threads to access the app.

Moreover, it would be also possible to access the MainWindow from a different thread, since you can then call UiApplication.Current.MainWindow, resulting in creating a new UiApplication for calling thread and then accessing the MainWindow originally constructed and owned by WPF thread.

I don't know.. is this a "wanted" feature? Is this really necessary? Accessing MainWindow in the wrong thread offcourse would result in an exception, so maybe just making the App thread-local is safe, as you said.
But maybe we can see this as a limitation of Xunit+StaFact attribute, and this issue should be addressed there, not in WpfUI library.

[chucker]

But maybe we can see this as a limitation of Xunit+StaFact attribute

I don't see how they can address it.

The way I look at it:

• when using WPF built-in controls, I can instantiate them from different threads just fine, as long as they don't have to communicate with each other.
• with WPF UI controls, though, there's tight internal coupling (e.g., controls trying to access a resource via a static member that was originally created on a different thread, therefore causing potential cross-thread issues) causing exceptions that are hard to avoid for the library consumer. This PR tries to address one of those.

More generally speaking, I don't think the static members are a good design even if we only assume a single thread. WPF UI already does introduce DI in some places, and really, something like UiApplication (leaving aside the question of whether its scope is too broad) should probably be injected instead.

[luca-domenichini]

with WPF UI controls, though, there's tight internal coupling (e.g., controls trying to access a resource via a static member that was originally created on a different thread, therefore causing potential cross-thread issues) causing exceptions that are hard to avoid for the library consumer. This PR tries to address one of those.

Ok I got your point. But still, I think there is mismatch between UiApplication being threadLocal vs its method/props like Shutdown(), MainWindow, Resources etc.. that acts on WPF singleton Application.
Take Shutdown method for instance: it will shutdown the whole singleton WPF application, and not your own thread-local UiApplication.

That said, I understand there are some good benefits with PR 👍