Fix crash on exit introduced in ac265aa #10042
Conversation
| // teardown, to avoid the render thread being deadlocked. The | ||
| // renderer may be waiting to acquire the terminal lock, while | ||
| // we're waiting for the renderer to finish. | ||
| auto lock = _terminal->LockForWriting(); |
There was a problem hiding this comment.
(we don't need to lock any longer?)
There was a problem hiding this comment.
The core thesis of the fix earlier was that we needed to lock to steal away the renderer. I know the rest of the fix pulled the code out of Close so that _renderer continued to exist ... so perhaps this is not a problem
There was a problem hiding this comment.
This line was only added because j4james (not tagging to not spam them) was experiencing crashes. But as we now know we can fix it much more thoroughly by ensuring that no one is using the renderer anymore.
Originally there was no LockForWriting before calling TriggerTeardown. Assuming this was correct back then, we won't need it anymore now.
lhecker
force-pushed
the
dev/lhecker/fix-exit-crash
branch
from
aa157e5
to
5988fce
Compare
ControlCore::AttachUiaEngine receives a IRenderEngine as a raw pointer, which TermControl owns. We must ensure that we first destroy the ControlCore before the UiaEngine instance (both owned by TermControl). Otherwise a deallocated IRenderEngine is accessed when ControlCore calls Renderer::TriggerTeardown.
lhecker
force-pushed
the
dev/lhecker/fix-exit-crash
branch
from
5988fce
to
f470f49
Compare
DHowett
added
zPreview-Service-Queued-1.13
ghost
requested review from
zadjii-msft,
miniksa,
carlos-zamora,
leonMSFT and
PankajBhojwani
|
@msftbot merge this in 20 hours |
ghost
added
the
AutoMerge
|
Hello @carlos-zamora! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
|
@msftbot merge this in 1 minutes |
|
Hello @DHowett! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
DHowett
removed
the
zStable-Service-Queued-1.12
ControlCore::AttachUiaEngine receives a IRenderEngine as a raw pointer, which TermControl owns. We must ensure that we first destroy the ControlCore before the UiaEngine instance (both owned by TermControl). Otherwise a deallocated IRenderEngine is accessed when ControlCore calls Renderer::TriggerTeardown. This crash was introduced in #10031. * [x] I work here * [x] Tests added/passed * Run accevent.exe to cause a UiaEngine to be attached to a TermControl. * Close the current tab * Ensured no crashes occur (cherry picked from commit 43040ef)
ControlCore::AttachUiaEngine receives a IRenderEngine as a raw pointer, which TermControl owns. We must ensure that we first destroy the ControlCore before the UiaEngine instance (both owned by TermControl). Otherwise a deallocated IRenderEngine is accessed when ControlCore calls Renderer::TriggerTeardown. This crash was introduced in #10031. * [x] I work here * [x] Tests added/passed * Run accevent.exe to cause a UiaEngine to be attached to a TermControl. * Close the current tab * Ensured no crashes occur (cherry picked from commit 43040ef) (cherry picked from commit ad45139)
|
🎉 Handy links: |
|
🎉 Handy links: |
DHowett
removed
the
zPreview-Service-Queued-1.13
Summary of the Pull Request
ControlCore::AttachUiaEngine receives a IRenderEngine as a raw pointer,
which TermControl owns. We must ensure that we first destroy the
ControlCore before the UiaEngine instance (both owned by TermControl).
Otherwise a deallocated IRenderEngine is accessed when
ControlCore calls Renderer::TriggerTeardown.
References
This crash was introduced in #10031.
PR Checklist
Validation Steps Performed