From 96bbe53634e557aef75fe1b34a9b8651e88cdc98 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Fri, 7 Aug 2026 17:32:36 +0200 Subject: [PATCH] Fix panel delegate segfault and the network panel's open stall (#6605) * fix(network): drop the redundant rescan on the bar click Opening from the bar ran open() and then a bare refresh(). open() already triggers onOpenedChanged -> refresh(true), which defers the PHY scan by disabling the scanner and re-enabling it from scanRestart. The bare refresh() that followed defaults scanWifi to false, so it took the other branch and set wifiDevice.scannerEnabled synchronously on the click frame, undoing the deferral and stalling the open on NetworkManager's access-point flood. It also double-started the DNS and band probes. Co-Authored-By: shrijit Co-Authored-By: Claude Opus 5 (1M context) * fix(network): keep wifi rows QObject-free to prevent a delegate crash wifiRow() embedded the WifiNetwork QObject in the row it returns, and those rows are list-model data, so every delegate held a live QObject wrapper in a var property. When NetworkManager churns the list -- a scan's access-point flood, an AP disappearing -- the object can be destroyed while a delegate is still incubating, and quickshell segfaults in QObjectWrapper::wrap_slowPath on the dangling wrapper. Project primitives only and resolve the backend object at action time via the existing networkForSsid(). Both failNetworkAction() and checkActionCompletion() already no-op on a null network, so a row whose network has since vanished is handled the same way it was before. Co-Authored-By: shrijit Co-Authored-By: Claude Opus 5 (1M context) * fix(bluetooth): keep device rows QObject-free to prevent a delegate crash Same crash class as the wifi rows: scrollRows embedded the BlueZ Device QObject in list-model data, so every delegate held a live wrapper in a var property. Discovery churn -- a scan timeout dropping a device, an unpair -- can destroy the object while a delegate is still incubating, and quickshell segfaults on the dangling wrapper. Project primitives for both the scroll rows and the connected rows, and resolve the backend object by address in deviceFor() for the click actions. The keyboard flow already went through deviceAt(), which reads the live device arrays directly rather than model data, so it is untouched. Co-Authored-By: shrijit Co-Authored-By: Claude Opus 5 (1M context) * fix(network): guard row disconnects against a vanished network Row activation resolved the WifiNetwork with networkForSsid() and passed the result straight to disconnect(), which falls back to connectedWifiNetwork when handed null. A row is a primitive snapshot, so scan churn can remove its backing object while the row is still on screen -- activating it then tore down whatever happened to be connected at that moment rather than doing nothing. Route both row paths through disconnectRow(), which resolves first and only acts when the row still maps to a live network. disconnect() keeps its fallback for callers that mean "drop the current connection". Also covers the bar-click open path, which had no regression: the suite already asserts against Panel.qml source, so assert the closed branch calls open() alone and never a second refresh(). Co-Authored-By: shrijit Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: shrijit Co-authored-by: Claude Opus 5 (1M context) --- shell/plugins/panels/bluetooth/Model.js | 20 +++++++++++ shell/plugins/panels/bluetooth/Panel.qml | 45 ++++++++++++++++++------ shell/plugins/panels/network/Model.js | 6 +++- shell/plugins/panels/network/Panel.qml | 34 +++++++++++++----- test/shell.d/bluetooth-test.sh | 11 ++++++ test/shell.d/network-test.sh | 30 ++++++++++++++++ 6 files changed, 126 insertions(+), 20 deletions(-) diff --git a/shell/plugins/panels/bluetooth/Model.js b/shell/plugins/panels/bluetooth/Model.js index 92961700..55b29bf0 100644 --- a/shell/plugins/panels/bluetooth/Model.js +++ b/shell/plugins/panels/bluetooth/Model.js @@ -80,6 +80,25 @@ function sortedByLabel(devices) { return list } +// Primitives-only projection of a BlueZ device for list-model rows. Holding +// the Device QObject in model data puts a live wrapper into every delegate's +// var property, and BlueZ churn (discovery timeouts, unpair) can destroy the +// object while a delegate is still incubating, which segfaults quickshell. +// Actions resolve the backend object via Panel.deviceFor(). +function deviceRow(d) { + if (!d) return null + return { + address: d.address || "", + name: d.name || "", + deviceName: d.deviceName || "", + connected: !!d.connected, + state: d.state !== undefined ? d.state : -1, + batteryAvailable: !!d.batteryAvailable, + battery: d.battery !== undefined ? d.battery : 0, + pairing: !!d.pairing + } +} + function deviceLists(devices) { var values = toArray(devices) var connected = [] @@ -147,6 +166,7 @@ if (typeof module !== "undefined") { nodeText: nodeText, bluetoothSinkMatchesDevice: bluetoothSinkMatchesDevice, sortedByLabel: sortedByLabel, + deviceRow: deviceRow, deviceLists: deviceLists, cloneMap: cloneMap, pendingAction: pendingAction, diff --git a/shell/plugins/panels/bluetooth/Panel.qml b/shell/plugins/panels/bluetooth/Panel.qml index 845b05b3..a27c7cd7 100644 --- a/shell/plugins/panels/bluetooth/Panel.qml +++ b/shell/plugins/panels/bluetooth/Panel.qml @@ -132,13 +132,36 @@ Panel { readonly property var scrollRows: { var rows = [] for (var k = 0; k < knownDevices.length; k++) - rows.push({ dev: knownDevices[k], section: "known", indexInSection: k }) + rows.push({ dev: Model.deviceRow(knownDevices[k]), section: "known", indexInSection: k }) if (sectionVisible("discovered")) for (var d = 0; d < discoveredDevices.length; d++) - rows.push({ dev: discoveredDevices[d], section: "discovered", indexInSection: d }) + rows.push({ dev: Model.deviceRow(discoveredDevices[d]), section: "discovered", indexInSection: d }) return rows } + // Connected devices render above the scroll area; same primitives-only + // projection so those delegates never hold Device QObject wrappers either. + readonly property var connectedRows: { + var rows = [] + for (var i = 0; i < connectedDevices.length; i++) + rows.push(Model.deviceRow(connectedDevices[i])) + return rows + } + + // Live BlueZ device behind a row. Rows carry primitives only, so actions + // resolve the backend object here rather than holding a wrapper that can + // dangle mid-incubation. `devices` is already the raw device array (see the + // property declaration), so it is iterated directly. + function deviceFor(row) { + if (!row || !row.dev) return null + var addr = row.dev.address || "" + var devs = devices || [] + for (var i = 0; i < devs.length; i++) { + if ((devs[i].address || "") === addr) return devs[i] + } + return null + } + // Flat position of the keyboard cursor, or -1 while it sits on the hero or // in the connected list (both of which live outside the scroll area). readonly property int scrollRowIndex: { @@ -660,7 +683,7 @@ Panel { } Repeater { - model: root.connectedDevices + model: root.connectedRows DeviceRow { required property var modelData required property int index @@ -823,14 +846,15 @@ Panel { } onClicked: function(mouse) { - if (!row.dev) return + var dev = root.deviceFor(row) + if (!dev) return if (mouse.button === Qt.RightButton) { - if (row.isConnected) root.disconnectDevice(row.dev) - else if (!row.isDiscovered) root.forgetDevice(row.dev) + if (row.isConnected) root.disconnectDevice(dev) + else if (!row.isDiscovered) root.forgetDevice(dev) return } - if (row.isConnected) root.disconnectDevice(row.dev) - else root.connectDevice(row.dev) + if (row.isConnected) root.disconnectDevice(dev) + else root.connectDevice(dev) } } @@ -909,8 +933,9 @@ Panel { root.actionFocused = true } onClicked: { - if (!row.dev) return - root.forgetDevice(row.dev) + var dev = root.deviceFor(row) + if (!dev) return + root.forgetDevice(dev) } } } diff --git a/shell/plugins/panels/network/Model.js b/shell/plugins/panels/network/Model.js index ff10f489..01d0f34a 100644 --- a/shell/plugins/panels/network/Model.js +++ b/shell/plugins/panels/network/Model.js @@ -263,8 +263,12 @@ function formatPingLatency(ms, hasSamples) { function wifiRow(network) { if (!network) return null + // Primitives only: rows become list-model data, so a WifiNetwork here puts a + // live QObject wrapper in every delegate's var property. NetworkManager churn + // (scans, AP removals) can destroy the object while a delegate is still + // incubating, which segfaults quickshell in wrap_slowPath on the dangling + // wrapper. Callers that need the object resolve it via networkForSsid(). return { - network: network, connected: !!network.connected, known: !!network.known, ssid: network.name || "", diff --git a/shell/plugins/panels/network/Panel.qml b/shell/plugins/panels/network/Panel.qml index bb160f07..2ed28294 100644 --- a/shell/plugins/panels/network/Panel.qml +++ b/shell/plugins/panels/network/Panel.qml @@ -397,7 +397,10 @@ Panel { var net = wifiNetworks[selectedIndex] if (!net) return if (wifiActionFocused && canForgetNetwork(net)) { forget(net); return } - if (net.connected) { disconnect(net.network); return } + // Only act on a row that still resolves. disconnect() falls back to + // connectedWifiNetwork when handed null, so a row left stale by scan churn + // would otherwise tear down whatever is connected now instead. + if (net.connected) { disconnectRow(net.ssid); return } if (isProtected(net.security) && !net.known) { openPasswordPrompt(net.ssid); return } connectKnown(net.ssid) } @@ -756,8 +759,17 @@ Panel { runNetworkAction("disconnect", network || connectedWifiNetwork, function(net) { net.disconnect() }) } + // Disconnect from a row's SSID. Rows are primitive snapshots that can outlive + // their WifiNetwork, and disconnect()'s null fallback targets whatever is + // connected now, so a stale row must do nothing rather than hit an unrelated + // network. Callers that mean "drop the current connection" call disconnect(). + function disconnectRow(ssid) { + var network = networkForSsid(ssid) + if (network) disconnect(network) + } + function forget(net) { - runNetworkAction("forget", net ? net.network : null, function(network) { network.forget() }) + runNetworkAction("forget", net ? networkForSsid(net.ssid) : null, function(network) { network.forget() }) } implicitWidth: button.implicitWidth @@ -918,7 +930,11 @@ Panel { onPressed: function(b) { if (root.opened) root.close() - else { root.open(); root.refresh() } + // open() is enough: onOpenedChanged runs refresh(true), which defers the + // PHY scan past the first frame. The bare refresh() that used to follow + // took the no-scan branch and set scannerEnabled synchronously, undoing + // that deferral and stalling the open on NetworkManager's AP flood. + else root.open() } } @@ -1577,23 +1593,23 @@ Panel { } Connections { - target: row.net ? row.net.network : null + target: row.net ? root.networkForSsid(row.net.ssid) : null function onConnectionFailed(reason) { // Background auto-connect retries fire this too; only reprompt for // the connect started from this panel. Checked before // failNetworkAction, which clears the action state. var ours = root.actionKind === "connect" && root.actionSsid === (row.net.ssid || "") - root.failNetworkAction(row.net.network, reason) + root.failNetworkAction(root.networkForSsid(row.net.ssid), reason) if (ours && root.shouldRepromptPassphrase(reason, row.isProtected)) root.openPasswordPrompt(row.net.ssid) } function onConnectedChanged() { - if (row.net) root.checkActionCompletion(row.net.network) + if (row.net) root.checkActionCompletion(root.networkForSsid(row.net.ssid)) } function onKnownChanged() { - if (row.net) root.checkActionCompletion(row.net.network) + if (row.net) root.checkActionCompletion(root.networkForSsid(row.net.ssid)) } function onStateChangingChanged() { - if (row.net) root.checkActionCompletion(row.net.network) + if (row.net) root.checkActionCompletion(root.networkForSsid(row.net.ssid)) } } @@ -1642,7 +1658,7 @@ Panel { root.selectedIndex = row.index root.wifiActionFocused = false if (row.isConnected) { - root.disconnect(row.net.network) + root.disconnectRow(row.net.ssid) return } if (row.isProtected && !row.isKnown) { diff --git a/test/shell.d/bluetooth-test.sh b/test/shell.d/bluetooth-test.sh index 9f46f628..9a3b8fe8 100644 --- a/test/shell.d/bluetooth-test.sh +++ b/test/shell.d/bluetooth-test.sh @@ -58,6 +58,17 @@ assertDeepEqual(arrayLikeLists.connected.map(bluetooth.deviceLabel), ['Earbuds'] assertDeepEqual(arrayLikeLists.known.map(bluetooth.deviceLabel), ['Trackpad'], 'bluetooth groups known devices from array-like values') assertDeepEqual(arrayLikeLists.discovered.map(bluetooth.deviceLabel), ['Gamepad'], 'bluetooth groups discovered devices from array-like values') +assertDeepEqual( + bluetooth.deviceRow({ name: 'Deadbeef', address: '1', connected: false }), + { address: '1', name: 'Deadbeef', deviceName: '', connected: false, state: -1, batteryAvailable: false, battery: 0, pairing: false }, + 'bluetooth projects device rows with primitives only' +) +assertEqual( + bluetooth.deviceLabel(bluetooth.deviceRow({ name: 'Generic', deviceName: 'MX Master 3S', address: '2', connected: true })), + 'MX Master 3S', + 'bluetooth keeps deviceName in row projections so labels survive QObject-free rows' +) + assertDeepEqual( bluetooth.withPendingAction({ a: 'connecting' }, 'b', 'forgetting'), { a: 'connecting', b: 'forgetting' }, diff --git a/test/shell.d/network-test.sh b/test/shell.d/network-test.sh index 18007aea..c6b4a947 100644 --- a/test/shell.d/network-test.sh +++ b/test/shell.d/network-test.sh @@ -12,6 +12,24 @@ const panelSource = fs.readFileSync(root + '/shell/plugins/panels/network/Panel. assert(/IpcHandler[\s\S]*?function toggleNetwork\(\) \{ root\.toggleNetwork\(\) \}/.test(panelSource), 'network exposes the Wi-Fi radio toggle over IPC') assert(/manageIpc: false/.test(panelSource), 'network owns its IPC handler so it can extend the target methods') +// Opening from the bar must call open() and nothing else. open() runs +// refresh(true), which defers the PHY scan; a second bare refresh() defaults +// scanWifi to false, sets scannerEnabled synchronously, and stalls the open on +// NetworkManager's access-point flood. +const barPress = panelSource.match(/onPressed: function\(b\) \{[\s\S]*?\n {4}\}/) +assert(barPress, 'network bar button has an onPressed handler') +const barPressCode = barPress[0].replace(/\/\/.*$/gm, '') +assert(!/refresh\(/.test(barPressCode), 'network bar click opens the panel without a second refresh that would undo the deferred scan') + +// A row is a primitive snapshot that can outlive its WifiNetwork, and +// disconnect() falls back to the live connection when handed null, so row +// activation must go through the guarded disconnectRow(). +assert( + /function disconnectRow\(ssid\) \{\s*var network = networkForSsid\(ssid\)\s*if \(network\) disconnect\(network\)/.test(panelSource), + 'network guards row disconnects so a stale row cannot drop an unrelated connection' +) +assert(!/disconnect\(\s*(root\.)?networkForSsid\(/.test(panelSource), 'network never passes an unguarded networkForSsid() lookup to disconnect()') + assertDeepEqual( network.parseNetworkStatus('wifi\tCafe WiFi\t78\t5200\n'), { kind: 'wifi', label: 'Cafe WiFi', signalStrength: 78, frequency: '5200' }, @@ -105,6 +123,18 @@ assertDeepEqual(rows.map(row => row.ssid), ['Connected', 'Known', 'Open'], 'netw assertEqual(network.wifiSectionTitle(rows, 0), 'KNOWN NETWORKS', 'network labels known wifi section') assertEqual(network.wifiSectionTitle(rows, 2), 'OTHER NETWORKS', 'network labels other wifi section') +const wifiRow = network.wifiRow({ connected: true, known: true, name: 'Home', signalStrength: 0.8, security: 1 }) +assertDeepEqual( + wifiRow, + { connected: true, known: true, ssid: 'Home', signal: 80, security: 1 }, + 'network projects wifi rows with primitives so delegates never hold the live WifiNetwork object' +) +assertDeepEqual( + Object.keys(wifiRow).sort(), + ['connected', 'known', 'security', 'signal', 'ssid'], + 'network wifi rows project exactly the primitive fields, so each delegate stores no live QObject' +) + const reasons = { NoSecrets: 1, WifiAuthTimeout: 2, WifiNetworkLost: 3, WifiClientDisconnected: 4, WifiClientFailed: 5 } assertEqual(network.networkFailureReason(1, reasons), 'Passphrase required', 'network maps missing passphrase failures') assertEqual(network.networkFailureReason(2, reasons), 'Wrong password', 'network maps auth timeout failures')