Fix self-capturing-lambda memory leaks in context menus and ColorPickerDialog#96
Open
dovvnloading wants to merge 1 commit into
Open
Fix self-capturing-lambda memory leaks in context menus and ColorPickerDialog#96dovvnloading wants to merge 1 commit into
dovvnloading wants to merge 1 commit into
Conversation
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 byself(aQAction/QPushButtonchild 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 usedlambda ...: self.something(...). Also pinned theChatNodeitself alive forever viaself.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 stockQPushButton.clickedcalling a plain method (noSignal.emitinvolved) 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 readsself.sender().data()/.property(...)— removes the self-capturing closure without changing behavior.Test plan
weakref+gc.collect()Follow-up to the recent adversarial bug-scan series (#91–#95).