From 498fe75c4aad0a36579ee62539098f2c6d7401db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ji=C5=99=C3=AD=20Pol=C3=A1=C5=A1ek?= Date: Mon, 28 Jul 2025 16:06:23 +0200 Subject: [PATCH] CmdPal: Avoid reentrancy issues when loading more items (#40715) ## Summary of the Pull Request When checking the HasMoreItems flag, COM can start a nested message pump, which allows a reentrant call on the XAML UI and causes a fast fail. This change moves the check off the UI thread to prevent reentrancy, but the loading flag is set before we know for sure that there is something to load. This update also introduces a change: if LoadMore fails, we clear the loading flag immediately. ## PR Checklist - [x] **Closes:** #40707 - [ ] **Communication:** I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected - [ ] **Tests:** Added/updated and all pass - [ ] **Localization:** All end-user-facing strings can be localized - [ ] **Dev docs:** Added/updated - [ ] **New binaries:** Added on the required places - [ ] [JSON for signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json) for new binaries - [ ] [WXS for installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs) for new binaries and localization folder - [ ] [YML for CI pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml) for new test projects - [ ] [YML for signed pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml) - [ ] **Documentation updated:** If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys) and link it here: #xxx ## Detailed Description of the Pull Request / Additional comments ## Validation Steps Performed --- .../Helpers/InterlockedBoolean.cs | 46 +++++++++++++++++++ .../ListViewModel.cs | 40 +++++++++++----- 2 files changed, 75 insertions(+), 11 deletions(-) create mode 100644 src/modules/cmdpal/Microsoft.CmdPal.Common/Helpers/InterlockedBoolean.cs diff --git a/src/modules/cmdpal/Microsoft.CmdPal.Common/Helpers/InterlockedBoolean.cs b/src/modules/cmdpal/Microsoft.CmdPal.Common/Helpers/InterlockedBoolean.cs new file mode 100644 index 0000000000..8113ef9990 --- /dev/null +++ b/src/modules/cmdpal/Microsoft.CmdPal.Common/Helpers/InterlockedBoolean.cs @@ -0,0 +1,46 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Threading; + +namespace Microsoft.CmdPal.Common.Helpers; + +/// +/// Thread-safe boolean implementation using atomic operations +/// +public struct InterlockedBoolean(bool initialValue = false) +{ + private int _value = initialValue ? 1 : 0; + + /// + /// Gets or sets the boolean value atomically + /// + public bool Value + { + get => Volatile.Read(ref _value) == 1; + set => Interlocked.Exchange(ref _value, value ? 1 : 0); + } + + /// + /// Atomically sets the value to true + /// + /// True if the value was previously false, false if it was already true + public bool Set() + { + return Interlocked.Exchange(ref _value, 1) == 0; + } + + /// + /// Atomically sets the value to false + /// + /// True if the value was previously true, false if it was already false + public bool Clear() + { + return Interlocked.Exchange(ref _value, 0) == 1; + } + + public override int GetHashCode() => Value.GetHashCode(); + + public override string ToString() => Value.ToString(); +} diff --git a/src/modules/cmdpal/Microsoft.CmdPal.Core.ViewModels/ListViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.Core.ViewModels/ListViewModel.cs index 57ae504dff..8d54dd141f 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.Core.ViewModels/ListViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.Core.ViewModels/ListViewModel.cs @@ -6,6 +6,7 @@ using System.Collections.ObjectModel; using CommunityToolkit.Mvvm.ComponentModel; using CommunityToolkit.Mvvm.Input; using CommunityToolkit.Mvvm.Messaging; +using Microsoft.CmdPal.Common.Helpers; using Microsoft.CmdPal.Core.ViewModels.Messages; using Microsoft.CmdPal.Core.ViewModels.Models; using Microsoft.CommandPalette.Extensions; @@ -31,7 +32,7 @@ public partial class ListViewModel : PageViewModel, IDisposable private readonly Lock _listLock = new(); - private bool _isLoading; + private InterlockedBoolean _isLoading; private bool _isFetching; public event TypedEventHandler? ItemsUpdated; @@ -121,7 +122,7 @@ public partial class ListViewModel : PageViewModel, IDisposable ItemsUpdated?.Invoke(this, EventArgs.Empty); UpdateEmptyContent(); - _isLoading = false; + _isLoading.Clear(); } } @@ -221,7 +222,7 @@ public partial class ListViewModel : PageViewModel, IDisposable } ItemsUpdated?.Invoke(this, EventArgs.Empty); - _isLoading = false; + _isLoading.Clear(); }); } @@ -469,21 +470,38 @@ public partial class ListViewModel : PageViewModel, IDisposable return; } - if (model.HasMoreItems && !_isLoading) + if (!_isLoading.Set()) { - _isLoading = true; - _ = Task.Run(() => + return; + + // NOTE: May miss newly available items until next scroll if model + // state changes between our check and this reset + } + + _ = Task.Run(() => + { + // Execute all COM calls on background thread to avoid reentrancy issues with UI + // with the UI thread when COM starts inner message pump + try { - try + if (model.HasMoreItems) { model.LoadMore(); + + // _isLoading flag will be set as a result of LoadMore, + // which must raise ItemsChanged to end the loading. } - catch (Exception ex) + else { - ShowException(ex, model.Name); + _isLoading.Clear(); } - }); - } + } + catch (Exception ex) + { + _isLoading.Clear(); + ShowException(ex, model.Name); + } + }); } protected override void FetchProperty(string propertyName)