[Installer] Ask a hidden instance to close, then offer to end it once (#7683)

* [Installer] Request quit for hidden Cockatrice instances and record holders in abort telemetry

* [Installer] Fix compiler warnings in holder probe and installer quit handler

* [VisualDeckStorage] Order the includes the way clang-format wants them

* [Installer] Ask a hidden instance to close, then offer to end it once

The previous attempt had the client answer a registered window message, which
cannot work for the update it was reported on: the instance being replaced is a
client from before that message exists, so it never answers, and the user is
left with "Cockatrice is still running" exactly as reported. Every step that can
free the files has to be the installer's, so the client side is reverted.

The probe now posts WM_CLOSE to the instance's own window whether that window is
on screen or only in the notification area: the request does not need a visible
window, an instance in the tray is the one this update has no other way to
reach, and the confirmation Cockatrice answers with is a window Qt shows itself,
so it is painted - unlike one restored from outside. A visible window is brought
forward first, and Qt's bookkeeping windows stay excluded, because a close to the
notification-area one terminates the instance without its confirmation.

A question of the instance's own no longer suppresses the close. Withholding it
protected nobody: the installer asked the user to close a program it had never
asked, and a current client refuses the close while its confirmation is pending
anyway, leaving the question where it is. The launcher mode keeps the
visible-only rule, because a launcher without a window has already answered its
own close and the wait ends it after a grace instead of asking a user who is not
there.

The guard asked the same question until the user gave up: after the automatic
close failed it offered to end the programs, then re-ran the probe and showed
the same dialog word for word when the answer was No, which is how an abort was
reported three times over. It now asks once - Yes ends the Cockatrice, Oracle
and Servatrice programs the payload installs and says what that costs, No
cancels - and what survives an end is named in the abort instead of being put to
the user as the same question again. The uninstaller's guard and the recovery
path after a failed purge follow the same shape.

Kept: the abort telemetry that records the holders, the close result and the end
result as event data.

---------

Co-authored-by: sentry[bot] <39604003+sentry[bot]@users.noreply.github.com>
Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>

tools/installer-holder-probe/close_holders.cpp

@@ -103,44 +103,50 @@ bool isAnswerableWindow(HWND window)
 /// Picks the window to post the close to and the one the user should end up looking at.
 struct CloseTarget
 {
-    HWND main = nullptr;    ///< first visible window without an owner: the application's own
+    HWND main = nullptr;    ///< first answerable window without an owner: the application's own
     HWND toRaise = nullptr; ///< topmost visible window: a dialog when one is up
-    bool modalOpen = false; ///< the only window a user could answer is the instance's own question
+    bool hidden = false;    ///< the window being closed is not on screen (notification area)
 };
 
-/// Only the windows the user could answer are candidates - see isAnswerableWindow. An
-/// instance left with nothing but Qt's own bookkeeping windows visible has no window to ask:
-/// the caller reports that instead of posting a close the application would answer with a
-/// terminate that skips its own confirmation.
-CloseTarget chooseCloseTarget(const std::vector<HWND> &windows)
+/// Only the windows the user could be asked to answer are candidates - see isAnswerableWindow.
+///
+/// A hidden window is a candidate like a visible one: WM_CLOSE does not need a window on
+/// screen, so an instance that is only in the notification area answers it the way it answers
+/// its own close button, and the confirmation it raises then is a window Qt shows itself -
+/// painted, unlike one restored from outside. Nothing is shown from here. Qt's own bookkeeping
+/// windows stay excluded, because a close posted to the notification-area one terminates the
+/// instance without the confirmation and that is worse than no close at all.
+///
+/// @param windows every top-level window the process owns, in Z-order
+/// @param askHidden when false, only a window that is on screen is a target: the launcher
+///        mode reads "no window" as "past its own prompts and stuck", which stops being true
+///        the moment a hidden window is asked to close
+CloseTarget chooseCloseTarget(const std::vector<HWND> &windows, bool askHidden)
 {
     CloseTarget target;
-    HWND topmost = nullptr;
     for (const HWND window : windows) {
-        if (IsWindowVisible(window) == FALSE || !isAnswerableWindow(window)) {
+        if (!isAnswerableWindow(window)) {
             continue;
         }
-        if (topmost == nullptr) {
-            topmost = window;
+        const bool visible = IsWindowVisible(window) != FALSE;
+        if (visible && target.toRaise == nullptr) {
+            target.toRaise = window;
         }
-        if (target.main == nullptr && GetWindow(window, GW_OWNER) == nullptr) {
+        if (target.main == nullptr && GetWindow(window, GW_OWNER) == nullptr && (visible || askHidden)) {
             target.main = window;
+            target.hidden = !visible;
         }
     }
-    target.toRaise = topmost != nullptr ? topmost : target.main;
-    // A modal dialog disables the windows behind it, so a disabled main window is the
-    // instance asking a question of its own. A visible window with no unowned main window
-    // next to it is one too: what is on screen is then the question itself - the window
-    // behind it is hidden, not gone - and there is no window a close could be posted to
-    // without hiding the question with it.
-    target.modalOpen = target.toRaise != nullptr && (target.main == nullptr || IsWindowEnabled(target.main) == FALSE);
     return target;
 }
 
 /// What is left of the window the close was posted to, so a log line tells an instance that
 /// answered by hiding from one that never answered at all. A process that hid its window and
-/// stayed alive is the one that needs the next close to bring it back into view.
-std::wstring describeWindowState(HWND window)
+/// stayed alive is the one the guard's end answer is for.
+///
+/// @param window the window the close was posted to, or null when there was none
+/// @param wasHidden whether that window was already off screen before the close
+std::wstring describeWindowState(HWND window, bool wasHidden)
 {
     if (window == nullptr || IsWindow(window) == FALSE) {
         return L", its window is gone";
@@ -151,7 +157,12 @@ std::wstring describeWindowState(HWND window)
     DWORD_PTR ignored = 0;
     const bool responding =
         SendMessageTimeoutW(window, WM_NULL, 0, 0, SMTO_ABORTIFHUNG | SMTO_BLOCK, 1000, &ignored) != 0;
-    std::wstring state = IsWindowVisible(window) != FALSE ? L", window still open" : L", window hidden again";
+    std::wstring state;
+    if (IsWindowVisible(window) != FALSE) {
+        state = L", window still open";
+    } else {
+        state = wasHidden ? L", window still hidden" : L", window hidden again";
+    }
     if (!responding) {
         state += L" and not responding";
     }
@@ -214,13 +225,15 @@ bool isCockatriceImage(const std::wstring &imageName)
     return CompareStringOrdinal(imageName.c_str(), -1, COCKATRICE_IMAGE, -1, TRUE) == CSTR_EQUAL;
 }
 
-CloseHoldersResult closeCockatriceHolders(const std::vector<Holder> &holders, unsigned long waitMilliseconds)
+CloseHoldersResult
+closeCockatriceHolders(const std::vector<Holder> &holders, unsigned long waitMilliseconds, bool askHiddenWindows)
 {
     CloseHoldersResult result;
     std::vector<std::wstring> lines;
     std::vector<HANDLE> asked;
     std::vector<std::size_t> askedLines;
     std::vector<HWND> askedWindows;
+    std::vector<bool> askedWasHidden;
 
     for (const Holder &holder : holders) {
         if (holder.isService || !isCockatriceImage(holder.imageName)) {
@@ -233,38 +246,36 @@ CloseHoldersResult closeCockatriceHolders(const std::vector<Holder> &holders, un
         WindowSearch search;
         search.processId = holder.processId;
         EnumWindows(collectProcessWindow, reinterpret_cast<LPARAM>(&search));
-        if (search.windows.empty()) {
+        CloseTarget target = chooseCloseTarget(search.windows, askHiddenWindows);
+        if (target.main == nullptr) {
+            // Nothing of the application's own to post a close to: every window left is one of
+            // Qt's bookkeeping windows, or the only thing on screen is a question whose owner
+            // window is gone. Neither is a close the application would answer - a close to the
+            // notification-area window terminates without the confirmation - so the instance is
+            // named and the guard's end answer is what can free its files.
             ++result.withoutWindow;
-            lines.push_back(processLabel(holder) + L" has no window to close, left to you");
+            lines.push_back(processLabel(holder) + L" has no window of its own to close");
             continue;
         }
 
-        const CloseTarget target = chooseCloseTarget(search.windows);
-        if (target.toRaise == nullptr) {
-            // Nothing visible that a user could answer: the instance is hidden - in the
-            // notification area, say - or has nothing but Qt's own bookkeeping windows on
-            // screen. Neither is a window a close could reach, and showing one from here does
-            // not help: Windows would display it while the application still believes it is
-            // hidden, so it stays blank and answers nothing. The instance is named and left to
-            // the user, who can open it from the notification area.
-            ++result.withoutWindow;
-            lines.push_back(processLabel(holder) + L" has no window to ask, left to you");
-            continue;
-        }
         std::wstring line = processLabel(holder);
-        line += bringForward(target.toRaise) ? L" brought forward" : L" could not be brought forward";
-
-        // Posting the close to an instance that is asking a question of its own is what made
-        // an earlier attempt useless: the client accepts it, hides this window and the dialog
-        // with it, and stays alive inside the nested loop the dialog opened, where nothing can
-        // reach it any more. The question is the request now, so it is brought forward and
-        // left alone - including a question whose owner window is hidden, which is the only
-        // thing left on screen and has no unowned window a close could be posted to.
-        if (target.modalOpen) {
-            lines.push_back(line + L", and is waiting for an answer to a question of its own");
-            continue;
+        if (target.hidden) {
+            line += L" is hidden in the notification area";
+        } else {
+            if (target.toRaise == nullptr) {
+                target.toRaise = target.main;
+            }
+            line += bringForward(target.toRaise) ? L" brought forward" : L" could not be brought forward";
         }
 
+        // Posted to the instance's own window even when a question of its own is up. The close
+        // is what that window's close button does, and the application is the one that answers
+        // it: a current client refuses the close while its confirmation is pending, leaving the
+        // question in front, and only a client from before that guard accepts it and hides the
+        // window with the question. Withholding the request therefore protected nobody - it
+        // left the installer asking the user to close a program it had never asked - while the
+        // guard's end answer is what frees the files when the close cannot.
+        //
         // Kept open across the wait so a pid reused after the process exits cannot be
         // mistaken for the instance that was asked to close.
         const HANDLE process = OpenProcess(SYNCHRONIZE, FALSE, holder.processId);
@@ -287,6 +298,7 @@ CloseHoldersResult closeCockatriceHolders(const std::vector<Holder> &holders, un
             askedLines.push_back(lines.size());
             asked.push_back(process);
             askedWindows.push_back(target.main);
+            askedWasHidden.push_back(target.hidden);
         }
         lines.push_back(std::move(line));
     }
@@ -300,7 +312,7 @@ CloseHoldersResult closeCockatriceHolders(const std::vector<Holder> &holders, un
                 lines[askedLines[index]] += L", exited after " + std::to_wstring(elapsed) + L" ms";
             } else {
                 lines[askedLines[index]] += L", still running after " + std::to_wstring(elapsed) + L" ms" +
-                                            describeWindowState(askedWindows[index]);
+                                            describeWindowState(askedWindows[index], askedWasHidden[index]);
             }
         }
     }

tools/installer-holder-probe/close_holders.h

@@ -10,15 +10,15 @@
  * @brief What asking the holders to close did, in the form the installer logs.
  *
  * @c report is one line naming every holder, including the ones this deliberately
- * leaves alone, because the installer repeats it in its log and in the second prompt.
- * @c withoutWindow counts the holders that had no visible window a user could answer: they
- * were named and left alone, and the launcher mode reads that count as "nothing to ask"
- * rather than as a window the user still has to answer.
+ * leaves alone, because the installer repeats it in its log.
+ * @c withoutWindow counts the holders that had no window of their own to ask: they were
+ * named and left alone, and the launcher mode reads that count as "nothing to ask" rather
+ * than as a window the user still has to answer.
  */
 struct CloseHoldersResult
 {
     unsigned int askedToClose = 0;  ///< holders a WM_CLOSE was posted to
-    unsigned int withoutWindow = 0; ///< holders with no visible window a user could answer
+    unsigned int withoutWindow = 0; ///< holders with no window of their own to ask
     std::wstring report;            ///< one line, narrowed to ASCII when written
 };
 
@@ -42,27 +42,34 @@ bool isCockatriceImage(const std::wstring &imageName);
  * asked to close. Oracle and Servatrice are left to the user - Oracle writes the card
  * database in place - and so is every service.
  *
- * One thing happens per instance: its main window is brought to the foreground, because
- * Cockatrice may answer the close with a confirmation of its own that would otherwise open
- * behind the installer's prompt. Then WM_CLOSE is posted to the main window, the same
- * request as clicking its close button - unless the instance is asking a question of its
- * own, which disables that window: posting the close then only hides the question, so it is
- * brought forward and left to the user. An instance with no window a user could answer -
- * one hidden in the notification area, say - is named and left to the user as well; showing
- * its windows from here does not help, because the application would still believe they are
- * hidden and never paint them. Stopping the process is never attempted.
+ * One thing happens per instance: its own window is asked to close with WM_CLOSE, the same
+ * request as clicking that window's close button - whether it is on screen or only in the
+ * notification area, since the request does not need the window to be visible and the
+ * confirmation Cockatrice answers it with is a window Qt shows itself, painted, unlike one
+ * restored from outside. A window that is on screen is brought forward first, because that
+ * confirmation would otherwise open behind the installer's prompt; nothing is shown from
+ * here. Qt's own bookkeeping windows are never the target - posting a close to the
+ * notification-area one terminates the instance without its confirmation - so a process
+ * without a window of its own is named and left to the guard, which offers to end it.
+ * Stopping the process is never attempted here.
  *
  * Once the requests are out, this waits up to @c waitMilliseconds for the processes to
  * exit, so the caller can measure the purge immediately afterwards instead of sleeping
  * for a fixed time or re-asking a question this call already answered. A process that is
- * still running is described further: whether its window is gone, still open, hidden
- * again, and whether it answers at all, so the log can tell an instance that refused the
- * close from one that never received it.
+ * still running is described further: whether its window is gone, still open, still
+ * hidden or hidden again, and whether it answers at all, so the log can tell an instance
+ * that refused the close from one that never received it.
  *
  * @param holders the Restart Manager answer, one entry per application or service
  * @param waitMilliseconds how long to wait for the asked instances to exit, 0 to not wait
+ * @param askHiddenWindows whether a window that is not on screen is asked to close too.
+ *        The guard wants that: an instance in the notification area is the one this update
+ *        has no other way to reach. The launcher mode does not - a launcher without a
+ *        visible window has already answered its own close and is stuck in shutdown, and
+ *        the installer ends it instead of putting a question to a user who is not there.
  */
-CloseHoldersResult closeCockatriceHolders(const std::vector<Holder> &holders, unsigned long waitMilliseconds);
+CloseHoldersResult
+closeCockatriceHolders(const std::vector<Holder> &holders, unsigned long waitMilliseconds, bool askHiddenWindows);
 
 /**
  * @brief What ending the holders did, in the form the installer logs.

tools/installer-holder-probe/holder_line.h

@@ -29,9 +29,9 @@ inline constexpr std::size_t HOLDER_LINE_MAX = 5;
  * The line is not the whole string: every prompt puts it after a fixed paragraph and before
  * the answers, and one NSIS string holds 1024 characters. Those paragraphs grew when the
  * guard started spelling out what ending the programs costs and what is left to do by hand,
- * so this budget is what keeps the longest message - the one that also says the last end
- * freed nothing, plus the "; and N more" the summary appends after the budget - complete,
- * including the Cancel line at its end. Windows caps one application or service name well
+ * so this budget is what keeps the longest message - the one with the longest listing, plus
+ * the "; and N more" the summary appends after the budget - complete, including the sentence
+ * that offers the other answer at its end. Windows caps one application or service name well
  * below this, so it bounds the list without ever cutting a single holder short.
  */
 inline constexpr std::size_t HOLDER_LINE_CHAR_MAX = 320;

tools/installer-holder-probe/main.cpp

@@ -148,9 +148,9 @@ bool launcherProcessIdFromEnvironment(unsigned long &processId)
 ///
 /// The guard closes a set and reports every holder; here there is exactly one process, and
 /// the installer needs an answer it can act on rather than a sentence: a launcher with no
-/// window to ask is past its own prompts and stuck in shutdown, so no answer the installer
-/// offers would ever reach it. Everything else - a window that was asked, or one that is
-/// waiting on a question of its own - is still the user's decision.
+/// visible window has already answered its own close and is stuck in shutdown, so no answer
+/// the installer offers would ever reach it. A window that was asked - including one the
+/// instance is showing a question in - is still the user's decision.
 int closeLauncher(const std::wstring &installDir)
 {
     unsigned long processId = 0;
@@ -170,7 +170,7 @@ int closeLauncher(const std::wstring &installDir)
             writeLine(L"the launcher is not cockatrice.exe, left to you");
             return EXIT_PROBE_FAILED;
         }
-        const CloseHoldersResult result = closeCockatriceHolders({holder}, CLOSE_WAIT_MILLISECONDS);
+        const CloseHoldersResult result = closeCockatriceHolders({holder}, CLOSE_WAIT_MILLISECONDS, false);
         writeLine(result.report);
         if (result.askedToClose > 0) {
             return EXIT_HOLDERS_FOUND;
@@ -251,7 +251,9 @@ int main(int argumentCount, char **arguments)
         return result.ended > 0 ? EXIT_HOLDERS_FOUND : EXIT_NO_HOLDERS;
     }
 
-    const CloseHoldersResult result = closeCockatriceHolders(holders, CLOSE_WAIT_MILLISECONDS);
+    // Hidden windows are asked to close here: an instance in the notification area is the one
+    // this update has no other way to reach, and the guard's end answer is behind that attempt.
+    const CloseHoldersResult result = closeCockatriceHolders(holders, CLOSE_WAIT_MILLISECONDS, true);
     writeLine(result.report);
     return result.askedToClose > 0 ? EXIT_HOLDERS_FOUND : EXIT_NO_HOLDERS;
 }