Windows 7 Blocked Shutdown UX & Logoff Sequence 1.0 - #5215
Conversation
|
Thanks for the pull request! This repository uses a two-stage review: an AI review that you run yourself, followed by a human review. To get started, comment See the pull request review process for the full details. |
|
/ai-review |
|
Something went wrong while preparing the AI review, so there is nothing to post this time. This is a problem on the reviewer's side, not with this pull request. Comment |
|
/ai-review |
|
Something went wrong while preparing the AI review, so there is nothing to post this time. This is a problem on the reviewer's side, not with this pull request. Comment |
|
/ai-review |
|
Something went wrong while preparing the AI review, so there is nothing to post this time. This is a problem on the reviewer's side, not with this pull request. Comment |
Added known issues section and important usage note for the mod.
|
/ai-review |
|
This pull request has already had 3 AI reviews in the last 24 hours, which is the limit, so no review was posted this time. Comment |
|
/ai-review |
Submission reviewNote: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. The previous round is resolved: the settings-change path no longer re-runs the probe for settings winlogon doesn't read, 1. The winlogon probe still runs inline on winlogon.exe's startup path.
I raised the probe design twice already and you've kept it, which is your call — I'm not re-litigating that. But the one concrete piece from that discussion is still open and is cheap: static HANDLE g_hProbeThread, g_hProbeStopEvent;
void Wh_ModAfterInit() {
if (!g_isWinlogon || !p_g_fShutdownResolverDisabled) return;
g_origResolverDisabledState = *p_g_fShutdownResolverDisabled;
if (g_origResolverDisabledState) return;
g_hProbeStopEvent = CreateEventW(nullptr, TRUE, FALSE, nullptr);
g_hProbeThread = CreateThread(nullptr, 0, [](LPVOID) -> DWORD {
if (IsLogonUiInjectionEnabled()) // waits on {pi.hProcess, g_hProbeStopEvent}
*p_g_fShutdownResolverDisabled = 1;
return 0;
}, nullptr, 0, nullptr);
}with the matching signal + Optional improvements
Minor polish — none of this affects users in normal operation, so it's your call.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
|
/ai-review |
Submission reviewNote: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. The mod is in good shape — the lifecycle handling is correct (worker thread signalled and joined in The DWORD result = WaitForSingleObject(pi.hProcess, 2000);
if (result != WAIT_OBJECT_0) {
// Should exit immediately but just to be safe
Wh_Log(L"LogonUI wait timed out or failed");
// Let it exit automatically
}The "auto-exit on an unknown argument" behaviour is undocumented and could change in a future build. If it ever does, you're left with a stray DWORD result = WaitForSingleObject(pi.hProcess, 2000);
if (result != WAIT_OBJECT_0) {
Wh_Log(L"LogonUI load check timed out (GLE=%u), terminating the probe", GetLastError());
TerminateProcess(pi.hProcess, 0);
}Optional improvements
Minor polish — none of this affects users in practice, so it's your call.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
|
/ai-review |
|
This pull request has already had 3 AI reviews in the last 24 hours, which is the limit, so no review was posted this time. Comment |
|
/ai-review |
Submission reviewNote: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. The item from last round is fixed — the probe child is terminated instead of being left running — and the new hardening in this round is good: the 1. void CustomBSDR::Hide() {
std::lock_guard lock(pendingAppsMutex);
if (hBgWnd) {
ShowWindow(hBgWnd, SW_HIDE);
}
}
Meanwhile the BSDR thread takes
Neither ever proceeds: the dialog is never destroyed, Note the lock doesn't buy anything here either: static std::atomic<HWND> hBgWnd = nullptr;
void CustomBSDR::Hide() {
HWND hBgWndLocal = hBgWnd.load();
if (hBgWndLocal) {
ShowWindow(hBgWndLocal, SW_HIDE); // no lock held
}
}( 2. int CustomBSDR::GetScaleFactor() {
std::lock_guard lock(pendingAppsMutex);
if (hDlg) {
return GetDpiForWindow(hDlg);
} else {
return GetDpiForSystem();
}
}
...
*scaleFactor = CustomBSDR::GetScaleFactor();The int CustomBSDR::GetScaleFactor() {
std::lock_guard lock(pendingAppsMutex);
int dpi = hDlg ? GetDpiForWindow(hDlg) : GetDpiForSystem();
return MulDiv(dpi, 100, 96);
}Optional improvements
Minor polish — none of this affects users, so it's your call.
Functionality notes
Non-critical observations about the feature behavior itself — mostly carried over so nothing gets lost.
Next steps:
See the review process for details. |
…checks Added a noInvoke parameter to the Resolve function to control invocation behavior. Updated various static variables to non-static for better thread safety and mutex handling. Enhanced dialog initialization checks and improved resource management.
|
/ai-review |
|
This pull request has already had 3 AI reviews in the last 24 hours, which is the limit, so no review was posted this time. Comment |
|
/ai-review |
Submission reviewNote: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. Overall this is a well-built mod: the worker thread is properly signalled and joined in 1. Failing to create the dialog leaves a full-screen black window on screen. In } else {
// Bail out
Wh_Log(L"Dialog creation failed!!");
Resolve(BlockedShutdownResolution_Force);
return 0;
}Returning 2. The "previous thread still running" bail-out in lock.unlock();
Resolve(BlockedShutdownResolution_Force); // no-op: still holds the previous session's value
return;So the intended "force" never happens; the new session ends up resolved with whatever the previous session resolved to, via the stale value that {
std::lock_guard lock(g_resolvedMutex);
g_resolvedValue = BlockedShutdownResolution_None;
g_wasClicked = false;
}
lock.unlock();
Resolve(BlockedShutdownResolution_Force);3. Icon bitmaps are deleted while the controls still reference them. In for (auto& tile : appTiles) {
if (tile.hIconBitmap) DeleteObject(tile.hIconBitmap);
}
appTiles.clear();
...
DestroyWindow(hDlgLocal);The 4. Provenance of the embedded bitmap resources. Optional improvements
Minor polish — none of this affects users, so it's your call.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
|
/ai-review |
Submission reviewNote: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding. Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it. Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them. The mod is in good shape after the previous rounds — the settings block matches the code exactly, the window class is registered/unregistered per session, all the 1. The ~1,300 lines of hex bitmap data are still unreviewable (lines 326-1622). This is the maintainer's open inline question on this PR ("Is there a way to include this data in textual form to make these parts of the mod reviewable?"), which hasn't been answered yet. It's ~20 KB of opaque bytes in a mod that runs as SYSTEM in
2. The LogonUI-readiness safety check can't detect the failure it exists for, and depends on Windhawk's internal registry layout ( Windhawk's per-mod storage is shared across processes and is a documented API, so it can carry the real signal instead: // LogonUI.exe, at the end of Wh_ModInit once the hooks are in:
Wh_SetIntValue(L"logonUiHooksReady", 1);
// ...and Wh_SetIntValue(L"logonUiHooksReady", 0) on each LogonUI-side bail-out
// (keep it 1 for the AuthUX path at line 4050 — the logoff sequence works there).
// winlogon.exe, in IsLogonUiInjectionEnabled():
if (!Wh_GetIntValue(L"logonUiHooksReady", 0)) {
return false;
}This covers the inclusion list, mod-specific exclusions, symbol-resolution failures and AuthUX in one check, and drops the dependency on Windhawk's internal registry keys. mods/classic-min-max-animations.wh.cpp#L377 uses the same storage across processes. The portable-Windhawk service probe (lines 3973-4004) is orthogonal and can stay as-is. 3. The unload path resolves the shutdown after the mod's own hooks are already gone ( The fix is just to move the handshake one callback earlier, where the hooks are still installed: void Wh_ModBeforeUninit() {
if (g_isWinlogon) {
return;
}
g_isExiting.store(true);
using namespace CustomBSDR;
HANDLE hThreadLocal = nullptr;
{
std::lock_guard lock(workerMutex);
hThreadLocal = hThread;
hThread = nullptr;
}
if (hThreadLocal) {
bool needsResolve;
{
std::lock_guard lock(g_resolvedMutex);
needsResolve = g_resolvedValue == BlockedShutdownResolution_None;
}
if (needsResolve) {
Resolve(BlockedShutdownResolution_Cancel);
}
if (hStopEvent) {
SetEvent(hStopEvent);
}
WaitForSingleObject(hThreadLocal, INFINITE);
CloseHandle(hThreadLocal);
}
}
4. Uninitialized HSTRING caption = nullptr, blockReason = nullptr;
blockingApp->get_Id(&appId);
blockingApp->get_IsBlocking(&isBlocking);
HRESULT hrCaption = blockingApp->get_Caption(&caption);
HRESULT hrReason = blockingApp->get_BlockReason(&blockReason);
Wh_Log(L"BlockedShutdownUXImpl::AddApplication, AppId=%u, IsBlocking=%d, Caption=%s, BlockReason=%s",
appId, isBlocking,
SUCCEEDED(hrCaption) ? WindowsGetStringRawBuffer(caption, nullptr) : L"<failed>",
SUCCEEDED(hrReason) ? WindowsGetStringRawBuffer(blockReason, nullptr) : L"<failed>");
WindowsDeleteString(blockReason);
WindowsDeleteString(caption);Note that Optional improvements
Minor polish — none of this affects users, so it's your call.
Functionality notes
Non-critical observations and ideas about the feature behavior itself.
Next steps:
See the review process for details. |
Another note: LogonUI just exits when idle. It isn't even running most of the time; more likely to be not running when the user changes the mod settings from the WH UI. |
Changelog
If this pull request updates an existing mod, describe the changes below:
Mod authorship
If this pull request introduces a new mod, please complete the section below.
This mod was created by:
Please select the options that best apply. Your selection does not affect the acceptance criteria, but it helps reviewers understand the context of the code and provide relevant feedback.