Skip to content

Fix self-capturing-lambda memory leaks in context menus and ColorPickerDialog#96

Open
dovvnloading wants to merge 1 commit into
mainfrom
hotfix/context-menu-and-colorpicker-lambda-leaks
Open

Fix self-capturing-lambda memory leaks in context menus and ColorPickerDialog#96
dovvnloading wants to merge 1 commit into
mainfrom
hotfix/context-menu-and-colorpicker-lambda-leaks

Conversation

@dovvnloading

Copy link
Copy Markdown
Owner

Summary

An adversarial bug-scan workflow found the same lambda-GC-cycle pattern already fixed for worker-owning nodes (PR #93) in an unrelated feature area: right-click context menus and the frame/container/note "change color" popup.

Root cause, identical to #93: a Python lambda closing over self, connected to a Qt signal on a widget owned by self (a QAction/QPushButton child of the menu/dialog). PySide6's GC does not reclaim this shape — a bound-method connection to the same signal is reclaimed fine, a lambda is not — so every menu/dialog instance ever constructed leaked forever, for the rest of the process.

Found and fixed in four places

  • ChatNodeContextMenu: the 5 export-format actions, the per-docked-node "reveal" actions, and the per-chart-type actions all used lambda ...: self.something(...). Also pinned the ChatNode itself alive forever via self.node, on every right-click. (The reveal/chart-type lambdas weren't in the original report — found by grepping this file for the full pattern rather than trusting only the cited line numbers.)
  • CodeNodeContextMenu / DocumentNodeContextMenu: the same 5-action export-menu shape.
  • ColorPickerDialog: the "reset to default" button and every palette swatch button. Notably not limited to custom Signals or worker threads — a stock QPushButton.clicked calling a plain method (no Signal.emit involved) forms the identical GC-invisible cycle.

Fix

Moved the per-instance variant onto the widget itself (QAction.setData() / QWidget.setProperty()) and connected every action/button to one shared bound-method dispatcher that reads self.sender().data()/.property(...) — removes the self-capturing closure without changing behavior.

Test plan

  • Verified empirically for all four (real menu/dialog + real canvas node, no mocks) that both the widget and, for the three context menus, the node it stores now collect cleanly via weakref + gc.collect()
  • New tests confirm every export/reveal/chart/swatch action still dispatches to the correct variant
  • Full suite: 1730 passed, zero regressions

Follow-up to the recent adversarial bug-scan series (#91#95).

…erDialog

An adversarial bug-scan workflow found the same lambda-GC-cycle pattern
already fixed for worker-owning nodes (PR #93) in an unrelated feature
area: right-click context menus and the frame/container/note "change
color" popup.

Root cause, identical to #93: a Python lambda closing over self,
connected to a Qt signal on a widget owned by self (a QAction/QPushButton
child of the menu/dialog). PySide6's GC does not reclaim this shape - a
bound-method connection to the same signal is reclaimed fine, a lambda
is not - so every menu/dialog instance ever constructed leaked forever,
for the rest of the process.

Confirmed and fixed in four places:
- ChatNodeContextMenu: the 5 export-format actions, the per-docked-node
  "reveal" actions, and the per-chart-type actions all used
  `lambda ...: self.something(...)`. Also pinned the ChatNode itself
  alive forever via self.node, on EVERY right-click.
- CodeNodeContextMenu / DocumentNodeContextMenu: the same 5-action
  export-menu shape (found by grepping the identical code pattern, not
  just trusting the report's line numbers - both matched exactly).
- ColorPickerDialog: the "reset to default" button and every palette
  swatch button. Notably NOT limited to custom Signals or worker
  threads - a stock QPushButton.clicked calling a plain method (no
  Signal.emit involved) forms the identical GC-invisible cycle.

Fixed by moving the per-instance variant onto the widget itself
(QAction.setData() / QWidget.setProperty()) and connecting every
action/button to ONE shared bound-method dispatcher that reads
self.sender().data()/.property(...) - removes the self-capturing
closure without changing behavior.

Verified empirically for all four (real menu/dialog + real canvas node,
no mocks - weakref + gc.collect() after construction) that both the
widget and, for the three context menus, the node it stores now
collect cleanly. New tests also confirm every export/reveal/chart/
swatch action still dispatches to the correct variant. Full suite 1730
passed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant