Merge pull request #7783 from omacom/fix/issue-6952

Keep PwNode objects out of the audio panel's Repeater models

shell/plugins/panels/audio/Model.js

@@ -18,8 +18,31 @@ function isAudioSource(node) {
     || mediaClass.indexOf("Source") !== -1
 }
 
-function listSnapshot(list) {
-  return list && list.slice ? list.slice() : []
+// Project a node list to the primitive rows a Repeater model can hold. A row
+// holding the PwNode itself is a QObject that Qt dereferences without a null
+// check while it builds the delegate, and PipeWire deletes nodes outright, so a
+// graph teardown segfaults the shell. Primitives cannot dangle, which is what
+// closes that race -- not the check below, since the node usually dies after the
+// snapshot is taken. That is also why filtering the list here never worked.
+//
+// The name rides along because PipeWire reuses object ids: without it a row
+// could resolve to whatever inherited its id. PwNode.name is CONSTANT, so it
+// identifies the node it was taken from for as long as that node lives.
+function nodeRow(node) {
+  if (!node) return null
+  var id = node.id
+  if (id === undefined || id === null) return null
+  return { id: id, name: String(node.name || "") }
+}
+
+function rowSnapshot(list) {
+  var values = list && list.length ? list : []
+  var rows = []
+  for (var i = 0; i < values.length; i++) {
+    var row = nodeRow(values[i])
+    if (row) rows.push(row)
+  }
+  return rows
 }
 
 function outputVolumeName(volume, muted) {
@@ -237,7 +260,8 @@ if (typeof module !== "undefined") {
   module.exports = {
     isPlaybackStream: isPlaybackStream,
     isAudioSource: isAudioSource,
-    listSnapshot: listSnapshot,
+    nodeRow: nodeRow,
+    rowSnapshot: rowSnapshot,
     outputVolumeName: outputVolumeName,
     parseSinkAvailability: parseSinkAvailability,
     friendlyDeviceLabel: friendlyDeviceLabel,

shell/plugins/panels/audio/Panel.qml

@@ -100,11 +100,13 @@ Panel {
     return list
   }
 
-  // Feed Repeaters with panel-local snapshots instead of the live PipeWire
-  // model. PipeWire can remove nodes while Quickshell is dispatching the
-  // removal signal; rebuilding a Repeater from that signal path has crashed
-  // in Quickshell's PipeWire service. The snapshot timer lets that mutation
-  // settle first, and closed panels keep their repeaters detached entirely.
+  // Feed Repeaters with panel-local snapshots of primitives instead of the live
+  // PipeWire model. PipeWire can remove nodes while Quickshell is dispatching
+  // the removal signal, and a row still holding the PwNode is a QObject Qt
+  // dereferences without a null check, which segfaults the shell. Rows carry id
+  // and name only; every delegate resolves its own node through nodeFor(). The
+  // snapshot timer lets the mutation settle first, and closed panels keep their
+  // repeaters detached entirely.
   property var displayAudioSinks: []
   property var displayAudioSources: []
   property var displayAudioStreams: []
@@ -281,7 +283,7 @@ Panel {
       return
     }
     if (focusSection === "streams" && selectedIndex >= 0 && selectedIndex < displayAudioStreams.length) {
-      var s = displayAudioStreams[selectedIndex]
+      var s = nodeFor(displayAudioStreams[selectedIndex])
       if (s && s.audio) s.audio.volume = Math.max(0, Math.min(1.5, s.audio.volume + delta))
     }
   }
@@ -291,18 +293,18 @@ Panel {
     if (focusSection === "header") { toggleAllMuted(); return }
     if (focusSection === "output") {
       if (selectedIndex === -1) { toggleOutputMute(); return }
-      var sink = displayAudioSinks[selectedIndex]
+      var sink = nodeFor(displayAudioSinks[selectedIndex])
       if (sink) setDefaultSink(sink)
       return
     }
     if (focusSection === "input") {
       if (selectedIndex === -1) { toggleInputMute(); return }
-      var src = displayAudioSources[selectedIndex]
+      var src = nodeFor(displayAudioSources[selectedIndex])
       if (src) setDefaultSource(src)
       return
     }
     if (focusSection === "streams" && selectedIndex >= 0) {
-      var st = displayAudioStreams[selectedIndex]
+      var st = nodeFor(displayAudioStreams[selectedIndex])
       if (st && st.audio) st.audio.muted = !st.audio.muted
     }
   }
@@ -324,15 +326,27 @@ Panel {
   onAudioSourcesChanged: scheduleDisplayAudioModelRefresh()
   onAudioStreamsChanged: scheduleDisplayAudioModelRefresh()
 
-  function listSnapshot(list) {
-    return Model.listSnapshot(list)
+  function rowSnapshot(list) {
+    return Model.rowSnapshot(list)
+  }
+
+  // A row that outlives its node resolves to null, or to a node PipeWire recreated
+  // under the same id and name; every binding below guards for null, and a row
+  // that never held the node cannot dangle.
+  function nodeFor(row) {
+    if (!row) return null
+    for (var i = 0; i < nodes.length; i++) {
+      var n = nodes[i]
+      if (n && n.id === row.id && String(n.name || "") === row.name) return n
+    }
+    return null
   }
 
   function refreshDisplayAudioModels() {
     if (!opened) return
-    displayAudioSinks = listSnapshot(audioSinks)
-    displayAudioSources = listSnapshot(audioSources)
-    displayAudioStreams = listSnapshot(audioStreams)
+    displayAudioSinks = rowSnapshot(audioSinks)
+    displayAudioSources = rowSnapshot(audioSources)
+    displayAudioStreams = rowSnapshot(audioStreams)
     clampCursor()
   }
 
@@ -559,15 +573,18 @@ Panel {
     // Spotify exposes its PipeWire stream as "audio-src". For generic stream
     // names, use the one MPRIS player not already represented by another audio
     // stream (e.g. Chromium, or ALSA apps like cliamp).
-    return Model.unmatchedMprisStreamLabel(label, mprisPlayers, displayAudioStreams)
+    return Model.unmatchedMprisStreamLabel(label, mprisPlayers, audioStreams)
   }
 
+  // These read the other streams' node properties, so they take the live list --
+  // displayAudioStreams carries primitives now, and resolving each row back
+  // would only rebuild what audioStreams already is.
   function streamLabel(node) {
-    return Model.streamLabel(node, mprisPlayers, displayAudioStreams)
+    return Model.streamLabel(node, mprisPlayers, audioStreams)
   }
 
   function streamRepresentsPlayer(node, player) {
-    return Model.streamRepresentsPlayer(node, player, mprisPlayers, displayAudioStreams)
+    return Model.streamRepresentsPlayer(node, player, mprisPlayers, audioStreams)
   }
 
   implicitWidth: button.implicitWidth
@@ -675,7 +692,7 @@ Panel {
           if (!root.cursorActive) return
           if (root.focusSection === "streams" && root.selectedIndex >= 0
               && root.selectedIndex < root.displayAudioStreams.length) {
-            var s = root.displayAudioStreams[root.selectedIndex]
+            var s = root.nodeFor(root.displayAudioStreams[root.selectedIndex])
             if (s && s.audio) s.audio.muted = !s.audio.muted
           } else if (root.focusSection === "input") {
             root.toggleInputMute()
@@ -857,7 +874,7 @@ Panel {
                 required property var modelData
                 required property int index
                 width: panelColumn.width
-                node: modelData
+                node: root.nodeFor(modelData)
                 rowIndex: index
               }
             }
@@ -965,7 +982,7 @@ Panel {
                 required property var modelData
                 required property int index
                 width: panelColumn.width
-                node: modelData
+                node: root.nodeFor(modelData)
                 rowIndex: index
               }
             }
@@ -995,7 +1012,7 @@ Panel {
                 required property var modelData
                 required property int index
                 width: panelColumn.width
-                node: modelData
+                node: root.nodeFor(modelData)
                 rowIndex: index
               }
             }

test/shell.d/audio-test.sh

@@ -13,6 +13,17 @@ assert(!audio.isPlaybackStream({ isStream: false, isSink: true }), 'audio reject
 assert(audio.isAudioSource({ audio: {} }), 'audio detects nodes with audio as sources')
 assert(audio.isAudioSource({ type: 'Audio/Source' }), 'audio detects typed source nodes')
 
+// A destroyed PwNode stays truthy but reads back no id, which is the shape the
+// third entry stands in for: it must not reach a Repeater row. The first carries
+// an object like a live node's audio, so a row that is the node itself fails here.
+assertDeepEqual(
+  audio.rowSnapshot([{ id: 0, name: 'alsa_output', audio: { volume: 1 } }, { id: 42 }, {}, null]),
+  [{ id: 0, name: 'alsa_output' }, { id: 42, name: '' }],
+  'audio projects nodes to primitive rows and drops nodes without an id'
+)
+assertDeepEqual(audio.rowSnapshot(undefined), [], 'audio projects a missing list to no rows')
+assert(audio.nodeRow({ id: 7, name: 'bluez_output' }).name === 'bluez_output', 'audio rows carry the node name that identifies them')
+
 assertEqual(audio.outputVolumeName(0, false), 'Silenced', 'audio labels silent output')
 assertEqual(audio.outputVolumeName(0.9, false), 'Party mode', 'audio labels loud output')
 assertEqual(audio.outputVolumeName(0.5, true), 'Muted', 'audio labels muted output')