Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Settings reload causes occasional crash in SUI #9273

Closed
DHowett opened this issue Feb 24, 2021 · 7 comments · Fixed by #10390
Closed

Settings reload causes occasional crash in SUI #9273

DHowett opened this issue Feb 24, 2021 · 7 comments · Fixed by #10390
Assignees
@carlos-zamora
Labels
Area-Settings UI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Crash Crashes are real bad news.
Milestone
Terminal v1.10

Comments

@DHowett
Copy link
Member

DHowett commented Feb 24, 2021

It looks like manipulating the Navigation View's menu still wigs it out a bit.

08 (Inline Function) --------`--------     Microsoft_Terminal_Settings_Editor!winrt::check_hresult+0xb [E:\BA\271\s\src\cascadia\TerminalSettingsEditor\Generated Files\winrt\base.h @ 4825] 
09 (Inline Function) --------`--------     Microsoft_Terminal_Settings_Editor!winrt::impl::consume_Windows_Foundation_Collections_IVector<winrt::Windows::Foundation::Collections::IVector<winrt::Windows::Foundation::IInspectable>,winrt::Windows::Foundation::IInspectable>::ReplaceAll+0x4f [E:\BA\271\s\src\cascadia\TerminalSettingsEditor\Generated Files\winrt\Windows.Foundation.Collections.h @ 247] 
0a 00000068`690fefd0 00007ff8`83d99611     Microsoft_Terminal_Settings_Editor!winrt::Microsoft::Terminal::Settings::Editor::implementation::MainPage::UpdateSettings$_ResumeCoro$1+0xafd [E:\BA\271\s\src\cascadia\TerminalSettingsEditor\MainPage.cpp @ 117] 
0b (Inline Function) --------`--------     Microsoft_Terminal_Settings_Editor!std::experimental::coroutine_handle<void>::resume+0xd [C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.28.29333\include\experimental\coroutine @ 107] 
0c (Inline Function) --------`--------     Microsoft_Terminal_Settings_Editor!std::experimental::coroutine_handle<void>::operator()+0xd [C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.28.29333\include\experimental\coroutine @ 99] 
0d (Inline Function) --------`--------     Microsoft_Terminal_Settings_Editor!winrt::resume_foreground::__l2::awaitable::await_suspend::__l2::<lambda_2ab9998b3c4eacdb9b89a7a0699bc21a>::operator()+0xd [E:\BA\271\s\src\cascadia\TerminalSettingsEditor\Generated Files\winrt\Windows.UI.Core.h @ 3326] 
0e 00000068`690ff390 00007ff8`acb454a8     Microsoft_Terminal_Settings_Editor!winrt::impl::delegate<winrt::Windows::UI::Core::DispatchedHandler,<lambda_2ab9998b3c4eacdb9b89a7a0699bc21a> >::Invoke+0x11 [E:\BA\271\s\src\cascadia\TerminalSettingsEditor\Generated Files\winrt\Windows.UI.Core.h @ 1277] 
0f 00000068`690ff3c0 00007ff8`acb0505f     Windows_UI!CreateControlInput+0x1a658
@DHowett DHowett added Issue-Bug It either shouldn't be doing this or needs an investigation. Severity-Crash Crashes are real bad news. Product-Terminal The new Windows Terminal. Priority-1 A description (P1) Area-Settings UI Anything specific to the SUI labels Feb 24, 2021
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Feb 24, 2021
@DHowett
Copy link
Member Author

DHowett commented Feb 24, 2021

Pointed-to line:

image

@DHowett DHowett added this to the Terminal v1.8 milestone Feb 26, 2021
@DHowett
Copy link
Member Author

DHowett commented Feb 26, 2021

Marked for 1.8 -- @carlos-zamora FYI. Not so you fix it, just so you know I took Triage off. 😄

@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Feb 26, 2021
@carlos-zamora
Copy link
Member

@DHowett Did you have a repro for this?

@DHowett
Copy link
Member Author

DHowett commented Mar 4, 2021

SURE DO!

PowerShell:

while($true){sleep 0.5; touch $Env:LOCALAPPDATA\packages\WindowsTerminalDev_8wekyb3d8bbwe\localstate\settings.json }

that will reload your settings EVERY 0.5 SECONDS. Keep the SUI open and navigate it.

@carlos-zamora carlos-zamora self-assigned this Mar 10, 2021
@carlos-zamora
Copy link
Member

Some notes from looking into this a bit:

touch isn't actually a cmdlet (it's installed by yori, it seems). Instead I can use the powershell command below to append a space to the end of the settings.json. It's annoying to deal with the extra spaces, but not the end of the world.

while($true){sleep 0.5; Add-Content $Env:LOCALAPPDATA\packages\WindowsTerminalDev_8wekyb3d8bbwe\localstate\settings.json " " }

Working branch: dev/cazamor/sui/bugfix-reload-crash

For whatever reason, ReplaceAll keeps failing. It looks like it's because menuItems is null? But I've added checks for nullness everywhere (including right before the ReplaceAll call) and no luck. I'll try looking into this a bit more later because I'm just annoyed with it now haha.

@lhecker lhecker mentioned this issue Jun 11, 2021
Merged
3 tasks
@ghost ghost added the In-PR This issue has a related PR label Jun 11, 2021
@ghost ghost closed this as completed in #10390 Jun 11, 2021
@ghost ghost removed the In-PR This issue has a related PR label Jun 11, 2021
ghost pushed a commit that referenced this issue Jun 11, 2021
@lhecker
Fix SUI race conditions when reloading settings (#10390)
b034fc9 
## Summary of the Pull Request

This commit fixes various race conditions regarding the settings UI. It's unsafe to write to class members from background threads without acquiring mutexes or yielding to the main thread first.
By changing the settings reload code path to yield to the main thread early, we're able to cut down on code complexity and unsafe member accesses.

## PR Checklist
* [x] Closes #9273
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed

* Settings UI reloads without crashing ✔️
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Jun 11, 2021
DHowett pushed a commit that referenced this issue Jul 7, 2021
@lhecker @DHowett
Fix SUI race conditions when reloading settings (#10390)
dbe688d 
## Summary of the Pull Request

This commit fixes various race conditions regarding the settings UI. It's unsafe to write to class members from background threads without acquiring mutexes or yielding to the main thread first.
By changing the settings reload code path to yield to the main thread early, we're able to cut down on code complexity and unsafe member accesses.

## PR Checklist
* [x] Closes #9273
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed

* Settings UI reloads without crashing ✔️

(cherry picked from commit b034fc9)

# Conflicts:
#	src/cascadia/TerminalApp/AppLogic.cpp
#	src/cascadia/TerminalApp/AppLogic.h
@ghost
Copy link

ghost commented Jul 14, 2021

🎉This issue was addressed in #10390, which has now been successfully released as Windows Terminal v1.9.1942.0.:tada:

Handy links:

@ghost
Copy link

ghost commented Jul 14, 2021

🎉This issue was addressed in #10390, which has now been successfully released as Windows Terminal Preview v1.10.1933.0.:tada:

Handy links:

This was referenced Jan 4, 2023
Closed
Closed
Merged
DHowett pushed a commit that referenced this issue Mar 16, 2023
@zadjii-msft
Rebuild the profile nav via MenuItemsSource; mitigate a crash (#14630)
8c17475 
Directly manipulating the `NavigationView::MenuItems` vector is bad. If
you do that, you're gonna get crashes, in WinUI code for `Measure`.
However, a WinUI PR (below) gave me an idea: Changing
`NavigationView::MenuItemsSource` will wholly invalidate the entirety of
the nav view items, and that will avoid the crash.

This code does that. It's a wee bit janky, but it works. 

Closes #13673

_might_ affect #12333, need to try and repro. 

See also:
* #9273
* #10390
* microsoft/microsoft-ui-xaml#6302
* microsoft/microsoft-ui-xaml#3138, which was
the fix for microsoft/microsoft-ui-xaml#2818
DHowett pushed a commit that referenced this issue Mar 31, 2023
@zadjii-msft @DHowett
Rebuild the profile nav via MenuItemsSource; mitigate a crash (#14630)
9670230 
Directly manipulating the `NavigationView::MenuItems` vector is bad. If
you do that, you're gonna get crashes, in WinUI code for `Measure`.
However, a WinUI PR (below) gave me an idea: Changing
`NavigationView::MenuItemsSource` will wholly invalidate the entirety of
the nav view items, and that will avoid the crash.

This code does that. It's a wee bit janky, but it works.

Closes #13673

_might_ affect #12333, need to try and repro.

See also:
* #9273
* #10390
* microsoft/microsoft-ui-xaml#6302
* microsoft/microsoft-ui-xaml#3138, which was
the fix for microsoft/microsoft-ui-xaml#2818

(cherry picked from commit 8c17475)
Service-Card-Id: 88428128
Service-Version: 1.17
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@carlos-zamora carlos-zamora

Labels
Area-Settings UI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Crash Crashes are real bad news.
Projects
None yet
Milestone
Terminal v1.10
Development

Successfully merging a pull request may close this issue.

Fix SUI race conditions when reloading settings microsoft/terminal
3 participants
@DHowett @carlos-zamora @cinnamon-msft