From 1925009748ac513eb24ddcd3a7e2f726ff70ab91 Mon Sep 17 00:00:00 2001 From: Mathew-D Date: Sun, 10 May 2026 13:08:33 -0400 Subject: [PATCH] MPRIS: more async and error handling improvements --- src/dbus/mpris/mpris_service.cpp | 81 ++++----- src/dbus/mpris/mpris_service.h | 2 + src/shell/control_center/media_tab.cpp | 222 +++++++++++++------------ src/shell/control_center/media_tab.h | 4 + 4 files changed, 167 insertions(+), 142 deletions(-) diff --git a/src/dbus/mpris/mpris_service.cpp b/src/dbus/mpris/mpris_service.cpp index d7fa402c4..9c38544b0 100644 --- a/src/dbus/mpris/mpris_service.cpp +++ b/src/dbus/mpris/mpris_service.cpp @@ -745,11 +745,7 @@ bool MprisService::seek(const std::string& busName, int64_t offsetUs) { proxyIt->second->callMethodAsync("Seek") .onInterface(k_mpris_player_interface) .withArguments(offsetUs) - .uponReplyInvoke([busName](std::optional err) { - if (err.has_value()) { - kLog.warn("seek failed name={} err={}", busName, err->what()); - } - }); + .uponReplyInvoke(makeAsyncReplyHandler("seek", busName)); m_lastSeekCommandAt[busName] = std::chrono::steady_clock::now(); return true; } catch (const sdbus::Error& e) { @@ -777,9 +773,12 @@ bool MprisService::setPosition(const std::string& busName, int64_t positionUs) { return false; } - auto fallback_seek = [&]() { - const int64_t currentPositionUs = it->second.positionUs; + // Capture needed values by value for fallback_seek, not iterator references. + // This makes it safe even if control flow changes or seek() becomes async. + const int64_t currentPositionUs = it->second.positionUs; + const bool preferRelativeSeek = it->second.trackId.empty() || busName.find("spotify") != std::string::npos; + auto fallback_seek = [this, busName, currentPositionUs, positionUs]() { const int64_t offsetUs = positionUs - currentPositionUs; if (offsetUs == 0) { return true; @@ -787,7 +786,6 @@ bool MprisService::setPosition(const std::string& busName, int64_t positionUs) { return seek(busName, offsetUs); }; - const bool preferRelativeSeek = it->second.trackId.empty() || busName.find("spotify") != std::string::npos; if (preferRelativeSeek) { // Some players don't expose track_id consistently; emulate absolute position with Seek. kLog.debug("mpris set-position using relative Seek fallback for {}", busName); @@ -798,11 +796,7 @@ bool MprisService::setPosition(const std::string& busName, int64_t positionUs) { proxyIt->second->callMethodAsync("SetPosition") .onInterface(k_mpris_player_interface) .withArguments(sdbus::ObjectPath{it->second.trackId}, positionUs) - .uponReplyInvoke([busName](std::optional err) { - if (err.has_value()) { - kLog.warn("set-position failed name={} err={}", busName, err->what()); - } - }); + .uponReplyInvoke(makeAsyncReplyHandler("set-position", busName)); m_lastSeekCommandAt[busName] = std::chrono::steady_clock::now(); return true; } catch (const sdbus::Error& e) { @@ -820,6 +814,10 @@ bool MprisService::setPositionActive(int64_t positionUs) { } bool MprisService::setVolume(const std::string& busName, double volume) { + if (!std::isfinite(volume) || volume < 0.0) { + return false; + } + const auto it = m_players.find(busName); if (it == m_players.end()) { return false; @@ -834,11 +832,7 @@ bool MprisService::setVolume(const std::string& busName, double volume) { proxyIt->second->callMethodAsync("Set") .onInterface(k_properties_interface) .withArguments(std::string{k_mpris_player_interface}, std::string{"Volume"}, sdbus::Variant{volume}) - .uponReplyInvoke([busName](std::optional err) { - if (err.has_value()) { - kLog.warn("set-volume failed name={} err={}", busName, err->what()); - } - }); + .uponReplyInvoke(makeAsyncReplyHandler("set-volume", busName)); return true; } catch (const sdbus::Error& e) { kLog.warn("set-volume dispatch failed name={} err={}", busName, e.what()); @@ -869,11 +863,7 @@ bool MprisService::setShuffle(const std::string& busName, bool shuffle) { proxyIt->second->callMethodAsync("Set") .onInterface(k_properties_interface) .withArguments(std::string{k_mpris_player_interface}, std::string{"Shuffle"}, sdbus::Variant{shuffle}) - .uponReplyInvoke([busName](std::optional err) { - if (err.has_value()) { - kLog.warn("set-shuffle failed name={} err={}", busName, err->what()); - } - }); + .uponReplyInvoke(makeAsyncReplyHandler("set-shuffle", busName)); return true; } catch (const sdbus::Error& e) { kLog.warn("set-shuffle dispatch failed name={} err={}", busName, e.what()); @@ -890,6 +880,10 @@ bool MprisService::setShuffleActive(bool shuffle) { } bool MprisService::setLoopStatus(const std::string& busName, std::string loopStatus) { + if (!is_valid_loop_status(loopStatus)) { + return false; + } + const auto it = m_players.find(busName); if (it == m_players.end()) { return false; @@ -905,11 +899,7 @@ bool MprisService::setLoopStatus(const std::string& busName, std::string loopSta .onInterface(k_properties_interface) .withArguments(std::string{k_mpris_player_interface}, std::string{"LoopStatus"}, sdbus::Variant{std::move(loopStatus)}) - .uponReplyInvoke([busName](std::optional err) { - if (err.has_value()) { - kLog.warn("set-loop-status failed name={} err={}", busName, err->what()); - } - }); + .uponReplyInvoke(makeAsyncReplyHandler("set-loop-status", busName)); return true; } catch (const sdbus::Error& e) { kLog.warn("set-loop-status dispatch failed name={} err={}", busName, e.what()); @@ -1816,25 +1806,42 @@ bool MprisService::isBlacklisted(const MprisPlayerInfo& player) const { return false; } +std::function err)> +MprisService::makeAsyncReplyHandler(std::string op, std::string busName, std::optional method) { + return [this, op = std::move(op), busName = std::move(busName), + method = std::move(method)](std::optional err) { + if (err.has_value()) { + if (method.has_value()) { + kLog.warn("{} failed name={} method={} err={}", op, busName, *method, err->what()); + } else { + kLog.warn("{} failed name={} err={}", op, busName, err->what()); + } + return; + } + + if (method.has_value()) { + kLog.debug("{} name={} method={}", op, busName, *method); + } + + DeferredCall::callLater([this, busName]() { addOrRefreshPlayer(busName); }); + }; +} + bool MprisService::callPlayerMethod(const std::string& busName, const char* methodName) { const auto it = m_playerProxies.find(busName); if (it == m_playerProxies.end()) { return false; } + const std::string method{methodName}; // Capture as owned string, not dangling pointer + try { - it->second->callMethodAsync(methodName) + it->second->callMethodAsync(method.c_str()) .onInterface(k_mpris_player_interface) - .uponReplyInvoke([busName, methodName](std::optional err) { - if (err.has_value()) { - kLog.warn("control failed name={} method={} err={}", busName, methodName, err->what()); - return; - } - kLog.debug("control name={} method={}", busName, methodName); - }); + .uponReplyInvoke(makeAsyncReplyHandler("control", busName, method)); return true; } catch (const sdbus::Error& e) { - kLog.warn("control dispatch failed name={} method={} err={}", busName, methodName, e.what()); + kLog.warn("control dispatch failed name={} method={} err={}", busName, method, e.what()); return false; } } diff --git a/src/dbus/mpris/mpris_service.h b/src/dbus/mpris/mpris_service.h index e11321593..b1af20853 100644 --- a/src/dbus/mpris/mpris_service.h +++ b/src/dbus/mpris/mpris_service.h @@ -108,6 +108,8 @@ private: [[nodiscard]] std::int64_t projectedPositionUs(const MprisPlayerInfo& player) const; [[nodiscard]] std::optional chooseActivePlayer() const; [[nodiscard]] bool isBlacklisted(const MprisPlayerInfo& player) const; + std::function err)> + makeAsyncReplyHandler(std::string op, std::string busName, std::optional method = std::nullopt); [[nodiscard]] bool callPlayerMethod(const std::string& busName, const char* methodName); [[nodiscard]] bool canInvoke(const MprisPlayerInfo& player, const char* methodName) const; diff --git a/src/shell/control_center/media_tab.cpp b/src/shell/control_center/media_tab.cpp index 7029a7750..bc74a60e5 100644 --- a/src/shell/control_center/media_tab.cpp +++ b/src/shell/control_center/media_tab.cpp @@ -72,7 +72,11 @@ MediaTab::MediaTab(MprisService* mpris, HttpClient* httpClient, PipeWireSpectrum : m_mpris(mpris), m_httpClient(httpClient), m_spectrum(spectrum), m_wayland(wayland), m_renderContext(renderContext) {} -MediaTab::~MediaTab() = default; +MediaTab::~MediaTab() { + // Signal to any pending DeferredCall callbacks that this object is being destroyed. + // This prevents use-after-free if callbacks run after destruction. + m_alive = false; +} void MediaTab::openPlayerMenu() { if (m_playerMenuPopup == nullptr || m_mpris == nullptr || m_playerMenuButton == nullptr) { @@ -273,7 +277,7 @@ std::unique_ptr MediaTab::create() { m_pendingSeekUntil = now + std::chrono::milliseconds(3000); DeferredCall::callLater([this, seekBusName, targetUs]() { - if (m_mpris == nullptr) { + if (!m_alive || m_mpris == nullptr) { return; } if (!seekBusName.empty()) { @@ -308,7 +312,7 @@ std::unique_ptr MediaTab::create() { repeat->setRadius(Style::radiusLg * scale); repeat->setOnClick([this]() { DeferredCall::callLater([this]() { - if (m_mpris == nullptr) { + if (!m_alive || m_mpris == nullptr) { return; } const auto current = m_mpris->loopStatusActive().value_or("None"); @@ -329,126 +333,134 @@ std::unique_ptr MediaTab::create() { previous->setRadius(Style::radiusLg * scale); previous->setOnClick([this]() { DeferredCall::callLater([this]() { - if (m_mpris != nullptr) { - (void)m_mpris->previousActive(); - PanelManager::instance().refresh(); + if (!m_alive || m_mpris == nullptr) { + return; + } + (void)m_mpris->previousActive(); + PanelManager::instance().refresh(); } - }); }); - m_prevButton = previous.get(); - controls->addChild(std::move(previous)); +}); +m_prevButton = previous.get(); +controls->addChild(std::move(previous)); - auto playPause = std::make_unique