Manually quit when the OS tells us to update #13614
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Refer to https://docs.microsoft.com/en-us/windows/win32/rstmgr/guidelines-for-applications The OS will send us a WM_QUERYENDSESSION when it's preparing an update for our app. It will then send us a WM_ENDSESSION, which gives us a small timeout (~30s) to actually shut down gracefully. After that timeout, it will send us a WM_CLOSE. If we still don't close after the WM_CLOSE, it'll force-kill us (causing a crash which will be bucketed to moapphang). We will manually start a quit, so that we can persist the state. If we refuse to gracefully shut down, the OS will crash us to focefully terminate us. We choose to quit here, rather than just close, to skip over any warning dialogs (e.g. "Are you sure you want to close all tabs?") which might prevent a WM_CLOSE from cleanly closing the window. This will cause a appHost._RequestQuitAll, which will notify the monarch to collect up all the window state and save it. This "crash" caused by the OS force killing us constitutes 80% of all our crashes. 80%. See MSFT:38947155, MSFT:38877540, MSFT:21058878, MSFT:31710054, MSFT:39764652, MSFT:26883776. Closes #13569
ghost
added
Area-Windowing
This comment has been minimized.
This comment has been minimized.
carlos-zamora
approved these changes
Jul 27, 2022
lhecker
reviewed
Jul 28, 2022
Comment on lines
680
to
685
| TraceLoggingWrite( | ||
| g_hWindowsTerminalProvider, | ||
| "QueryEndSession", | ||
| TraceLoggingDescription("Emitted when the OS has begun the process of updating the Terminal"), | ||
| TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), | ||
| TraceLoggingKeyword(TIL_KEYWORD_TRACE)); |
There was a problem hiding this comment.
Isn't this event redundant with the one for WM_ENDSESSION? If we return true here, then almost always we should also receive a WM_ENDSESSION event anyways right?
In that case you could remove this entire case because the DefWindowProc already returns true.
There was a problem hiding this comment.
Meh, yea probably. I was thinking originally that we would shove some logic in there, but turns out we really don't need it.
lhecker
approved these changes
Jul 28, 2022
|
Do we want to service this? CC @DHowett |
Talked to Dustin about this earlier, and the suggestion was meh. It's a large number of raw crashes, but ultimately it's one per user. Probably fine to just wait till it bubbles up the normal way. |
zadjii-msft
added
the
AutoMerge
|
Hello @zadjii-msft! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit 
DHowett
pushed a commit
that referenced
this pull request
Aug 8, 2022
Refer to https://docs.microsoft.com/en-us/windows/win32/rstmgr/guidelines-for-applications The OS will send us a WM_QUERYENDSESSION when it's preparing an update for our app. It will then send us a WM_ENDSESSION, which gives us a small timeout (~30s) to actually shut down gracefully. After that timeout, it will send us a WM_CLOSE. If we still don't close after the WM_CLOSE, it'll force-kill us (causing a crash which will be bucketed to moapphang). We will manually start a quit, so that we can persist the state. If we refuse to gracefully shut down, the OS will crash us to focefully terminate us. We choose to quit here, rather than just close, to skip over any warning dialogs (e.g. "Are you sure you want to close all tabs?") which might prevent a WM_CLOSE from cleanly closing the window. This will cause a appHost._RequestQuitAll, which will notify the monarch to collect up all the window state and save it. This "crash" caused by the OS force killing us constitutes 80% of all our crashes. 80%. See MSFT:38947155, MSFT:38877540, MSFT:21058878, MSFT:31710054, MSFT:39764652, MSFT:26883776. Closes #13569 It also fixes the issue where if you've got Terminal Dev running (outside VS), and you try to Deploy, you have to make sure to close the "Are you sure you want to close all tabs" dialog before the deployment can proceed. A deploy in VS sends the same sequence of messages as a real update. (cherry picked from commit b3604ba) Service-Card-Id: 84836638 Service-Version: 1.15
|
🎉 Handy links: |
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refer to https://docs.microsoft.com/en-us/windows/win32/rstmgr/guidelines-for-applications
The OS will send us a WM_QUERYENDSESSION when it's preparing an
update for our app. It will then send us a WM_ENDSESSION, which gives
us a small timeout (~30s) to actually shut down gracefully. After
that timeout, it will send us a WM_CLOSE. If we still don't close
after the WM_CLOSE, it'll force-kill us (causing a crash which will be
bucketed to moapphang).
We will manually start a quit, so that we can persist the state. If we refuse to
gracefully shut down, the OS will crash us to focefully terminate us. We
choose to quit here, rather than just close, to skip over any warning dialogs
(e.g. "Are you sure you want to close all tabs?") which might prevent a WM_CLOSE
from cleanly closing the window.
This will cause a appHost._RequestQuitAll, which will notify the
monarch to collect up all the window state and save it.
This "crash" caused by the OS force killing us constitutes 80% of all our crashes. 80%. See MSFT:38947155, MSFT:38877540, MSFT:21058878, MSFT:31710054, MSFT:39764652, MSFT:26883776.
Closes #13569
It also fixes the issue where if you've got Terminal Dev running (outside VS), and you try to Deploy, you have to make sure to close the "Are you sure you want to close all tabs" dialog before the deployment can proceed. A deploy in VS sends the same sequence of messages as a real update.