[Bug] Use the correct Dispatcher

[KPixel]

Description

There are a few places in X.F where the code uses the Device's Dispatcher. This should be avoided as much as possible because multi-window apps will have multiple dispatchers. So using the correct one is important.

I went through all these places, with suggestions on how to fix them:

1. AppThemeBinding

https://github.com/xamarin/Xamarin.Forms/blob/52592fac6158c9f3abb9118c2ceca6c348e21c98/Xamarin.Forms.Core/AppThemeBinding.cs#L10

Should use _weakTarget's Dispatcher.

2. Shell

https://github.com/xamarin/Xamarin.Forms/blob/52592fac6158c9f3abb9118c2ceca6c348e21c98/Xamarin.Forms.Core/Shell/Shell.cs#L561 https://github.com/xamarin/Xamarin.Forms/blob/52592fac6158c9f3abb9118c2ceca6c348e21c98/Xamarin.Forms.Core/Shell/Shell.cs#L571

The Shell should use its own Dispatcher. So, all the InvokeOnMainThreadAsync() methods can be turned into an extensions. For that, we can just copy them from the Device class and add this IDispatcher dispatcher as the first parameter.

3. ShellSection

https://github.com/xamarin/Xamarin.Forms/blob/52592fac6158c9f3abb9118c2ceca6c348e21c98/Xamarin.Forms.Core/Shell/ShellSection.cs#L89

ShellSection should use its own Dispatcher.

4. BindingExpression

https://github.com/xamarin/Xamarin.Forms/blob/52592fac6158c9f3abb9118c2ceca6c348e21c98/Xamarin.Forms.Core/BindingExpression.cs#L743

Should be changed to:

if (_expression._weakTarget != null && _expression._weakTarget.TryGetTarget(out BindableObject obj))
{
    if (obj.Dispatcher.IsInvokeRequired)
        obj.Dispatcher.BeginInvokeOnMainThread(action);
    else
        action();
}

5. MarshalingObservableCollection

https://github.com/xamarin/Xamarin.Forms/blob/52592fac6158c9f3abb9118c2ceca6c348e21c98/Xamarin.Forms.Core/Items/MarshalingObservableCollection.cs#L60-L62

I think that this class should take a dispatcher as parameter (it can default to null if you don't want to burden most users who create single-window apps).

6. UWP Dispatcher

https://github.com/xamarin/Xamarin.Forms/blob/52592fac6158c9f3abb9118c2ceca6c348e21c98/Xamarin.Forms.Platform.UAP/Dispatcher.cs#L28

Should be changed to: !_coreDispatcher.HasThreadAccess

7. ObservableItemTemplateCollection

https://github.com/xamarin/Xamarin.Forms/blob/52592fac6158c9f3abb9118c2ceca6c348e21c98/Xamarin.Forms.Platform.UAP/CollectionView/ObservableItemTemplateCollection.cs#L60-L62

Should be changed to use: _container.Dispatcher

8. ItemsViewRenderer

https://github.com/xamarin/Xamarin.Forms/blob/52592fac6158c9f3abb9118c2ceca6c348e21c98/Xamarin.Forms.Platform.UAP/CollectionView/ItemsViewRenderer.cs#L116

This if statement should be removed. Everything inside should happen regardless of Element?.ItemsSource state. (This is not Dispatcher-related.)

9. WindowsTicker

https://github.com/xamarin/Xamarin.Forms/blob/52592fac6158c9f3abb9118c2ceca6c348e21c98/Xamarin.Forms.Platform.UAP/WindowsTicker.cs#L30

I don't know why we change the behavior based on the number of windows currently opened. I think we should always return the ThreadStatic ticker. Unless there is some scenario that matters? (This is not Dispatcher-related.)

10. By the way, there are many places with code like this:

https://github.com/xamarin/Xamarin.Forms/blob/52592fac6158c9f3abb9118c2ceca6c348e21c98/Xamarin.Forms.Core/Layout.cs#L368-L375

However, the implementation of the Dispatcher property and GetDispatcher() don't seem capable of returning null. So, this check seems unneeded... Maybe this is Android/iOS-related?

Basic Information

• Version with issue: 4.8.0.1269
• Platform Target Frameworks:
  • iOS: Haven't checked
  • Android: Haven't checked
  • UWP: 18362

[KPixel]

For the implementation of UWP's GetDispatcher():

I propose the following implementation:

public IDispatcher GetDispatcher(object context)
{
    if (context is VisualElement element)
    {
        var renderer = Platform.GetRenderer(element);
        if (renderer?.ContainerElement != null)
        {
            return new Dispatcher(renderer.ContainerElement.Dispatcher);
        }
    }

    return s_current = s_current ?? new Dispatcher();
}

If a VisualElement is provided, we don't know if it comes from the current thread, so we should always return its Dispatcher (in priority over s_current). For the same reason, we shouldn't keep this dispatcher in s_current.

Small detail: We could avoid instantiating many dispatchers per window by comparing s_current with renderer.ContainerElement.Dispatcher before returning it. Something like:

// Pseudo-code to put inside: if (renderer?.ContainerElement != null)
if (Equals(s_current?._coreDispatcher, renderer.ContainerElement.Dispatcher))
    return s_current;
return new Dispatcher(renderer.ContainerElement.Dispatcher);

I think the current implementation will fail if we create two windows: A and B, then we try to access the Dispatcher of a view in B for the first time while we are on the thread of A...

[hartez]

@KPixel For number 8, if you think this is a bug please open a separate issue.

[KPixel]

@hartez Done: #11908

[KPixel]

@samhouts Can you please triage this issue? Is there anything that I can do to help?