Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: in Emit(), don't leak converted Arg Local<Values> into caller's scope #43729

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

ckerr
Copy link
Member

@ckerr ckerr commented Sep 16, 2024

Description of Change

The two Emit(isolate, obj, name, Args&&... args) methods convert args into an array of Local<Value>s to pass to Node/V8. But they don't use a HandleScope, so these temporary handles are leaked into the caller's scopes.

In most calls this is OK since either args is empty or since the caller creates a scope right before emission, but not everywhere. It's more reliable to handle this in the Emit() functions, so this PR adds EscapableHandleScopes to plug the leaks.

Checklist

Release Notes

Notes: none.

@ckerr ckerr added semver/patch backwards-compatible bug fixes target/30-x-y PR should also be added to the "30-x-y" branch. target/31-x-y PR should also be added to the "31-x-y" branch. target/32-x-y PR should also be added to the "32-x-y" branch. target/33-x-y PR should also be added to the "33-x-y" branch. labels Sep 16, 2024
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Sep 16, 2024
@codebytere codebytere changed the title fix: in Emit(), don't leak converted Arg Local<Values> into caller's scope fix: in Emit(), don't leak converted Arg Local<Values> into caller's scope Sep 16, 2024
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Sep 17, 2024
@ckerr ckerr merged commit 5a76655 into main Sep 17, 2024
67 checks passed
@ckerr ckerr deleted the fix/emit-leaks-converted-arg-handles branch September 17, 2024 14:00
Copy link

release-clerk bot commented Sep 17, 2024

No Release Notes

@trop
Copy link
Contributor

trop bot commented Sep 17, 2024

I have automatically backported this PR to "33-x-y", please check out #43746

@trop trop bot added in-flight/33-x-y and removed target/33-x-y PR should also be added to the "33-x-y" branch. labels Sep 17, 2024
@trop
Copy link
Contributor

trop bot commented Sep 17, 2024

I have automatically backported this PR to "32-x-y", please check out #43747

@trop
Copy link
Contributor

trop bot commented Sep 17, 2024

I have automatically backported this PR to "31-x-y", please check out #43748

@trop
Copy link
Contributor

trop bot commented Sep 17, 2024

I have automatically backported this PR to "30-x-y", please check out #43749

@trop trop bot added in-flight/32-x-y in-flight/31-x-y in-flight/30-x-y and removed target/32-x-y PR should also be added to the "32-x-y" branch. target/31-x-y PR should also be added to the "31-x-y" branch. target/30-x-y PR should also be added to the "30-x-y" branch. labels Sep 17, 2024
@trop trop bot added merged/33-x-y PR was merged to the "33-x-y" branch. merged/30-x-y PR was merged to the "30-x-y" branch. merged/32-x-y PR was merged to the "32-x-y" branch. merged/31-x-y PR was merged to the "31-x-y" branch. and removed in-flight/33-x-y in-flight/30-x-y in-flight/32-x-y in-flight/31-x-y labels Sep 17, 2024
yangannyx pushed a commit to yangannyx/electron that referenced this pull request Oct 21, 2024
…r's scope (electron#43729)

fix: Emit() should not leak converted arg handles into caller's HandleScope
yangannyx pushed a commit to yangannyx/electron that referenced this pull request Oct 21, 2024
…r's scope (electron#43729)

fix: Emit() should not leak converted arg handles into caller's HandleScope
yangannyx pushed a commit to yangannyx/electron that referenced this pull request Oct 21, 2024
…r's scope (electron#43729)

fix: Emit() should not leak converted arg handles into caller's HandleScope
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/30-x-y PR was merged to the "30-x-y" branch. merged/31-x-y PR was merged to the "31-x-y" branch. merged/32-x-y PR was merged to the "32-x-y" branch. merged/33-x-y PR was merged to the "33-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants