Accessible_HighContrastChanged threading issue in dev/7.0.0 impl of ThemeListener

[0x7c13]

This is regarding to the new dispatcher queue implementation of ThemeListener in dev/7.0.0 branch.

I made a copy of it and embedded it in my own app. Then I am start to seeing this weird exception from some of my users:

Taking look at the source code, I found this:

The error happens within this code path:
_accessible.HighContrastChanged += Accessible_HighContrastChanged;

Since high contrast change will trigger both Accessible_HighContrastChanged event as well as Settings_ColorValuesChanged event at same time. But the dispatcher is only used in Settings_ColorValuesChanged code path but not Accessible_HighContrastChanged path. I was wondering if Accessible_HighContrastChanged is getting fired off UI thread which could leads problem here?

However, after testing, I could not re-pro it on my machine. But it definitely is an issue since not only one of my users but actually 5 of them are affected. That is happening a lot after only one day of release.

Before I jump to the conclusion, I quickly did below changes in my app and the issue is gone:

Can you guys take a look and see if you can find out why this is causing the issue here?

[ghost]

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

[0x7c13]

@azchohfi

[michael-hawker]

@azchohfi @Sergio0694 thoughts on this potential double-update conflict?

[Sergio0694]

Not super familiar with the ThemeListener class in particular, though I don't see any problems at all adding that dispatcher queue dispatching in the other method as well, makes perfect sense. The TryEnqueue extension (by the way, that code in the screen is a bit out of date, those methods are different now) will skip the dispatching if they already have thread access anyway. Also now that I look at that screen, that async state machine in the second method is not needed and can be removed as well 🙂

@Jasonstein Would you be ok creating a PR based off of master now and do these changes?
We can take care of it otherwise. Thanks for the report!

[Kyaa-dost]

@Jasonstein ⬆️

[Kyaa-dost]

@Jasonstein let us know if you can proceed with the PR