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

1.18 - RACE - Settings can reload while window is being constructed #15209

Closed
Tracked by #14957
DHowett opened this issue Apr 19, 2023 · 1 comment · Fixed by #15251
Closed
Tracked by #14957

1.18 - RACE - Settings can reload while window is being constructed #15209

DHowett opened this issue Apr 19, 2023 · 1 comment · Fixed by #15251
Assignees
@zadjii-msft
Labels
Area-Windowing Window frame, quake mode, tearout In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree.
Milestone
Terminal v1.18

Comments

@DHowett
Copy link
Member

DHowett commented Apr 19, 2023

At least 1x on my dev box, I observed the following:

  • Main thread is crashing in TerminalWindow::UpdateSettings because _root is nullptr, while trying to call _root->Dispatcher().
  • The window thread is actively constructing the UI and has not yet assigned _root.

Boom
@microsoft-github-policy-service microsoft-github-policy-service bot 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 Apr 19, 2023
@DHowett
Copy link
Member Author

DHowett commented Apr 19, 2023

Noted while digging:

AppLogic is reaching inside TerminalWindow to register TerminalWindow for AppLogic's SettingsChanged event. It does this quite early (and I was bothered that it happened inside AppLogic?)

@zadjii-msft zadjii-msft mentioned this issue Apr 20, 2023
Open
6 tasks
@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree. Area-Windowing Window frame, quake mode, tearout labels Apr 20, 2023
@zadjii-msft zadjii-msft added this to the Terminal v1.18 milestone Apr 20, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Tag-Fix Doesn't match tag requirements label Apr 20, 2023
@zadjii-msft zadjii-msft added Needs-Tag-Fix Doesn't match tag requirements and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Apr 20, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Tag-Fix Doesn't match tag requirements label Apr 20, 2023
@zadjii-msft zadjii-msft self-assigned this Apr 25, 2023
@zadjii-msft zadjii-msft mentioned this issue Apr 27, 2023
Merged
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Apr 27, 2023
microsoft-github-policy-service bot pushed a commit that referenced this issue May 1, 2023
@zadjii-msft
Another theoretical fix for another race condition (#15251)
20eabb3 
Basically, just make sure that we register our `SettingsChanged` handler
in `TerminalWindow` _after_ `TerminalWindow` is actually ready to handle
it. _duh_.

Closes #15209
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-Windowing Window frame, quake mode, tearout In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree.
Projects
None yet
Milestone
Terminal v1.18
Development

Successfully merging a pull request may close this issue.

Another theoretical fix for another race condition microsoft/terminal
2 participants
@DHowett @zadjii-msft