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

Crash when discarding settings rapidly #12333

Closed
mimvdb opened this issue Feb 2, 2022 · 2 comments
Closed

Crash when discarding settings rapidly #12333

mimvdb opened this issue Feb 2, 2022 · 2 comments
Assignees
@zadjii-msft
Labels
Area-SettingsUI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news.
Milestone
Terminal v1.18

Comments

@mimvdb
Copy link
Contributor

mimvdb commented Feb 2, 2022

Windows Terminal version

1.12.3472.0

Windows build number

10.0.19043.0

Other Software

No response

Steps to reproduce

Start fresh window, go to settings
Rapidly click on save, then rapidly click on discard in the settings UI

To reliable reproduce this I used an autohotkey script I had lying around to click the leftmousebutton very rapidly, this may be useful (Usage: Shift+F12, then after first left click starts clicking very rapidly, press F12 to stop)

#NoEnv  ; Recommended for performance and compatibility with future AutoHotkey releases.
; #Warn  ; Enable warnings to assist with detecting common errors.
SendMode Input  ; Recommended for new scripts due to its superior speed and reliability.
SetWorkingDir %A_ScriptDir%  ; Ensures a consistent starting directory.

Suspend, on

F12::Reload
+F12::Suspend, off

Lbutton::

Loop
{
;GetKeyState, state, Lbutton, P
;if state=U
;break
Sendinput {Click down left}
Sleep 10
Sendinput {Click up left}
Sleep 10
}

Expected Behavior

Nothing much

Actual Behavior

Crash 50% of the time, freeze in other cases

WindowsTerminal_2022-02-02_17-19-35.mp4
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Feb 2, 2022
@mimvdb mimvdb mentioned this issue Feb 2, 2022
Closed
@zadjii-msft zadjii-msft added Area-SettingsUI Anything specific to the SUI Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news. labels Feb 17, 2022
@zadjii-msft zadjii-msft added this to the 22H2 milestone Feb 17, 2022
@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Feb 17, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Feb 17, 2022
@ashemedai ashemedai mentioned this issue Aug 4, 2022
Closed
@zadjii-msft zadjii-msft modified the milestones: 22H2, Terminal v1.18 Dec 5, 2022
@zadjii-msft zadjii-msft mentioned this issue Jan 4, 2023
Merged
@zadjii-msft
Copy link
Member

Interesting. I can repro this crash on main, but I can't repro it on the #14630 branch. However, the stack I'm seeing here is fully different than the #13673 stack, so maybe it's coincidence.

Assuming that PR merges for 1.17, there's a chance this will be fixed in 1.17.

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
@zadjii-msft
Copy link
Member

I can't get this to repro anymore on 1.18. I bet this was fixed after all.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zadjii-msft zadjii-msft

Labels
Area-SettingsUI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news.
Projects
None yet
Milestone
Terminal v1.18
Development

No branches or pull requests
2 participants
@mimvdb @zadjii-msft