fix(dom): break frame-loop closure Rc cycle in auto_update cleanup#357
Open
ademilsonfp wants to merge 1 commit into
Open
fix(dom): break frame-loop closure Rc cycle in auto_update cleanup#357ademilsonfp wants to merge 1 commit into
ademilsonfp wants to merge 1 commit into
Conversation
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
Fixes #349.
auto_updatebuilds a self-rescheduling animation-frame closure that is storedin an
Rc<RefCell<Option<Closure>>>and captures a clone of that sameRcsoit can reschedule itself, forming a strong reference cycle. The cleanup returned
by
auto_updateonly cancelled the pending animation frame and never clearedthe cell, so the cycle was never broken — every call leaked the closure together
with the reference element clone and the
updatecallback it captures. Theclosure is built unconditionally, so the leak happened in both
animation_framemodes.
Fix
Keep a separate handle to the cell and drop its contents in the returned
cleanup, after the animation frame is cancelled (so the closure can no
longer run before it is dropped). This breaks the cycle and lets the closure and
everything it captures be freed.
Safety
The pending frame is cancelled before the closure is dropped, so there is no
use-after-free in wasm, and external cleanup never runs concurrently with a RAF
tick.
Verification
cargo check -p floating-ui-dom --target wasm32-unknown-unknowncompiles.cargo clippy -p floating-ui-domreports no new warnings.Note
A sibling instance of the same self-referential-
Rc-closure pattern exists inobserve_move'srefresh_closure(see the "Possibly related" section of #349).It is trickier because its cleanup is also invoked from inside the refresh
closure, so it is intentionally left out of this PR and can be addressed
separately.