-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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.
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
Fix crash in terminal when tab closed while data is being output #8982
Conversation
Co-authored-by: Mike Griese <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clever fix. I hate it. I mean, I very much appreciate you fixing it, but I hate that our locking story is so poorly-told that you need to take and then surrender a lock as a sort of acq-rel barrier. 😄
Hello @DHowett! 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 (
|
(Thank you, as always!) |
If there is data being output when a tab is closed, that can sometimes result in the application crashing, because the renderer may still be in use at the time is it destroyed. This PR attempts to prevent that from happening by adding a lock in the `TermControl::Close` method. What we're trying to prevent is the connection thread still being active, and potentially accessing the renderer, after it has been destroyed. So by acquiring the terminal lock in `TermControl::Close`, after we've stopped accepting new output, we can be sure that the connection thread is no longer active (it holds the lock while it is processing output). So once we've acquired and released the lock, it should be safe to tear down and destroy the renderer. ## Validation Steps Performed While this crash is difficult to reproduce in general usage, it occurred quite frequently when testing my `DECPS` implementation (there is some tricky thread synchronisation, which seems more likely to trigger the issue). With this patch applied, though, those crashes have stopped occurring. I've also stepped through the shutdown code in the debugger, manually freezing threads to get them aligned in the right way to trigger the crash (as explained in issue #8734). Again with the patch applied, I can no longer get the crash to occur. Closes #8734 (cherry picked from commit 40ebe5a)
If there is data being output when a tab is closed, that can sometimes result in the application crashing, because the renderer may still be in use at the time is it destroyed. This PR attempts to prevent that from happening by adding a lock in the `TermControl::Close` method. What we're trying to prevent is the connection thread still being active, and potentially accessing the renderer, after it has been destroyed. So by acquiring the terminal lock in `TermControl::Close`, after we've stopped accepting new output, we can be sure that the connection thread is no longer active (it holds the lock while it is processing output). So once we've acquired and released the lock, it should be safe to tear down and destroy the renderer. ## Validation Steps Performed While this crash is difficult to reproduce in general usage, it occurred quite frequently when testing my `DECPS` implementation (there is some tricky thread synchronisation, which seems more likely to trigger the issue). With this patch applied, though, those crashes have stopped occurring. I've also stepped through the shutdown code in the debugger, manually freezing threads to get them aligned in the right way to trigger the crash (as explained in issue #8734). Again with the patch applied, I can no longer get the crash to occur. Closes #8734 (cherry picked from commit 40ebe5a)
🎉 Handy links: |
🎉 Handy links: |
If there is data being output when a tab is closed, that can sometimes
result in the application crashing, because the renderer may still be in
use at the time is it destroyed. This PR attempts to prevent that from
happening by adding a lock in the
TermControl::Close
method.What we're trying to prevent is the connection thread still being
active, and potentially accessing the renderer, after it has been
destroyed. So by acquiring the terminal lock in
TermControl::Close
,after we've stopped accepting new output, we can be sure that the
connection thread is no longer active (it holds the lock while it is
processing output). So once we've acquired and released the lock, it
should be safe to tear down and destroy the renderer.
Validation Steps Performed
While this crash is difficult to reproduce in general usage, it occurred
quite frequently when testing my
DECPS
implementation (there is sometricky thread synchronisation, which seems more likely to trigger the
issue). With this patch applied, though, those crashes have stopped
occurring.
I've also stepped through the shutdown code in the debugger, manually
freezing threads to get them aligned in the right way to trigger the
crash (as explained in issue #8734). Again with the patch applied, I can
no longer get the crash to occur.
Closes #8734