n-api: free instance data as reference #31638
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
the
c++
gabrielschulhof
added
node-api-semver-major
gabrielschulhof
force-pushed
the
finalize-instance-data-via-ref-tracker
branch
from
1aa5958
to
fff128c
Compare
addaleax
approved these changes
Feb 4, 2020
| virtual ~RefTracker() {} | ||
| virtual void Finalize(bool isEnvTeardown) {} | ||
|
|
||
| typedef RefTracker RefList; |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
I filed #31639 for this.
This was referenced Feb 4, 2020
gabrielschulhof
force-pushed
the
finalize-instance-data-via-ref-tracker
branch
from
fff128c
to
69169fd
Compare
devnexen
approved these changes
Feb 5, 2020
gabrielschulhof
force-pushed
the
finalize-instance-data-via-ref-tracker
branch
from
69169fd
to
5042714
Compare
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
force-pushed
the
finalize-instance-data-via-ref-tracker
branch
from
5042714
to
38b4b6f
Compare
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>
|
Landed in 884e287. |
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>
Merged
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>
Merged
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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instance data associated with a
napi_envis no longer stored on theenv itself but is instead rendered as a reference. Since
v8impl::Referenceis tied to a JS object, this modification factorsout the
v8impl::Referencerefcounting and the deletion process intoa base class for
v8impl::Reference, calledv8impl::RefBase. Theinstance data is then stored as a
v8impl::RefBase, along with otherreferences, preventing a segfault that arises from the fact that, up
until now, upon
napi_envdestruction, the instance data was freedafter all references had already been forcefully freed. If the addon
freed a reference during the
napi_set_instance_datafinalizercallback, 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), orvcbuild test(Windows) passes