From 9f30892dffb68a5551e20ecee65db5429be7f354 Mon Sep 17 00:00:00 2001 From: "sentry[bot]" <39604003+sentry[bot]@users.noreply.github.com> Date: Mon, 2 Nov 2026 13:28:23 +0100 Subject: [PATCH] [Client] Fix crash when local game commands are sent after shutdown (#7714) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [Client] Fix crash when local game commands are sent after shutdown * [Network] Fix formatting in local client debug log * [CI] Keep the vcpkg submodule pointer on the manifest baseline vcpkg.json asks for builtin-baseline 9eec002 (vcpkg 2026.10.16), but the submodule gitlink was still at 9e593bb. The runners check submodules out shallowly, so vcpkg-tool cannot read versions/baseline.json out of the baseline commit and every vcpkg leg (Windows and all macOS) fails in CMake configure with: failed to `git show` versions/baseline.json git phrases that as "path ... exists on disk, but not in " even when the commit is simply absent from the shallow clone, so it reads like a tooling or network failure rather than a mismatched pointer. Point the submodule back at 9eec002, and add a cheap job that fails when the gitlink and builtin-baseline disagree, before the five build legs pay for it. --------- Co-authored-by: sentry[bot] <39604003+sentry[bot]@users.noreply.github.com> Co-authored-by: Lukas Brübach --- .github/workflows/desktop-build.yml | 32 +++- cockatrice/src/interface/window_main.cpp | 6 +- .../network/client/local/local_client.cpp | 10 +- .../network/client/local/local_client.h | 6 +- tests/CMakeLists.txt | 12 ++ tests/local_client_lifetime_test.cpp | 149 ++++++++++++++++++ vcpkg | 2 +- 7 files changed, 208 insertions(+), 9 deletions(-) create mode 100644 tests/local_client_lifetime_test.cpp diff --git a/.github/workflows/desktop-build.yml b/.github/workflows/desktop-build.yml index 54068f5a0..df054ea26 100644 --- a/.github/workflows/desktop-build.yml +++ b/.github/workflows/desktop-build.yml @@ -101,6 +101,36 @@ jobs: --title "$release_name" \ --notes-file "$body_path" + vcpkg-baseline: + name: vcpkg baseline + runs-on: ubuntu-slim + steps: + - name: "Checkout" + uses: actions/checkout@v7 + + # vcpkg-tool reads versions/baseline.json out of the commit named by + # `builtin-baseline`, through the `vcpkg` submodule's git history. A submodule + # pointer that lags the manifest - e.g. a stale `vcpkg/` worktree captured by + # `git add` in a PR - makes every vcpkg leg fail in configure with "failed to + # `git show` versions/baseline.json". That reads like a baseline or network + # problem, when the pointer is what has to move. Check the two agree here in + # seconds instead of on five build legs. + - name: "Check the submodule pointer against the manifest baseline" + shell: bash + run: | + pointer="$(git rev-parse HEAD:vcpkg 2>/dev/null || true)" + baseline="$(sed -nE 's/.*"builtin-baseline"[[:space:]]*:[[:space:]]*"([0-9a-f]{40})".*/\1/p' vcpkg.json)" + if [[ -z "$pointer" || -z "$baseline" ]]; then + echo "::error file=vcpkg.json::could not read the vcpkg submodule pointer or the builtin-baseline" + exit 1 + fi + if [[ "$pointer" != "$baseline" ]]; then + echo "::error file=vcpkg.json::vcpkg submodule is at $pointer but builtin-baseline is $baseline" + echo "Move them together: git -C vcpkg checkout $baseline && git add vcpkg" + exit 1 + fi + echo "vcpkg submodule and builtin-baseline agree on $baseline" + build-linux: strategy: fail-fast: false @@ -466,7 +496,7 @@ jobs: vcpkg_upload: 1 # only one leg per vcpkg_scope hands its archives over name: ${{ matrix.os }} ${{ matrix.target }}${{ matrix.soc == 'Intel' && ' Intel' || '' }}${{ matrix.type == 'Debug' && ' Debug' || '' }} - needs: configure + needs: [configure, vcpkg-baseline] runs-on: ${{ matrix.runner }} timeout-minutes: 100 env: diff --git a/cockatrice/src/interface/window_main.cpp b/cockatrice/src/interface/window_main.cpp index f8ab5d3f6..d546676bc 100644 --- a/cockatrice/src/interface/window_main.cpp +++ b/cockatrice/src/interface/window_main.cpp @@ -254,7 +254,11 @@ void MainWindow::actWatchReplay() void MainWindow::localGameEnded() { - delete localServer; + // Defer deletion so the LocalClients (already scheduled with deleteLater() in TabSupervisor::stop) + // and any queued commands are processed before the server and its interfaces are destroyed. + if (localServer) { + localServer->deleteLater(); + } localServer = nullptr; aConnect->setEnabled(true); diff --git a/libcockatrice_network/libcockatrice/network/client/local/local_client.cpp b/libcockatrice_network/libcockatrice/network/client/local/local_client.cpp index f7d8645e8..b9a1566f8 100644 --- a/libcockatrice_network/libcockatrice/network/client/local/local_client.cpp +++ b/libcockatrice_network/libcockatrice/network/client/local/local_client.cpp @@ -33,16 +33,18 @@ LocalClient::LocalClient(LocalServerInterface *_lsi, sendCommand(prepareSessionCommand(joinCmd)); } -LocalClient::~LocalClient() -{ -} - void LocalClient::sendCommandContainer(const CommandContainer &cont) { if (LocalClientLog().isDebugEnabled()) { qCDebug(LocalClientLog).noquote() << userName << "OUT" << getSafeDebugString(cont); } + if (lsi.isNull()) { + qCWarning(LocalClientLog).noquote() + << userName << "dropping command, local server interface no longer exists:" << getSafeDebugString(cont); + return; + } + lsi->itemFromClient(cont); } diff --git a/libcockatrice_network/libcockatrice/network/client/local/local_client.h b/libcockatrice_network/libcockatrice/network/client/local/local_client.h index 3ae570255..15b36d3db 100644 --- a/libcockatrice_network/libcockatrice/network/client/local/local_client.h +++ b/libcockatrice_network/libcockatrice/network/client/local/local_client.h @@ -10,6 +10,7 @@ #include "../abstract/abstract_client.h" #include +#include #include #include @@ -24,14 +25,15 @@ class LocalClient : public AbstractClient { Q_OBJECT private: - LocalServerInterface *lsi; + // Guarded pointer: the LocalServer (and its interfaces) may be destroyed while this client + // still has pending commands queued on the event loop. + QPointer lsi; public: LocalClient(LocalServerInterface *_lsi, const QString &_playerName, const QString &_clientId, QObject *parent = nullptr); - ~LocalClient() override; void sendCommandContainer(const CommandContainer &cont) override; private slots: diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 261900c01..0bb9654a1 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -350,6 +350,18 @@ cockatrice_add_test( LABELS ci ) +# The local game runs one client and one server in the same process. +# Either of them can be destroyed first; the survivor has to stay usable. +cockatrice_add_test( + NAME + local_client_lifetime_test + SOURCES + local_client_lifetime_test.cpp + LIBS + libcockatrice_network + LABELS + ci +) cockatrice_add_test( NAME server_room_channels_test diff --git a/tests/local_client_lifetime_test.cpp b/tests/local_client_lifetime_test.cpp new file mode 100644 index 000000000..95a2b1380 --- /dev/null +++ b/tests/local_client_lifetime_test.cpp @@ -0,0 +1,149 @@ +/** + * @file local_client_lifetime_test.cpp + * @brief Tests that a LocalClient survives the death of the LocalServer it talks to. + * @ingroup Tests + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +// The server library refers to this global; the real applications define it in main.cpp. +RNG_Abstract *rng = nullptr; + +namespace +{ + +// GTest's main() creates no QCoreApplication, which QObject::deleteLater() needs. +const QCoreApplication &application() +{ + static int argc = 1; + static char executable[] = "local_client_lifetime_test"; + static char *argv[] = {executable, nullptr}; + static const QCoreApplication app(argc, argv); + return app; +} + +/// Collects the warnings one logging category emits while this object is alive. +/// +/// The handler is process-global and Qt does not nest handlers, so only one capture can be +/// installed at a time; GTest running one case at a time is what makes that safe. +class CategoryWarningCapture +{ +public: + explicit CategoryWarningCapture(const char *_category) : category(_category) + { + previousHandler = qInstallMessageHandler(captureMessage); + s_active = this; + } + + ~CategoryWarningCapture() + { + qInstallMessageHandler(previousHandler); + s_active = nullptr; + } + + CategoryWarningCapture(const CategoryWarningCapture &) = delete; + CategoryWarningCapture &operator=(const CategoryWarningCapture &) = delete; + + const QStringList &warnings() const + { + return capturedWarnings; + } + +private: + static void captureMessage(QtMsgType type, const QMessageLogContext &context, const QString &message) + { + if (s_active != nullptr && type == QtWarningMsg && context.category != nullptr && + std::strcmp(s_active->category, context.category) == 0) { + s_active->capturedWarnings.append(message); + } + + // Forward everything else, so a failure elsewhere in the test is still visible. + if (previousHandler != nullptr) { + previousHandler(type, context, message); + return; + } + std::fprintf(stderr, "%s\n", qPrintable(qFormatLogMessage(type, context, message))); + } + + const char *category; + QStringList capturedWarnings; + + static CategoryWarningCapture *s_active; + static QtMessageHandler previousHandler; +}; + +CategoryWarningCapture *CategoryWarningCapture::s_active = nullptr; +QtMessageHandler CategoryWarningCapture::previousHandler = nullptr; + +class LocalClientLifetimeTest : public ::testing::Test +{ +protected: + void SetUp() override + { + (void)application(); + + server = std::make_unique(); + LocalServerInterface *lsi = server->newConnection(); + ASSERT_NE(lsi, nullptr); + // Building the client logs in and joins room 0 through that interface, exactly as + // MainWindow::startLocalGame() does. + client = std::make_unique(lsi, QStringLiteral("lifetime-test-player"), + QStringLiteral("lifetime-test-client-id")); + } + + void TearDown() override + { + // Either case may already have deleted one of these. + client.reset(); + server.reset(); + } + + std::unique_ptr server; + std::unique_ptr client; +}; + +TEST_F(LocalClientLifetimeTest, SendAfterServerDeletedIsDropped) +{ + server.reset(); // also destroys the LocalServerInterface the client was built on + + CategoryWarningCapture capture("local_client"); + // A default container stands in for a command still queued when the local game ended. + client->sendCommandContainer(CommandContainer()); + + ASSERT_EQ(capture.warnings().size(), 1); + EXPECT_TRUE(capture.warnings().first().contains(QStringLiteral("dropping command"))) + << capture.warnings().first().toStdString(); + + // The surviving client is intact: reading its state must not touch the deleted interface. + EXPECT_EQ(client->getUserName(), QStringLiteral("lifetime-test-player")); +} + +TEST_F(LocalClientLifetimeTest, DestroyingClientAfterServerIsSafe) +{ + server.reset(); + + CategoryWarningCapture capture("local_client"); + client.reset(); // must neither crash nor touch the destroyed interface + + // The destructor itself must have been silent. + EXPECT_TRUE(capture.warnings().isEmpty()); + + // A working capture proves the emptiness above is real rather than a broken handler. + qCWarning(LocalClientLog) << "local_client_lifetime_test capture self-check"; + EXPECT_EQ(capture.warnings().size(), 1); +} + +} // namespace diff --git a/vcpkg b/vcpkg index 9e593bb18..9eec002df 160000 --- a/vcpkg +++ b/vcpkg @@ -1 +1 @@ -Subproject commit 9e593bb18ea69cc5095e012465dcd675a822ed0d +Subproject commit 9eec002df1cd109f8f104cd966a763daf9751489