fix(bec_widget): release cell widgets from item views on cleanup

bec_widgets/utils/bec_widget.py

@@ -9,9 +9,16 @@ from typing import TYPE_CHECKING
 
 import shiboken6
 from bec_lib.logger import bec_logger
-from qtpy.QtCore import QBuffer, QByteArray, QIODevice, QObject, Qt, QTimer
+from qtpy.QtCore import SIGNAL, SLOT, QBuffer, QByteArray, QIODevice, QObject, Qt, QTimer
 from qtpy.QtGui import QFont, QPixmap
-from qtpy.QtWidgets import QApplication, QFileDialog, QLabel, QVBoxLayout, QWidget
+from qtpy.QtWidgets import (
+    QAbstractItemView,
+    QApplication,
+    QFileDialog,
+    QLabel,
+    QVBoxLayout,
+    QWidget,
+)
 
 import bec_widgets.widgets.containers.qt_ads as QtAds
 from bec_widgets.utils.bec_connector import BECConnector, ConnectionConfig
@@ -57,6 +64,35 @@ def _forget_destroyed_widget(gui_id: str, *_args) -> None:
             logger.warning(f"Dead-slot sweep for destroyed widget {gui_id} failed: {exc}")
 
 
+def _forget_in_item_view_on_destroy(widget: QObject) -> None:
+    """
+    Make the item view that holds ``widget`` as a cell widget drop it when it is destroyed.
+
+    Item views keep raw pointers to the widgets placed in their cells with setCellWidget,
+    setItemWidget or setIndexWidget, and only track the destruction of editors their delegate
+    created. Deleting such a cell widget directly (e.g. from the cleanup of a BEC ancestor)
+    leaves a dangling pointer behind, and the next layout pass of the view
+    (QAbstractItemView::updateEditorGeometries) crashes. This makes the same
+    destroyed -> editorDestroyed connection Qt makes for delegate editors, so the view
+    forgets the widget as soon as it is destroyed, by whatever path.
+    """
+    if not isinstance(widget, QWidget) or not shiboken6.isValid(widget):
+        return
+    viewport = widget.parentWidget()
+    view = viewport.parentWidget() if viewport is not None else None
+    if not isinstance(view, QAbstractItemView) or view.viewport() is not viewport:
+        return
+    # String-based on purpose: editorDestroyed is a protected C++ slot, and the call must stay
+    # in C++ because it happens while the widget is being destroyed.
+    QObject.connect(
+        widget,
+        SIGNAL("destroyed(QObject*)"),
+        view,
+        SLOT("editorDestroyed(QObject*)"),
+        Qt.ConnectionType.UniqueConnection,
+    )
+
+
 class BECWidget(BECConnector):
     """Mixin class for all BEC widgets, to handle cleanup"""
 
@@ -428,6 +464,9 @@ class BECWidget(BECConnector):
         theme_connection = getattr(self, "_theme_connection", None)
         if theme_connection is not None:
             theme_connection.invalidate()
+        # A BEC widget living in a table/tree/list cell is deleted after cleanup (by a BEC
+        # ancestor or deleteLater); the owning item view must not keep a dangling pointer.
+        _forget_in_item_view_on_destroy(self)
         with RPCRegister.delayed_broadcast():
             # All widgets need to call super().cleanup() in their cleanup method
             logger.info(f"Registry cleanup for widget {self.__class__.__name__}")
@@ -487,6 +526,8 @@ class BECWidget(BECConnector):
                 self.cleanup()
             except Exception:
                 logger.exception(f"Cleanup on deleteLater failed for {self.__class__.__name__}")
+        # Also when cleanup failed early: the item view holding this widget must forget it.
+        _forget_in_item_view_on_destroy(self)
         super().deleteLater()  # pylint: disable=no-member
 
     def closeEvent(self, event):

tests/unit_tests/test_bec_widget_cell_widgets.py

@@ -0,0 +1,240 @@
+"""
+BECWidget.cleanup() must not leave item views with dangling pointers to the BEC widgets
+it deletes from their cells (setCellWidget / setItemWidget / setIndexWidget).
+
+An item view keeps raw pointers to its index widgets. If cleanup deletes such a widget
+without the view releasing it first, the next layout pass of the view
+(QAbstractItemView::updateEditorGeometries) dereferences freed memory and the process
+dies with SIGSEGV. Every scenario therefore runs in a child pytest process, so a native
+crash fails one test here instead of taking down the whole session.
+"""
+
+import os
+import subprocess
+import sys
+
+import pytest
+import shiboken6
+from bec_lib import messages
+from qtpy.QtCore import QEvent
+from qtpy.QtWidgets import (
+    QApplication,
+    QListWidget,
+    QListWidgetItem,
+    QTableWidget,
+    QTreeWidget,
+    QTreeWidgetItem,
+    QVBoxLayout,
+    QWidget,
+)
+
+from bec_widgets.tests.client_mocks import dap_plugin_message, mocked_client, mocked_client_with_dap
+from bec_widgets.utils.bec_widget import BECWidget
+from bec_widgets.widgets.control.buttons.button_abort.button_abort import AbortButton
+from bec_widgets.widgets.plots.waveform.settings.curve_settings.curve_tree import CurveTree
+from bec_widgets.widgets.plots.waveform.waveform import Waveform
+from bec_widgets.widgets.services.bec_queue.bec_queue import BECQueue
+
+from .conftest import create_widget
+
+INNER = "BW_CELL_WIDGET_INNER"
+inner_only = pytest.mark.skipif(
+    os.environ.get(INNER) != "1", reason="runs only inside the child process"
+)
+
+
+def _flush_deferred_deletes():
+    QApplication.sendPostedEvents(None, QEvent.Type.DeferredDelete)
+    QApplication.processEvents()
+
+
+class CellBECWidget(BECWidget, QWidget):
+    """Minimal BEC widget used as a cell widget."""
+
+    PLUGIN = False
+    RPC = False
+
+
+class ViewHost(BECWidget, QWidget):
+    """A BEC widget whose table, tree and list hold BEC widgets as cell widgets."""
+
+    PLUGIN = False
+    RPC = False
+
+    def __init__(self, parent=None, client=None, **kwargs):
+        super().__init__(parent=parent, client=client, **kwargs)
+        layout = QVBoxLayout(self)
+        self.table = QTableWidget(3, 2, self)
+        self.tree = QTreeWidget(self)
+        self.tree.setColumnCount(2)
+        self.list = QListWidget(self)
+        for view in (self.table, self.tree, self.list):
+            layout.addWidget(view)
+
+        self.cells = []
+        for row in range(3):
+            table_cell = CellBECWidget(parent=self, client=client)
+            self.table.setCellWidget(row, 1, table_cell)
+
+            tree_item = QTreeWidgetItem(self.tree, [f"row {row}"])
+            tree_cell = CellBECWidget(parent=self, client=client)
+            self.tree.setItemWidget(tree_item, 1, tree_cell)
+
+            list_item = QListWidgetItem(self.list)
+            list_cell = CellBECWidget(parent=self, client=client)
+            self.list.setItemWidget(list_item, list_cell)
+
+            self.cells += [table_cell, tree_cell, list_cell]
+
+    def views(self):
+        return (self.table, self.tree, self.list)
+
+
+def _queue_entry(scan_number: int, rid: str, status: str = "PENDING") -> dict:
+    """Build a single primary-queue entry with one request block."""
+    msg = messages.ScanQueueMessage(
+        metadata={"RID": rid}, scan_type="line_scan", parameter={"args": {}, "kwargs": {}}
+    )
+    return {
+        "queue_id": f"queue-{scan_number}",
+        "scan_id": [f"scan-{scan_number}"],
+        "is_scan": [True],
+        "request_blocks": [
+            {
+                "msg": msg,
+                "RID": rid,
+                "readout_priority": {"monitored": [], "baseline": [], "on_request": []},
+                "is_scan": True,
+                "scan_number": scan_number,
+                "scan_id": f"scan-{scan_number}",
+            }
+        ],
+        "scan_number": [scan_number],
+        "status": status,
+    }
+
+
+def _three_row_queue_content() -> dict:
+    content = {
+        "primary": {
+            "info": [
+                _queue_entry(1001, "rid-1001", status="RUNNING"),
+                _queue_entry(1002, "rid-1002"),
+                _queue_entry(1003, "rid-1003"),
+            ],
+            "status": "RUNNING",
+        }
+    }
+    return messages.ScanQueueStatusMessage(metadata={}, queue=content).content
+
+
+#####################################################################
+# Scenarios, executed only inside the child process
+#####################################################################
+
+
+@inner_only
+def test_inner_view_host_close_releases_cell_widgets(qtbot, mocked_client):
+    host = create_widget(qtbot, ViewHost, client=mocked_client)
+    cells = list(host.cells)
+    assert all(isinstance(cell, BECWidget) for cell in cells)
+
+    host.close()  # BECWidget.cleanup() closes and deletes every BECWidget child
+    _flush_deferred_deletes()
+    for view in host.views():
+        view.updateEditorGeometries()  # SIGSEGV if a view still holds a deleted cell widget
+
+    # cleanup still releases the cell widgets, and the views no longer reference them
+    assert not any(shiboken6.isValid(cell) for cell in cells)
+    assert all(host.table.cellWidget(row, 1) is None for row in range(3))
+    for row in range(3):
+        assert host.tree.itemWidget(host.tree.topLevelItem(row), 1) is None
+        assert host.list.itemWidget(host.list.item(row)) is None
+
+
+@inner_only
+def test_inner_nested_view_close_releases_cell_widgets(qtbot, mocked_client):
+    """The view sits deeper in the closed widget, below a plain (non-BEC) container."""
+    outer = create_widget(qtbot, CellBECWidget, client=mocked_client)
+    container = QWidget(outer)
+    QVBoxLayout(outer).addWidget(container)
+    table = QTableWidget(3, 1, container)
+    QVBoxLayout(container).addWidget(table)
+    cells = [CellBECWidget(client=mocked_client) for _ in range(3)]
+    for row, cell in enumerate(cells):
+        table.setCellWidget(row, 0, cell)
+
+    outer.close()
+    _flush_deferred_deletes()
+    table.updateEditorGeometries()
+
+    assert not any(shiboken6.isValid(cell) for cell in cells)
+    assert all(table.cellWidget(row, 0) is None for row in range(3))
+
+
+@inner_only
+def test_inner_bec_queue_close_then_relayout(qtbot, mocked_client):
+    queue = BECQueue(client=mocked_client, refresh_upon_start=False)
+    qtbot.addWidget(queue)
+    qtbot.waitExposed(queue)
+    queue.update_queue(_three_row_queue_content(), {})
+    assert queue.table.rowCount() == 3
+    buttons = [queue.table.cellWidget(row, 4) for row in range(3)]
+    assert all(isinstance(button, AbortButton) for button in buttons)
+
+    queue.close()
+    _flush_deferred_deletes()
+    queue.table.updateEditorGeometries()  # SIGSEGV on the deleted abort buttons
+
+    assert not any(shiboken6.isValid(button) for button in buttons)
+    assert all(queue.table.cellWidget(row, 4) is None for row in range(3))
+
+
+@inner_only
+def test_inner_curve_tree_close_then_relayout(qtbot, mocked_client_with_dap):
+    """Guard: CurveTree keeps BEC combo boxes in tree cells and removes its rows on cleanup."""
+    wf = create_widget(qtbot, Waveform, client=mocked_client_with_dap)
+    curve_tree = create_widget(qtbot, CurveTree, parent=None, waveform=wf)
+    first = curve_tree.add_new_curve(device="bpm4i", signal="bpm4i")
+    curve_tree.add_new_curve(device="samx", signal="samx")
+    first.add_dap_row()
+    cell_widgets = [
+        w
+        for w in (first.device_edit, first.entry_edit, first.scan_index_combo, first.color_button)
+        if isinstance(w, BECWidget)
+    ]
+    assert cell_widgets
+
+    curve_tree.close()
+    _flush_deferred_deletes()
+    curve_tree.tree.updateEditorGeometries()
+
+    assert not any(shiboken6.isValid(w) for w in cell_widgets)
+
+
+#####################################################################
+# Child-process wrappers (the tests that actually run in the suite)
+#####################################################################
+
+
+@pytest.mark.parametrize(
+    "scenario",
+    [
+        "test_inner_view_host_close_releases_cell_widgets",
+        "test_inner_nested_view_close_releases_cell_widgets",
+        "test_inner_bec_queue_close_then_relayout",
+        "test_inner_curve_tree_close_then_relayout",
+    ],
+)
+def test_cell_widgets_survive_owner_cleanup(scenario):
+    env = dict(os.environ, **{INNER: "1"})
+    proc = subprocess.run(
+        [sys.executable, "-m", "pytest", "-p", "no:cacheprovider", "-q", f"{__file__}::{scenario}"],
+        env=env,
+        capture_output=True,
+        text=True,
+        timeout=300,
+        check=False,
+    )
+    print(proc.stdout[-3000:], proc.stderr[-3000:])
+    assert proc.returncode == 0, f"child exited with {proc.returncode} (negative = signal)"