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

n-api: free instance data as reference #31638

Conversation

gabrielschulhof
Copy link
Contributor

Instance data associated with a napi_env is no longer stored on the
env itself but is instead rendered as a reference. Since
v8impl::Reference is tied to a JS object, this modification factors
out the v8impl::Reference refcounting and the deletion process into
a base class for v8impl::Reference, called v8impl::RefBase. The
instance data is then stored as a v8impl::RefBase, along with other
references, preventing a segfault that arises from the fact that, up
until now, upon napi_env destruction, the instance data was freed
after all references had already been forcefully freed. If the addon
freed a reference during the napi_set_instance_data finalizer
callback, such a reference had already been freed during environment
teardown, causing a double free.

Re: nodejs/node-addon-api#663

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 4, 2020
@gabrielschulhof gabrielschulhof added node-api-semver-major semver-major changes that need to be considered for N-API node-api Issues and PRs related to the Node-API. and removed node-api-semver-major semver-major changes that need to be considered for N-API labels Feb 4, 2020
@gabrielschulhof gabrielschulhof force-pushed the finalize-instance-data-via-ref-tracker branch from 1aa5958 to fff128c Compare February 4, 2020 22:54
virtual ~RefTracker() {}
virtual void Finalize(bool isEnvTeardown) {}

typedef RefTracker RefList;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can replace this entire custom linked list implementation by ListHead/ListNode from util.h? That works pretty well for other parts of the codebase :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I filed #31639 for this.

src/js_native_api_v8.cc Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@gabrielschulhof gabrielschulhof force-pushed the finalize-instance-data-via-ref-tracker branch from 69169fd to 5042714 Compare February 6, 2020 18:23
Instance data associated with a `napi_env` is no longer stored on the
env itself but is instead rendered as a reference. Since
`v8impl::Reference` is tied to a JS object, this modification factors
out the `v8impl::Reference` refcounting and the deletion process into
a base class for `v8impl::Reference`, called `v8impl::RefBase`. The
instance data is then stored as a `v8impl::RefBase`, along with other
references, preventing a segfault that arises from the fact that, up
until now, upon `napi_env` destruction, the instance data was freed
after all references had already been forcefully freed. If the addon
freed a reference during the `napi_set_instance_data` finalizer
callback, such a reference had already been freed during environment
teardown, causing a double free.

Re: nodejs/node-addon-api#663
@gabrielschulhof gabrielschulhof force-pushed the finalize-instance-data-via-ref-tracker branch from 5042714 to 38b4b6f Compare February 6, 2020 18:30
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

gabrielschulhof pushed a commit that referenced this pull request Feb 6, 2020
Instance data associated with a `napi_env` is no longer stored on the
env itself but is instead rendered as a reference. Since
`v8impl::Reference` is tied to a JS object, this modification factors
out the `v8impl::Reference` refcounting and the deletion process into
a base class for `v8impl::Reference`, called `v8impl::RefBase`. The
instance data is then stored as a `v8impl::RefBase`, along with other
references, preventing a segfault that arises from the fact that, up
until now, upon `napi_env` destruction, the instance data was freed
after all references had already been forcefully freed. If the addon
freed a reference during the `napi_set_instance_data` finalizer
callback, such a reference had already been freed during environment
teardown, causing a double free.

Re: nodejs/node-addon-api#663
PR-URL: #31638
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
@gabrielschulhof
Copy link
Contributor Author

Landed in 884e287.

@nodejs-github-bot
Copy link
Collaborator

codebytere pushed a commit that referenced this pull request Feb 17, 2020
Instance data associated with a `napi_env` is no longer stored on the
env itself but is instead rendered as a reference. Since
`v8impl::Reference` is tied to a JS object, this modification factors
out the `v8impl::Reference` refcounting and the deletion process into
a base class for `v8impl::Reference`, called `v8impl::RefBase`. The
instance data is then stored as a `v8impl::RefBase`, along with other
references, preventing a segfault that arises from the fact that, up
until now, upon `napi_env` destruction, the instance data was freed
after all references had already been forcefully freed. If the addon
freed a reference during the `napi_set_instance_data` finalizer
callback, such a reference had already been freed during environment
teardown, causing a double free.

Re: nodejs/node-addon-api#663
PR-URL: #31638
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
@codebytere codebytere mentioned this pull request Feb 17, 2020
codebytere pushed a commit that referenced this pull request Mar 15, 2020
Instance data associated with a `napi_env` is no longer stored on the
env itself but is instead rendered as a reference. Since
`v8impl::Reference` is tied to a JS object, this modification factors
out the `v8impl::Reference` refcounting and the deletion process into
a base class for `v8impl::Reference`, called `v8impl::RefBase`. The
instance data is then stored as a `v8impl::RefBase`, along with other
references, preventing a segfault that arises from the fact that, up
until now, upon `napi_env` destruction, the instance data was freed
after all references had already been forcefully freed. If the addon
freed a reference during the `napi_set_instance_data` finalizer
callback, such a reference had already been freed during environment
teardown, causing a double free.

Re: nodejs/node-addon-api#663
PR-URL: #31638
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
Instance data associated with a `napi_env` is no longer stored on the
env itself but is instead rendered as a reference. Since
`v8impl::Reference` is tied to a JS object, this modification factors
out the `v8impl::Reference` refcounting and the deletion process into
a base class for `v8impl::Reference`, called `v8impl::RefBase`. The
instance data is then stored as a `v8impl::RefBase`, along with other
references, preventing a segfault that arises from the fact that, up
until now, upon `napi_env` destruction, the instance data was freed
after all references had already been forcefully freed. If the addon
freed a reference during the `napi_set_instance_data` finalizer
callback, such a reference had already been freed during environment
teardown, causing a double free.

Re: nodejs/node-addon-api#663
PR-URL: #31638
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
@codebytere codebytere mentioned this pull request Mar 17, 2020
codebytere pushed a commit that referenced this pull request Mar 30, 2020
Instance data associated with a `napi_env` is no longer stored on the
env itself but is instead rendered as a reference. Since
`v8impl::Reference` is tied to a JS object, this modification factors
out the `v8impl::Reference` refcounting and the deletion process into
a base class for `v8impl::Reference`, called `v8impl::RefBase`. The
instance data is then stored as a `v8impl::RefBase`, along with other
references, preventing a segfault that arises from the fact that, up
until now, upon `napi_env` destruction, the instance data was freed
after all references had already been forcefully freed. If the addon
freed a reference during the `napi_set_instance_data` finalizer
callback, such a reference had already been freed during environment
teardown, causing a double free.

Re: nodejs/node-addon-api#663
PR-URL: #31638
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
@gabrielschulhof gabrielschulhof deleted the finalize-instance-data-via-ref-tracker branch April 21, 2020 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants