[Client] Hand off single-instance payloads outside the readyRead signal (#7373)

* [Client] Hand off single-instance payloads outside the readyRead signal

Opening a cockatrice:// link from outside a running client showed the
"Join game ...?" confirmation and then crashed on both answers. The
join reached the server first, so the other players saw the user join
and immediately disconnect.

SingleInstanceManager::handleNewConnection() emitted filesReceived from
inside the forwarding socket's readyRead lambda and only disconnected
afterwards. A joingame link makes the handler show a modal QMessageBox,
which runs a nested event loop. The sending instance exits as soon as
it has the acknowledgment, so the socket was disconnected and
deleteLater'd inside that loop: it died in the middle of its own
readyRead signal, and both this lambda and Qt's signal dispatch kept
using the freed memory.

Disconnect right after the acknowledgment and emit filesReceived
through a queued call, so no handler and no nested event loop ever runs
inside the socket's signal. Guarding the disconnect call alone is not
enough, since Qt still touches the socket after the lambda returns.

Add a regression test that reproduces the nested-event-loop hand-off.

* [Tests] Apply cmake-format to the single-instance test wiring

* [Client] Defer the single-instance socket teardown past the readyRead frame

The forwarded payload is emitted synchronously from inside the forwarding
socket's readyRead, which is what lets the modal "Join game?" confirmation
appear. A handler can spin a nested event loop, and the sending instance
exits as soon as it has the ACK, so the socket is disconnected and
deleteLater'd while that loop runs. Control then returns to this frame with
the socket already destroyed, and the inline disconnectFromServer() touched
freed memory.

Queue the teardown instead and guard it with a QPointer taken before the
handlers run -- building the guard afterwards is itself a use-after-free,
since QPointer dereferences the pointer it is constructed from. The payload
still reaches the handlers synchronously, so the confirmation is unchanged.

The regression test covers both halves: the handlers must run while the
socket is still connected, and the socket must not be torn down until the
readyRead frame has returned.

* Lint.

* [Client] Only delete the single-instance socket outside the readyRead frame

The auto-delete on `disconnected` was the remaining use-after-free. A
handler may spin a nested event loop (the modal "Join game?" box), the
sending instance exits as soon as it has the ACK, and so the peer closes
while that loop runs. deleteLater() issued from inside such a loop is not
deferred to the outer loop: Qt frees the object as soon as the nested loop
unwinds, which is still inside this readyRead emission, so Qt's own signal
dispatch and the handler lambda both keep using a freed socket. When the
notification happens to arrive late the bug hides, which is why this only
reproduced on some platforms.

Drop the auto-delete and let the queued teardown own the socket's lifetime:
it disconnects and then deletes, once the frame has unwound. The QPointer
guard was necessary but not sufficient -- it stopped us dereferencing a
dead pointer, not the socket being freed underneath the frame.

The test waits on the socket's destroyed signal instead of polling, so the
teardown assertion does not depend on platform event timing.

* [Client] Do not queue the single-instance socket teardown

The previous commit moved the deleteLater() off the disconnected signal
and into a queued invocation, believing that "queued" meant "after the
readyRead frame returns". It does not. A queued call is delivered by
whichever event loop is running, and the modal "Join game?" box runs a
nested one -- so the teardown was still executed inside the readyRead
emission, which is exactly the use-after-free it claimed to prevent.

Queueing was never necessary. Because nothing connects deleted/close to
the socket's disconnected signal any more, no nested loop can free it
while the handlers run, and emit filesReceived() is synchronous: once it
returns, the frame has unwound and the socket can be closed and deleted
inline. The QPointer stays, covering the one case that remains -- the
primary shutting down while the confirmation box is open, since
QLocalServer destroys its pending connections.

---------

Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>

.ci/compile.sh

@@ -352,6 +352,62 @@ if [[ $MAKE_PACKAGE ]]; then
         exit 1
       fi
       echo "Installer content is clean"
+
+      # Fail the build if the installer is missing a Qt runtime the applications
+      # link against. The DLLs reach the package by globbing every *.dll out of
+      # the build output directories, so a module the client links statically
+      # (Qt6Multimedia.dll, Qt6QuickWidgets.dll) can be left out without anything
+      # else noticing, and the only symptom is a client that refuses to start
+      # after an update. required-qt-runtime.txt is written by the top level
+      # CMakeLists.txt from the same module list the targets are built against.
+      if [[ ! -f required-qt-runtime.txt ]]; then
+        echo "::error file=$0::required-qt-runtime.txt not found in the build dir, cannot verify the Qt runtime"
+        exit 1
+      fi
+      # 7-Zip writes CRLF on Windows, so the \r has to come off before any
+      # whole-line comparison below. Normalize separators to /, and strip the
+      # install-root prefixes an NSIS listing may carry, so the entries can be
+      # compared as exact relative paths. -xF keeps a '.' in a DLL name from
+      # being a wildcard; -i because that is how the installed tree behaves -
+      # the uninstaller clears both $INSTDIR\plugins and $INSTDIR\Plugins.
+      # The archive side and the manifest side have to agree on line endings.
+      # CMake writes required-qt-runtime.txt with the platform's line ending and
+      # 7-Zip writes CRLF on Windows, so a trailing \r survives on $entry and
+      # defeats the whole-line match - every entry then reads as missing even
+      # though the listing holds it verbatim.
+      # [$] rather than \$ so the literal dollar is a bracket expression and the
+      # linter does not read these as unexpanded shell variables (SC2016).
+      installer_paths="$("$seven_zip" l -slt "$package" | tr -d '\r' |
+        sed -n 's/^Path = //p' |
+        sed -e 's|\\|/|g' -e 's|^[$]INSTDIR/||' -e 's|^[$]OUTDIR/||' -e 's|^[$]PLUGINSDIR/||' -e 's|^\./||' -e 's|^/||')"
+      manifest="$(tr -d '\r' <required-qt-runtime.txt)"
+      missing=""
+      checked=0
+      while IFS= read -r entry; do
+        if [[ -z $entry || $entry == \#* ]]; then
+          continue
+        fi
+        checked=$((checked + 1))
+        if ! grep -qxiF "$entry" <<<"$installer_paths"; then
+          missing+=" $entry"
+        fi
+      done <<<"$manifest"
+      if [[ -n $missing ]]; then
+        echo "::error file=$0::Installer is missing required Qt runtime files:$missing"
+        # Print both sides of the comparison. A mismatch here has twice been
+        # caused by the shape of 7-Zip's output rather than by a missing file,
+        # and a bare "missing" line cannot tell those apart.
+        echo "required-qt-runtime.txt asked for $checked path(s); 7-Zip listed $(grep -c '' <<<"$installer_paths") path(s)"
+        # Indent with parameter expansion rather than sed 's/^/  /', which the
+        # linter reports as SC2001.
+        echo "--- required ---"
+        echo "${manifest//$'\n'/$'\n  '}"
+        echo "--- first 40 paths 7-Zip lists in the installer ---"
+        listed_head="$(head -40 <<<"$installer_paths")"
+        echo "${listed_head//$'\n'/$'\n  '}"
+        exit 1
+      fi
+      echo "Installer contains the full Qt runtime ($checked files)"
     fi
     echo "::endgroup::"
   fi

.github/workflows/desktop-build.yml

@@ -458,6 +458,17 @@ jobs:
           VCPKG_FEATURE_FLAGS: dependencygraph
         run: .ci/compile.sh --server --test --vcpkg
 
+      # CPack reports a failed makensis run as "Problem running NSIS command"
+      # plus a path to NSISOutput.log, and that log lives only on the runner, so
+      # the actual compile error is otherwise nowhere in the workflow output.
+      # Dump it, or a broken installer script is undiagnosable from CI alone.
+      # shell: bash matters here - the default pwsh resolves `find` to Windows'
+      # text-search find.exe, which errors instead of searching.
+      - name: "Dump NSIS compile log"
+        if: failure() && matrix.os == 'Windows'
+        shell: bash
+        run: find . -name NSISOutput.log -exec cat {} +
+
       # Delete used cache to emulate a ccache update. See https://github.com/actions/cache/issues/342
       - name: "[macOS] Delete remote compiler cache (ccache)"
         if: matrix.os == 'macOS' && matrix.use_ccache == 1 && github.ref == 'refs/heads/master' && steps.restore_ccache.outputs.cache-hit

CMakeLists.txt

@@ -326,6 +326,39 @@ elseif(WIN32)
   # Configure file with custom definitions for NSIS.
   configure_file("${COCKATRICE_CMAKE_PATH}/NSIS.definitions.nsh.in" "${PROJECT_BINARY_DIR}/NSIS.definitions.nsh")
 
+  # Every Qt module the client links against, as the DLL names a Windows install
+  # has to contain, plus the platform plugin without which the client cannot
+  # start. .ci/compile.sh reads this and fails the build when the packaged
+  # installer is missing one of them.
+  #
+  # Only the client is packaged on Windows, so this is the client's module list
+  # rather than the union across server, client and Oracle: demanding Oracle's or
+  # Servatrice's modules of the client installer checks nothing about the client.
+  #
+  # A statically linked Qt module ships no runtime DLL, so requiring one would
+  # fail the package check on a static build for no reason. cockatrice/CMakeLists.txt
+  # installs the runtime of every module listed here, and skips static ones the
+  # same way, so the two can never disagree.
+  #
+  # A DLL missing from the package only surfaces on a user's machine, after an
+  # update has already replaced their install, as "The code execution cannot
+  # proceed because Qt6Multimedia.dll was not found" or as a missing procedure
+  # entry point in a Qt6QuickWidgets.dll left over from the previous version.
+  # Assert it while the installer is still being built instead.
+  set(_required_qt_runtime "")
+  foreach(_qt_module IN LISTS COCKATRICE_QT_MODULES)
+    get_target_property(_qt_module_type ${_qt_module} TYPE)
+    if(_qt_module_type STREQUAL "SHARED_LIBRARY")
+      string(REGEX REPLACE "^Qt6::" "" _qt_component "${_qt_module}")
+      list(APPEND _required_qt_runtime "Qt6${_qt_component}.dll")
+    endif()
+  endforeach()
+  list(APPEND _required_qt_runtime "Plugins/platforms/qwindows.dll")
+  list(REMOVE_DUPLICATES _required_qt_runtime)
+  list(SORT _required_qt_runtime)
+  string(REPLACE ";" "\n" _required_qt_runtime "${_required_qt_runtime}")
+  file(WRITE "${PROJECT_BINARY_DIR}/required-qt-runtime.txt" "${_required_qt_runtime}\n")
+
   # include vcredist into the package; NSIS will take care of running it
   if(VCREDISTRUNTIME_FOUND)
     install(FILES "${VCREDISTRUNTIME_FILE}" DESTINATION ./)

cmake/NSIS.template.in

@@ -15,6 +15,12 @@ Var ReinstallMode
 Var AllowForceClose
 Var GuardDir
 Var OldUninstallString
+Var StaleFindHandle
+Var StaleFindName
+Var IsUpgrade
+Var AbortReason
+Var WaitPid
+Var LauncherExited
 
 !include LogicLib.nsh
 !include FileFunc.nsh
@@ -88,6 +94,7 @@ ${IfNot} ${Errors}
       /PORTABLE : Install in portable mode$\n\
       /S : Silent install$\n\
       /R : Silent upgrade$\n\
+      /PID=%pid% : Process to wait for before replacing files (used by the in-app updater)$\n\
       /D=%directory% : Specify destination directory$\n"
     Quit
 ${EndIf}
@@ -115,6 +122,23 @@ ${Else}
     StrCpy $ReinstallMode 0
 ${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.
+; 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 ""
+StrCpy $LauncherExited 0
+ClearErrors
+${GetOptions} $9 "/PID=" $8
+${IfNot} ${Errors}
+    StrCpy $WaitPid $8
+${EndIf}
+
 ${If} $InstDir == ""
     ; User did not use /D to specify a directory,
     ; we need to set a default based on the install mode
@@ -134,17 +158,24 @@ ${EndIf}
 ; Now that $PortableMode reflects reality, commit InstDir into the correct slot
 Call SetModeDestinationFromInstdir
 
-; Make sure no 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
+; 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.
+;
+; 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.
 ; 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.
 ${If} $ReinstallMode = 1
+    Call WaitForLauncherToExit
     Call EnsureAppsNotRunning
 ${EndIf}
 
@@ -331,8 +362,13 @@ Function WaitForAppToClose
     Call IsAppRunning
     ; 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 - EnsureAppsNotRunning has warned about that case already.
-    ${If} $R0 != 1
+    ; 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}
 
@@ -351,7 +387,8 @@ Function WaitForAppToClose
             DetailPrint "$R1 closed."
             Return
         ${EndIf}
-        ${If} $R0 != 1
+        ${If} $R0 = 2
+            Call HandleUnknownAppState
             Return
         ${EndIf}
         System::Call 'kernel32::GetTickCount()i.s'
@@ -366,18 +403,9 @@ Function WaitForAppToClose
         ; update and preserves the user's data.
         ${If} $AllowForceClose = 0
             DetailPrint "$R1 is still running after 60 s; aborting the update so that no data is lost."
-            ; 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. A /S install without /R on the
-            ; other hand is unattended - by 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.
-            ${If} $ReinstallMode = 1
-                MessageBox MB_OK|MB_ICONEXCLAMATION \
-                    "$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."
-            ${EndIf}
-            SetErrorLevel 1
-            Abort
+            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
@@ -390,16 +418,14 @@ Function WaitForAppToClose
             DetailPrint "$R1 closed."
             Return
         ${EndIf}
-        ${If} $R0 != 1
+        ${If} $R0 = 2
+            Call HandleUnknownAppState
             Return
         ${EndIf}
         DetailPrint "$R1 is still running after 60 s; aborting the update."
-        ${If} $ReinstallMode = 1
-            MessageBox MB_OK|MB_ICONEXCLAMATION \
-                "$R1 could not be stopped, so the installation was aborted.$\r$\n$\r$\nPlease close $R1 and run the installer again."
-        ${EndIf}
-        SetErrorLevel 1
-        Abort
+        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 \
@@ -409,7 +435,8 @@ Function WaitForAppToClose
         ${If} $R0 = 0
             Return
         ${EndIf}
-        ${If} $R0 != 1
+        ${If} $R0 = 2
+            Call HandleUnknownAppState
             Return
         ${EndIf}
         Goto ck_wait_prompt
@@ -418,12 +445,102 @@ Function WaitForAppToClose
     ${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.
+;
+; 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
+        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."
+        Call AbortInstall
+    ${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}
+FunctionEnd
+
 Function EnsureAppsNotRunning
     ; guards the install directory
     StrCpy $GuardDir $INSTDIR
     Call GuardAppsNotRunning
 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.
+Function AbortInstall
+    ${IfNot} ${Silent}
+        MessageBox MB_OK|MB_ICONEXCLAMATION "$AbortReason"
+    ${EndIf}
+    SetErrorLevel 1
+    Abort
+FunctionEnd
+
+; Waits for the application that launched this installer to exit, using the /PID=
+; it passed. Sets $LauncherExited to 1 when that process is provably gone.
+;
+; 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
+; 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.
+;
+; 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.
+Function WaitForLauncherToExit
+    StrCpy $LauncherExited 0
+    ${If} $WaitPid == ""
+        Return
+    ${EndIf}
+    ; PROCESS_SYNCHRONIZE is the only access this needs - enough to wait on the
+    ; handle, and the least likely to be refused for a reason that has nothing to
+    ; do with the upgrade.
+    ClearErrors
+    System::Call 'kernel32::OpenProcess(i 0x00100000, i 0, i $WaitPid) p.r0'
+    ${If} ${Errors}
+    ; The launcher is gone - OpenProcess cannot open a dead process - which is
+    ; exactly the state the wait was for.
+        DetailPrint "The application that started this installer has already exited."
+        StrCpy $LauncherExited 1
+        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::CloseHandle(p r0)'
+    ${If} $R1 != 0
+        DetailPrint "The application that started this installer did not exit within 60 s."
+        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
+    ${EndIf}
+    DetailPrint "The application that started this installer has exited."
+    StrCpy $LauncherExited 1
+FunctionEnd
+
 Function EnsureOldInstallNotRunning
     ; Guards the directory of a *previous* installation, as recorded in the
     ; registry. Needed by componentsPagePre, which runs the old uninstaller
@@ -447,13 +564,32 @@ Function GuardAppsNotRunning
     Call IsProcessLookupAvailable
     ${If} $R0 = 0
         ; Without PowerShell we cannot tell whether a running instance is
-        ; holding locks on the files we are about to replace. Warn once and
-        ; continue: refusing to install would leave the user with an installer
-        ; that can never complete, which is worse than the locked-file failure
-        ; this check exists to prevent. MessageBox is shown even in a silent
-        ; install - the user has to be told to close the applications by hand.
+        ; holding locks on the files we are about to replace.
         DetailPrint "Could not start powershell.exe; skipping the running instance check. Close Cockatrice, Oracle and Servatrice manually if the installation reports a problem."
-        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."
+        ; 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.
+        ${IfNot} ${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"
@@ -494,27 +630,65 @@ Function IsExistingCockatriceInstall
     ; executables and uninstall.exe but silently skipped the DLLs its running
     ; instance had locked - which is exactly the broken install the purge is
     ; meant to heal, and without these markers none of the files above would be
-    ; there to prove that it is one.
-    ${If} ${FileExists} "$INSTDIR\Qt6Core.dll"
-        StrCpy $R0 1
-    ${EndIf}
+    ; there to prove that it is one. Every Qt module Cockatrice links is part of
+    ; the payload, so match them all: probing only Qt6Core.dll would miss a
+    ; directory where that one file happened to be deletable and the others
+    ; were not.
+    StrCpy $R3 "Qt6*.dll"
+    Call CheckStaleRuntimeMask
     ${If} ${FileExists} "$INSTDIR\qt.conf"
         StrCpy $R0 1
     ${EndIf}
 FunctionEnd
 
 ; Sets $R0 to 1 if a runtime file of a previous version is still present after
-; the purge. NSIS silently skips whatever it cannot delete, so one surviving DLL
-; is enough to prove that something still holds a lock on the install
-; directory. The purge works on wildcards, which the file existence checks do
-; not, so this looks for files that are certainly part of the payload.
+; the purge. NSIS silently skips whatever it cannot delete, so one surviving file
+; is enough to prove that something still holds a lock on the install directory.
+;
+; The masks below have to cover the purge above one for one. A check that only
+; probed named files could not see a locked Qt6Multimedia.dll or a Plugins
+; directory the purge was unable to remove, so the exact files that broke the
+; install were also the ones this check was blind to.
 Function HasStaleRuntimeFiles
     StrCpy $R0 0
-    ${If} ${FileExists} "$INSTDIR\Qt6Core.dll"
+    StrCpy $R3 "Qt*.dll"
+    Call CheckStaleRuntimeMask
+    StrCpy $R3 "libcrypto*.dll"
+    Call CheckStaleRuntimeMask
+    StrCpy $R3 "libssl*.dll"
+    Call CheckStaleRuntimeMask
+    StrCpy $R3 "zlib*.dll"
+    Call CheckStaleRuntimeMask
+    StrCpy $R3 "icu*.dll"
+    Call CheckStaleRuntimeMask
+    StrCpy $R3 "libeay32.dll"
+    Call CheckStaleRuntimeMask
+    StrCpy $R3 "ssleay32.dll"
+    Call CheckStaleRuntimeMask
+    ${If} $R0 = 1
+        Return
+    ${EndIf}
+    ${If} ${FileExists} "$INSTDIR\libmysql.dll"
+        StrCpy $R0 1
+        Return
+    ${EndIf}
+    ; IfFileExists takes wildcards, so a Plugins directory the purge could not
+    ; remove counts as a leftover too.
+    ${If} ${FileExists} "$INSTDIR\Plugins\*.*"
         StrCpy $R0 1
     ${EndIf}
-    ${If} ${FileExists} "$INSTDIR\Qt6Network.dll"
+FunctionEnd
+
+; usage: set $R3 to a file mask, call this. Sets $R0 to 1 when $INSTDIR holds a
+; matching file. Returns early once something was found, so $R0 stays 1.
+Function CheckStaleRuntimeMask
+    ${If} $R0 = 1
+        Return
+    ${EndIf}
+    FindFirst $StaleFindHandle $StaleFindName "$INSTDIR\$R3"
+    ${If} $StaleFindHandle != ""
         StrCpy $R0 1
+        FindClose $StaleFindHandle
     ${EndIf}
 FunctionEnd
 
@@ -839,6 +1013,12 @@ ${EndIf}
 ; Remove any runtime DLLs left over from older versions, so a mismatched
 ; Qt/OpenSSL set cannot survive next to the new executables.
 Call IsExistingCockatriceInstall
+; Remembered because the stale-runtime check below must only ever fire for a
+; directory this installer recognised as a Cockatrice install. Scanning for
+; leftovers in an unrelated directory (a first-time install into a folder that
+; happens to hold some Qt5 DLLs, say) would otherwise turn those files into an
+; abort reason even though the payload is about to overwrite nothing of theirs.
+StrCpy $IsUpgrade $R0
 ${If} $PortableMode = 0
 ${AndIf} $R0 = 1
 	RMDir /r "$INSTDIR\Plugins"
@@ -857,12 +1037,28 @@ ${EndIf}
 ; performed at all - is still on disk, and the new executables are about to be
 ; written next to it. That mix is exactly what makes the next start fail with
 ; "The procedure entry point X could not be located in the dynamic link library
-; ...Qt6Network.dll", so say so instead of leaving it to the user to find out.
-Call HasStaleRuntimeFiles
+; ...Qt6QuickWidgets.dll" or "The code execution cannot proceed because
+; Qt6Multimedia.dll was not found", so stop here instead of compounding it.
+; Nothing of the new payload has been written at this point, so the next run -
+; with the applications closed - gets a clean install.
+${If} $IsUpgrade = 1
+    Call HasStaleRuntimeFiles
+${Else}
+    StrCpy $R0 0
+${EndIf}
 ${If} $R0 = 1
-    DetailPrint "Files of a previous version are still present; the installation may be incomplete."
-    MessageBox MB_OK|MB_ICONEXCLAMATION \
-        "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.$\r$\n$\r$\nPlease close Cockatrice, Oracle and Servatrice and run the installer again."
+    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.
+    ${IfNot} ${Silent}
+        MessageBox MB_OK|MB_ICONEXCLAMATION \
+            "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."
+    ${EndIf}
+    SetErrorLevel 1
+    Abort
 ${EndIf}
 
 @CPACK_NSIS_EXTRA_PREINSTALL_COMMANDS@

cockatrice/CMakeLists.txt

@@ -683,6 +683,24 @@ if(WIN32)
     PATTERN ".lupdate*" EXCLUDE
   )
 
+  # fixup_bundle further down copies only the DLLs it can find in Cockatrice.exe's
+  # import table, and the glob above only sees DLLs that were already copied next
+  # to the executable. Qt6::Concurrent is linked but never imported - QtConcurrent::run
+  # and QFutureWatcher are header-only in Qt 6 - so neither step ships
+  # Qt6Concurrent.dll, even though required-qt-runtime.txt lists it and the
+  # Windows package check then fails on a package that is fine to run.
+  # Qt6::ShaderTools is skipped for the same reason.
+  #
+  # Install the runtime of every linked module explicitly. An unused DLL costs a
+  # few hundred kilobytes; a missing one is what stops the client starting after
+  # an update, and it stays invisible until then.
+  foreach(_qt_module IN LISTS COCKATRICE_QT_MODULES)
+    get_target_property(_qt_module_type ${_qt_module} TYPE)
+    if(_qt_module_type STREQUAL "SHARED_LIBRARY")
+      install(FILES "$<TARGET_FILE:${_qt_module}>" DESTINATION ./)
+    endif()
+  endforeach()
+
   install(
     DIRECTORY "${CMAKE_BINARY_DIR}/cockatrice/"
     DESTINATION ./
@@ -729,11 +747,15 @@ if(WIN32)
     PATTERN "tls/qschannelbackend.dll"
   )
 
+  # The Windows install has no Resources directory: the .qm files go to
+  # ${COCKATRICE_WIN32_QM_INSTALL_DIR} and main.cpp looks for them at
+  # applicationDirPath() + "/translations". The macOS bundle above keeps
+  # Resources/translations, which is correct for it.
   install(
     CODE "
         file(WRITE \"\${CMAKE_INSTALL_PREFIX}/${qtconf_dest_dir}/qt.conf\" \"[Paths]
 Plugins = Plugins
-Translations = Resources/translations
+Translations = translations
 Data = Resources\")
     "
     COMPONENT Runtime

cockatrice/src/game/game_state.cpp

@@ -29,13 +29,17 @@ void GameState::incrementGameTime()
 void GameState::setGameTime(int _secondsElapsed)
 {
     secondsElapsed = _secondsElapsed;
+    emit updateTimeElapsedLabel(formatElapsedTime(_secondsElapsed));
+}
+
+QString GameState::formatElapsedTime(int _secondsElapsed)
+{
     int seconds = _secondsElapsed;
     int minutes = seconds / 60;
     seconds -= minutes * 60;
     int hours = minutes / 60;
     minutes -= hours * 60;
 
-    emit updateTimeElapsedLabel(QString::number(hours).rightJustified(2, '0') + ":" +
-                                QString::number(minutes).rightJustified(2, '0') + ":" +
-                                QString::number(seconds).rightJustified(2, '0'));
-}
+    return QString::number(hours).rightJustified(2, '0') + ":" + QString::number(minutes).rightJustified(2, '0') + ":" +
+           QString::number(seconds).rightJustified(2, '0');
+}

cockatrice/src/game/game_state.h

@@ -110,6 +110,11 @@ public:
         return hostId;
     }
 
+    int getSecondsElapsed() const
+    {
+        return secondsElapsed;
+    }
+
 signals:
     void updateTimeElapsedLabel(QString newTime);
     void gameStarted(bool resuming);
@@ -121,6 +126,9 @@ public slots:
     void incrementGameTime();
     void setGameTime(int _secondsElapsed);
 
+public:
+    static QString formatElapsedTime(int _secondsElapsed);
+
 private:
     QTimer *gameTimer;
     int secondsElapsed;

cockatrice/src/game/zones/view_zone_logic.cpp

@@ -154,6 +154,14 @@ void ZoneViewZoneLogic::removeCard(int position, bool toNewZone)
     reorganizeCards();
 }
 
+void ZoneViewZoneLogic::clearCards()
+{
+    for (auto card : cards) {
+        card->deleteLater();
+    }
+    cards.clear();
+}
+
 void ZoneViewZoneLogic::setWriteableRevealZone(bool _writeableRevealZone)
 {
 

cockatrice/src/game/zones/view_zone_logic.h

@@ -41,6 +41,10 @@ public:
     bool prepareAddCard(int x);
     void removeCard(int position, bool toNewZone);
     void updateCardIds(CardAction action);
+
+    /** @brief Removes all cards from the view, without touching anything else. Used in replay rewind */
+    void clearCards();
+
     int getNumberCards() const
     {
         return numberCards;

cockatrice/src/game_graphics/game_scene.cpp

@@ -735,6 +735,16 @@ void GameScene::clearViews()
     }
 }
 
+void GameScene::emptyActiveViews()
+{
+    for (auto zoneView : zoneViews) {
+        auto logic = qobject_cast<ZoneViewZoneLogic *>(zoneView->getZone()->getLogic());
+        if (!logic->getRevealZone()) {
+            logic->clearCards();
+        }
+    }
+}
+
 /**
  * @brief Closes the most recently added zone view.
  */

cockatrice/src/game_graphics/game_scene.h

@@ -227,6 +227,9 @@ public slots:
     /** @brief Closes all zone views. */
     void clearViews();
 
+    /** @brief Empties the contents of all non-reveal zone views. Used for replay rewinding */
+    void emptyActiveViews();
+
     /** @brief Closes the most recently added zone view. */
     void closeMostRecentZoneView();
     QTransform getViewTransform() const;

cockatrice/src/game_graphics/log/message_log_widget.cpp

@@ -1,7 +1,9 @@
 #include "message_log_widget.h"
 
+#include "../../client/settings/cache_settings.h"
 #include "../../client/settings/card_counter_settings.h"
 #include "../../client/sound_engine.h"
+#include "../../game/game_state.h"
 #include "../../game/phase.h"
 #include "../../game/player/player_logic.h"
 #include "../../interface/widgets/tabs/tab_game.h"
@@ -10,6 +12,7 @@
 
 #include <libcockatrice/protocol/pb/context_move_card.pb.h>
 #include <libcockatrice/protocol/pb/context_mulligan.pb.h>
+#include <libcockatrice/settings/chat_settings.h>
 #include <libcockatrice/utility/zone_names.h>
 #include <utility>
 
@@ -626,13 +629,12 @@ void MessageLogWidget::logSetActivePhase(int phaseNumber)
 
     soundEngine->playSound(phase.soundFileName);
 
-    appendHtml("<font color=\"" + phase.color + "\"><b>" + QDateTime::currentDateTime().toString("[hh:mm:ss] ") +
-               phase.getName() + "</b></font>");
+    appendHtml("<font color=\"" + phase.color + "\"><b>" + getCurrentTime() + phase.getName() + "</b></font>");
 }
 
 void MessageLogWidget::logSetActivePlayer(PlayerLogic *player)
 {
-    appendHtml("<br><font color=\"green\"><b>" + QDateTime::currentDateTime().toString("[hh:mm:ss] ") +
+    appendHtml("<br><font color=\"green\"><b>" + getCurrentTime() +
                QString(tr("%1's turn.")).arg(player->getPlayerInfo()->getName()) + "</b></font><br>");
 }
 
@@ -826,6 +828,14 @@ void MessageLogWidget::appendHtmlServerMessage(const QString &html, bool optiona
     ChatView::appendHtmlServerMessage(messagePrefix + html + messageSuffix, optionalIsBold, optionalFontColor);
 }
 
+QString MessageLogWidget::getCurrentTime() const
+{
+    if (SettingsCache::instance().chat().getUseGameTime()) {
+        return "[" + GameState::formatElapsedTime(game->getGameState()->getSecondsElapsed()) + "] ";
+    }
+    return ChatView::getCurrentTime();
+}
+
 void MessageLogWidget::connectToPlayerEventHandler(PlayerEventHandler *playerEventHandler)
 {
     connect(playerEventHandler, &PlayerEventHandler::logSay, this, &MessageLogWidget::logSay);

cockatrice/src/game_graphics/log/message_log_widget.h

@@ -104,6 +104,9 @@ public slots:
     void appendHtmlServerMessage(const QString &html,
                                  bool optionalIsBold = false,
                                  QString optionalFontColor = QString()) override;
+
+private:
+    [[nodiscard]] QString getCurrentTime() const override;
 };
 
 #endif

cockatrice/src/interface/widgets/dialogs/dlg_update.cpp

@@ -265,8 +265,16 @@ void DlgUpdate::downloadSuccessful(const QUrl &filepath)
     // NSIS needs the /D= argument to be an UNQUOTED string, even if it contains spaces. Qt likes to quote arguments if
     // they contain spaces, so we use the windows exclusive QProcess::setNativeArguments in the only case where this is
     // relevant, which preserves the argument unquoted.
+    //
+    // /PID= tells the installer which process to wait for before it replaces anything. We launch the installer first
+    // and only quit afterwards, so it starts while this process is still alive and still has every Qt runtime DLL
+    // mapped, and a mapped DLL cannot be deleted. Without the handle the installer can only poll for an image name and
+    // guess, and a wrong guess is what leaves the update writing new executables next to runtime files of the previous
+    // version, which fails on the next start with "The procedure entry point ... could not be located in the dynamic
+    // link library
+    // ...Qt6QuickWidgets.dll". /D= has to stay the last parameter, which is why it is last here too.
 #ifdef Q_OS_WIN
-    process.setNativeArguments(QString("/R /D=%1").arg(appDir));
+    process.setNativeArguments(QString("/R /PID=%1 /D=%2").arg(QCoreApplication::applicationPid()).arg(appDir));
 #else
     // Linux/macOS: normal argument passing (not relevant since they update differently.)
     process.setArguments({"/R", QString("/D=%1").arg(appDir)});

cockatrice/src/interface/widgets/server/chat_view/chat_view.cpp

@@ -167,7 +167,7 @@ void ChatView::appendHtmlServerMessage(const QString &html, bool optionalIsBold,
 
     QString htmlText =
         "<font color=" + ((optionalFontColor.size() > 0) ? optionalFontColor : serverMessageColor.name()) + ">" +
-        QDateTime::currentDateTime().toString("[hh:mm:ss] ") + html + "</font>";
+        getCurrentTime() + html + "</font>";
 
     if (optionalIsBold) {
         htmlText = "<b>" + htmlText + "</b>";
@@ -179,6 +179,11 @@ void ChatView::appendHtmlServerMessage(const QString &html, bool optionalIsBold,
     }
 }
 
+QString ChatView::getCurrentTime() const
+{
+    return QDateTime::currentDateTime().toString("[hh:mm:ss] ");
+}
+
 void ChatView::appendCardTag(QTextCursor &cursor, const QString &cardName)
 {
     QTextCharFormat oldFormat = cursor.charFormat();
@@ -290,7 +295,7 @@ void ChatView::appendMessage(QString message,
         timeFormat.setForeground(serverMessageColor);
         timeFormat.setFontWeight(QFont::Bold);
         cursor.setCharFormat(timeFormat);
-        cursor.insertText(QDateTime::currentDateTime().toString("[hh:mm:ss] "));
+        cursor.insertText(getCurrentTime());
     }
 
     // nickname

cockatrice/src/interface/widgets/server/chat_view/chat_view.h

@@ -107,9 +107,8 @@ public:
     ChatView(TabSupervisor *_tabSupervisor, AbstractGame *_game, bool _showTimestamps, QWidget *parent = nullptr);
     void retranslateUi();
     void appendHtml(const QString &html);
-    void virtual appendHtmlServerMessage(const QString &html,
-                                         bool optionalIsBold = false,
-                                         QString optionalFontColor = QString());
+    virtual void
+    appendHtmlServerMessage(const QString &html, bool optionalIsBold = false, QString optionalFontColor = QString());
     void appendMessage(QString message,
                        RoomMessageTypeFlags messageType = {},
                        const ServerInfo_User &userInfo = {},
@@ -119,6 +118,7 @@ public:
     QString getRecentChatLog(int maxMessages = 50) const;
 
 protected:
+    [[nodiscard]] virtual QString getCurrentTime() const;
     void enterEvent(QEnterEvent *event) override;
     void leaveEvent(QEvent *event) override;
     void mouseMoveEvent(QMouseEvent *event) override;

cockatrice/src/interface/widgets/server/user/user_info_popup.cpp

@@ -634,9 +634,10 @@ void UserInfoPopup::onGamesContextMenu(const QPoint &pos)
                        "QMenu::item:selected{background:%4;}")
             .arg(colorStr(theme.bg), colorStr(theme.text), colorStr(theme.border), colorStr(theme.buttonHover)));
 
-    const bool canJoin = !game.started() && game.player_count() < game.max_players();
+    // Never grey the join action out: GameSelector::joinGame() asks whether to
+    // fall back to spectating when the game is full or already running, so a
+    // disabled entry would just read as a dead button.
     QAction *join = menu.addAction(tr("Join game"));
-    join->setEnabled(canJoin);
 
     QAction *spec = nullptr;
     if (game.spectators_allowed()) {

cockatrice/src/interface/widgets/server/user/user_list_widget.cpp

@@ -430,9 +430,6 @@ UserListWidget::UserListWidget(TabSupervisor *_tabSupervisor,
             hidePopup(true);
             requestVisibleItemResources();
         });
-
-        // Forward join requests from popup upward
-        connect(userInfoPopup, &UserInfoPopup::joinGameRequested, this, &UserListWidget::joinGameRequested);
     } else {
         // Dialog mode: keyboard selection drives the Invite button.
         connect(userTree, &QTreeWidget::currentItemChanged, this, [this](QTreeWidgetItem *current, QTreeWidgetItem *) {
@@ -692,6 +689,17 @@ void UserListWidget::connectPopupSignals()
             [this](const QString &n) { userContextMenu->execAdjustJudge(n, true); });
     connect(userInfoPopup, &UserInfoPopup::demoteFromJudgeRequested, this,
             [this](const QString &n) { userContextMenu->execAdjustJudge(n, false); });
+
+    // Joining is a tab-level action, not a per-user one: the popup's games list
+    // spans every room the user is in, so the target room may not be open yet.
+    // Hand it to the supervisor, which owns the intent that joins the room if
+    // needed. The popup closes here because that intent switches tabs, which
+    // would otherwise strand it on a list that is no longer on screen.
+    connect(userInfoPopup, &UserInfoPopup::joinGameRequested, this,
+            [this](const int gameId, const int roomId, const bool asSpectator) {
+                hidePopup(true);
+                tabSupervisor->joinGameFromUserCard(gameId, roomId, asSpectator);
+            });
 }
 
 bool UserListWidget::eventFilter(QObject *obj, QEvent *event)

cockatrice/src/interface/widgets/server/user/user_list_widget.h

@@ -172,7 +172,6 @@ signals:
     void removeBuddy(const QString &userName);
     void addIgnore(const QString &userName);
     void removeIgnore(const QString &userName);
-    void joinGameRequested(int gameId, int roomId, bool asSpectator);
     void sectionExpanded(Section section, bool expanded);
     /** Dialog mode: the user activated (Enter/double-click) the given row. */
     void userActivated(const QString &userName);

cockatrice/src/interface/widgets/settings_page/messages_settings_page.cpp

@@ -64,6 +64,10 @@ MessagesSettingsPage::MessagesSettingsPage()
     connect(&ignoreAllPrivateMessagesCheckBox, &QCheckBox::QT_STATE_CHANGED, &SettingsCache::instance().chat(),
             &ChatSettings::setIgnoreAllPrivateMessages);
 
+    useGameTimeCheckBox.setChecked(SettingsCache::instance().chat().getUseGameTime());
+    connect(&useGameTimeCheckBox, &QCheckBox::QT_STATE_CHANGED, &SettingsCache::instance().chat(),
+            &ChatSettings::setUseGameTime);
+
     customAlertString = new QLineEdit();
     customAlertString->setText(SettingsCache::instance().chat().getHighlightWords());
     connect(customAlertString, &QLineEdit::textChanged, &SettingsCache::instance().chat(),
@@ -82,6 +86,7 @@ MessagesSettingsPage::MessagesSettingsPage()
     chatGrid->addWidget(&mentionPopups, 6, 0);
     chatGrid->addWidget(&roomHistory, 7, 0);
     chatGrid->addWidget(&ignoreAllPrivateMessagesCheckBox, 8, 0);
+    chatGrid->addWidget(&useGameTimeCheckBox, 9, 0);
     chatGroupBox = new QGroupBox;
     chatGroupBox->setLayout(chatGrid);
 
@@ -263,6 +268,7 @@ void MessagesSettingsPage::retranslateUi()
     mentionPopups.setText(tr("Enable desktop notification for mentions"));
     roomHistory.setText(tr("Enable room message history on join"));
     ignoreAllPrivateMessagesCheckBox.setText(tr("Ignore all private messages"));
+    useGameTimeCheckBox.setText(tr("Use game time instead of local time in game logs"));
     hexLabel.setText(tr("(Color is hexadecimal)"));
     hexHighlightLabel.setText(tr("(Color is hexadecimal)"));
     customAlertStringLabel.setText(tr("Separate words with a space, alphanumeric characters only"));

cockatrice/src/interface/widgets/settings_page/messages_settings_page.h

@@ -41,6 +41,7 @@ private:
     QCheckBox mentionPopups;
     QCheckBox roomHistory;
     QCheckBox ignoreAllPrivateMessagesCheckBox;
+    QCheckBox useGameTimeCheckBox;
     QGroupBox *chatGroupBox;
     QGroupBox *highlightGroupBox;
     QGroupBox *messageGroupBox;

cockatrice/src/interface/widgets/tabs/tab_game.cpp

@@ -259,7 +259,7 @@ void TabGame::linkCardToChat(const QString &cardName)
     sayEdit->setFocus();
 }
 
-void TabGame::resetChatAndPhase()
+void TabGame::resetForRewind()
 {
     // reset chat log
     messageLog->clearChat();
@@ -267,6 +267,9 @@ void TabGame::resetChatAndPhase()
     // reset phase markers
     game->getGameState()->setCurrentPhase(-1);
 
+    // reset view zones
+    static_cast<GameScene *>(gameView->scene())->emptyActiveViews();
+
     // reset spectator state so the replay can rebuild it from the start
     game->getPlayerManager()->clearSpectators();
     playerListWidget->clearSpectators();
@@ -1270,7 +1273,7 @@ void TabGame::createReplayDock(GameReplay *replay)
     replayDock->setWidget(replayWidget);
     replayDock->setFloating(false);
 
-    connect(replayWidget, &ReplayWidget::rewound, this, &TabGame::resetChatAndPhase);
+    connect(replayWidget, &ReplayWidget::rewound, this, &TabGame::resetForRewind);
     connect(replayWidget, &ReplayWidget::eventReplayed, game->getGameEventHandler(),
             [this](const auto &event, auto options) {
                 game->getGameEventHandler()->processGameEventContainer(event, nullptr, options);

cockatrice/src/interface/widgets/tabs/tab_game.h

@@ -204,7 +204,7 @@ public:
 
 public slots:
     void viewCardInfo(const CardRef &cardRef = {}) const;
-    void resetChatAndPhase();
+    void resetForRewind();
     void updateTimeElapsedLabel(QString newTime);
     void addPlayerToAutoCompleteList(QString playerName);
     void removePlayerFromAutoCompleteList(QString playerName);

cockatrice/src/interface/widgets/tabs/tab_supervisor.cpp

@@ -1100,10 +1100,27 @@ void TabSupervisor::replayLeft(TabGame *tab)
 }
 
 void TabSupervisor::joinReportGame(const int gameId, const int roomId)
+{
+    joinGameByIntent(gameId, roomId, true);
+}
+
+void TabSupervisor::joinGameFromUserCard(const int gameId, const int roomId, const bool asSpectator)
+{
+    joinGameByIntent(gameId, roomId, asSpectator);
+}
+
+/**
+ * Runs the join/spectate intent for @p gameId in @p roomId.
+ *
+ * The intent owns the whole flow: if the room tab is not open yet it joins the
+ * room first, then retries the game, so callers do not have to care whether the
+ * room happens to be on screen.
+ */
+void TabSupervisor::joinGameByIntent(const int gameId, const int roomId, const bool asSpectator)
 {
     auto *remoteClient = qobject_cast<RemoteClient *>(client);
     if (!remoteClient) {
-        actShowPopup(tr("Report joins are only available on a remote server."));
+        actShowPopup(tr("Game joins are only available on a remote server."));
         return;
     }
 
@@ -1112,7 +1129,7 @@ void TabSupervisor::joinReportGame(const int gameId, const int roomId)
     ctx->roomContext.serverContext.port = QString::number(remoteClient->peerPort());
     ctx->roomContext.roomId = roomId;
     ctx->gameId = gameId;
-    ctx->asSpectator = true;
+    ctx->asSpectator = asSpectator;
 
     auto *joinGameIntent = new IntentJoinServerGame(this, remoteClient, std::move(ctx));
     joinGameIntent->setParent(this);

cockatrice/src/interface/widgets/tabs/tab_supervisor.h

@@ -128,6 +128,8 @@ private:
     static QString sanitizeTabName(QString dirty);
     static QString sanitizeHtml(QString dirty);
     void resetTabsMenu();
+    /** Shared join/spectate driver behind the report tab and the profile hover card. */
+    void joinGameByIntent(int gameId, int roomId, bool asSpectator);
 
 public:
     explicit TabSupervisor(AbstractClient *_client, QMenu *tabsMenu, QWidget *parent = nullptr);
@@ -198,6 +200,8 @@ public slots:
     TabEdhRec *addEdhrecTab(const CardInfoPtr &cardToQuery, bool isCommander = false);
     void openReplay(GameReplay *replay);
     void joinReportGame(int gameId, int roomId);
+    /** Joins or spectates a game listed on a user's profile hover card. */
+    void joinGameFromUserCard(int gameId, int roomId, bool asSpectator);
     void openTabModeration(const QString &userName = {});
     void switchToFirstAvailableNetworkTab();
     void maximizeMainWindow();

cockatrice/src/single_instance_manager.cpp

@@ -1,6 +1,7 @@
 #include "single_instance_manager.h"
 
 #include <QDir>
+#include <QPointer>
 
 namespace
 {
@@ -155,19 +156,44 @@ void SingleInstanceManager::handleNewConnection()
             socket->write(ACK_MESSAGE);
             socket->flush();
 
-            // Drop the payload from the buffer before handling it: the handlers
-            // run synchronously and can spin a nested event loop (e.g. a modal
-            // dialog) that re-reads this socket, which would re-parse and re-emit
-            // the same files.
+            // Drop the payload from the buffer before handing it off, so a
+            // re-entrant event loop cannot re-parse and re-emit the same files.
             buffer->clear();
             *expectedSize = 0;
 
+            // Take the guard while the socket is still alive: QPointer has to be
+            // built from a live pointer, and by the time the handlers below return
+            // the socket may already be gone.
+            QPointer<QLocalSocket> guardedSocket(socket);
+
+            // Hand the payload over synchronously, exactly as before, so the
+            // "Join game?" confirmation still appears. A handler can spin a nested
+            // event loop (that modal box is one), and the sending instance exits as
+            // soon as it has the ACK above, so while that loop runs the peer closes
+            // and this socket is disconnected.
             emit filesReceived(files);
 
-            socket->disconnectFromServer();
+            // The handlers run synchronously, so returning from the emit means the
+            // readyRead frame has unwound and it is safe to touch the socket again.
+            // This teardown must therefore NOT be queued: a queued call would be
+            // picked up by the nested loop above while the frame is still on the
+            // stack, which is precisely the use-after-free this guards against.
+            // The guard covers the one remaining way the socket can die under us --
+            // the primary being torn down (e.g. quit) while the dialog was open --
+            // since QLocalServer owns and destroys its pending connections.
+            if (guardedSocket) {
+                guardedSocket->disconnectFromServer();
+                guardedSocket->deleteLater();
+            }
             return;
         }
     });
 
-    connect(socket, &QLocalSocket::disconnected, socket, &QLocalSocket::deleteLater);
+    // Deliberately no `disconnected -> deleteLater()` here. The handlers above run
+    // synchronously and may spin a nested event loop, during which the sender exits
+    // and this socket is disconnected. A deleteLater() triggered from there is not
+    // deferred to the outer loop: Qt frees the object as soon as that nested loop
+    // unwinds, which is still inside this readyRead emission, so Qt's own signal
+    // dispatch is left holding a freed socket. The socket is instead closed and
+    // deleted once the handlers have returned, on the code path above.
 }

libcockatrice_interfaces/libcockatrice/interfaces/interface_chat_settings_provider.h

@@ -21,6 +21,7 @@ public:
     [[nodiscard]] virtual bool getShowMentionPopup() const = 0;
     [[nodiscard]] virtual bool getRoomHistory() const = 0;
     [[nodiscard]] virtual bool getIgnoreAllPrivateMessages() const = 0;
+    [[nodiscard]] virtual bool getUseGameTime() const = 0;
     [[nodiscard]] virtual QString getHighlightWords() const = 0;
 };
 

libcockatrice_settings/libcockatrice/settings/chat_settings.cpp

@@ -70,6 +70,11 @@ bool ChatSettings::getIgnoreAllPrivateMessages() const
     return getValue("ignoreAllPrivateMessages", QString(), QString(), false).toBool();
 }
 
+bool ChatSettings::getUseGameTime() const
+{
+    return getValue("useGameTime", QString(), QString(), false).toBool();
+}
+
 QString ChatSettings::getHighlightWords() const
 {
     return getValue("highlightWords").toString();
@@ -141,6 +146,11 @@ void ChatSettings::setIgnoreAllPrivateMessages(bool _ignoreAllPrivateMessages)
     setValue(_ignoreAllPrivateMessages, "ignoreAllPrivateMessages");
 }
 
+void ChatSettings::setUseGameTime(bool _useGameTime)
+{
+    setValue(_useGameTime, "useGameTime");
+}
+
 void ChatSettings::setHighlightWords(const QString &_highlightWords)
 {
     setValue(_highlightWords, "highlightWords");

libcockatrice_settings/libcockatrice/settings/chat_settings.h

@@ -24,6 +24,7 @@ public:
     [[nodiscard]] bool getShowMentionPopup() const override;
     [[nodiscard]] bool getRoomHistory() const override;
     [[nodiscard]] bool getIgnoreAllPrivateMessages() const override;
+    [[nodiscard]] bool getUseGameTime() const override;
     [[nodiscard]] QString getHighlightWords() const override;
 
     void setChatMention(bool _chatMention);
@@ -39,6 +40,7 @@ public:
     void setShowMentionPopups(bool _showMentionPopups);
     void setRoomHistory(bool _roomHistory);
     void setIgnoreAllPrivateMessages(bool _ignoreAllPrivateMessages);
+    void setUseGameTime(bool _useGameTime);
     void setHighlightWords(const QString &_highlightWords);
 
 signals:

tests/CMakeLists.txt

@@ -55,6 +55,17 @@ add_executable(
 )
 target_include_directories(loader_local_matching_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src)
 
+# Guard must match the condition for src/single_instance_manager.cpp in cockatrice/CMakeLists.txt.
+if(WITH_CLIENT)
+  add_executable(
+    single_instance_manager_test ${CMAKE_SOURCE_DIR}/cockatrice/src/single_instance_manager.cpp
+                                 single_instance_manager_test.cpp
+  )
+  target_include_directories(single_instance_manager_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src)
+
+  add_test(NAME single_instance_manager_test COMMAND single_instance_manager_test)
+endif()
+
 find_package(GTest)
 
 if(NOT GTEST_FOUND)
@@ -98,6 +109,7 @@ if(NOT GTEST_FOUND)
   add_dependencies(latency_tracker_test gtest)
   add_dependencies(metrics_registry_test gtest)
   add_dependencies(loader_local_matching_test gtest)
+  add_dependencies(single_instance_manager_test gtest)
 endif()
 
 include_directories(${GTEST_INCLUDE_DIRS})
@@ -153,6 +165,10 @@ target_link_libraries(
   loader_local_matching_test libcockatrice_settings Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES}
 )
 
+if(WITH_CLIENT)
+  target_link_libraries(single_instance_manager_test Threads::Threads ${GTEST_BOTH_LIBRARIES} ${TEST_QT_MODULES})
+endif()
+
 add_subdirectory(card_zone_algorithms)
 add_subdirectory(carddatabase)
 add_subdirectory(deck_list_model)

tests/settings/settings_defaults_test.cpp

@@ -306,6 +306,12 @@ TEST_F(SettingsDefaultsTest, Chat_IgnoreAllPrivateMessages_Default)
     ASSERT_EQ(s.getIgnoreAllPrivateMessages(), false);
 }
 
+TEST_F(SettingsDefaultsTest, Chat_UseGameTime_Default)
+{
+    ChatSettings s(settingsPath, nullptr);
+    ASSERT_EQ(s.getUseGameTime(), false);
+}
+
 // --- PersonalSettings ---
 
 TEST_F(SettingsDefaultsTest, Personal_Lang_Default)

tests/single_instance_manager_test.cpp

@@ -0,0 +1,134 @@
+#include "single_instance_manager.h"
+
+#include <QCoreApplication>
+#include <QDataStream>
+#include <QEventLoop>
+#include <QLocalServer>
+#include <QLocalSocket>
+#include <QTimer>
+#include <gtest/gtest.h>
+
+namespace
+{
+constexpr int HANDSHAKE_TIMEOUT_MS = 5000;
+constexpr int MODAL_DIALOG_MS = 50;
+
+const QStringList PAYLOAD{QStringLiteral("cockatrice://joingame?hostname=127.0.0.1&port=4747&roomid=0&gameid=1")};
+
+/// Serializes a payload the way SingleInstanceManager::forwardToPrimary() does.
+QByteArray encodePayload(const QStringList &files)
+{
+    QByteArray payload;
+    QDataStream out(&payload, QIODevice::WriteOnly);
+    out << files;
+
+    QByteArray message;
+    QDataStream msgStream(&message, QIODevice::WriteOnly);
+    msgStream << static_cast<quint32>(payload.size());
+    message.append(payload);
+    return message;
+}
+} // namespace
+
+// Regression test for the use-after-free in SingleInstanceManager::handleNewConnection().
+//
+// The forwarded payload is handed to the handlers from inside the forwarding
+// socket's readyRead emission, which is what lets the "Join game?" confirmation
+// appear at all. A handler can spin a nested event loop (a modal QMessageBox),
+// and the sending instance exits as soon as it has the acknowledgment, so the
+// peer closes *while that loop runs*. deleteLater() called from inside such a
+// nested loop does not wait for the outer loop: Qt frees the object as soon as
+// the nested loop unwinds, still inside the readyRead emission, so Qt's own
+// signal dispatch and the handler lambda both end up holding a freed socket.
+// Nothing may therefore delete the socket from the disconnected signal, and the
+// teardown is queued until that frame has unwound.
+TEST(SingleInstanceManagerTest, HandlersRunSynchronouslyAndSocketIsNotTouchedAfterTheyReturn)
+{
+    // Scope the hand-off socket to this test process so a Cockatrice the
+    // developer happens to be running cannot claim it.
+    qputenv("USER", QByteArrayLiteral("cockatrice-test-") + QByteArray::number(QCoreApplication::applicationPid()));
+
+    SingleInstanceManager primary;
+    ASSERT_TRUE(primary.tryRun(QStringList())) << "could not become the primary instance";
+
+    auto *server = primary.findChild<QLocalServer *>();
+    ASSERT_NE(nullptr, server) << "the primary instance is not listening";
+
+    QLocalSocket sender;
+    QStringList handled;
+    QByteArray acknowledgment;
+    int handlerCalls = 0;
+    bool socketConnectedWhenHandled = false;
+    bool socketReleased = false;
+
+    // Waited on after the handlers return: the deferred teardown deletes the
+    // forwarding socket once the readyRead frame has unwound.
+    QEventLoop teardownLoop;
+
+    QEventLoop loop;
+    QObject::connect(&primary, &SingleInstanceManager::filesReceived, &primary, [&](const QStringList &files) {
+        ++handlerCalls;
+
+        // The payload must reach the handlers while the forwarding socket is
+        // still connected: deferring the emit would suppress the modal "Join
+        // game?" confirmation this hand-off exists to show.
+        auto *connection = server->findChild<QLocalSocket *>();
+        socketConnectedWhenHandled = connection != nullptr && connection->state() == QLocalSocket::ConnectedState;
+
+        // Observe the socket's destruction rather than polling for it. `destroyed`
+        // is emitted just before the object goes away, so this stays correct
+        // however the teardown ends up deleting it, and `&primary` keeps the
+        // observer alive even once the socket itself is gone.
+        if (connection != nullptr) {
+            QObject::connect(connection, &QObject::destroyed, &primary, [&] {
+                socketReleased = true;
+                teardownLoop.quit();
+            });
+        }
+
+        // The primary acknowledges the payload before handling it, so that a
+        // sender waiting on the acknowledgment never mistakes a busy primary
+        // for a dead one.
+        sender.waitForReadyRead(HANDSHAKE_TIMEOUT_MS);
+        acknowledgment = sender.readAll();
+
+        // Stand in for the modal "Join game?" confirmation: the sending instance
+        // goes away while this nested loop is running, which is what used to
+        // destroy the socket underneath this readyRead frame.
+        sender.abort();
+
+        QEventLoop modalDialog;
+        QTimer::singleShot(MODAL_DIALOG_MS, &modalDialog, &QEventLoop::quit);
+        modalDialog.exec();
+
+        handled = files;
+        loop.quit();
+    });
+
+    sender.connectToServer(server->serverName());
+    ASSERT_TRUE(sender.waitForConnected(HANDSHAKE_TIMEOUT_MS));
+    sender.write(encodePayload(PAYLOAD));
+    sender.flush();
+
+    QTimer::singleShot(HANDSHAKE_TIMEOUT_MS, &loop, &QEventLoop::quit);
+    loop.exec();
+
+    EXPECT_EQ(1, handlerCalls) << "the payload must be handed to the handlers exactly once";
+    EXPECT_EQ(PAYLOAD, handled);
+    EXPECT_TRUE(socketConnectedWhenHandled) << "the payload was not handled from inside the readyRead emission";
+    EXPECT_FALSE(acknowledgment.isEmpty()) << "the primary never acknowledged the payload";
+
+    // The deferred teardown must not be skipped, and must not delete the socket
+    // from inside the nested loop above: either would show up here as a crash or
+    // as a socket that outlives the hand-off and leaks a connection per payload.
+    QTimer::singleShot(HANDSHAKE_TIMEOUT_MS, &teardownLoop, &QEventLoop::quit);
+    teardownLoop.exec();
+    EXPECT_TRUE(socketReleased) << "the forwarding socket was never released after the hand-off";
+}
+
+int main(int argc, char **argv)
+{
+    QCoreApplication app(argc, argv);
+    ::testing::InitGoogleTest(&argc, argv);
+    return RUN_ALL_TESTS();
+}