fix(napi): memory leak in PromiseRaw cleanup callback#2995
fix(napi): memory leak in PromiseRaw cleanup callback#2995Brooooooklyn merged 4 commits intomainfrom
Conversation
This commit fixes a memory leak that occurred when converting a Promise from an Unknown object, specifically manifesting in Deno environments. The issue was in the `promise_callback_finalizer` function, which had a type mismatch when cleaning up callback allocations: 1. The callback was stored as `Box<(Cb, *mut bool)>` (a tuple containing the callback and an executed flag pointer) 2. The finalizer incorrectly cast it as `Box<Cb>` when trying to free it 3. This caused improper deallocation and memory leaks The fix ensures: - The executed flag allocation is always properly cleaned up - The callback tuple is cast to the correct type `(Cb, *mut bool)` before being dropped - All allocations are properly freed in both execution paths (callback executed or not executed) Fixes #2926
How to use the Graphite Merge QueueAdd the label ready-to-merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a memory management bug in the promise callback finalizer where the cleanup logic had incorrect type casts. The finalizer now properly cleans up both the execution flag allocation and the callback tuple.
- Fixed the finalizer to always clean up the executed flag allocation instead of conditionally
- Corrected the type cast for the callback cleanup from
Cbto(Cb, *mut bool)to match the actual data structure - Added clarifying comments explaining the cleanup logic
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
cursor review |
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This commit fixes a memory leak that occurred when converting a Promise from an Unknown object, specifically manifesting in Deno environments.
The issue was in the
promise_callback_finalizerfunction, which had a type mismatch when cleaning up callback allocations:Box<(Cb, *mut bool)>(a tuple containing the callback and an executed flag pointer)Box<Cb>when trying to free itThe fix ensures:
(Cb, *mut bool)before being droppedFixes #2926
Note
Fix memory leak by always freeing the executed-flag and correctly dropping
(Cb, *mut bool)when the Promise callback was never executed.crates/napi/src/bindgen_runtime/js_values/promise_raw.rs:promise_callback_finalizerto always free theexecutedflag (*mut bool).finalize_hintasBox<(Cb, *mut bool)>instead ofBox<Cb>to free all allocations.Written by Cursor Bugbot for commit e1cfa4c. This will update automatically on new commits. Configure here.