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.

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 on exit introduced in ac265aa #10042

Merged
1 commit merged into from
May 11, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 2 additions & 20 deletions src/cascadia/TerminalControl/ControlCore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -101,27 +101,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
Close();

// Before destroying this instance we must ensure that we destroy the _renderer
// before the _renderEngine, as well as calling _renderer->TriggerTeardown().
// _renderEngine will be destroyed naturally after this ~destructor() returns.

decltype(_renderer) renderer;
{
// GH#8734:
// We lock the terminal here to make sure it isn't still being
// used in the connection thread before we destroy the renderer.
// However, we must unlock it again prior to triggering the
// 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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(we don't need to lock any longer?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


_renderer.swap(renderer);
}

if (renderer)
if (_renderer)
{
renderer->TriggerTeardown();
_renderer->TriggerTeardown();
}
}

Expand Down
7 changes: 6 additions & 1 deletion src/cascadia/TerminalControl/ControlCore.h
Original file line number Diff line number Diff line change
Expand Up @@ -172,8 +172,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation

std::unique_ptr<::Microsoft::Terminal::Core::Terminal> _terminal{ nullptr };

std::unique_ptr<::Microsoft::Console::Render::Renderer> _renderer{ nullptr };
// NOTE: _renderEngine must be ordered before _renderer.
//
// As _renderer has a dependency on _renderEngine (through a raw pointer)
// we must ensure the _renderer is deallocated first.
// (C++ class members are destroyed in reverse order.)
std::unique_ptr<::Microsoft::Console::Render::DxEngine> _renderEngine{ nullptr };
std::unique_ptr<::Microsoft::Console::Render::Renderer> _renderer{ nullptr };

IControlSettings _settings{ nullptr };

Expand Down
18 changes: 11 additions & 7 deletions src/cascadia/TerminalControl/TermControl.h
Original file line number Diff line number Diff line change
Expand Up @@ -130,22 +130,26 @@ namespace winrt::Microsoft::Terminal::Control::implementation
private:
friend struct TermControlT<TermControl>; // friend our parent so it can bind private event handlers

// NOTE: _uiaEngine must be ordered before _core.
//
// ControlCore::AttachUiaEngine receives a IRenderEngine as a raw pointer, which we own.
// We must ensure that we first destroy the ControlCore before the UiaEngine instance
// in order to safely resolve this unsafe pointer dependency. Otherwise a deallocated
// IRenderEngine is accessed when ControlCore calls Renderer::TriggerTeardown.
// (C++ class members are destroyed in reverse order.)
std::unique_ptr<::Microsoft::Console::Render::UiaEngine> _uiaEngine;

winrt::com_ptr<ControlCore> _core;
winrt::com_ptr<ControlInteractivity> _interactivity;

bool _initializedTerminal;

winrt::com_ptr<SearchBoxControl> _searchBox;

std::unique_ptr<::Microsoft::Console::Render::UiaEngine> _uiaEngine;

IControlSettings _settings;
bool _focused;
std::atomic<bool> _closing;
bool _focused;
bool _initializedTerminal;

std::shared_ptr<ThrottledFunc<>> _tsfTryRedrawCanvas;
std::shared_ptr<ThrottledFunc<>> _updatePatternLocations;

std::shared_ptr<ThrottledFunc<>> _playWarningBell;

struct ScrollBarUpdate
Expand Down