Skip to content

fix: delay JS environment release to prevent V8 global double free on exit #48377

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
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: delay JS environment release to prevent V8 global double free on exit #48377

wants to merge 1 commit into from
+7 −1

Conversation

loufultoncz-coder
Copy link

@loufultoncz-coder loufultoncz-coder commented Sep 25, 2025 •
edited
Loading

Description of Change

fixes: #47999
maybe fixes: #33041

This PR introduces a delayed release of the js_env to prevent double free issues with V8 global instances when Electron exits.

Currently, when closing an Electron window while certain asynchronous operations (such as process.getProcessMemoryInfo()) are active, a double free can occur in V8. The root cause is that JavascriptEnvironment is released before pending PromiseBase objects, which hold V8 global references, are properly disposed.

This change ensures that the JS environment is released only after all asynchronous references have completed, preventing crashes related to premature disposal of V8 globals.

Motivation and Context
  • Electron's shutdown sequence currently destroys the JavascriptEnvironment before all PromiseBase objects are freed.
  • This leads to a crash with a reproducible stack trace in v8::internal::GlobalHandles::NodeSpace<NodeType>::Free().
  • Delaying the release of JavascriptEnvironment ensures proper order of destruction, maintaining stability.

Crash Reproduction:

In main.js, the following code reliably triggers the double free on exit:

setInterval(() => {
  process.getProcessMemoryInfo().then((r) => {
    console.log(r)
  });
}, 4);

Checklist

Release Notes

Notes: fix: delay JS environment release to prevent V8 global double free on exit

@welcome Welcome
Copy link

welcome bot commented Sep 25, 2025

💖 Thanks for opening this pull request! 💖

Semantic PR titles

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Commit signing

This repo enforces commit signatures for all incoming PRs.
To sign your commits, see GitHub's documentation on Telling Git about your signing key.

PR tips

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Sep 25, 2025
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Oct 2, 2025
ckerr
ckerr requested changes Oct 3, 2025
View reviewed changes
Copy link
Member

@ckerr ckerr left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferring the JS Env's shutdown could be a plausible approach?

I would rank this (and any PR that changes order-of-destruction during shutdown) as high risk because of difficult-to-predict side effects. Just to make one example, this change also means Pinnables are destroyed later, so we should make sure none of their destructors rely on anything that might now be destroyed because of this timing change.

Before I start doing any due diligence on timing side-effects, I'd like to at least know that this fixes the bug it reports to fix, which is difficult since nobody's submitted a repro case for 47999.

I know timing issues are difficult to repro, but still -- is there no way to create a fiddle that I could run 100 times in a loop before & after this change to confirm the fix?

@codebytere codebytere requested a review from deepak1556 October 3, 2025 18:10
@deepak1556
Copy link
Member

Agree with @ckerr, delaying environment exit is not the right solution and only adds more problems.

  1. If the class attempting to enter JS can be managed by V8, then its lifetime is clearly defined
  2. If the class is not managed by V8, then we should a signal like env->is_shutting_down to avoid entering JS

In this case, we can make the Promise class managed by V8 specifically with the work happening for #47922

If you want to take a stab at converting the class feel free to, otherwise I would recommend closing this PR and await for transition as a proper fix to the underlying issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckerr ckerr ckerr requested changes

@deepak1556 deepak1556 Awaiting requested review from deepak1556

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Double Free During Shutdown process.getProcessMemoryInfo() crash the main process
4 participants
@loufultoncz-coder @deepak1556 @ckerr @Inuyasha-666