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

buffer,n-api: fix double ArrayBuffer::Detach() during cleanup #33039

Closed
wants to merge 1 commit into from
Closed

buffer,n-api: fix double ArrayBuffer::Detach() during cleanup #33039

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

These calls could fail if the ArrayBuffer had already been explicitly
detached at some point in the past.

The necessary test changes already came with 4f523c2 and could
be ported back to v12.x with a backport of this PR.

Fixes: #33022
Refs: #30551

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@addaleax
buffer,n-api: fix double ArrayBuffer::Detach() during cleanup
5a88437 
These calls could fail if the `ArrayBuffer` had already been explicitly
detached at some point in the past.

The necessary test changes already came with 4f523c2 and could
be ported back to v12.x with a backport of this PR.

Fixes: nodejs#33022
Refs: nodejs#30551
@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Apr 24, 2020
@addaleax addaleax mentioned this pull request Apr 24, 2020
Closed
@addaleax addaleax added lts-watch-v12.x node-api Issues and PRs related to the Node-API. labels Apr 24, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30946/

@targos
Copy link
Member

targos commented Apr 24, 2020

Do we want to include this in v12.16.3?

@addaleax
Copy link
Member Author

@targos Sure, I’m okay with that. As you can guess, this is a really low-risk change.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30947/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30953/

mhdawson
mhdawson approved these changes Apr 24, 2020
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 24, 2020 •
edited by addaleax
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/30961/ (:heavy_check_mark:)

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 24, 2020
targos pushed a commit that referenced this pull request Apr 27, 2020
@addaleax @targos
buffer,n-api: fix double ArrayBuffer::Detach() during cleanup
36993c0 
These calls could fail if the `ArrayBuffer` had already been explicitly
detached at some point in the past.

The necessary test changes already came with 4f523c2 and could
be ported back to v12.x with a backport of this PR.

Fixes: #33022
Refs: #30551

PR-URL: #33039
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@targos
Copy link
Member

targos commented Apr 27, 2020

Landed in 36993c0

@targos targos closed this Apr 27, 2020
@targos targos removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lts-watch-v12.x labels Apr 27, 2020
targos pushed a commit that referenced this pull request Apr 27, 2020
@addaleax @targos
buffer,n-api: fix double ArrayBuffer::Detach() during cleanup
39a3cc1 
These calls could fail if the `ArrayBuffer` had already been explicitly
detached at some point in the past.

The necessary test changes already came with 4f523c2 and could
be ported back to v12.x with a backport of this PR.

Fixes: #33022
Refs: #30551

PR-URL: #33039
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@targos targos mentioned this pull request Apr 27, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Apr 27, 2020
@addaleax @BethGriggs
buffer,n-api: fix double ArrayBuffer::Detach() during cleanup
8cde2f9 
These calls could fail if the `ArrayBuffer` had already been explicitly
detached at some point in the past.

The necessary test changes already came with 4f523c2 and could
be ported back to v12.x with a backport of this PR.

Fixes: #33022
Refs: #30551

PR-URL: #33039
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Apr 27, 2020
Merged
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
@addaleax @BridgeAR
buffer,n-api: fix double ArrayBuffer::Detach() during cleanup
9bcfc8e 
These calls could fail if the `ArrayBuffer` had already been explicitly
detached at some point in the past.

The necessary test changes already came with 4f523c2 and could
be ported back to v12.x with a backport of this PR.

Fixes: #33022
Refs: #30551

PR-URL: #33039
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
@addaleax @BridgeAR
buffer,n-api: fix double ArrayBuffer::Detach() during cleanup
18fd841 
These calls could fail if the `ArrayBuffer` had already been explicitly
detached at some point in the past.

The necessary test changes already came with 4f523c2 and could
be ported back to v12.x with a backport of this PR.

Fixes: #33022
Refs: #30551

PR-URL: #33039
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Apr 28, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Apr 28, 2020
@addaleax @BethGriggs
buffer,n-api: fix double ArrayBuffer::Detach() during cleanup
1af08e1 
These calls could fail if the `ArrayBuffer` had already been explicitly
detached at some point in the past.

The necessary test changes already came with 4f523c2 and could
be ported back to v12.x with a backport of this PR.

Fixes: #33022
Refs: #30551

PR-URL: #33039
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Flarna Flarna Flarna approved these changes

@juanarbol juanarbol juanarbol approved these changes

@mhdawson mhdawson mhdawson approved these changes

@legendecas legendecas legendecas approved these changes

@devnexen devnexen devnexen approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

napi_detach_arraybuffer aborts in Node 12.16.2 with an external arraybuffer
9 participants
@addaleax @nodejs-github-bot @targos @jasnell @devnexen @legendecas @mhdawson @juanarbol @Flarna