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

module: rework of memory management in vm APIs with the importModuleDynamically option #48510

Closed
wants to merge 3 commits into from

Commits on Sep 12, 2023

  1. module: use symbol in WeakMap to manage host defined options

    Previously when managing the importModuleDynamically callback of
    vm.compileFunction(), we use an ID number as the host defined option
    and maintain a per-Environment ID -> CompiledFnEntry map to retain
    the top-level referrer function returned by vm.compileFunction() in
    order to pass it back to the callback, but it would leak because with
    how we used v8::Persistent to maintain this reference, V8 would not
    be able to understand the cycle and would just think that the
    CompiledFnEntry was supposed to live forever. We made an attempt
    to make that reference known to V8 by making the CompiledFnEntry weak
    and using a private symbol to make CompiledFnEntry strongly
    references the top-level referrer function in
    nodejs#46785, but that turned out to be
    unsound, because the there's no guarantee that the top-level function
    must be alive while import() can still be initiated from that
    function, since V8 could discard the top-level function and only keep
    inner functions alive, so relying on the top-level function to keep
    the CompiledFnEntry alive could result in use-after-free which caused
    a revert of that fix.
    
    With this patch we use a symbol in the host defined options instead of
    a number, because with the stage-3 symbol-as-weakmap-keys proposal
    we could directly use that symbol to keep the referrer alive using a
    WeakMap. As a bonus this also keeps the other kinds of referrers
    alive as long as import() can still be initiated from that
    Script/Module, so this also fixes the long-standing crash caused by
    vm.Script being GC'ed too early when its importModuleDynamically
    callback still needs it.
    joyeecheung committed Sep 12, 2023
    Configuration menu
    Copy the full SHA
    684efdc View commit details
    Browse the repository at this point in the history
  2. module: fix leak of vm.SyntheticModule

    Previously we maintain a strong persistent reference to the
    ModuleWrap to retrieve the ID-to-ModuleWrap mapping from
    the HostImportModuleDynamicallyCallback using the number ID
    stored in the host-defined options. As a result the ModuleWrap
    would be kept alive until the Environment is shut down, which
    would be a leak for user code. With the new symbol-based
    host-defined option we can just get the ModuleWrap from the
    JS-land WeakMap so there's now no need to maintain this
    strong reference. This would at least fix the leak for
    vm.SyntheticModule. vm.SourceTextModule is still leaking
    due to the strong persistent reference to the v8::Module.
    joyeecheung committed Sep 12, 2023
    Configuration menu
    Copy the full SHA
    3df92b1 View commit details
    Browse the repository at this point in the history

Commits on Sep 13, 2023

  1. module: fix the leak in SourceTextModule and ContextifySript

    Replace the persistent handles to v8::Module and
    v8::UnboundScript with an internal reference that V8's GC is
    aware of to fix the leaks.
    joyeecheung committed Sep 13, 2023
    Configuration menu
    Copy the full SHA
    b439ba6 View commit details
    Browse the repository at this point in the history