MPRIS: more async and error handling improvements

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<sdbus::Error> 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<sdbus::Error> 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<sdbus::Error> 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<sdbus::Error> 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<sdbus::Error> 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<void(std::optional<sdbus::Error> err)>
+MprisService::makeAsyncReplyHandler(std::string op, std::string busName, std::optional<std::string> method) {
+  return [this, op = std::move(op), busName = std::move(busName),
+          method = std::move(method)](std::optional<sdbus::Error> 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<sdbus::Error> 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;
   }
 }

src/dbus/mpris/mpris_service.h

@@ -108,6 +108,8 @@ private:
   [[nodiscard]] std::int64_t projectedPositionUs(const MprisPlayerInfo& player) const;
   [[nodiscard]] std::optional<std::string> chooseActivePlayer() const;
   [[nodiscard]] bool isBlacklisted(const MprisPlayerInfo& player) const;
+  std::function<void(std::optional<sdbus::Error> err)>
+  makeAsyncReplyHandler(std::string op, std::string busName, std::optional<std::string> method = std::nullopt);
   [[nodiscard]] bool callPlayerMethod(const std::string& busName, const char* methodName);
   [[nodiscard]] bool canInvoke(const MprisPlayerInfo& player, const char* methodName) const;
 

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<Flex> 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<Flex> 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<Flex> 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<Button>();
-  playPause->setGlyph("media-play");
-  playPause->setVariant(ButtonVariant::Accent);
-  playPause->setMinWidth(kMediaPlayPauseHeight * scale);
-  playPause->setMinHeight(kMediaPlayPauseHeight * scale);
-  playPause->setPadding(Style::spaceSm * scale, Style::spaceSm * scale);
-  playPause->setRadius(Style::radiusLg * scale);
-  playPause->setOnClick([this]() {
+auto playPause = std::make_unique<Button>();
+playPause->setGlyph("media-play");
+playPause->setVariant(ButtonVariant::Accent);
+playPause->setMinWidth(kMediaPlayPauseHeight* scale);
+playPause->setMinHeight(kMediaPlayPauseHeight* scale);
+playPause->setPadding(Style::spaceSm* scale, Style::spaceSm* scale);
+playPause->setRadius(Style::radiusLg* scale);
+playPause->setOnClick([this]() {
     DeferredCall::callLater([this]() {
-      if (m_mpris != nullptr) {
-        (void)m_mpris->playPauseActive();
-        PanelManager::instance().refresh();
+    if (!m_alive || m_mpris == nullptr) {
+      return;
+    }
+    (void)m_mpris->playPauseActive();
+    PanelManager::instance().refresh();
       }
-    });
-  });
-  m_playPauseButton = playPause.get();
-  controls->addChild(std::move(playPause));
+});
+});
+m_playPauseButton = playPause.get();
+controls->addChild(std::move(playPause));
 
-  auto next = std::make_unique<Button>();
-  next->setGlyph("media-next");
-  next->setVariant(ButtonVariant::Ghost);
-  next->setMinWidth(kMediaControlsHeight * scale);
-  next->setMinHeight(kMediaControlsHeight * scale);
-  next->setPadding(Style::spaceSm * scale, Style::spaceSm * scale);
-  next->setRadius(Style::radiusLg * scale);
-  next->setOnClick([this]() {
+auto next = std::make_unique<Button>();
+next->setGlyph("media-next");
+next->setVariant(ButtonVariant::Ghost);
+next->setMinWidth(kMediaControlsHeight* scale);
+next->setMinHeight(kMediaControlsHeight* scale);
+next->setPadding(Style::spaceSm* scale, Style::spaceSm* scale);
+next->setRadius(Style::radiusLg* scale);
+next->setOnClick([this]() {
     DeferredCall::callLater([this]() {
-      if (m_mpris != nullptr) {
-        (void)m_mpris->nextActive();
-        PanelManager::instance().refresh();
+    if (!m_alive || m_mpris == nullptr) {
+      return;
+    }
+    (void)m_mpris->nextActive();
+    PanelManager::instance().refresh();
       }
-    });
-  });
-  m_nextButton = next.get();
-  controls->addChild(std::move(next));
+});
+});
+m_nextButton = next.get();
+controls->addChild(std::move(next));
 
-  auto shuffle = std::make_unique<Button>();
-  shuffle->setGlyph("shuffle");
-  shuffle->setVariant(ButtonVariant::Ghost);
-  shuffle->setMinWidth(kMediaControlsHeight * scale);
-  shuffle->setMinHeight(kMediaControlsHeight * scale);
-  shuffle->setPadding(Style::spaceSm * scale, Style::spaceSm * scale);
-  shuffle->setRadius(Style::radiusLg * scale);
-  shuffle->setOnClick([this]() {
+auto shuffle = std::make_unique<Button>();
+shuffle->setGlyph("shuffle");
+shuffle->setVariant(ButtonVariant::Ghost);
+shuffle->setMinWidth(kMediaControlsHeight* scale);
+shuffle->setMinHeight(kMediaControlsHeight* scale);
+shuffle->setPadding(Style::spaceSm* scale, Style::spaceSm* scale);
+shuffle->setRadius(Style::radiusLg* scale);
+shuffle->setOnClick([this]() {
     DeferredCall::callLater([this]() {
-      if (m_mpris != nullptr) {
-        const bool enabled = m_mpris->shuffleActive().value_or(false);
-        (void)m_mpris->setShuffleActive(!enabled);
-        PanelManager::instance().refresh();
+    if (!m_alive || m_mpris == nullptr) {
+      return;
+    }
+    const bool enabled = m_mpris->shuffleActive().value_or(false);
+    (void)m_mpris->setShuffleActive(!enabled);
+    PanelManager::instance().refresh();
       }
-    });
-  });
-  m_shuffleButton = shuffle.get();
-  controls->addChild(std::move(shuffle));
+});
+});
+m_shuffleButton = shuffle.get();
+controls->addChild(std::move(shuffle));
 
-  controlsRow->addChild(std::move(controls));
-  mediaStack->addChild(std::move(controlsRow));
+controlsRow->addChild(std::move(controls));
+mediaStack->addChild(std::move(controlsRow));
 
-  nowCard->addChild(std::move(mediaStack));
-  mediaColumn->addChild(std::move(nowCard));
+nowCard->addChild(std::move(mediaStack));
+mediaColumn->addChild(std::move(nowCard));
 
-  auto visualizerColumn = std::make_unique<Flex>();
-  visualizerColumn->setDirection(FlexDirection::Vertical);
-  visualizerColumn->setAlign(FlexAlign::Stretch);
-  visualizerColumn->setGap(Style::spaceSm * scale);
-  visualizerColumn->setFlexGrow(2.0f);
-  applySectionCardStyle(*visualizerColumn, scale);
-  visualizerColumn->setClipChildren(true);
-  m_visualizerColumn = visualizerColumn.get();
+auto visualizerColumn = std::make_unique<Flex>();
+visualizerColumn->setDirection(FlexDirection::Vertical);
+visualizerColumn->setAlign(FlexAlign::Stretch);
+visualizerColumn->setGap(Style::spaceSm* scale);
+visualizerColumn->setFlexGrow(2.0f);
+applySectionCardStyle(*visualizerColumn, scale);
+visualizerColumn->setClipChildren(true);
+m_visualizerColumn = visualizerColumn.get();
 
-  auto visualizerBody = std::make_unique<Flex>();
-  visualizerBody->setDirection(FlexDirection::Horizontal);
-  visualizerBody->setAlign(FlexAlign::Stretch);
-  visualizerBody->setJustify(FlexJustify::Start);
-  visualizerBody->setFillWidth(true);
-  visualizerBody->setFlexGrow(1.0f);
-  m_visualizerBody = visualizerBody.get();
+auto visualizerBody = std::make_unique<Flex>();
+visualizerBody->setDirection(FlexDirection::Horizontal);
+visualizerBody->setAlign(FlexAlign::Stretch);
+visualizerBody->setJustify(FlexJustify::Start);
+visualizerBody->setFillWidth(true);
+visualizerBody->setFlexGrow(1.0f);
+m_visualizerBody = visualizerBody.get();
 
-  auto visualizerSpectrum = std::make_unique<AudioSpectrum>();
-  visualizerSpectrum->setGradient(colorForRole(ColorRole::Secondary), colorForRole(ColorRole::Tertiary));
-  visualizerSpectrum->setOrientation(AudioSpectrumOrientation::Vertical);
-  visualizerSpectrum->setMirrored(true);
-  visualizerSpectrum->setCentered(true);
-  visualizerSpectrum->setFlexGrow(1.0f);
-  m_visualizerSpectrum = visualizerSpectrum.get();
-  visualizerBody->addChild(std::move(visualizerSpectrum));
-  visualizerColumn->addChild(std::move(visualizerBody));
-  tab->addChild(std::move(mediaColumn));
-  tab->addChild(std::move(visualizerColumn));
+auto visualizerSpectrum = std::make_unique<AudioSpectrum>();
+visualizerSpectrum->setGradient(colorForRole(ColorRole::Secondary), colorForRole(ColorRole::Tertiary));
+visualizerSpectrum->setOrientation(AudioSpectrumOrientation::Vertical);
+visualizerSpectrum->setMirrored(true);
+visualizerSpectrum->setCentered(true);
+visualizerSpectrum->setFlexGrow(1.0f);
+m_visualizerSpectrum = visualizerSpectrum.get();
+visualizerBody->addChild(std::move(visualizerSpectrum));
+visualizerColumn->addChild(std::move(visualizerBody));
+tab->addChild(std::move(mediaColumn));
+tab->addChild(std::move(visualizerColumn));
 
-  if (m_wayland != nullptr && m_renderContext != nullptr) {
-    m_playerMenuPopup = std::make_unique<ContextMenuPopup>(*m_wayland, *m_renderContext);
-    m_playerMenuPopup->setOnActivate([this](const ContextMenuControlEntry& entry) {
-      DeferredCall::callLater([this, entry]() {
-        if (m_mpris == nullptr) {
-          return;
+if (m_wayland != nullptr && m_renderContext != nullptr) {
+  m_playerMenuPopup = std::make_unique<ContextMenuPopup>(*m_wayland, *m_renderContext);
+  m_playerMenuPopup->setOnActivate([this](const ContextMenuControlEntry& entry) {
+    DeferredCall::callLater([this, entry]() {
+      if (!m_alive || m_mpris == nullptr) {
+        return;
+      }
+      if (entry.id == 0) {
+        m_mpris->clearPinnedPlayerPreference();
+      } else {
+        const std::size_t idx = static_cast<std::size_t>(entry.id - 1);
+        if (idx < m_playerBusNames.size()) {
+          m_mpris->setPinnedPlayerPreference(m_playerBusNames[idx]);
         }
-        if (entry.id == 0) {
-          m_mpris->clearPinnedPlayerPreference();
-        } else {
-          const std::size_t idx = static_cast<std::size_t>(entry.id - 1);
-          if (idx < m_playerBusNames.size()) {
-            m_mpris->setPinnedPlayerPreference(m_playerBusNames[idx]);
-          }
-        }
-        PanelManager::instance().refresh();
-      });
+      }
+      PanelManager::instance().refresh();
     });
-  }
+  });
+}
 
-  return tab;
+return tab;
 }
 
 void MediaTab::doLayout(Renderer& renderer, float contentWidth, float bodyHeight) {
@@ -619,7 +631,7 @@ void MediaTab::setActive(bool active) {
     // progress slider starts at the current playback position.
     m_positionSampleAt = {};
     DeferredCall::callLater([this]() {
-      if (m_mpris == nullptr) {
+      if (!m_alive || m_mpris == nullptr) {
         return;
       }
       m_mpris->refreshPlayers();
@@ -699,7 +711,7 @@ void MediaTab::refresh(Renderer& renderer) {
       m_lastMprisRefreshAttempt = now;
       kLog.debug("media tab retrying mpris discovery players={} active={}", players.size(), active.has_value());
       DeferredCall::callLater([this]() {
-        if (m_mpris == nullptr) {
+        if (!m_alive || m_mpris == nullptr) {
           return;
         }
         m_mpris->refreshPlayers();

src/shell/control_center/media_tab.h

@@ -43,6 +43,10 @@ private:
 
   void openPlayerMenu();
 
+  // Guard flag to detect use-after-free in deferred callbacks.
+  // Set to false in destructor so pending DeferredCall callbacks can safely check if this object is alive.
+  bool m_alive = true;
+
   MprisService* m_mpris = nullptr;
   HttpClient* m_httpClient = nullptr;
   PipeWireSpectrum* m_spectrum = nullptr;