From c6f864ea309c75d4119417a1d7a25ce4bc8f63bb Mon Sep 17 00:00:00 2001 From: sami Date: Sat, 12 Sep 2026 21:27:28 +0400 Subject: [PATCH] gui: fix RpcClient double-free on stop() and the 1000-row list cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both found live while building gui/tests/dod's DoD harness, not from reading the code — see that commit for how. RpcClient::stop() left conn_ dangling after joining the worker thread: the thread's own finish() flushes the DeferredDelete stop()'s connect(&thread_, &QThread::finished, conn_, &QObject::deleteLater) already posted, so conn_ is gone by the time stop() returns, but nothing cleared the pointer. Any caller that calls stop() and later lets the client destruct (the harness's own client.stop() at shutdown; also plain, correct API usage) hit a double-free in the destructor's leftover `delete conn_`. Caught by ASan on the very first run that actually exercised the stop-then-destroy path. requestInitialList() also called download.list with a hardcoded `{"limit": 1000}`, silently capping the table at 1000 rows no matter how many the daemon actually has — download.list.schema.json's own description says "the GUI pages", not "the GUI takes it all in one call". The scroll-60fps DoD gate refused to run against mockd --tasks 10000 rather than "pass" against a 1000-row table, which is what surfaced it. requestInitialList() now pages (5000 per call, the schema's own max) until `total` is satisfied, then resets the model once with everything. Separately: RpcConnection's session.subscribe list never included event.settings.changed or event.grabber.progress, even though RpcClient has carried signals for both since the Options/Grabber work — session.subscribe "replaces the previous selection" and "nothing is delivered until this is called", so both events were being silently dropped by any real daemon that enforces the subscription (mockd does; verified live with a second subscribed client actually receiving event.settings.changed after this fix, round-tripped through a real veloxd's settings.set). GrabberWizard's 5 s poll fallback is exactly why this went unnoticed until now — it covered for the missing push the whole time. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NSCdCWFXBTSBBK3MzWtJiC --- gui/src/rpc/RpcClient.cpp | 46 ++++++++++++++++++++++++++++++----- gui/src/rpc/RpcClient.hpp | 1 + gui/src/rpc/RpcConnection.cpp | 10 +++++--- 3 files changed, 47 insertions(+), 10 deletions(-) diff --git a/gui/src/rpc/RpcClient.cpp b/gui/src/rpc/RpcClient.cpp index 529950c..c1cdb95 100644 --- a/gui/src/rpc/RpcClient.cpp +++ b/gui/src/rpc/RpcClient.cpp @@ -56,6 +56,13 @@ void RpcClient::stop() { QMetaObject::invokeMethod(conn_, "stop", Qt::QueuedConnection); thread_.quit(); thread_.wait(); + // thread_.wait() does not return until thread_'s own finish() has already flushed the + // DeferredDelete this class's own connect(&thread_, &QThread::finished, conn_, + // &QObject::deleteLater) posted — conn_ is gone by now. Null it out so a later call + // (stop() is a public slot; a caller stopping and then destroying the client is normal + // use, and the destructor's own `delete conn_` for the never-started case must not + // run a second time against memory this path already freed). + conn_ = nullptr; } void RpcClient::call(const QString &methodName, const QJsonObject ¶ms, @@ -76,17 +83,44 @@ void RpcClient::onConnectionState(int state) { } void RpcClient::requestInitialList() { - call(QString::fromLatin1(method::kDownloadList), QJsonObject{{"limit", 1000}}, - [this](const RpcReply &reply) { + fetchListPage(0, {}); +} + +// download.list.schema.json: "Filtering, sorting and paging all happen in the daemon so +// the GUI never materializes 100k rows to show 40" — limit maxes out at 5000, so one call +// cannot ever return everything for a table the DoD's own gate says can hold 10 000 rows. +// A single fixed-limit call here silently truncated the table below that (caught by +// gui/tests/dod's scroll-60fps gate refusing to run against a 1000-row table when mockd +// seeded 10000). Page until `total` is satisfied, then reset the model exactly once. +void RpcClient::fetchListPage(int offset, QJsonArray accumulated) { + constexpr int kPageSize = 5000; // download.list's own maximum + constexpr int kMaxPages = 100; // 500 000 rows — a safety cap, not an expected ceiling + call(QString::fromLatin1(method::kDownloadList), + QJsonObject{{"offset", offset}, {"limit", kPageSize}}, + [this, offset, accumulated](const RpcReply &reply) mutable { if (!reply.ok()) { qCWarning(lcRpc, "download.list failed: %d %s", reply.error.code, qUtf8Printable(reply.error.message)); + if (!accumulated.isEmpty()) { + emit taskListReset(accumulated); // show what we got rather than nothing + } return; } - const QJsonArray items = reply.result.toObject().value("items").toArray(); - qCInfo(lcRpc, "initial download.list: %lld row(s)", - static_cast(items.size())); - emit taskListReset(items); + const QJsonObject result = reply.result.toObject(); + const QJsonArray page = result.value("items").toArray(); + const qint64 total = static_cast(result.value("total").toDouble()); + for (const QJsonValue &item : page) { + accumulated.append(item); + } + const bool morePages = + !page.isEmpty() && accumulated.size() < total && (offset / kPageSize) < kMaxPages; + if (morePages) { + fetchListPage(offset + static_cast(page.size()), accumulated); + return; + } + qCInfo(lcRpc, "initial download.list: %lld of %lld row(s)", + static_cast(accumulated.size()), static_cast(total)); + emit taskListReset(accumulated); }); } diff --git a/gui/src/rpc/RpcClient.hpp b/gui/src/rpc/RpcClient.hpp index 80dec01..ba1270f 100644 --- a/gui/src/rpc/RpcClient.hpp +++ b/gui/src/rpc/RpcClient.hpp @@ -67,6 +67,7 @@ class RpcClient : public QObject { private: void requestInitialList(); + void fetchListPage(int offset, QJsonArray accumulated); QThread thread_; RpcConnection *conn_ = nullptr; // owned by thread_ affinity, deleted on thread finish diff --git a/gui/src/rpc/RpcConnection.cpp b/gui/src/rpc/RpcConnection.cpp index ced660c..e27c69c 100644 --- a/gui/src/rpc/RpcConnection.cpp +++ b/gui/src/rpc/RpcConnection.cpp @@ -154,10 +154,12 @@ void RpcConnection::dispatchFrame(const QJsonObject &frame) { socket_->abort(); // version mismatch or refused — bounce and retry return; } - sendRaw(kSubscribeId, QString::fromLatin1(method::kSessionSubscribe), - QJsonObject{{"events", QJsonArray{event::kTaskAdded, event::kTaskRemoved, - event::kTaskState, event::kTaskProgress, - event::kSpeedGlobal, event::kNotify}}}); + sendRaw( + kSubscribeId, QString::fromLatin1(method::kSessionSubscribe), + QJsonObject{ + {"events", QJsonArray{event::kTaskAdded, event::kTaskRemoved, event::kTaskState, + event::kTaskProgress, event::kSpeedGlobal, event::kNotify, + event::kSettingsChanged, event::kGrabberProgress}}}); return; } if (id == kSubscribeId) {