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.

Sign up for GitHub

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

Jump to bottom

Use TerminateProcess to exit early #16575

Merged
merged 7 commits into from
Jan 26, 2024
Merged

Use TerminateProcess to exit early #16575

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e91fa75
this is capital-B bodgy
zadjii-msft Jan 16, 2024
1ce5f13
fine
zadjii-msft Jan 17, 2024
ad2ad9d
clean, add comments
zadjii-msft Jan 19, 2024
8b1ffb1
Use TerminateProcess
lhecker Jan 23, 2024
1405749
Address feedback
lhecker Jan 25, 2024
5ad7a42
Add a comment
lhecker Jan 25, 2024
059ca4a
Merge remote-tracking branch 'origin/main' into dev/migrie/b/leak-the…
lhecker Jan 26, 2024
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
20 changes: 18 additions & 2 deletions src/cascadia/WindowsTerminal/WindowEmperor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,26 @@ WindowEmperor::WindowEmperor() noexcept :
::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()
{
_app.Close();
_app = nullptr;
// 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);
Copy link
Member

@lhecker lhecker Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this is the right place to put this. A destructor that exits the process seems bodgy indeed, but it doesn't need to be bodgy if we simply exit outside of the destructor. I'd either put this into main itself or into HandleCommandlineArgs, after calling WaitForWindows() directly after the _becomeMonarch() line. It already has a call to quick_exit in the other branch and now this branch will have an explicit exit. It seems we've found that there aren't any circumstances where we want a clean exit to occur. I'd simply call quick_exit under both circumstances.

Finally, I've found that this trick won't reliably work for unpackaged WT. There, the CRT will call ExitProcess instead of TerminateProcess which has similar issues. Among others, we still run into the deadlock issue with wil::unique_folder_change_reader_nothrow. Personally, I'd call TerminateProcess explicitly on #ifdef NDEBUG and quick_exit otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

This is what I had in mind: https://github.com/microsoft/terminal/compare/dev/lhecker/16575-TerminateProcess

The benefits of the larger changeset are that it also works for unpackaged WT, it avoids shutdown issues by not calling WindowThread::ThrowAway on Win10, it fixes the TerminateThread call in AppHost::_HandleCommandlineArgs (unless I misunderstood its intention and it's supposed to be PostQuitMessage?), and it simplifies the WindowEmperor::HandleCommandlineArgs function by unifying the two exit paths (as mentioned in my above comment).

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I'm cool with that instead of this. Did you test #15410 on Windows 10 to make sure we don't regress it? If that works, then I'm inclined to say that yours is more correct and overall less bodgy (whereas this PR is more bodgy).

Copy link
Member

@lhecker lhecker Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't test it the way the issue describes it specifically but I did run that code on a Windows 10 VM and it seemed fine. Thinking about it, it should work fine too, because WindowEmperor now doesn't get a chance anymore to go out of scope in wWinMain which means that the destructor can't run either.

Copy link
Member

Choose a reason for hiding this comment

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

(Can y'all work this out so we can cut a 1.18 test build soon? :))

Copy link
Member

Choose a reason for hiding this comment

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

I've updated this PR with my branch and you could review it now @DHowett.
The only thing I'm worried about is...

}

void _buildArgsFromCommandline(std::vector<winrt::hstring>& args)
Expand Down
Loading