Fix scrollbar update after terminal close #3256
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
zadjii-msft
approved these changes
Oct 25, 2019
zadjii-msft
added
Product-Terminal
ghost
requested review from
miniksa
approved these changes
Nov 1, 2019
There was a problem hiding this comment.
If there isn't already an issue to try to work this out systemically by improving synchronization between all the modules, please file a follow on issue.
Otherwise, I'm happy enough for now with small targeted fixes that at least improve the ongoing experience for selfhosters. So let's get this merged.
|
🎉 Handy links: |
5 tasks
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.
Summary of the Pull Request
This resolves (possibly not all) crashes when quickly opening and closing a tab that stem from accessing UI elements after control has closed. This looks strange to me that xaml sill runs dispatches after closing, but this seems to happen here (but it really shouldn't right?). If so, this might also explain some other crashes.
Other investigations:
There is definitely some use after free going on, mostly at xaml level. It also looks like terminal is invalidated even before
TermControl::Close()has a chance to run.The code seems to be not completely thread safe (e.g. removing
TerminalOutputevent handler onConhostConnectionis not synchronized). Although this could explain some occasional crashes like Terminal closes itself #2655, I rather doubt so; if these are only some single fields then x86, being strong architecture, should handle that. However I feel like it should be worthwhile to rewrite synchronization in theTerminal,TermControl,TermConnectioncircle.References
Partly solves #2248, but not #2947 (which got merged into #2248).
PR Checklist
Validation Steps Performed
Open tab and instantly close it.