Powertoys: [Settings app] Crash when toggling FancyZones on/off

[enricogior]

ℹ Computer information

• PowerToys version: 0.20.1 and current master https://github.com/microsoft/PowerToys/commit/9e8b0d28079c6985d36e4738ba4ce1bd517d417b
• PowerToy Utility: new Settings app
• Running PowerToys as Admin: no
• Windows build number: 2004 build 19041.450

📝 Provide detailed reproduction steps (if any)

1. Start PT
2. Open PT Settings
3. Toggle the Enable FancyZones switch on and off

✔️ Expected result

Settings app to remain open.

❌ Actual result

The new settings are saved but the Settings app crashes.

Note: it only happens in release mode, the debug build doesn't crash for me.

Debugging the crash dump show different call stacks, in one case the call stack looks like this (and it doesn't make a lot of sense):

In another instance it shows the crash in https://github.com/microsoft/PowerToys/blob/9592fab414fe373a15e6a83dcc71ebf7c1c55d95/src/core/Microsoft.PowerToys.Settings.UI.Lib/SettingsUtils.cs#L55 because the file was in use by another process:

The crash doesn't happen all the time, but very often and it only happens when toggling the FZ module, I tried all other modules several times and it never happened.

[enricogior]

@laviusmotileng-ms
can you reproduce it?
Adding a try catch for var jsonSettingsString = File.ReadAllText(GetSettingsPath(powertoy, fileName)); seems to avoid the crash, so that should be the first thing to investigate.

[crutkas]

I've seen issues regarding files in use.

[crutkas]

could this be both FZ and settings are trying to access the file as FZ starts up?

[enricogior]

@crutkas
FZ doesn't access that file, the runner does, but not while the Settings app is running and it also closes it right after it uses it.
The fact that it doesn't repro in debug mode is quite strange. Seems like a file descriptor not being garbage collected by the .NET Core runtime.

[crutkas]

@laviusmotileng-ms Is this going to be done in the next few days?

[ryanbodrug-microsoft]

@laviusmotileng-ms Marking this as in progress. Let me know if that's not the case.

[alekhyareddy28]

@enricogior, @ryanbodrug-microsoft I've been looking into this issue and it seems like it's happening with shortcut guide as well and not only FZ. Link to watson cab for shortcut guide: https://watsonportal.microsoft.com/CabAnalysis?CabIdentifier=https://weus2watcab01.blob.core.windows.net/global-202009/ce8a2c37-59a6-408f-990e-05457720d033.zip

Please let me know if I'm missing something @enricogior, because runner seems to access the general settings.json file even when settings app is running and this according to me is causing the IOException when settings tries to access the same file. (C:\Users\USERNAME\AppData\Local\Microsoft\PowerToys\settings.json)

Whenever any change is made in the settings ui regarding enabling and disabling a powerToy, the sendConfigMSG function (code can be found in the settings.ui.lib project/viewmodels for each PT) is called which notifies the runner of the change using ipc. The dispatch_received_json function is called in the runner which in turn calls the apply_general_settings function and that saves information to the settings.json file.

On the other hand, whenever a UI change is invoked, the WPF converter of settings is invoked which is in the ModuleEnabledToForegroundConverter.cs class. The converter function tries to read the general settings file but since it's still being written to by the runner, it throws an IOException.

Converter:

<TextBlock x:Uid="FancyZones_ZoneBehavior_GroupSettings"
                       Style="{StaticResource SettingsGroupTitleStyle}"
                       Foreground="{x:Bind Mode=OneWay, Path=ViewModel.IsEnabled, Converter={StaticResource ModuleEnabledToForegroundConverter}}"/>

C#:

public sealed class ModuleEnabledToForegroundConverter : IValueConverter
    {
        public object Convert(object value, Type targetType, object parameter, string language)
        {
            bool isEnabled = (bool)value;
            GeneralSettings generalSettings = SettingsUtils.GetSettings<GeneralSettings>(string.Empty);
            ...
        }
}

My guess about why we're able to reproduce it more often in release mode is because of optimizations. We're now probably trying to access the file in settings faster than in debug mode, even before runner gets a chance to finish writing to it.

[alekhyareddy28]

I was thinking we should unify reading and writing the settings.json file to either settings or runner, in a way that it is compatible with the older settings as well. I was thinking it would be ideal to do this in the runner. Any thoughts on this @enricogior?

[enricogior]

@alekhyareddy28

I was thinking we should unify reading and writing the settings.json file to either settings or runner

It was unified, then we introduced the new settings that had different requirements and until we remove the old settings it won't be possible without making too many changes to the new settings.

We're now probably trying to access the file in settings faster than in debug mode, even before runner gets a chance to finish writing to it.

I suspect the problem is not the runner, that does a very quick access to the settings using C++ APIs, while the Settings uses the .NET runtime that may not be fast enough to react to OS events. This is just a guess, I didn't debug it, but in case we should simply make the Settings code more resilient with a retry loop.

[enricogior]

Fixed in 0.23.0 https://github.com/microsoft/PowerToys/releases/tag/v0.23.0