From d42b9607c54348f6ade066ef881470b282a67618 Mon Sep 17 00:00:00 2001 From: "sentry[bot]" <39604003+sentry[bot]@users.noreply.github.com> Date: Mon, 26 Oct 2026 19:25:10 +0100 Subject: [PATCH] [Installer] Check for running instances with the Restart Manager probe (#7671) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [Installer] Check for running instances with the Restart Manager probe The running-instance guard was built on powershell.exe started from the 32-bit NSIS stub, which is the 32-bit Windows PowerShell: its Get-Process cannot read the path of a 64-bit process, so a running 64-bit Cockatrice was reported as not running, the update went ahead, and the purge then failed on the DLLs that instance still had mapped. Use tools/installer-holder-probe instead - the same Restart Manager lookup the abort path already had - before anything is replaced and in the uninstaller as well. It is bitness-agnostic, so the Sysnative and WMI fallbacks are gone. The guard looks only at the payload, asks Cockatrice to close once by itself (WM_CLOSE, the same request as the window's close button) and then offers close again / retry after closing by hand / cancel for whatever the close cannot reach - Oracle writes the card database in place, Servatrice has no window - without force-stopping anything. A failed purge offers the same close-and-retry, tries one last purge when nobody is named, and every dialog shows in every mode: a silent reinstall stays silent until something goes wrong. * [Installer] Name processes running from the install directory The running-instance guard answers from the Restart Manager, which reports who has a file open. A running instance does not have to have one open: its own image and the modules it loaded are mapped into it rather than opened by it, and a mapping is not reliably reported. That is the state the probe was added for - every DLL still on disk, an instance running from the directory, and "nothing is holding them any more" as the answer. A guard built on file handles alone is blind to exactly that instance, so it passes, the purge fails, the holder lookup after it names nobody either, and the install ends in a warning that no running program was found: no close is attempted and no retry is offered. A manual install with another instance open never asks it to close at all. Name every process whose image lives in the directory being replaced as well, with the same bitness-safe QueryFullProcessImageNameW the probe already uses to label its holders. The image lookup is listed first - it is the instance the user is asked to close - and the Restart Manager entries follow with the pids already named dropped, so the close still reaches everything that holds a file. A Restart Manager call that fails only makes the probe unable to answer when the image lookup named nobody either. Ask instead of dead-ending when the probe cannot answer at all: Retry checks again, Ignore carries on, Abort cancels, in the installer and the uninstaller alike, and the abort dialog says the check failed instead of claiming no running program was found. The Windows CI probe check covers the new lookup with a process started from the directory that holds no file open. * [Installer] Offer to wait again when the launcher has not exited A client that is still closing after 60 s is usually not hung: it is showing its own "quit anyway?" confirmation for an unsaved deck, an open game or a card database update, and the user is the one who has to answer it. The installer answered that with a plain warning and stopped, so the only way forward was to close Cockatrice by hand and run the update again - which on an in-app update means finding the downloaded installer. Ask instead: Retry brings the client to the front, asks it to close again (WM_CLOSE, nothing forced) and keeps waiting on its handle, Cancel stops as before. There is no "install anyway" answer because a live process has its image and Qt DLLs mapped, so writing the payload over them is the broken install this wait exists to prevent - and the guard that runs right after names the same process by its image path and stops there, so ignoring would only move the same question a moment later. * [Installer] Ask about a launcher that has not exited after ten seconds A minute of silence is a long time to sit in front of an installer that has no window yet, and the case it is waiting for is not a slow flush: a client that started an update exits in well under a second, so a minute means it is almost always showing its own "quit anyway?" confirmation with nobody looking at it. Wait ten seconds for the first question instead, and the full grace for the ones after it. The normal path is untouched - WaitForSingleObject returns as soon as the handle signals, so a client that exits in 200 ms still costs 200 ms - and answering the first question once does not turn into a stream of prompts while the user is in Cockatrice's window. --------- Co-authored-by: Lukas Brübach --- .ci/compile.sh | 22 +- cmake/NSIS.template.in | 1207 +++++++---------- tools/installer-holder-probe/CMakeLists.txt | 13 +- tools/installer-holder-probe/main.cpp | 40 +- .../restart_manager_holders.cpp | 106 +- .../restart_manager_holders.h | 41 +- 6 files changed, 721 insertions(+), 708 deletions(-) diff --git a/.ci/compile.sh b/.ci/compile.sh index d60683b6e..e85e2b5ba 100755 --- a/.ci/compile.sh +++ b/.ci/compile.sh @@ -515,7 +515,27 @@ try { New-Item -ItemType Directory -Force $empty | Out-Null $nothing = Invoke-Probe $empty if ($nothing.Code -ne 0) { throw "expected exit 0 for a directory with nothing open, got $($nothing.Code) : $($nothing.Output)" } - Write-Host "holder lookup named pid $PID for a locked file and named nothing for an empty directory" + # A process running from the directory is a holder even when it has no file open: an + # instance holds its payload as mapped images, not as handles, and that is the state + # the guard used to miss - it passed, the purge failed, and the install ended in a + # warning with no offer to close and try again. The image is a copy of a native + # Windows program, so it starts without its usual directory next to it. + $copy = Join-Path $dir "fake-cockatrice.exe" + Copy-Item -LiteralPath (Join-Path $env:SystemRoot "System32\cmd.exe") -Destination $copy + $running = Start-Process -FilePath $copy -ArgumentList "/c", "ping -n 20 127.0.0.1 >nul" -PassThru + try { + Start-Sleep -Milliseconds 1500 + $byImage = Invoke-Probe $dir + } finally { + try { $running.Kill() } catch { } + # Wait for the image to be released, or the cleanup below leaves the copy behind. + try { $running.WaitForExit(5000) | Out-Null } catch { } + } + if ($byImage.Code -ne 1) { throw "expected exit 1 for a process running from the directory, got $($byImage.Code) : $($byImage.Output)" } + if ($byImage.Output -notmatch "fake-cockatrice\.exe \(pid\s+$($running.Id)") { + throw "the lookup did not name the process running from the directory: $($byImage.Output)" + } + Write-Host "holder lookup named pid $PID for a locked file, named nothing for an empty directory, and named pid $($running.Id) for a process running from the directory" } finally { Remove-Item -Recurse -Force $dir -ErrorAction SilentlyContinue } diff --git a/cmake/NSIS.template.in b/cmake/NSIS.template.in index 6ce765a35..d54111a84 100644 --- a/cmake/NSIS.template.in +++ b/cmake/NSIS.template.in @@ -12,7 +12,6 @@ Var NormalDestDir Var PortableDestDir Var PortableMode Var ReinstallMode -Var AllowForceClose Var GuardDir Var OldUninstallString Var OldInstallDir @@ -38,11 +37,19 @@ Var AbortReason Var HolderExit Var HolderOutput Var HolderFound -; OfferToCloseAndRetryPurge counts the accepted close-and-retry rounds, so the -; prompt can stop repeating an offer that provably did not help (see there). -Var HolderCloseRounds +; What the last RunHolderProbe call answered: its exit code and its line. Kept +; apart from $HolderExit/$HolderOutput because the guard only wants to know +; whether the payload is free, while the abort dialog reports what survived. +Var GuardExit +Var GuardOutput +; Whether the guard already asked Cockatrice to close once (see GuardAppsNotRunning) +Var GuardCloseTried Var WaitPid Var LauncherExited +; How long one WaitForSingleObject on the launcher's handle waits before asking the user +; (see WaitForLauncherToExit). A variable because the first slice is short and the ones +; after the first question are not. +Var WaitSlice Var InstallLogPath Var LogHandle Var LogMessage @@ -107,7 +114,10 @@ Page Custom PortableModePageCreate PortableModePageLeave Call CockLogWrite !macroend -Function CockLogWrite +; The body is shared because the uninstaller needs its own copy: un.* functions are +; compiled into a separate binary, and inside one a Call may only name an un. function, +; so CockLog's Call to CockLogWrite cannot be used there. +!macro CockLogWriteBody Pop $LogMessage FileOpen $LogHandle "$InstallLogPath" a ${If} $LogHandle == "" @@ -119,6 +129,20 @@ Function CockLogWrite FileSeek $LogHandle 0 END FileWrite $LogHandle "$LogMessage$\r$\n" FileClose $LogHandle +!macroend + +Function CockLogWrite + !insertmacro CockLogWriteBody +FunctionEnd + +; CockLog for un.* code: same lines, prefixed call target. +!macro CockLogUn text + Push "${text}" + Call un.CockLogWrite +!macroend + +Function un.CockLogWrite + !insertmacro CockLogWriteBody FunctionEnd ; Logs every file matching $INSTDIR\. Used to record the DLL inventory @@ -231,10 +255,10 @@ ${EndIf} ; /PID= carries the process id of the application that started this installer. ; The in-app updater launches the installer and only then quits itself, so ; without it the installer has to guess whether the launching process is still -; alive by polling powershell.exe for an image name - a guess that can come back -; "not running" or "could not determine" while the process is very much alive and -; still has its Qt DLLs mapped. An older client does not pass /PID= and an older -; installer ignores it, so both sides of the pair have to keep working. +; alive from the files it still has open - a guess that can come back "nothing +; is holding them" while the client is very much alive and still has its Qt DLLs +; mapped. An older client does not pass /PID= and an older installer ignores it, +; so both sides of the pair have to keep working. ; Note that /D= must stay the last parameter on the command line, which is why ; the client puts it last and this one anywhere. StrCpy $WaitPid "" @@ -272,18 +296,16 @@ ${Else} ${EndIf} ; Wait for the process that launched this installer to exit before anything is -; replaced or deleted. It goes first because it is the one check here that needs -; no powershell.exe: on a machine where the process lookup cannot run at all, the -; launcher is still proven gone here instead of the whole upgrade being cancelled -; by a check that was only ever a proxy for this one. +; replaced or deleted. It goes first because a real handle on that process is +; authority: the holder probe that follows asks the same question indirectly, by +; looking at which files are still open. ; ; Then make sure no *other* application instance is still running (and holding -; file locks) before the previous version is uninstalled. On a silent update -; (/R /S) the running processes are asked to close gracefully and waited for; -; Cockatrice itself is never force-killed (data loss, e.g. an unsaved deck) so the -; update is aborted instead, while the console applications Oracle and Servatrice -; are force-stopped by PID once the grace period is over. On interactive installs -; the user is prompted to close them instead. +; file locks) before the previous version is uninstalled. The probe names them +; and the user is asked to close them; nothing is ever force-stopped, because +; Cockatrice may hold an unsaved deck and Oracle writes the card database in +; place. On /R the old version is about to be removed, so an answer the probe +; cannot give cancels the upgrade before it starts rather than after it. ; Only needed this early for /R, where AutoUninstallIfNeeded runs below; for ; interactive installs the section re-checks after the directory page, when ; $INSTDIR is the directory the user actually picked. @@ -307,6 +329,14 @@ ${If} ${NSIS_IS_64_BIT} == 1 SetRegView 64 ${EndIf} +; The uninstaller logs to its own file: the installer truncates its log on every run, so +; sharing one would mean either side erasing the other's account of a failed upgrade. +StrCpy $InstallLogPath "$TEMP\Cockatrice-uninstaller.log" +FileOpen $LogHandle "$InstallLogPath" w +FileClose $LogHandle +!insertmacro CockLogUn "==================================================================" +!insertmacro CockLogUn "uninstaller start: @CPACK_PACKAGE_NAME@ @CPACK_PACKAGE_VERSION@ dir=$INSTDIR" + ; Ensure no application instance is still running before removing files. Call un.EnsureAppsNotRunning @@ -685,230 +715,196 @@ FunctionEnd ; "The procedure entry point X could not be located in the dynamic link ; library ...Qt6Network.dll" on the next start. ; -; On a silent update (/R /S) matching processes are asked to close gracefully -; (WM_CLOSE) and waited for up to 60 s. Cockatrice is never force-stopped: it -; may be holding an unsaved deck or showing a "really leave this game?" -; confirmation, so if it refuses to close the install is aborted - nothing has -; been replaced yet, so the update is simply deferred. Oracle and Servatrice -; are console applications without a main window, so CloseMainWindow() does -; nothing for them and waiting cannot help; they hold no unsaved user data and -; are force-stopped by PID once the grace period is over. On interactive -; installs and uninstalls the user is prompted to close them instead. +; Both questions this installer has about running processes - "may I replace +; the payload?" and "what is holding this file I could not delete?" - are +; answered by tools/installer-holder-probe, a small executable built from this +; repository and embedded in the installer and the uninstaller alike. It names +; every process and service that has one of the payload files open or mapped +; (the Restart Manager, RmRegisterResources/RmGetList) and, separately, every +; process whose image lives in the directory being replaced. ; -; Processes are matched by image name AND by their executable path living -; under the guarded directory ($GuardDir, normally $INSTDIR), so unrelated -; processes that merely share an image name (e.g. the Oracle DB instance -; "oracle.exe") are never touched. - -; usage: set $R2 to the base image name (without extension, e.g. "cockatrice", -; as accepted by Get-Process -Name), call this, result in $R0 -; (1 = running from $GuardDir, 0 = not running, 2 = could not determine) -Function IsAppRunning - ; Every caller has to set $GuardDir; an empty one would make the path check - ; below match every process that merely shares the image name, which is the - ; opposite of what this guard is for. - ${If} $GuardDir == "" - StrCpy $R0 2 - Return - ${EndIf} - ; Pass the directory to guard to PowerShell via an environment variable so - ; it never has to be embedded into the -Command string: apostrophes in a - ; path would terminate the command early, and the path in a -like pattern - ; would treat [ and ] as wildcard characters. - System::Call 'kernel32::SetEnvironmentVariable(t, t) i ("COCKATRICE_GUARD_DIR", "$GuardDir")' - ; Exit the process explicitly so nsExec's pop reflects whether a matching - ; process was found: powershell.exe exits 0 whether or not the pipeline - ; produced objects. 0 = not running, 1 = running, anything else = could not - ; be determined (see IsProcessLookupAvailable). - ; A process is only a match when its path is readable and lives under - ; $GuardDir. A process we cannot inspect is not one of ours: the updater is - ; always started by the application it updates, so it runs in the same user - ; session, and an interactive install is elevated and can read the paths of - ; the user's own processes. Assuming a match instead would be a hard stop - ; rather than a fail-closed guard - an Oracle database instance running as - ; SYSTEM has a process named "oracle" whose path raises Access denied, and - ; that would make every silent update abort and every interactive install - ; loop on the Retry prompt with no way out but Cancel. - nsExec::ExecToLog 'powershell -NoProfile -Command "if (Get-Process -Name $\'$R2$\' -ErrorAction SilentlyContinue | Where-Object { try { $$p = $$_.Path; if ($$p) { $$p.StartsWith($$env:COCKATRICE_GUARD_DIR, [StringComparison]::OrdinalIgnoreCase) } else { $$false } } catch { $$false } } | Select-Object -First 1) { exit 1 } else { exit 0 }"' - Pop $R0 - ${If} $R0 == "0" - StrCpy $R0 0 - ${ElseIf} $R0 == "1" - StrCpy $R0 1 - ${Else} - ; powershell.exe could not be run at all (nsExec pushes the string - ; "error"), or died with an unexpected code. That is "could not - ; determine", which has to stay distinguishable from "definitely - ; running": a state that can never clear would block the installer - ; forever. IsProcessLookupAvailable warns about that case once. - StrCpy $R0 2 - ${EndIf} -FunctionEnd - -; Sets $R0 to 1 if PowerShell can be started at all, 0 if it cannot. The -; running instance check is built on top of it, so a machine where -; powershell.exe is missing, blocked by policy or refused by antivirus is -; reported once instead of being reported as "Cockatrice is running" on every -; single check. -Function IsProcessLookupAvailable - nsExec::ExecToLog 'powershell -NoProfile -Command "exit 0"' - Pop $R0 - ${If} $R0 == "0" - StrCpy $R0 1 - ${Else} - StrCpy $R0 0 - ${EndIf} -FunctionEnd - -Function CloseMatchingApps - ; gracefully ask every matching instance to close (sends WM_CLOSE) - ; - ; The filter here is deliberately the strict one: a process is only asked to - ; close when its path is readable and lives under $GuardDir. Treating a - ; process whose path cannot be read (an unrelated oracle.exe / servatrice.exe, - ; or one owned by another account running as SYSTEM) as a match would make - ; the installer close a third party application, and it runs elevated, so - ; CloseMainWindow() would succeed on it. - ; - ; The window is activated first, because WM_CLOSE is answered, not obeyed: - ; Cockatrice replies with a confirmation of its own while a card database - ; update is running, a game is open or a deck is unsaved. Without the - ; activation that dialog opens behind the installer's own prompt, where it - ; looks like the close did nothing and the second WM_CLOSE is delivered to a - ; main window its own modal dialog has disabled, which is ignored. Failing to - ; activate is harmless and swallowed: the close request still goes out. - System::Call 'kernel32::SetEnvironmentVariable(t, t) i ("COCKATRICE_GUARD_DIR", "$GuardDir")' - nsExec::ExecToLog 'powershell -NoProfile -Command "Get-Process -Name $\'$R2$\' -ErrorAction SilentlyContinue | Where-Object { try { $$p = $$_.Path; if ($$p) { $$p.StartsWith($$env:COCKATRICE_GUARD_DIR, [StringComparison]::OrdinalIgnoreCase) } else { $$false } } catch { $$false } } | ForEach-Object { try { $$null = (New-Object -ComObject WScript.Shell).AppActivate($$_.Id) } catch { }; $$null = $$_.CloseMainWindow() }"' - Pop $R3 -FunctionEnd - -Function ForceCloseApps - ; stop the matching instances by PID once the grace period is over. Only - ; used for the console applications ($AllowForceClose), which have no window - ; to close and hold no unsaved user data. Same strict filter as above. - System::Call 'kernel32::SetEnvironmentVariable(t, t) i ("COCKATRICE_GUARD_DIR", "$GuardDir")' - nsExec::ExecToLog 'powershell -NoProfile -Command "Get-Process -Name $\'$R2$\' -ErrorAction SilentlyContinue | Where-Object { try { $$p = $$_.Path; if ($$p) { $$p.StartsWith($$env:COCKATRICE_GUARD_DIR, [StringComparison]::OrdinalIgnoreCase) } else { $$false } } catch { $$false } } | ForEach-Object { Stop-Process -Id $$_.Id -Force -ErrorAction SilentlyContinue }"' - Pop $R3 -FunctionEnd - -Function WaitForAppToClose - ; usage: set $R1 to the display name (e.g. "cockatrice.exe") and $R2 to the - ; base image name (e.g. "cockatrice") - Call IsAppRunning - !insertmacro CockLog "guard $R1: state=$R0 (dir=$GuardDir)" - ; Only a definite "running" is worth waiting for. "Not running" is done, and - ; "could not determine" must not keep the installer waiting on an answer it - ; will never get - but it cannot simply be waved through either, see - ; HandleUnknownAppState. - ${If} $R0 = 0 - Return - ${EndIf} - ${If} $R0 = 2 - Call HandleUnknownAppState - Return - ${EndIf} - - ${If} ${Silent} - ; ask the application to close gracefully (WM_CLOSE), then wait for it to exit - DetailPrint "Closing $R1 ..." - Call CloseMatchingApps - ; wall-clock deadline: each poll has to spawn a fresh powershell.exe, so - ; a bare loop counter would not bound the real wait time to 60 s. - System::Call 'kernel32::GetTickCount()i.s' - Pop $R8 - ck_wait_loop: - Sleep 500 - Call IsAppRunning - ${If} $R0 = 0 - !insertmacro CockLog "guard $R1: closed gracefully" - DetailPrint "$R1 closed." - Return - ${EndIf} - ${If} $R0 = 2 - Call HandleUnknownAppState - Return - ${EndIf} - System::Call 'kernel32::GetTickCount()i.s' - Pop $R9 - IntOp $R9 $R9 - $R8 - ${If} $R9 < 60000 - Goto ck_wait_loop - ${EndIf} - ; The grace period is over. Cockatrice is never force-stopped: it may be - ; showing a "really leave this game?" confirmation or holding an unsaved - ; deck, and nothing has been replaced yet, so aborting just defers the - ; update and preserves the user's data. - ${If} $AllowForceClose = 0 - !insertmacro CockLog "guard $R1: still running after 60s, aborting (no force close)" - DetailPrint "$R1 is still running after 60 s; aborting the update so that no data is lost." - StrCpy $AbortReason \ - "$R1 did not close within 60 seconds, so the installation was aborted to avoid losing unsaved data.$\r$\n$\r$\nPlease close $R1 and run the installer again." - Call AbortInstall - ${EndIf} - ; Oracle and Servatrice are console applications: CloseMainWindow() - ; returned false for them and waiting longer cannot help. They hold no - ; unsaved user data, so stop them by PID. - !insertmacro CockLog "guard $R1: still running after 60s, force-stopping" - DetailPrint "Force-stopping $R1 ..." - Call ForceCloseApps - Sleep 2000 - Call IsAppRunning - ${If} $R0 = 0 - !insertmacro CockLog "guard $R1: force-stopped" - DetailPrint "$R1 closed." - Return - ${EndIf} - ${If} $R0 = 2 - Call HandleUnknownAppState - Return - ${EndIf} - !insertmacro CockLog "guard $R1: could not be stopped, aborting" - DetailPrint "$R1 is still running after 60 s; aborting the update." - StrCpy $AbortReason \ - "$R1 could not be stopped, so the installation was aborted.$\r$\n$\r$\nPlease close $R1 and run the installer again." - Call AbortInstall - ${Else} - ck_wait_prompt: - MessageBox MB_RETRYCANCEL|MB_ICONEXCLAMATION|MB_DEFBUTTON1 \ - "$R1 is still running.$\r$\n$\r$\nPlease close it, then click Retry.$\r$\nClick Cancel to abort." \ - IDCANCEL ck_abort_install - Call IsAppRunning - ${If} $R0 = 0 - Return - ${EndIf} - ${If} $R0 = 2 - Call HandleUnknownAppState - Return - ${EndIf} - Goto ck_wait_prompt - ck_abort_install: - Abort - ${EndIf} -FunctionEnd - -; A process lookup came back "could not determine": powershell.exe ran but the -; answer was not usable, e.g. the process could not be inspected at all. That -; state is fine to carry on from when the user is at the keyboard - they can -; close the applications by hand and the stale-runtime check still stands -; between a failed guard and a mismatched install. +; The second lookup is not redundant. Restart Manager answers "who has this +; file open", and a running instance does not have to have one open: its own +; image and the modules it loaded are mapped into it rather than opened by it, +; and a mapped image is not reliably reported. That is the state the failed +; updates this probe exists for were in - every DLL still on disk, an instance +; running from the directory, and "nothing is holding them any more" as the +; answer. A guard built on file handles alone therefore misses exactly the +; instance it exists to find; it would pass, the purge would fail, and the +; install would end in a warning with no offer to close and try again. ; -; It is not fine on a silent upgrade. There the whole point of this guard is to -; know that nothing holds locks on the files about to be replaced, and this is -; exactly the state where that is unknown, with no one to click a dialog: the -; old code returned and let the install carry on into the mixed Qt DLL set that -; made the next start fail. Cancel the update instead; the next attempt runs -; with the applications closed and succeeds. -Function HandleUnknownAppState - ${If} $ReinstallMode = 1 +; Why not enumerate processes from this script: the NSIS stub is a 32-bit +; process, so a bare "powershell" started from it is the 32-bit Windows +; PowerShell, whose Get-Process cannot read the path of a 64-bit process. A +; guard built on that misses exactly the running instance it exists to find. +; The probe opens each process with PROCESS_QUERY_LIMITED_INFORMATION and calls +; QueryFullProcessImageNameW, which has no bitness mismatch, and Restart +; Manager is a system API rather than a view of this process's architecture. +; +; The files that count are the payload the purge removes and the installer +; replaces: the three executables, the top-level DLLs, qt.conf and everything +; below Plugins. A document, a log or a screenshot the user happens to have +; open in the install directory is not part of it and cannot block an upgrade; +; the image lookup matches on the directory a process runs from, so it does not +; widen that set. +; +; Nothing is force-stopped anywhere: Cockatrice may be holding an unsaved deck +; or showing a "really leave this game?" confirmation, Oracle writes the card +; database in place and killing it mid-write would corrupt it, and Servatrice +; has no window to ask. A close is a request (WM_CLOSE), made automatically once +; because that is what the user started the installer to do, and again whenever +; they ask for it from the prompt; when it is not answered the user is told which +; program is in the way instead of having it killed. + +; Emitted once for the installer and once for the uninstaller: the uninstaller +; cannot call installer functions, and its own copy of the probe keeps a running +; 64-bit instance from being missed there too. +!macro CockHolderProbeFunctions prefix tag +Function ${prefix}ExtractHolderProbe + InitPluginsDir + IfFileExists "$PLUGINSDIR\installer-holder-probe.exe" ck_probe_ready_${tag} + File "/oname=$PLUGINSDIR\installer-holder-probe.exe" "..\..\..\installer-holder-probe.exe" + ck_probe_ready_${tag}: +FunctionEnd + +; usage: set $GuardDir to the directory to check, then call this. Sets +; $GuardExit to the probe's exit code and $GuardOutput to its one-line answer. +; 0 = nothing holds the payload and nothing runs from the directory, 1 = the +; line names what does, anything else = the probe could not answer (2 = no +; directory was given, 3 = the Restart Manager call failed and no process runs +; from the directory either, "timeout" = it did not return in time). +Function ${prefix}RunHolderProbe + Call ${prefix}ExtractHolderProbe + ; The directory travels in an environment variable, never on the command + ; line: a path with spaces, quotes or a trailing backslash then does not + ; have to survive NSIS quoting. + System::Call 'kernel32::SetEnvironmentVariable(t, t) i ("COCKATRICE_GUARD_DIR", "$GuardDir")' + ; /TIMEOUT because this can run before the first page: a Restart Manager + ; that does not answer must not leave the installer with no window and no + ; way out. + nsExec::ExecToStack /TIMEOUT=20000 '"$PLUGINSDIR\installer-holder-probe.exe"' + Pop $GuardExit + Pop $GuardOutput +FunctionEnd + +; usage: set $GuardDir to the directory being replaced, then call this. Asks the running +; Cockatrice to close, and waits for it to exit. +; +; The set is what the probe names: the processes holding a file of the version being +; replaced, plus the processes running from the directory. tools\installer-holder-probe +; closes that set by pid, which is why nothing here has to match an executable path. Only +; cockatrice.exe is asked: Oracle writes the card database in place and Servatrice has no +; window, so both stay the user's job, as the prompt says. +; +; Each instance is brought to the foreground before WM_CLOSE is posted to its main window. +; WM_CLOSE is answered, not obeyed - Cockatrice asks before it quits while a card database +; update is running, a game is open or a deck is unsaved - and that question is only useful +; if it is in front of the installer instead of behind it. +; +; The probe waits up to 30 s for the processes it asked to exit, so the check that follows +; measures a settled directory rather than sleeping for a fixed time. Its line goes into the +; log as it stands: it names every holder, including the ones left to the user, so a close +; that did nothing can be told apart from one that was never sent. +; +; Never a force-close: the application decides whether to quit, and one that does not answer +; is left to the user instead of being killed. +Function ${prefix}CloseHolders + Call ${prefix}ExtractHolderProbe + System::Call 'kernel32::SetEnvironmentVariable(t, t) i ("COCKATRICE_GUARD_DIR", "$GuardDir")' + ; The probe waits 30 s itself. The timeout here only bounds a probe that never returns; + ; the Restart Manager call inside it is the same one the holder listing makes. + nsExec::ExecToStack /TIMEOUT=40000 '"$PLUGINSDIR\installer-holder-probe.exe" --close-holders' + Pop $R3 + Pop $R4 + ; Not the CockLog macro: this function exists twice and the uninstaller copy may only + ; Call un. functions, so the macro's two lines are spelled with the prefix here. + Push "close-holders: probe exit=$R3 [$R4]" + Call ${prefix}CockLogWrite + DetailPrint "Closing Cockatrice: $R4" +FunctionEnd +!macroend + +!insertmacro CockHolderProbeFunctions "" install +!insertmacro CockHolderProbeFunctions "un." uninstall + +; usage: set $GuardDir to the directory to guard, then call this. +; +; Asks Cockatrice to close once, then prompts until the payload is free. The automatic +; close is the same request clicking the window's close button makes, so the common case - +; the client was still shutting down - never turns into a question. The prompt is what +; remains when the close cannot reach everything: Oracle writes the card database in +; place, Servatrice has no window, and Cockatrice may still be waiting for an answer in +; its own confirmation. It offers one more close (Yes), a plain re-check for after the +; user closed them by hand (No), and Cancel. +; +; An error is always shown, in every mode. A /S install without /R has nobody at +; the keyboard, but this installer does not promise unattended runs: when an +; error needs a decision, asking for one is better than silently leaving a +; half-updated install behind. The happy path has no dialog, so a /S reinstall +; that has nothing to report stays silent. +Function GuardAppsNotRunning + !insertmacro CockLog "guard: checking $GuardDir (launcherExited=$LauncherExited)" + StrCpy $GuardCloseTried 0 + guard_check: + Call RunHolderProbe + ${If} $GuardExit == "0" + !insertmacro CockLog "guard: nothing is holding the payload" + DetailPrint "No running Cockatrice, Oracle or Servatrice instance was found." + Return + ${EndIf} + ${If} $GuardExit == "1" + !insertmacro CockLog "guard: still in use by $GuardOutput" + ; Close it for the user first. The probe knows exactly which cockatrice.exe holds + ; these files, and asking it to close is the same request as clicking the window's + ; close button - the application still answers its own "really quit?" prompt, so + ; nothing is lost. One attempt per guard call keeps the common case (the client was + ; still shutting down) from turning into a question, without hiding the case where + ; the close did not help: the pass after it names what is still holding the files. + ${If} $GuardCloseTried = 0 + StrCpy $GuardCloseTried 1 + !insertmacro CockLog "guard: asking cockatrice to close, then checking again" + Call CloseHolders + Goto guard_check + ${EndIf} + DetailPrint "The install directory is in use by: $GuardOutput" + ; Three answers, and the close is one of them: the automatic attempt above ran + ; before this prompt and could not have answered Cockatrice's own confirmation, so a + ; second close is often exactly what is needed once the user has dealt with that + ; window. NSIS does not let a MessageBox relabel its buttons, so the text says what + ; Yes, No and Cancel do. + MessageBox MB_YESNOCANCEL|MB_ICONEXCLAMATION|MB_DEFBUTTON1 \ + "Cockatrice, Oracle or Servatrice is still using files of the installation:$\r$\n$\r$\n$GuardOutput$\r$\n$\r$\nChoose Yes to close Cockatrice and check again. It may ask you to confirm quitting - it does while a card database update is running, a game is open or a deck is unsaved - so answer that window.$\r$\n$\r$\nClose Oracle, Servatrice and any other program listed above by hand. Choose No to check again after that, or Cancel to cancel the installation." \ + IDYES guard_close IDNO guard_check + !insertmacro CockLog "guard: cancelled by user" StrCpy $AbortReason \ - "The installation was cancelled because it could not determine whether Cockatrice is still running.$\r$\n$\r$\nPlease close Cockatrice, Oracle and Servatrice and run the installer again." + "The installation was cancelled because Cockatrice, Oracle or Servatrice is still running and holding files of the previous version.$\r$\n$\r$\nClose them and run the installer again." Call AbortInstall + ; Only reached through Yes: AbortInstall does not return. + guard_close: + !insertmacro CockLog "guard: closing cockatrice on request, then checking again" + Call CloseHolders + Goto guard_check ${EndIf} - DetailPrint "Could not determine whether $R1 is running; close Cockatrice, Oracle and Servatrice manually if the installation reports a problem." - ${IfNot} ${Silent} - MessageBox MB_OK|MB_ICONEXCLAMATION \ - "This installer could not determine whether $R1 is still running.$\r$\n$\r$\nPlease close Cockatrice, Oracle and Servatrice manually before continuing." - ${EndIf} + ; Not an answer at all: the probe was not configured, the Restart Manager + ; call failed and nothing runs from the directory, or it timed out. Carrying + ; on from that is a decision, so it is asked for rather than assumed - the + ; stale-runtime check after the purge still stands between a failed guard + ; and a mismatched install, but it only fires after files have been + ; replaced, and on an upgrade the launcher asked for nobody is watching the + ; installer's exit code. Retry asks the probe again (a Restart Manager that + ; was busy answers the second time), Ignore installs anyway, Abort cancels + ; instead of writing a payload next to files the guard could not rule out. + !insertmacro CockLog "guard: the holder probe could not answer ($GuardExit): $GuardOutput" + DetailPrint "Could not check whether Cockatrice is still running: $GuardOutput" + MessageBox MB_ABORTRETRYIGNORE|MB_ICONEXCLAMATION|MB_DEFBUTTON1 \ + "This installer could not check whether Cockatrice, Oracle or Servatrice is still running.$\r$\n$\r$\nThe check reported: $GuardOutput$\r$\n$\r$\nClose them, then choose Retry to check again. Choose Ignore to install anyway, or Abort to cancel the installation." \ + IDRETRY guard_check IDABORT guard_unknown_abort + !insertmacro CockLog "guard: continuing without an answer from the probe" + Return + guard_unknown_abort: + StrCpy $AbortReason \ + "The installation was cancelled because it could not check whether Cockatrice, Oracle or Servatrice is still running.$\r$\n$\r$\nThe check reported: $GuardOutput$\r$\n$\r$\nPlease close them and run the installer again." + Call AbortInstall FunctionEnd Function EnsureAppsNotRunning @@ -919,67 +915,55 @@ FunctionEnd ; Aborts the installation, showing $AbortReason to the user. ; -; Every abort goes through here so the unattended case is handled the same way -; everywhere: the in-app updater runs this installer silent and detached and -; never looks at its exit code, so a plain Abort would leave the user with no -; update and no explanation, while a /S install without /R is unattended - by a -; package manager, for instance - and a message box there would block forever. -; Only the first of those two gets a dialog; the second reports through the log -; and the exit code. +; Every abort goes through here so no failure ends silently: the in-app updater +; runs this installer silent and detached and never looks at its exit code, and +; a /S install without /R has nobody watching its exit code either, so a plain +; Abort would leave the user with no update and no explanation. An error always +; shows its dialog, in every mode; a silent run stays silent only for as long +; as nothing goes wrong. The log path is part of the dialog, so a report can be +; acted on without asking the user to find the file. Function AbortInstall !insertmacro CockLog "ABORT: $AbortReason" !insertmacro CockLog "full installer log: $InstallLogPath" - ; Show a dialog whenever someone is at the keyboard to see it: an interactive - ; install, or the in-app updater's /R upgrade. The /R upgrade is silent, but - ; the client that launched the installer has already quit, so the user is - ; still there. Only a /S install without /R is attended by a package manager - ; and must go straight to the exit code. - ${If} $ReinstallMode = 1 - ${OrIfNot} ${Silent} - MessageBox MB_OK|MB_ICONEXCLAMATION "$AbortReason$\r$\n$\r$\nA detailed installer log was written to:$\r$\n$InstallLogPath" - ; Offer to send the installer log to Sentry so aborted installs can be diagnosed. This block already runs - ; only when someone is at the keyboard (attended install, or the updater's /R upgrade whose launcher has - ; quit), so a silent or package-manager install never reaches the prompt and its behavior is unchanged. - ; Only a package built with a baked DSN offers to send, so the prompt can only appear when a send - ; can actually go out. A preprocessor guard rather than a runtime test: a package built without a - ; DSN compiles the block out entirely, so it cannot even carry the helper or its baked endpoint. - ; No /SD here: a /SD would suppress this prompt on the silent /R upgrade, which is exactly the - ; attended-update case this offer exists for. The runtime gate above (ReinstallMode/Silent) is the - ; hang protection — a package-manager /S install without /R never reaches this box, matching the - ; two sibling MessageBox sites in this file. - !if "@CPACK_SENTRY_ENABLED@" == "1" - MessageBox MB_YESNO|MB_ICONQUESTION "Send the installer log to the developers to help fix this? The log contains file paths from this machine, which may include your Windows user name." IDYES sentry_send - !insertmacro CockLog "sentry send: declined by user" - Goto sentry_done - sentry_send: - ; The abort reason is already in the log file (CockLog writes "ABORT: ..." as the - ; first thing AbortInstall does), so the helper reads it from there. Nothing - ; reason-shaped crosses the command line, which keeps quoting trivial and needs - ; no NSIS string functions. - InitPluginsDir - ; Embedded by the same relative path that resolves NSIS.definitions.nsh above (both are - ; configure_file'd into the build root, which is three levels up from project.nsi). An - ; absolute CMake path here carries forward slashes, which makensis does not treat as - ; absolute and so fails to find the file at package time. - File "/oname=$PLUGINSDIR\send-installer-log.ps1" "..\..\..\SendInstallerLog.ps1" - ; The send can take up to the /TIMEOUT below, so put something on the details pane - ; the moment the user says yes rather than leaving the window blank until it returns. - DetailPrint "Sending installer log..." - nsExec::ExecToStack /TIMEOUT=60000 'powershell -NoProfile -NonInteractive -ExecutionPolicy Bypass -File "$PLUGINSDIR\send-installer-log.ps1" -LogPath "$InstallLogPath"' - Pop $SentryResult - Pop $SentryOutput - ; $SentryOutput is a single line by construction: the helper flattens its message - ; and this log file is uploaded verbatim, so a stray newline must not be able to - ; forge extra lines into the artifact we send. - !insertmacro CockLog "sentry send: result=$SentryResult output=$SentryOutput" - ${If} $SentryResult == "0" - MessageBox MB_OK|MB_ICONINFORMATION "The installer log was sent. Thank you for helping improve Cockatrice!" - ${Else} - MessageBox MB_OK|MB_ICONEXCLAMATION "The installer log could not be sent. You can still share it manually; it was written to:$\r$\n$InstallLogPath" - ${EndIf} - sentry_done: - !endif - ${EndIf} + MessageBox MB_OK|MB_ICONEXCLAMATION "$AbortReason$\r$\n$\r$\nA detailed installer log was written to:$\r$\n$InstallLogPath" + ; Offer to send the installer log to Sentry so aborted installs can be diagnosed. + ; Only a package built with a baked DSN offers to send, so the prompt can only appear + ; when a send can actually go out. A preprocessor guard rather than a runtime test: a + ; package built without a DSN compiles the block out entirely, so it cannot even carry + ; the helper or its baked endpoint. No /SD: the abort is exactly the case the offer + ; exists for, including on the silent /R upgrade. + !if "@CPACK_SENTRY_ENABLED@" == "1" + MessageBox MB_YESNO|MB_ICONQUESTION "Send the installer log to the developers to help fix this? The log contains file paths from this machine, which may include your Windows user name." IDYES sentry_send + !insertmacro CockLog "sentry send: declined by user" + Goto sentry_done + sentry_send: + ; The abort reason is already in the log file (CockLog writes "ABORT: ..." as the + ; first thing AbortInstall does), so the helper reads it from there. Nothing + ; reason-shaped crosses the command line, which keeps quoting trivial and needs + ; no NSIS string functions. + InitPluginsDir + ; Embedded by the same relative path that resolves NSIS.definitions.nsh above (both are + ; configure_file'd into the build root, which is three levels up from project.nsi). An + ; absolute CMake path here carries forward slashes, which makensis does not treat as + ; absolute and so fails to find the file at package time. + File "/oname=$PLUGINSDIR\send-installer-log.ps1" "..\..\..\SendInstallerLog.ps1" + ; The send can take up to the /TIMEOUT below, so put something on the details pane + ; the moment the user says yes rather than leaving the window blank until it returns. + DetailPrint "Sending installer log..." + nsExec::ExecToStack /TIMEOUT=60000 'powershell -NoProfile -NonInteractive -ExecutionPolicy Bypass -File "$PLUGINSDIR\send-installer-log.ps1" -LogPath "$InstallLogPath"' + Pop $SentryResult + Pop $SentryOutput + ; $SentryOutput is a single line by construction: the helper flattens its message + ; and this log file is uploaded verbatim, so a stray newline must not be able to + ; forge extra lines into the artifact we send. + !insertmacro CockLog "sentry send: result=$SentryResult output=$SentryOutput" + ${If} $SentryResult == "0" + MessageBox MB_OK|MB_ICONINFORMATION "The installer log was sent. Thank you for helping improve Cockatrice!" + ${Else} + MessageBox MB_OK|MB_ICONEXCLAMATION "The installer log could not be sent. You can still share it manually; it was written to:$\r$\n$InstallLogPath" + ${EndIf} + sentry_done: + !endif SetErrorLevel 1 Abort FunctionEnd @@ -989,16 +973,15 @@ FunctionEnd ; ; This is the authority for the one question the rest of the installer can only ; approximate: is the process that started us still holding its Qt DLLs mapped? -; A mapped module cannot be deleted, so an answer of "not running" obtained by -; enumerating process names is what allows the update to write a new payload on +; A mapped module cannot be deleted, so an answer of "nothing is holding them" +; obtained from file handles is what allows the update to write a new payload on ; top of files it could not remove. Waiting on a real handle has no such gap, -; needs no powershell.exe, and is unaffected by a process whose path cannot be -; read - the case the name based check has to treat as "not ours" so it never -; closes a third party application. +; needs no process enumeration at all, and is unaffected by a process the holder +; probe cannot inspect. ; ; Sets $LauncherExited to 0 when no usable /PID= was given, which is the older ; client that does not pass one; the caller then has no authority available and -; falls back to the name based guard. +; falls back to the holder probe. Function WaitForLauncherToExit StrCpy $LauncherExited 0 !insertmacro CockLog "wait-launcher: pid='$WaitPid'" @@ -1014,6 +997,17 @@ Function WaitForLauncherToExit ; $RN. The caller reads $R0/$R1, so the uppercase .RN form is required; the ; original .r0/.r1 wrote $0/$1 and left $R1 empty, which made the "did not ; exit" test below compare "" against "0" and abort on every /PID= upgrade. + ; Reopened on every Retry below, so a launcher that exited in the meantime answers + ; "already exited" instead of being waited on again. + ; + ; Ten seconds, not the full grace: a client that started an update exits in well + ; under a second, so at ten seconds it is either flushing a card database or - far + ; more often - sitting on its own "quit anyway?" confirmation with nobody looking at + ; it. Silence for a minute is what makes that read as a hung installer. The full + ; grace is spent after the first question, so a user who answers Cockatrice's + ; confirmation is not asked again while they do it. + StrCpy $WaitSlice 10000 + wait_launcher_open: ClearErrors System::Call 'kernel32::OpenProcess(i 0x00100000, i 0, i $WaitPid) p.R0' ; OpenProcess returns a null handle for a process that is already gone. The @@ -1033,17 +1027,39 @@ Function WaitForLauncherToExit Return ${EndIf} DetailPrint "Waiting for the application that started this installer to exit ..." - ; Same 60 s grace period the name based check uses, so a client that is slow - ; to flush its card database does not get less room than it used to. ; WAIT_OBJECT_0 is 0; WAIT_TIMEOUT is 258 and a failed wait reports 0xFFFFFFFF. - System::Call 'kernel32::WaitForSingleObject(p R0, i 60000) i.R1' + System::Call 'kernel32::WaitForSingleObject(p R0, i $WaitSlice) i.R1' System::Call 'kernel32::CloseHandle(p R0)' ${If} $R1 != 0 - !insertmacro CockLog "wait-launcher: did not exit within 60s (wait=$R1)" - DetailPrint "The application that started this installer did not exit within 60 s." + !insertmacro CockLog "wait-launcher: did not exit within $WaitSlice ms (wait=$R1)" + DetailPrint "The application that started this installer did not exit in time." + ; Two answers, and "carry on anyway" is not one of them. A live process has its + ; image and its Qt DLLs mapped, so writing the payload over them is the broken + ; install this wait exists to prevent - and the guard that runs next names the + ; same process by its image path and stops there anyway, so an "ignore" would + ; only put the same question a moment later, after a close attempt nobody asked + ; for. Waiting again is what helps: the client is most likely showing its own + ; "quit anyway?" confirmation, and that window is the thing to answer. + MessageBox MB_RETRYCANCEL|MB_ICONEXCLAMATION|MB_DEFBUTTON1 \ + "Cockatrice has not finished closing, so the installation is paused rather than replace files it is still using.$\r$\n$\r$\nIt may be waiting for you to answer its own question about an unsaved deck, an open game or a card database update. Answer that window - choose Retry to bring it to the front, ask it to close again and keep waiting. If it does not answer at all, close it from Task Manager first.$\r$\n$\r$\nChoose Retry to wait again, or Cancel to cancel the installation." \ + IDRETRY wait_launcher_retry + !insertmacro CockLog "wait-launcher: cancelled by user" StrCpy $AbortReason \ "The application that started this update has not exited yet, so the installation was cancelled rather than replace files it is still using.$\r$\n$\r$\nPlease close Cockatrice and run the installer again." Call AbortInstall + ; Only reached through Retry: AbortInstall does not return. + wait_launcher_retry: + ; The question has been asked and answered once; from here the wait is the full + ; grace again, so a slow flush or a confirmation being answered does not turn + ; into a stream of prompts. + StrCpy $WaitSlice 60000 + ; The same request the window's close button makes, and the same close the guard + ; would send a moment later. It brings the client forward as well, which is the + ; part that helps when the confirmation is behind something else. Nothing is + ; force-stopped: a client that is waiting for an answer keeps the last word. + StrCpy $GuardDir $INSTDIR + Call CloseHolders + Goto wait_launcher_open ${EndIf} !insertmacro CockLog "wait-launcher: launcher exited" DetailPrint "The application that started this installer has exited." @@ -1068,57 +1084,6 @@ Function EnsureOldInstallNotRunning Call GuardAppsNotRunning FunctionEnd -Function GuardAppsNotRunning - ; usage: set $GuardDir to the directory to guard, then call this - Call IsProcessLookupAvailable - !insertmacro CockLog "guard: powershell available=$R0 dir=$GuardDir launcherExited=$LauncherExited" - ${If} $R0 = 0 - ; Without PowerShell we cannot tell whether a running instance is - ; holding locks on the files we are about to replace. - !insertmacro CockLog "guard: powershell unavailable, skipping instance check" - DetailPrint "Could not start powershell.exe; skipping the running instance check. Close Cockatrice, Oracle and Servatrice manually if the installation reports a problem." - ; On a silent upgrade that used to be a warning and nothing more, which - ; left the very process that launched the installer free to go on - ; replacing files it still had mapped - the one outcome this guard exists - ; to prevent - and then writing the new payload on top of them. Cancel - ; instead. An interactive install can still continue: the user is at the - ; keyboard, and refusing outright would leave them with an installer that - ; can never complete. - ; - ; Unless the launcher has already been waited on directly: WaitForLauncherToExit - ; needs no powershell.exe, so when it has run, Cockatrice is known to be - ; gone and the lookup failing to run costs nothing but the Oracle and - ; Servatrice checks. An older client that passes no /PID= leaves no such - ; authority, and there the upgrade is cancelled as before. - ${If} $LauncherExited != 1 - ${AndIf} $ReinstallMode = 1 - StrCpy $AbortReason \ - "The installation was cancelled because it could not check whether Cockatrice is still running, because powershell.exe could not be started.$\r$\n$\r$\nPlease close Cockatrice, Oracle and Servatrice and run the installer again." - Call AbortInstall - ${EndIf} - ; The user has to be told to close the applications by hand, but a /S - ; install without /R is unattended and would block on this forever. - ${If} $ReinstallMode = 1 - ${OrIfNot} ${Silent} - MessageBox MB_OK|MB_ICONEXCLAMATION "This installer could not check whether Cockatrice is still running, because powershell.exe could not be started.$\r$\n$\r$\nPlease close Cockatrice, Oracle and Servatrice manually before continuing." - ${EndIf} - Return - ${EndIf} - StrCpy $R1 "cockatrice.exe" - StrCpy $R2 "cockatrice" - ; a GUI application with unsaved user data: graceful close only - StrCpy $AllowForceClose 0 - Call WaitForAppToClose - StrCpy $R1 "oracle.exe" - StrCpy $R2 "oracle" - StrCpy $AllowForceClose 1 - Call WaitForAppToClose - StrCpy $R1 "servatrice.exe" - StrCpy $R2 "servatrice" - StrCpy $AllowForceClose 1 - Call WaitForAppToClose -FunctionEnd - ; Sets $R0 to 1 if $INSTDIR looks like an existing Cockatrice install. Used to ; guard the stale-runtime-DLL purge so a first-time install into a pre-existing ; directory with an unrelated "Plugins" folder is never touched, while an @@ -1321,16 +1286,18 @@ FunctionEnd ; survived the purge" said which files were present but never which one was held, ; and never whether the cause was a lock or a permission. ; -; The probe opens each survivor for DELETE access and then closes it again -; without deleting anything. The code is read by the plugin right after that call -; (?e), so a failure reports what Windows refused; clearing LastError first makes -; a zero code mean "the open worked". That is all it means: Windows lets a DLL a -; running process has mapped be opened for DELETE, and DeleteFile still refuses -; it until the image is unloaded, so a zero code is not evidence that the -; failure was transient - the holders line is what names a running process. The -; plugin parses the call's arguments between the clear and the call, so a -; non-zero code is reported as what Windows said rather than as proof of a -; cause - the wording below never claims one. +; Each survivor is opened for DELETE access and then asked the question DeleteFile +; asks: SetFileInformationByHandle(FileDispositionInfo) marks it for deletion on +; close, and the mark is cleared again right away, so nothing is deleted (and if +; that cancel ever failed, the file would go when the handle closes - a file the +; purge already tried to delete, and the line says so). That separates the three +; causes a failed Delete cannot tell apart: another handle that does not share +; delete refuses the mark, a read-only attribute refuses it, and a file nobody +; holds accepts it and would have been deleted. The mark is read through ?e, so a +; refusal reports Windows' own code; clearing LastError first keeps a zero code +; meaning "accepted". The wording below reports what Windows said and never claims +; a cause the code cannot prove. Which process is holding a survivor, if any, is +; what the holders line after the loop answers. Function LogStaleRuntimeDiagnostics FindFirst $StaleFindHandle $StaleFindName "$INSTDIR\*.dll" ${If} $StaleFindHandle != "" @@ -1347,22 +1314,52 @@ Function LogStaleRuntimeDiagnostics ${Else} Pop $R2 ${If} $R2 = 0 - ; A valid handle: close it. The open says nothing about Delete: - ; a DLL a running process has mapped opens for DELETE and still - ; refuses DeleteFile, and a read-only file does the same until its - ; attribute is cleared (the purge clears it before every attempt - ; now, so a survivor that is read-only had it set again). Report - ; that standing cause instead of calling the failure transient. - System::Call 'kernel32::CloseHandle(p R1)' - System::Call 'kernel32::GetFileAttributesW(w "$INSTDIR\$StaleFindName") i.R3' - ${If} $R3 = -1 - StrCpy $R0 "a delete-access open now succeeds, but its attributes could not be read" + ; The open worked, which says nothing about Delete: a DLL a running + ; process has mapped opens for DELETE and still refuses DeleteFile, and + ; a read-only file does the same until its attribute is cleared. Ask + ; Windows the question the purge actually asks - mark the file for + ; deletion on close, then cancel that again. This is the same request + ; DeleteFile makes on a temporary handle, so its answer says whether a + ; delete would succeed now. + ClearErrors + System::Call '*(&i1 1) p.R4' + System::Call 'kernel32::SetFileInformationByHandle(p R1, i 4, p R4, i 1) i.R5 ?e' + ${If} ${Errors} + StrCpy $R0 "a delete-access open now succeeds, but Windows could not be asked whether it would delete it" ${Else} + Pop $R6 + ${If} $R5 != 0 + ; Accepted: cancel the mark again, so this stays a question and + ; does not turn into the deletion it is asking about. A cancel + ; that fails leaves the file to go when the handle closes, which + ; is what the purge wanted anyway; the line says so. + ClearErrors + System::Call '*(&i1 0) p.R4' + System::Call 'kernel32::SetFileInformationByHandle(p R1, i 4, p R4, i 1) i.R5 ?e' + ${If} ${Errors} + StrCpy $R0 "a delete would succeed now, but the check could not undo the mark it set" + ${Else} + Pop $R6 + ${If} $R5 != 0 + StrCpy $R0 "a delete would succeed now - the earlier failure was transient" + ${Else} + StrCpy $R0 "a delete would succeed now, and it is marked for deletion from here on" + ${EndIf} + ${EndIf} + ${Else} + StrCpy $0 $R6 + Call GetWin32ErrorText + StrCpy $R0 "a delete-access open now succeeds, but Windows refused to mark it for deletion: win32 error $R6: $R0" + ${EndIf} + ${EndIf} + System::Call 'kernel32::CloseHandle(p R1)' + ; The read-only attribute is a cause on its own, and the purge clears it + ; before every attempt, so a survivor that is read-only had it set again. + System::Call 'kernel32::GetFileAttributesW(w "$INSTDIR\$StaleFindName") i.R3' + ${If} $R3 != -1 IntOp $R3 $R3 & 1 ${If} $R3 = 1 - StrCpy $R0 "a delete-access open now succeeds, but it is read-only, which stops Delete" - ${Else} - StrCpy $R0 "a delete-access open now succeeds, but a DLL a running process has mapped still refuses Delete - see holders" + StrCpy $R0 "$R0; it is read-only again" ${EndIf} ${EndIf} ${Else} @@ -1380,45 +1377,34 @@ Function LogStaleRuntimeDiagnostics Call LogProcessesUsingInstallDir FunctionEnd -; Names the applications and services that still have the install directory's files -; open or mapped. Writes one line to the log and sets $HolderFound (1 when anything was -; named), so the abort message can repeat that line to the user. +; Names the applications and services that still have the payload open or mapped. +; Writes one line to the log and sets $HolderFound (1 when anything was named), so the +; abort message can repeat that line to the user. ; -; The lookup is a small executable built from tools/installer-holder-probe, extracted to -; $PLUGINSDIR only when the purge has already failed: the successful path pays nothing -; for it, and $PLUGINSDIR takes it away again when the installer exits. It is compiled by -; the same Windows build that compiles this installer, which is what lets it call the -; Restart Manager API directly. The same call made from this script would have to -; marshal RM_PROCESS_INFO through System::Call as a table of hand-computed offsets, and -; that table is precisely what cannot be checked without a Windows toolchain; in -; compiled code the layout belongs to the Windows SDK header. +; The lookup is tools/installer-holder-probe, the same executable the guard runs before +; anything is replaced; here it answers the other question, about files a purge could not +; remove. It asks the Restart Manager (RmRegisterResources/RmGetList), which names a +; process holding a plain handle on a file - an antivirus scanner - and names services as +; well as user programs, and it adds every process running from the directory, because a +; running instance holds its payload as mapped images rather than as open handles and the +; Restart Manager does not reliably report those. Its remaining blind spot is a handle +; taken in kernel mode, where there is no process to name; then the per-file codes logged +; above are the whole story, and the probe says so by naming nothing rather than by +; failing. ; -; Restart Manager is what makes the answer complete. Walking processes can only name one -; whose image lives here or that loaded one of these libraries, while Restart Manager -; names a process that holds a plain handle on a file - an antivirus scanner - and names -; services as well as user programs. Its blind spot is a handle taken in kernel mode, -; where there is no process to name; then the codes logged above are the whole story, and -; the probe says so by naming nothing rather than by failing. -; -; $INSTDIR travels through the same COCKATRICE_GUARD_DIR environment variable the -; running-instance guard uses, so a path with spaces or apostrophes never has to be -; embedded in a command line. +; $INSTDIR travels through the same COCKATRICE_GUARD_DIR environment variable the guard +; uses, so a path with spaces or apostrophes never has to be embedded in a command line. Function LogProcessesUsingInstallDir StrCpy $HolderExit 0 StrCpy $HolderOutput "" StrCpy $HolderFound 0 - InitPluginsDir - ; Embedded by the same relative path that resolves NSIS.definitions.nsh above (both - ; are built into the build root, which is three levels up from project.nsi). - File "/oname=$PLUGINSDIR\installer-holder-probe.exe" "..\..\..\installer-holder-probe.exe" - System::Call 'kernel32::SetEnvironmentVariable(t, t) i ("COCKATRICE_GUARD_DIR", "$INSTDIR")' - ; /TIMEOUT because this runs on the abort path, ahead of the message box: one call - ; into the Restart Manager cannot take the minute a walk over every process can, but - ; if that service stalls it must not leave the installer hanging with nothing on - ; screen. On a timeout nsExec reports it through the exit code like any other failure. - nsExec::ExecToStack /TIMEOUT=20000 '"$PLUGINSDIR\installer-holder-probe.exe"' - Pop $HolderExit - Pop $HolderOutput + ; The directory that just failed the purge, not whatever a check before the directory + ; page looked at: a portable install into a custom folder reaches here with the two + ; being different. + StrCpy $GuardDir $INSTDIR + Call RunHolderProbe + StrCpy $HolderExit $GuardExit + StrCpy $HolderOutput $GuardOutput ; The exit code, not the text, decides what happened: it cannot be mangled by what ; nsExec leaves on the end of the captured output. 1 = the line below names what is ; holding the files, 0 = nothing holds them any more, anything else = the probe could @@ -1426,85 +1412,69 @@ Function LogProcessesUsingInstallDir ${If} $HolderExit == "1" StrCpy $HolderFound 1 ${ElseIf} $HolderExit == "0" - StrCpy $HolderOutput "nothing is holding them any more" + StrCpy $HolderOutput "nothing is holding them and no program is running from the directory" ${Else} StrCpy $HolderOutput "the holder probe could not run: $HolderOutput" ${EndIf} !insertmacro CockLog "holders: $HolderOutput" FunctionEnd -; Called after a purge left files behind and LogStaleRuntimeDiagnostics named who -; is holding them. Instead of aborting straight away - which on /R strands the -; user, since the old version is already uninstalled - ask, close what this -; installer owns, purge again, and retry while the user keeps asking. Sets -; $StaleRuntimeAfterPurge to the last purge's verdict, so the caller still aborts -; when the files survive. +; Called after a purge left files behind and LogStaleRuntimeDiagnostics named who is +; holding them. Instead of aborting straight away - which on /R strands the user, since +; the old version is already uninstalled - ask, close what this installer owns, purge +; again, and retry while the user keeps asking. Sets $StaleRuntimeAfterPurge to the last +; purge's verdict, so the caller still aborts when the files survive. ; -; Cockatrice is asked to close (WM_CLOSE, the same request as clicking the -; window's close button) and keeps the last word: it may be waiting for an answer -; to its own "really quit?" confirmation (a running card database update, an open -; game, an unsaved deck), and that stays its decision, exactly as the -; running-instance guard treats it. The instance is brought forward first, so that -; confirmation is on screen rather than behind this installer's prompt. Oracle and -; Servatrice have no window to send a close to at all, and nothing here is -; force-stopped: Oracle writes the card database in place and killing it mid-write -; would corrupt it. Everything the probe named that this installer cannot close - -; those two included - is left to the user, whom the prompt names it to. +; Cockatrice is asked to close (WM_CLOSE, the same request as clicking the window's close +; button) and keeps the last word: it may be waiting for an answer to its own "really +; quit?" confirmation (a running card database update, an open game, an unsaved deck), and +; that stays its decision. The instance is brought forward first, so that confirmation is +; on screen rather than behind this installer's prompt. Oracle and Servatrice have no +; window to send a close to at all, and nothing here is force-stopped: Oracle writes the +; card database in place and killing it mid-write would corrupt it. Everything the probe +; named that this installer cannot close - those two included - is left to the user, whom +; the prompt names it to. ; ; Restart Manager's RmShutdown was the alternative and was rejected: it shuts down -; services as well as applications, and the only way to bring them back is -; RmRestart, which would restart this very application into files that are about -; to be replaced. +; services as well as applications, and the only way to bring them back is RmRestart, +; which would restart this very application into files that are about to be replaced. ; -; No offer when nobody is at the keyboard (a /S install without /R, e.g. a package -; manager): a dialog there would block forever. /R is the in-app updater, whose -; client has already quit, so the user is there - the same reasoning AbortInstall -; uses for its message box. No offer either when the probe named nobody: there is -; nothing to close, and the abort's per-file diagnostics already record what -; Windows said. +; Always offered, in every mode: a /S install without /R is the only case without someone +; at the keyboard, and this installer does not promise unattended runs - if an error needs +; a decision, it asks for one. No offer when the probe named nobody: there is nothing to +; close, and the abort's per-file diagnostics already record what Windows said. ; -; The first round offers the close; every later one stops offering it and says -; what to do instead. Repeating the same question is what made an accepted close -; look like a Yes that was ignored: when Cockatrice was blocked on its own -; confirmation, the second Yes was delivered to a window that dialog had disabled, -; so nothing closed, the holders were unchanged, and the identical prompt came -; back with no hint of what had happened. -Function OfferToCloseAndRetryPurge - ${If} $ReinstallMode = 0 - ${AndIf} ${Silent} - !insertmacro CockLog "close-holders: unattended install, not offering to close holders" - Return - ${EndIf} +; Every round offers all three answers: Yes closes Cockatrice and purges again, No purges +; again without asking it to close (for when the user closed things by hand), and Cancel +; gives up. The close stays on offer after one that did nothing because the usual reason is +; Cockatrice's own confirmation still waiting for an answer - the probe waits for that, the +; user can answer it in the application's window, and the next round can then collect the +; process the earlier attempt could not. Each round re-reads the holders, so the prompt +; names what survived the last attempt rather than repeating the first listing. +Function RecoverFromHolders ${If} $HolderFound != 1 - !insertmacro CockLog "close-holders: no holder was named, nothing to offer" + !insertmacro CockLog "recover-holders: no holder was named, nothing to offer" Return ${EndIf} - StrCpy $HolderCloseRounds 0 - ocrp_prompt: - ${If} $HolderCloseRounds = 0 - MessageBox MB_YESNO|MB_ICONEXCLAMATION|MB_DEFBUTTON2 \ - "Files of the previous version of Cockatrice are still in use by:$\r$\n$\r$\n$HolderOutput$\r$\n$\r$\nThe installer can close Cockatrice for you. It may first ask you to confirm quitting - it does while a card database update is running, a game is open or a deck is unsaved - so answer that window. Close Oracle, Servatrice and any other program listed above by hand.$\r$\n$\r$\nChoose Yes to close Cockatrice and try again, or No to cancel the installation." \ - IDYES ocrp_close - !insertmacro CockLog "close-holders: declined by user" - Return - ${EndIf} - MessageBox MB_RETRYCANCEL|MB_ICONEXCLAMATION|MB_DEFBUTTON1 \ - "Files of the previous version of Cockatrice are still in use by:$\r$\n$\r$\n$HolderOutput$\r$\n$\r$\nCockatrice may be waiting for an answer in its own window - it asks before quitting while a card database update is running, a game is open or a deck is unsaved. Switch to it and confirm, then close Oracle, Servatrice and any other program listed above.$\r$\n$\r$\nChoose Retry once they are closed, or Cancel to cancel the installation." \ - IDRETRY ocrp_close - !insertmacro CockLog "close-holders: declined by user after $HolderCloseRounds round(s)" + rfh_prompt: + MessageBox MB_YESNOCANCEL|MB_ICONEXCLAMATION|MB_DEFBUTTON1 \ + "Files of the previous version of Cockatrice are still in use by:$\r$\n$\r$\n$HolderOutput$\r$\n$\r$\nChoose Yes to close Cockatrice and try the removal again. It may ask you to confirm quitting - it does while a card database update is running, a game is open or a deck is unsaved - so answer that window. Close Oracle, Servatrice and any other program listed above by hand.$\r$\n$\r$\nChoose No to try again after closing the programs yourself, or Cancel to cancel the installation." \ + IDYES rfh_close IDNO rfh_retry + !insertmacro CockLog "recover-holders: declined by user" Return - ocrp_close: - IntOp $HolderCloseRounds $HolderCloseRounds + 1 - !insertmacro CockLog "close-holders: accepted (round $HolderCloseRounds), holders were [$HolderOutput]" - ; The close is a request, not a fact: AskCockatriceAppsToClose waits for the - ; instances it asked to exit, so a confirmation of Cockatrice's that is still up is - ; reported as still running instead of being measured as a successful close. - Call AskCockatriceAppsToClose + rfh_close: + !insertmacro CockLog "recover-holders: closing cockatrice on request, holders were [$HolderOutput]" + ; The close is a request, not a fact: CloseHolders waits for the instances it asked to + ; exit, so a confirmation of Cockatrice's that is still up is reported as still running + ; instead of being measured as a successful close. + StrCpy $GuardDir $INSTDIR + Call CloseHolders + rfh_retry: Call RetryDllPurge StrCpy $StaleRuntimeAfterPurge $R0 - !insertmacro CockLogListing "after-close-purge" "*.dll" - !insertmacro CockLogListing "after-close-plugins" "Plugins\*.*" - !insertmacro CockLog "close-holders: stale-runtime after retry=$StaleRuntimeAfterPurge" + !insertmacro CockLogListing "after-retry-purge" "*.dll" + !insertmacro CockLogListing "after-retry-plugins" "Plugins\*.*" + !insertmacro CockLog "recover-holders: stale-runtime after retry=$StaleRuntimeAfterPurge" ${If} $StaleRuntimeAfterPurge = 0 DetailPrint "Files of the previous version were removed; continuing the installation." Return @@ -1515,179 +1485,12 @@ Function OfferToCloseAndRetryPurge ${If} $HolderFound != 1 Return ${EndIf} - Goto ocrp_prompt -FunctionEnd - -; Asks the running Cockatrice to close, and waits for it to exit. -; -; The set is the Restart Manager answer from LogProcessesUsingInstallDir: exactly the -; processes holding a file of the version being replaced. tools\installer-holder-probe -; closes that set by pid, which is why nothing here has to match an executable path or -; depend on powershell.exe. Only cockatrice.exe is asked: Oracle writes the card database -; in place and Servatrice has no window, so both stay the user's job, as the prompt says. -; -; Each instance is brought to the foreground before WM_CLOSE is posted to its main -; window. WM_CLOSE is answered, not obeyed - Cockatrice asks before it quits while a card -; database update is running, a game is open or a deck is unsaved - and that question is -; only useful if it is in front of the installer instead of behind it. -; -; The probe waits up to 30 s for the processes it asked to exit, so the purge that follows -; measures a settled directory rather than sleeping for a fixed time. Its line goes into -; the log as it stands: it names every holder, including the ones left to the user, so a -; close that did nothing can be told apart from one that was never sent. -Function AskCockatriceAppsToClose - InitPluginsDir - ; LogProcessesUsingInstallDir extracted it already; the check keeps a single copy of - ; the executable in the installer and still works if that order ever changes. - IfFileExists "$PLUGINSDIR\installer-holder-probe.exe" ck_close_probe_ready - File "/oname=$PLUGINSDIR\installer-holder-probe.exe" "..\..\..\installer-holder-probe.exe" - ck_close_probe_ready: - System::Call 'kernel32::SetEnvironmentVariable(t, t) i ("COCKATRICE_GUARD_DIR", "$INSTDIR")' - ; The probe waits 30 s itself. The timeout here only bounds a probe that never - ; returns; the Restart Manager call inside it is the same one the holder listing makes. - nsExec::ExecToStack /TIMEOUT=40000 '"$PLUGINSDIR\installer-holder-probe.exe" --close-holders' - Pop $R3 - Pop $R4 - !insertmacro CockLog "close-holders: probe exit=$R3 [$R4]" - DetailPrint "Closing Cockatrice: $R4" -FunctionEnd - -; Uninstaller copies of the same routines (the uninstaller gets its own -; function set compiled in, it cannot call the installer functions). -Function un.IsAppRunning - ; An unset $GuardDir would match every process that merely shares the image - ; name, see IsAppRunning in the installer. - ${If} $GuardDir == "" - StrCpy $R0 2 - Return - ${EndIf} - ; Pass the directory to guard to PowerShell via an environment variable so - ; it never has to be embedded into the -Command string: apostrophes in a - ; path would terminate the command early, and the path in a -like pattern - ; would treat [ and ] as wildcard characters. - System::Call 'kernel32::SetEnvironmentVariable(t, t) i ("COCKATRICE_GUARD_DIR", "$GuardDir")' - ; Exit the process explicitly so nsExec's pop reflects whether a matching - ; process was found: powershell.exe exits 0 whether or not the pipeline - ; produced objects. 0 = not running, 1 = running, anything else = could not - ; be determined (see un.IsProcessLookupAvailable). - ; Strict match: a readable path under $GuardDir, same as in the installer. - nsExec::ExecToLog 'powershell -NoProfile -Command "if (Get-Process -Name $\'$R2$\' -ErrorAction SilentlyContinue | Where-Object { try { $$p = $$_.Path; if ($$p) { $$p.StartsWith($$env:COCKATRICE_GUARD_DIR, [StringComparison]::OrdinalIgnoreCase) } else { $$false } } catch { $$false } } | Select-Object -First 1) { exit 1 } else { exit 0 }"' - Pop $R0 - ${If} $R0 == "0" - StrCpy $R0 0 - ${ElseIf} $R0 == "1" - StrCpy $R0 1 - ${Else} - ; powershell.exe could not be run at all (nsExec pushes the string - ; "error"), or died with an unexpected code. That is "could not - ; determine", which has to stay distinguishable from "definitely - ; running": a state that can never clear would block the uninstaller - ; forever. un.IsProcessLookupAvailable warns about that case once. - StrCpy $R0 2 - ${EndIf} -FunctionEnd - -Function un.IsProcessLookupAvailable - nsExec::ExecToLog 'powershell -NoProfile -Command "exit 0"' - Pop $R0 - ${If} $R0 == "0" - StrCpy $R0 1 - ${Else} - StrCpy $R0 0 - ${EndIf} -FunctionEnd - -Function un.CloseMatchingApps - ; gracefully ask every matching instance to close (sends WM_CLOSE) - ; - ; Strict filter, same as in the installer: never close a process whose path - ; cannot be read, it is not necessarily one of ours. - System::Call 'kernel32::SetEnvironmentVariable(t, t) i ("COCKATRICE_GUARD_DIR", "$GuardDir")' - nsExec::ExecToLog 'powershell -NoProfile -Command "Get-Process -Name $\'$R2$\' -ErrorAction SilentlyContinue | Where-Object { try { $$p = $$_.Path; if ($$p) { $$p.StartsWith($$env:COCKATRICE_GUARD_DIR, [StringComparison]::OrdinalIgnoreCase) } else { $$false } } catch { $$false } } | ForEach-Object { $$null = $$_.CloseMainWindow() }"' - Pop $R3 -FunctionEnd - -Function un.ForceCloseApps - ; stop the matching instances by PID once the grace period is over, see - ; ForceCloseApps in the installer. - System::Call 'kernel32::SetEnvironmentVariable(t, t) i ("COCKATRICE_GUARD_DIR", "$GuardDir")' - nsExec::ExecToLog 'powershell -NoProfile -Command "Get-Process -Name $\'$R2$\' -ErrorAction SilentlyContinue | Where-Object { try { $$p = $$_.Path; if ($$p) { $$p.StartsWith($$env:COCKATRICE_GUARD_DIR, [StringComparison]::OrdinalIgnoreCase) } else { $$false } } catch { $$false } } | ForEach-Object { Stop-Process -Id $$_.Id -Force -ErrorAction SilentlyContinue }"' - Pop $R3 -FunctionEnd - -Function un.WaitForAppToClose - Call un.IsAppRunning - ; Only a definite "running" is worth waiting for, see WaitForAppToClose. - ${If} $R0 != 1 - Return - ${EndIf} - - ${If} ${Silent} - DetailPrint "Closing $R1 ..." - Call un.CloseMatchingApps - ; wall-clock deadline: each poll has to spawn a fresh powershell.exe, so - ; a bare loop counter would not bound the real wait time to 60 s. - System::Call 'kernel32::GetTickCount()i.s' - Pop $R8 - un_ck_wait_loop: - Sleep 500 - Call un.IsAppRunning - ${If} $R0 = 0 - DetailPrint "$R1 closed." - Return - ${EndIf} - ${If} $R0 != 1 - Return - ${EndIf} - System::Call 'kernel32::GetTickCount()i.s' - Pop $R9 - IntOp $R9 $R9 - $R8 - ${If} $R9 < 60000 - Goto un_ck_wait_loop - ${EndIf} - ; The grace period is over. Cockatrice is never force-stopped, see - ; WaitForAppToClose in the installer. A silent uninstall only reports - ; through the log and its exit code: it is also run unattended (the /R - ; updater uses it via QuietUninstallString, package managers use it - ; directly), and a message box would block there. The installer that ran - ; us reports the incomplete result to the user itself. - ${If} $AllowForceClose = 0 - DetailPrint "$R1 is still running after 60 s; aborting so that no data is lost." - SetErrorLevel 1 - Abort - ${EndIf} - DetailPrint "Force-stopping $R1 ..." - Call un.ForceCloseApps - Sleep 2000 - Call un.IsAppRunning - ${If} $R0 = 0 - DetailPrint "$R1 closed." - Return - ${EndIf} - ${If} $R0 != 1 - Return - ${EndIf} - DetailPrint "$R1 is still running after 60 s; aborting." - SetErrorLevel 1 - Abort - ${Else} - un_ck_wait_prompt: - MessageBox MB_RETRYCANCEL|MB_ICONEXCLAMATION|MB_DEFBUTTON1 \ - "$R1 is still running.$\r$\n$\r$\nPlease close it, then click Retry.$\r$\nClick Cancel to abort." \ - IDCANCEL un_ck_abort_install - Call un.IsAppRunning - ${If} $R0 = 0 - Return - ${EndIf} - ${If} $R0 != 1 - Return - ${EndIf} - Goto un_ck_wait_prompt - un_ck_abort_install: - Abort - ${EndIf} + Goto rfh_prompt FunctionEnd +; Uninstaller copy of the guard (the uninstaller gets its own function set compiled in, it +; cannot call the installer functions). ExtractHolderProbe and RunHolderProbe come from +; CockHolderProbeFunctions above; only the policy differs. Function un.EnsureAppsNotRunning ; guards the install directory; the uninstaller only ever has to check this one StrCpy $GuardDir $INSTDIR @@ -1695,29 +1498,54 @@ Function un.EnsureAppsNotRunning FunctionEnd Function un.GuardAppsNotRunning - Call un.IsProcessLookupAvailable - ${If} $R0 = 0 - ; Same reasoning as in the installer: without PowerShell we cannot tell - ; whether a running instance still holds file locks, and refusing to - ; uninstall would leave the user with an uninstaller that never - ; completes. Warn once and continue. - DetailPrint "Could not start powershell.exe; skipping the running instance check. Close Cockatrice, Oracle and Servatrice manually if the uninstall reports a problem." - MessageBox MB_OK|MB_ICONEXCLAMATION "This uninstaller could not check whether Cockatrice is still running, because powershell.exe could not be started.$\r$\n$\r$\nPlease close Cockatrice, Oracle and Servatrice manually before continuing." + StrCpy $GuardCloseTried 0 + un_guard_check: + !insertmacro CockLogUn "uninst-guard: checking $GuardDir" + Call un.RunHolderProbe + ${If} $GuardExit == "0" + !insertmacro CockLogUn "uninst-guard: nothing is holding the payload" Return ${EndIf} - StrCpy $R1 "cockatrice.exe" - StrCpy $R2 "cockatrice" - ; a GUI application with unsaved user data: graceful close only - StrCpy $AllowForceClose 0 - Call un.WaitForAppToClose - StrCpy $R1 "oracle.exe" - StrCpy $R2 "oracle" - StrCpy $AllowForceClose 1 - Call un.WaitForAppToClose - StrCpy $R1 "servatrice.exe" - StrCpy $R2 "servatrice" - StrCpy $AllowForceClose 1 - Call un.WaitForAppToClose + ${If} $GuardExit == "1" + !insertmacro CockLogUn "uninst-guard: still in use by $GuardOutput" + ; Same convenience as in the installer: ask Cockatrice to close once, then report + ; what the close could not reach. + ${If} $GuardCloseTried = 0 + StrCpy $GuardCloseTried 1 + !insertmacro CockLogUn "uninst-guard: asking cockatrice to close, then checking again" + Call un.CloseHolders + Goto un_guard_check + ${EndIf} + DetailPrint "The install directory is in use by: $GuardOutput" + ; Same three answers as the installer's guard; the automatic close above already + ; ran once before this prompt. + MessageBox MB_YESNOCANCEL|MB_ICONEXCLAMATION|MB_DEFBUTTON1 \ + "Cockatrice, Oracle or Servatrice is still using files of the installation:$\r$\n$\r$\n$GuardOutput$\r$\n$\r$\nChoose Yes to close Cockatrice and check again. It may ask you to confirm quitting - it does while a card database update is running, a game is open or a deck is unsaved - so answer that window.$\r$\n$\r$\nClose Oracle, Servatrice and any other program listed above by hand. Choose No to check again after that, or Cancel to cancel the uninstallation." \ + IDYES un_guard_close IDNO un_guard_check + !insertmacro CockLogUn "uninst-guard: cancelled by user" + SetErrorLevel 1 + Abort + ; Only reached through Yes: Abort does not return. + un_guard_close: + !insertmacro CockLogUn "uninst-guard: closing cockatrice on request, then checking again" + Call un.CloseHolders + Goto un_guard_check + ${EndIf} + ; Same reasoning as in the installer: without an answer from the probe we cannot tell + ; whether a running instance still holds file locks, and refusing outright would leave + ; the user with an uninstaller that never completes. Same three answers too, so an + ; uninstall that hits a Restart Manager that was busy can simply be retried. + !insertmacro CockLogUn "uninst-guard: the holder probe could not answer ($GuardExit): $GuardOutput" + DetailPrint "Could not check whether Cockatrice is still running: $GuardOutput" + MessageBox MB_ABORTRETRYIGNORE|MB_ICONEXCLAMATION|MB_DEFBUTTON1 \ + "This uninstaller could not check whether Cockatrice, Oracle or Servatrice is still running.$\r$\n$\r$\nThe check reported: $GuardOutput$\r$\n$\r$\nClose them, then choose Retry to check again. Choose Ignore to uninstall anyway, or Abort to cancel the uninstallation." \ + IDRETRY un_guard_check IDABORT un_guard_unknown_abort + !insertmacro CockLogUn "uninst-guard: continuing without an answer from the probe" + Return + un_guard_unknown_abort: + !insertmacro CockLogUn "uninst-guard: cancelled after the probe could not answer" + SetErrorLevel 1 + Abort FunctionEnd Function PortableModePageCreate @@ -1984,24 +1812,43 @@ ${If} $StaleRuntimeAfterPurge = 1 ; answers the question the message can only guess at, and so the prompt can ; name them and offer to close the ones this installer owns. Call LogStaleRuntimeDiagnostics - ; On /R the previous version is already uninstalled at this point, so an abort - ; leaves the user with no working Cockatrice. Offer to close and purge again, - ; repeating while the user keeps saying Yes; this updates - ; $StaleRuntimeAfterPurge with the last retry's verdict. - Call OfferToCloseAndRetryPurge + ${If} $HolderFound = 1 + ; On /R the previous version is already uninstalled at this point, so an abort + ; leaves the user with no working Cockatrice. Offer to close and purge again, + ; repeating while the user keeps saying Yes; this updates + ; $StaleRuntimeAfterPurge with the last retry's verdict. + Call RecoverFromHolders + ${Else} + ; Nothing is holding them. The three attempts in RetryDllPurge were two seconds + ; apart and all failed with no process to blame, which is the transient case a + ; virus scanner or a background indexer creates - or a permission problem that + ; will not clear. Give the delete one last chance before giving up: it costs a + ; second on a path that is about to abort anyway, and it keeps a lock that was + ; released in the meantime from turning into a failed upgrade. + !insertmacro CockLog "section: no holder named, one final purge attempt" + Sleep 1000 + Call RetryDllPurge + StrCpy $StaleRuntimeAfterPurge $R0 + !insertmacro CockLog "section: final-purge stale-runtime=$StaleRuntimeAfterPurge" + ${EndIf} ${EndIf} ${If} $StaleRuntimeAfterPurge = 1 !insertmacro CockLog "section: aborting, files of a previous version survived the purge" DetailPrint "Files of a previous version could not be removed; aborting the installation." - ; The in-app updater runs this installer silent and detached and never looks - ; at its exit code, so the user has to be told what happened. A /S install - ; without /R is unattended (a package manager, for instance) and a message - ; box there would block forever, so that case only reports through the log - ; and the exit code. AbortInstall owns that decision. + ; Say what is known and nothing more: the guard ran before anything was replaced, + ; so "most likely still running" is no longer a fair guess and would send the user + ; to close programs that are already closed. StrCpy $AbortReason \ - "The installation could not remove all files of a previous version of Cockatrice, most likely because it is still running or was started as administrator, and was cancelled rather than leave a mismatched install behind.$\r$\n$\r$\nPlease close Cockatrice, Oracle and Servatrice and run the installer again." + "The installation could not remove all files of a previous version of Cockatrice, and was cancelled rather than leave a mismatched installation behind." ${If} $HolderFound = 1 - StrCpy $AbortReason "$AbortReason$\r$\n$\r$\nStill using the install directory:$\r$\n$HolderOutput" + StrCpy $AbortReason "$AbortReason$\r$\n$\r$\nStill using the install directory:$\r$\n$HolderOutput$\r$\n$\r$\nClose them and run the installer again." + ${ElseIf} $HolderExit == "0" + StrCpy $AbortReason "$AbortReason$\r$\n$\r$\nNo running program was found using those files. A file permission or a virus scanner may be preventing their removal; the installer log records what Windows reported for each file." + ${Else} + ; The check itself failed, so "no running program was found" would be a claim this + ; installer cannot make. Name the failure instead, and leave the per-file codes to + ; the log, where they already are. + StrCpy $AbortReason "$AbortReason$\r$\n$\r$\nThe check for a running program could not be completed: $HolderOutput$\r$\n$\r$\nClose Cockatrice, Oracle and Servatrice and run the installer again; the installer log records what Windows reported for each file." ${EndIf} ; "Run the installer again" is not enough on an in-app update: the old version ; was uninstalled before the purge, so point at the installer that was downloaded. diff --git a/tools/installer-holder-probe/CMakeLists.txt b/tools/installer-holder-probe/CMakeLists.txt index ab3bb3eff..e4def8c39 100644 --- a/tools/installer-holder-probe/CMakeLists.txt +++ b/tools/installer-holder-probe/CMakeLists.txt @@ -1,9 +1,10 @@ -# Windows-only. The installer runs this on the abort path of a failed purge to name the -# applications and services holding the install directory open (Restart Manager); see -# the LogProcessesUsingInstallDir function in cmake/NSIS.template.in. It ships inside -# the installer and is extracted to $PLUGINSDIR there, never installed, so it links -# nothing from this project: no Qt, and a static C runtime because the installed runtime -# is exactly what the purge may have failed to replace. +# Windows-only. The installer runs this before anything is replaced (the running-instance +# guard in .onInit) and on the abort path of a failed purge, to name the applications and +# services holding the payload open (Restart Manager); see the GuardAppsNotRunning and +# LogProcessesUsingInstallDir functions in cmake/NSIS.template.in. It ships inside the +# installer and the uninstaller and is extracted to $PLUGINSDIR there, never installed, so +# it links nothing from this project: no Qt, and a static C runtime because the installed +# runtime is exactly what the purge may have failed to replace. add_executable(installer-holder-probe close_holders.cpp holder_line.cpp main.cpp restart_manager_holders.cpp) target_link_libraries(installer-holder-probe PRIVATE rstrtmgr) diff --git a/tools/installer-holder-probe/main.cpp b/tools/installer-holder-probe/main.cpp index 269d22821..db46035d3 100644 --- a/tools/installer-holder-probe/main.cpp +++ b/tools/installer-holder-probe/main.cpp @@ -2,6 +2,7 @@ #include "holder_line.h" #include "restart_manager_holders.h" +#include #include #include #include @@ -22,7 +23,9 @@ constexpr unsigned long CLOSE_WAIT_MILLISECONDS = 30000; /// Exit codes the installer reads. "Something was found" is deliberately not 0, so an /// answer of "nothing holds the files" stays distinguishable from a probe that failed. /// In the close mode 1 means a close was actually sent and 0 that every holder was left -/// to the user, which is what the report line next to the code spells out. +/// to the user, which is what the report line next to the code spells out. 3 is only for +/// a lookup that failed and named nobody: one that failed but still named a process +/// running from the directory is answered with that process. enum ExitCode { EXIT_NO_HOLDERS = 0, @@ -33,13 +36,17 @@ enum ExitCode /// The probe has two modes, selected by the first argument: /// -/// (none) name what holds the install directory, one line, for the abort -/// dialog and the log +/// (none) name what holds the payload, one line, for the guard prompt and the +/// abort dialog/log /// --close-holders bring the running Cockatrice forward, ask it to close, and wait for /// it to exit, for the offer to close after a failed purge /// -/// Both answer from the same Restart Manager query, so the close can only ever reach a -/// process that is holding a file of the version being replaced. +/// Both answer from two lookups that cover each other's blind spot: the Restart Manager +/// names whoever has a payload file open, and every process running from the directory is +/// named as well, because a running instance holds its own image and the modules it +/// loaded as mappings rather than open files and the Restart Manager does not always +/// report those. Neither lookup alone is the guard's authority: an instance the file +/// handles do not name is exactly the case that left a half-replaced install behind. bool closeModeRequested(int argumentCount, char **arguments) { return argumentCount > 1 && std::string(arguments[1]) == "--close-holders"; @@ -85,9 +92,26 @@ int main(int argumentCount, char **arguments) return EXIT_NOT_CONFIGURED; } - std::vector holders; - const unsigned long error = findProcessesUsingFiles(collectProbeFiles(installDir), holders); - if (error != ERROR_SUCCESS) { + // Processes running from the directory first: they are the instance the user is asked + // to close, and the Restart Manager may not name any of them (see + // findProcessesRunningFrom). The file holders follow, deduplicated by process, so an + // instance that both runs from the directory and holds a payload file is named once. + std::vector holders = findProcessesRunningFrom(installDir); + std::vector fileHolders; + const unsigned long error = findProcessesUsingFiles(collectPayloadFiles(installDir), fileHolders); + for (const Holder &holder : fileHolders) { + const bool alreadyNamed = std::any_of(holders.begin(), holders.end(), [&holder](const Holder &named) { + return named.processId == holder.processId; + }); + if (!alreadyNamed) { + holders.push_back(holder); + } + } + + // A failed lookup only makes the probe unable to answer when it also named nobody: with + // a process running from the directory the installer still knows what to close, and + // that is worth more to the guard than the code of the query that missed it. + if (error != ERROR_SUCCESS && holders.empty()) { writeLine(L"the Restart Manager lookup failed with win32 error " + std::to_wstring(error)); return EXIT_PROBE_FAILED; } diff --git a/tools/installer-holder-probe/restart_manager_holders.cpp b/tools/installer-holder-probe/restart_manager_holders.cpp index 39f6a0049..c00e5da8c 100644 --- a/tools/installer-holder-probe/restart_manager_holders.cpp +++ b/tools/installer-holder-probe/restart_manager_holders.cpp @@ -5,10 +5,12 @@ // here is a requirement; Regroup would sort them the other way round. #include #include +#include // clang-format on #include #include +#include namespace { @@ -20,6 +22,9 @@ constexpr std::size_t MAX_PROBED_FILES = 2000; /// How deep the Plugins walk goes. Its real shape is Plugins\\.dll. constexpr int MAX_PLUGIN_DEPTH = 4; +/// The payload files the installer replaces by name; the DLLs are matched by mask below. +constexpr const wchar_t *PAYLOAD_FILES[] = {L"cockatrice.exe", L"oracle.exe", L"servatrice.exe", L"qt.conf"}; + void collectFilesOf(const std::wstring &directory, int depth, int maxDepth, std::vector &files) { if (files.size() >= MAX_PROBED_FILES) { @@ -76,6 +81,30 @@ std::wstring fileNameOf(const std::wstring &path) return separator == std::wstring::npos ? path : path.substr(separator + 1); } +/// @p directory with exactly one trailing separator, so that a sibling whose name merely +/// begins with the same letters ("Cockatrice-old" next to "Cockatrice") cannot pass for a +/// subdirectory of it. +std::wstring directoryPrefixOf(const std::wstring &directory) +{ + std::wstring prefix = directory; + while (!prefix.empty() && (prefix.back() == L'\\' || prefix.back() == L'/')) { + prefix.pop_back(); + } + prefix += L'\\'; + return prefix; +} + +/// Whether @p path lies inside @p prefix, which is a directory with a trailing separator. +/// Ordinal and case-insensitive, as Windows compares paths. +bool isUnderDirectory(const std::wstring &path, const std::wstring &prefix) +{ + if (path.size() < prefix.size()) { + return false; + } + const int length = static_cast(prefix.size()); + return CompareStringOrdinal(path.c_str(), length, prefix.c_str(), length, TRUE) == CSTR_EQUAL; +} + /// Copies a fixed-size, NUL-terminated WCHAR array the API filled in. template std::wstring fixedString(const wchar_t (&text)[N]) { @@ -96,13 +125,39 @@ Holder holderOf(const RM_PROCESS_INFO &info) } } // namespace -std::vector collectProbeFiles(const std::wstring &installDir) +std::vector collectPayloadFiles(const std::wstring &installDir) { std::vector files; - // depth 0, maxDepth 0: the install root's files and nothing below it. Its data - // directory holds downloaded pictures, which the purge does not touch and which - // would turn this into a walk over thousands of files. - collectFilesOf(installDir, 0, 0, files); + for (const wchar_t *name : PAYLOAD_FILES) { + const std::wstring path = installDir + L"\\" + name; + const DWORD attributes = GetFileAttributesW(path.c_str()); + if (attributes != INVALID_FILE_ATTRIBUTES && (attributes & FILE_ATTRIBUTE_DIRECTORY) == 0) { + files.push_back(path); + } + } + + // Every top-level DLL: the set the purge deletes and therefore the set a running + // instance can have mapped. Matched by mask rather than by a list of expected names, + // so a transitive dependency nobody thought of is covered as well. + WIN32_FIND_DATAW entry{}; + const HANDLE dlls = FindFirstFileW((installDir + L"\\*.dll").c_str(), &entry); + if (dlls != INVALID_HANDLE_VALUE) { + do { + if (files.size() >= MAX_PROBED_FILES) { + break; + } + if ((entry.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0 || + (entry.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0) { + continue; + } + files.push_back(installDir + L"\\" + entry.cFileName); + } while (FindNextFileW(dlls, &entry) != 0); + FindClose(dlls); + } + + // Plugins is the other tree the purge removes. Its data directory is not: it holds + // downloaded pictures, which the purge does not touch and which would turn this into + // a walk over thousands of files. if (files.size() < MAX_PROBED_FILES) { collectFilesOf(installDir + L"\\Plugins", 0, MAX_PLUGIN_DEPTH, files); } @@ -158,3 +213,44 @@ unsigned long findProcessesUsingFiles(const std::vector &files, st RmEndSession(session); return error; } + +std::vector findProcessesRunningFrom(const std::wstring &installDir) +{ + std::vector holders; + if (installDir.empty()) { + return holders; + } + + const std::wstring prefix = directoryPrefixOf(installDir); + // Never the probe itself: it runs from the directory being replaced when the + // uninstaller asks in place or the installer was copied next to the payload, and + // naming it would ask the user to close the process that is doing the asking. + const DWORD self = GetCurrentProcessId(); + + const HANDLE snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0); + if (snapshot == INVALID_HANDLE_VALUE) { + return holders; + } + PROCESSENTRY32W entry{}; + entry.dwSize = sizeof(entry); + if (Process32FirstW(snapshot, &entry) != FALSE) { + do { + if (entry.th32ProcessID == self) { + continue; + } + // One process at a time, because the path is per process; a process that + // refuses to be read is not named here and is still named by Restart + // Manager when it holds a handle. The idle process has no image at all. + const std::wstring imagePath = imagePathOf(entry.th32ProcessID); + if (!isUnderDirectory(imagePath, prefix)) { + continue; + } + Holder holder; + holder.processId = entry.th32ProcessID; + holder.imageName = fileNameOf(imagePath); + holders.push_back(std::move(holder)); + } while (Process32NextW(snapshot, &entry) != FALSE); + } + CloseHandle(snapshot); + return holders; +} diff --git a/tools/installer-holder-probe/restart_manager_holders.h b/tools/installer-holder-probe/restart_manager_holders.h index d69fc0259..6dcd91108 100644 --- a/tools/installer-holder-probe/restart_manager_holders.h +++ b/tools/installer-holder-probe/restart_manager_holders.h @@ -7,17 +7,19 @@ #include /** - * @brief Lists the files a failed purge can be holding up, as full paths. + * @brief Lists the payload files, as full paths. * - * Everything directly in @p installDir plus everything below its Plugins directory: - * those are the two things the purge removes, so they are the two sets of files a - * holder can be holding. Restart Manager refuses a directory with ERROR_ACCESS_DENIED, - * which is why the probe registers files and this has to name them first. + * The set is what the installer replaces and the purge removes: the three executables, + * qt.conf, every top-level DLL and everything below Plugins. A document, a log or a + * screenshot the user happens to have open in the install directory is not part of it, + * so it cannot be mistaken for a running instance and block an upgrade. * - * Missing or unreadable directories are skipped rather than reported, and the list is - * capped: this runs on the abort path and must not walk a tree of downloaded pictures. + * Restart Manager refuses a directory with ERROR_ACCESS_DENIED, which is why the probe + * registers files and this has to name them first. Missing files and unreadable + * directories are skipped rather than reported, and the list is capped: this runs before + * the first page and on the abort path, and must not walk a tree of downloaded pictures. */ -std::vector collectProbeFiles(const std::wstring &installDir); +std::vector collectPayloadFiles(const std::wstring &installDir); /** * @brief Names the applications and services that have one of @p files open or mapped. @@ -32,4 +34,27 @@ std::vector collectProbeFiles(const std::wstring &installDir); */ unsigned long findProcessesUsingFiles(const std::vector &files, std::vector &holders); +/** + * @brief Lists every process whose image lives in @p installDir, whether or not it has a + * payload file open. + * + * Restart Manager answers the question "who has this file open", and a running instance + * does not necessarily have one open: its own image and the modules it loaded are mapped + * into it, not opened by it, and RmGetList does not reliably report a mapping. That is + * the state the failed updates this probe exists for were in - every DLL still on disk, + * an instance running from the directory, and "nothing is holding them any more" as the + * answer - so an answer built on file handles alone cannot be the guard's authority. + * + * Reading the image path needs PROCESS_QUERY_LIMITED_INFORMATION and + * QueryFullProcessImageNameW, neither of which depends on the bitness of either process, + * so a 64-bit instance is named from this 32-bit stub. The comparison is on the + * directory, not on an image name, so a process that merely shares a name (an Oracle + * database server next to a Cockatrice install) is never named. + * + * @param installDir the directory being replaced, as a full path. + * @return one entry per process running from it, in snapshot order, without the probe + * itself; empty when the directory is empty or no process runs from it. + */ +std::vector findProcessesRunningFrom(const std::wstring &installDir); + #endif