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.

Sign up for GitHub

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

Jump to bottom

fix: crash when using vm after window.open() #37506

Closed
wants to merge 1 commit into from
Closed

fix: crash when using vm after window.open() #37506

wants to merge 1 commit into from

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Mar 6, 2023 •
edited

Description of Change

Closes #37404.
Refs #32057.

Fixes a potential crash when using the vm module in the renderer process after calling window.open(''). This started happening after nodejs/node#44252, which we rolled into Electron via 75d2caf. It appears that eagerly loading the Node.js environment when opening a child window with a blank url also requires a delay, otherwise Node.js isn't able to properly set up the v8::Context for use with vm.

Checklist

Release Notes

Notes: Fixed a potential crash when using the vm module in the renderer process after calling window.open('').

@codebytere codebytere requested a review from zcbenz March 6, 2023 13:29
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Mar 6, 2023
@codebytere codebytere mentioned this pull request Mar 6, 2023
Closed
3 tasks
@codebytere codebytere added semver/patch backwards-compatible bug fixes target/22-x-y PR should also be added to the "22-x-y" branch. target/23-x-y PR should also be added to the "23-x-y" branch. target/24-x-y PR should also be added to the "24-x-y" branch. labels Mar 6, 2023
@codebytere codebytere changed the title fix: crash when using vm after window.open() fix: crash when using vm after window.open() Mar 6, 2023
@codebytere codebytere removed the target/22-x-y PR should also be added to the "22-x-y" branch. label Mar 6, 2023
@codebytere codebytere marked this pull request as ready for review March 6, 2023 13:49
@codebytere codebytere marked this pull request as draft March 6, 2023 15:18
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Mar 7, 2023
@jkleinsc jkleinsc added the target/25-x-y PR should also be added to the "25-x-y" branch. label Apr 6, 2023
@codebytere codebytere closed this Apr 19, 2023
@codebytere codebytere deleted the fix-window-open-crash branch April 19, 2023 09:50
@gpetrov
Copy link

gpetrov commented Apr 19, 2023

@codebytere what happened here? Were the changes no good? Is there an alternative solution?
This is still a major road block for us.

@gpetrov
Copy link

gpetrov commented May 11, 2023

@codebytere does this fix got merged? As I don't see it anywhere and the problem still persist in Electron 23,24 and 25

@gpetrov gpetrov mentioned this pull request Jun 11, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zcbenz zcbenz Awaiting requested review from zcbenz

@nornagon nornagon Awaiting requested review from nornagon

@MarshallOfSound MarshallOfSound Awaiting requested review from MarshallOfSound

Assignees
No one assigned
Labels
semver/patch backwards-compatible bug fixes target/23-x-y PR should also be added to the "23-x-y" branch. target/24-x-y PR should also be added to the "24-x-y" branch. target/25-x-y PR should also be added to the "25-x-y" branch.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: proxyquire can't load modules after window.open method is called
3 participants
@codebytere @gpetrov @jkleinsc