Skip to content

Commit 09d8ac4

Browse files
authored
Fix order and robustness of CTRL_*_EVENTs (#18233)
* This fixes a regression in 391abaf, which caused attached clients to receive CTRL_CLOSE_EVENTs, etc., in oldest-to-newest order, while historically the opposite is expected. * It also changes the behavior of `ProcessCtrlEvents` to dispatch these events no matter whether a client is already dead. This restores the Windows XP to Windows 8.1 behavior. Both of these fixes would address the issue on their own. Closes #15373 ## Validation Steps Performed * CloseTest from our repository shows newest-to-oldest order again. * node gets killed when run under npm and closing the tab.
1 parent 220c7cd commit 09d8ac4

File tree

2 files changed

+35
-22
lines changed

2 files changed

+35
-22
lines changed

src/host/input.cpp

+28-21
Original file line numberDiff line numberDiff line change
@@ -321,28 +321,35 @@ void ProcessCtrlEvents()
321321
return;
322322
}
323323

324-
auto Status = STATUS_SUCCESS;
324+
const auto ctrl = ServiceLocator::LocateConsoleControl();
325+
325326
for (const auto& r : termRecords)
326327
{
327-
/*
328-
* Status will be non-successful if a process attached to this console
329-
* vetoes shutdown. In that case, we don't want to try to kill any more
330-
* processes, but we do need to make sure we continue looping so we
331-
* can close any remaining process handles. The exception is if the
332-
* process is inaccessible, such that we can't even open a handle for
333-
* query. In this case, use best effort to send the close event but
334-
* ignore any errors.
335-
*/
336-
if (SUCCEEDED_NTSTATUS(Status))
337-
{
338-
Status = ServiceLocator::LocateConsoleControl()
339-
->EndTask(r.dwProcessID,
340-
EventType,
341-
CtrlFlags);
342-
if (!r.hProcess)
343-
{
344-
Status = STATUS_SUCCESS;
345-
}
346-
}
328+
// Older versions of Windows would do various things if the EndTask() call failed:
329+
// * XP: Pops up a "Windows can't end this program" dialog for every already-dead process.
330+
// * Vista - Win 7: Simply skips over already-dead processes.
331+
// * Win 8 - Win 11 26100: Aborts on an already-dead process (you have to WM_CLOSE conhost multiple times).
332+
//
333+
// That last period had the following comment:
334+
// Status will be non-successful if a process attached to this console
335+
// vetoes shutdown. In that case, we don't want to try to kill any more
336+
// processes, but we do need to make sure we continue looping so we
337+
// can close any remaining process handles. The exception is if the
338+
// process is inaccessible, such that we can't even open a handle for
339+
// query. In this case, use best effort to send the close event but
340+
// ignore any errors.
341+
//
342+
// The corresponding logic worked like this:
343+
// if (FAILED(EndTask(...)) && r.hProcess) {
344+
// break;
345+
// }
346+
//
347+
// That logic was removed around the Windows 11 26100 time frame, because CSRSS
348+
// (which handles EndTask) now waits 5s and then force-kills the process for us.
349+
// Going back to the Win 7 behavior then should make shutdown a lot more robust.
350+
// The bad news is that EndTask() returns STATUS_UNSUCCESSFUL no matter whether
351+
// the process was already dead, or if the request actually failed for some reason.
352+
// Hopefully there aren't any regressions, but we can't know without trying.
353+
LOG_IF_NTSTATUS_FAILED(ctrl->EndTask(r.dwProcessID, EventType, CtrlFlags));
347354
}
348355
}

src/server/ProcessList.cpp

+7-1
Original file line numberDiff line numberDiff line change
@@ -211,9 +211,15 @@ ConsoleProcessHandle* ConsoleProcessList::GetOldestProcess() const
211211
{
212212
termRecords.clear();
213213

214+
// The caller (ProcessCtrlEvents) expects them in newest-to-oldest order,
215+
// because that's how Windows has historically always dispatched these events.
216+
auto it = _processes.crbegin();
217+
const auto end = _processes.crend();
218+
214219
// Dig through known processes looking for a match
215-
for (const auto& p : _processes)
220+
for (; it != end; ++it)
216221
{
222+
const auto p = *it;
217223
// If no limit was specified OR if we have a match, generate a new termination record.
218224
if (!dwLimitingProcessId ||
219225
p->_ulProcessGroupId == dwLimitingProcessId)

0 commit comments

Comments
 (0)