Clear karaxEvents after detaching handlers to stop per-redraw closure leak - #303
Merged
Merged
Conversation
… leak removeAllEventHandlers called removeEventListener for each recorded handler but never emptied the karaxEvents bookkeeping array, so every re-attach cycle (mergeEvents, once per event-carrying node per redraw) appended another wrapped-handler tuple forever. Each leaked closure pins its render generation's whole VNode tree via wrapEvent's captured n, and the detach loop re-walks the growing array on every redraw. Adds a regression case to tests/difftest.nim driving mergeEvents 10x against a stub event target: karaxEvents must not grow. Fixes karaxnim#302
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
2 participants
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.
Fixes #302
removeAllEventHandlersdetaches each handler recorded in the node'skaraxEventsbookkeeping array (added in the #139 fix) but never emptiesthe array, while
addEventShellappends on every attach. SinceupdateElementrunsmergeEventsfor every visited event-carrying node onevery redraw, each such node accumulates one wrapped-handler tuple per
redraw, forever:
VNode tree via
wrapEvent's capturedn;The fix resets the array after detaching. One line, plus a regression case
in
tests/difftest.nim(runs in the existingnim js -d:nodejs -r tests/difftest.nimCI step): it drivesmergeEvents10× against a stubevent target and asserts
karaxEventsdoesn't grow. On unpatched master itfails with "karaxEvents grew to 11 entries after 10 event re-merges";
with the fix the suite passes.