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

Use TerminateProcess to exit early #16575

Merged
merged 7 commits into from
Jan 26, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
3 changes: 2 additions & 1 deletion src/cascadia/WindowsTerminal/AppHost.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,8 @@ void AppHost::_HandleCommandlineArgs(const Remoting::WindowRequestedArgs& window

if (_windowLogic.ShouldExitEarly())
{
ExitThread(result);
TerminateProcess(GetCurrentProcess(), gsl::narrow_cast<UINT>(result));
__assume(false);
lhecker marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down
57 changes: 9 additions & 48 deletions src/cascadia/WindowsTerminal/WindowEmperor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,42 +38,12 @@ WindowEmperor::WindowEmperor() noexcept :
});

_dispatcher = winrt::Windows::System::DispatcherQueue::GetForCurrentThread();

// BODGY
//
// There's a mysterious crash in XAML on Windows 10 if you just let the App
// get dtor'd. By all accounts, it doesn't make sense. To mitigate this, we
// need to intentionally leak a reference to our App. Crazily, if you just
// let the app get cleaned up with the rest of the process when the process
// exits, then it doesn't crash. But if you let it get explicitly dtor'd, it
// absolutely will crash on exit.
//
// GH#15410 has more details.

auto a{ _app };
::winrt::detach_abi(a);
}

// Disable the "destructor never returns, potential memory leak" warning - we're literally already exiting here.
#pragma warning(suppress : 4722)
WindowEmperor::~WindowEmperor()
{
// BODGY: If the emperor is being dtor'd, it's because we've gone past the
// end of main, and released the ref in main. Here, we want to manually
// terminate our process.
//
// If you just do a
//
// _app.Close();
//
// here, then we might run into an edge case where main releases it's ref to
// the emperor, but one of the window threads might be in the process of
// exiting, and still holding a strong ref to the emperor. In that case, we
// can actually end up with the _window thread_ being the last reference,
// and calling App::Close on that thread will crash us with a E_WRONG_THREAD
//
// For more context, see MSFT:46744208
std::exit(0);
_app.Close();
_app = nullptr;
Copy link
Member

@DHowett DHowett Jan 23, 2024

Choose a reason for hiding this comment

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

We have tried to remove this App leak two times and every time it reintroduces a crash on shutdown. I really don't think we will succeed a third time.

Are we totally sure about this?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this will work. The destructor will never run because we terminate before the class leaves the scope.

}

void _buildArgsFromCommandline(std::vector<winrt::hstring>& args)
Expand All @@ -98,7 +68,7 @@ void _buildArgsFromCommandline(std::vector<winrt::hstring>& args)
}
}

bool WindowEmperor::HandleCommandlineArgs()
void WindowEmperor::HandleCommandlineArgs()
{
std::vector<winrt::hstring> args;
_buildArgsFromCommandline(args);
Expand Down Expand Up @@ -127,27 +97,27 @@ bool WindowEmperor::HandleCommandlineArgs()
Remoting::CommandlineArgs eventArgs{ { args }, { cwd }, showWindow, winrt::hstring{ currentEnv.to_string() } };

const auto isolatedMode{ _app.Logic().IsolatedMode() };

const auto result = _manager.ProposeCommandline(eventArgs, isolatedMode);
int exitCode = 0;

const bool makeWindow = result.ShouldCreateWindow();
if (makeWindow)
if (result.ShouldCreateWindow())
{
_createNewWindowThread(Remoting::WindowRequestedArgs{ result, eventArgs });

_becomeMonarch();
WaitForWindows();
}
else
{
const auto res = _app.Logic().GetParseCommandlineMessage(eventArgs.Commandline());
if (!res.Message.empty())
{
AppHost::s_DisplayMessageBox(res);
std::quick_exit(res.ExitCode);
}
exitCode = res.ExitCode;
}

return makeWindow;
TerminateProcess(GetCurrentProcess(), gsl::narrow_cast<UINT>(exitCode));
__assume(false);
}

void WindowEmperor::WaitForWindows()
Expand Down Expand Up @@ -600,15 +570,6 @@ LRESULT WindowEmperor::_messageHandler(UINT const message, WPARAM const wParam,
// we'll undoubtedly crash.
winrt::fire_and_forget WindowEmperor::_close()
{
{
auto fridge{ _oldThreads.lock() };
for (auto& window : *fridge)
{
window->ThrowAway();
}
fridge->clear();
}

// Important! Switch back to the main thread for the emperor. That way, the
// quit will go to the emperor's message pump.
co_await wil::resume_foreground(_dispatcher);
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/WindowsTerminal/WindowEmperor.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class WindowEmperor : public std::enable_shared_from_this<WindowEmperor>
~WindowEmperor();
void WaitForWindows();

bool HandleCommandlineArgs();
void HandleCommandlineArgs();

private:
void _createNewWindowThread(const winrt::Microsoft::Terminal::Remoting::WindowRequestedArgs& args);
Expand Down
5 changes: 1 addition & 4 deletions src/cascadia/WindowsTerminal/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,5 @@ int __stdcall wWinMain(HINSTANCE, HINSTANCE, LPWSTR, int)
winrt::init_apartment(winrt::apartment_type::single_threaded);

const auto emperor = std::make_shared<::WindowEmperor>();
if (emperor->HandleCommandlineArgs())
{
emperor->WaitForWindows();
}
emperor->HandleCommandlineArgs();
}
Loading