Napi handle scope mismatch #17161

neta-kedem · 2017-11-20T21:01:00Z

#goodness_squad

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

TimothyGu

Congrats on your first contribution! Left a few comments, but other than those it should be good to go.

TimothyGu · 2017-11-20T21:12:12Z

tools/doc/node_modules/js-yaml/index.js


-module.exports = require(realJSYaml);
+module.exports = yaml;


Stray change.

This is happening when you run make doc … we really should get rid of these npm installs :(

Or .gitignore them, everyone making a PR yesterday ran into this

If I'm not totally mistaken, adding an already tracked file to .gitignore has no effect 😞 . Something like git update-index --assume-unchanged would work, but every developer has to run that locally, not really a solution for this problem.

TimothyGu · 2017-11-20T21:12:59Z

src/node_api.cc

@@ -898,7 +898,8 @@ const char* error_messages[] = {nullptr,
                                "Unknown failure",
                                "An exception is pending",
                                "The async work item was cancelled",
-                                "napi_escape_handle already called on scope"};
+                                "napi_escape_handle already called on scope",
+                                "invalid handle scope usage"};


Capitalize "invalid" in accordance with the rest of error_messages.

The one immediately before this one is an exception, as napi_escape_handle is the name of a function, and function names are case-sensitive.

benjamingr

Thanks for doing this :)

refack · 2017-11-20T21:18:35Z

Hello @neta-kedem and תודה רבה.

refack · 2017-11-20T21:21:01Z

@neta-kedem You might want to set you local git Name & e-mail according to https://help.github.com/articles/setting-your-commit-email-address-in-git/ so that GitHub will recognize that these commits are yours.

neta-kedem · 2017-11-20T21:34:29Z

Thanks! I set them correctly and amended

…de into napi_handle_scope_mismatch

TimothyGu · 2017-11-20T23:47:02Z

#17161 (comment) still seems to be a problem after the latest amendment...

cjihrig

Changes to src/node_api.cc LGTM

mhdawson

LGTM once stray change to tools/doc/node_modules/js-yaml/index.js is removed

neta-kedem · 2017-11-21T20:01:40Z

I undid the changes in the node_modules index file

aqrln · 2017-11-22T08:56:47Z

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

mhdawson · 2017-11-22T14:45:03Z

Neither of those tests should be affected by this change so believe we are good to land.

mhdawson · 2017-11-22T15:06:07Z

In the process of landing

mhdawson · 2017-11-22T15:11:06Z

Landed as a4a9a3d

PR-URL: #17161 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>

PR-URL: nodejs#17161 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>

Backport-PR-URL: #19447 PR-URL: #17161 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>

neta added 2 commits November 20, 2017 22:39

src: updating status message list for napi_handle_scope_mismatch

178bb51

src: fixing the size of error_messages to last_error+1

0301123

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 20, 2017

neta-kedem mentioned this pull request Nov 20, 2017

Missing status message list update for napi_handle_scope_mismatch #16973

Closed

TimothyGu approved these changes Nov 20, 2017

View reviewed changes

benjamingr approved these changes Nov 20, 2017

View reviewed changes

refack approved these changes Nov 20, 2017

View reviewed changes

refack added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 20, 2017

src: capitalize error message- Invalid handle scope usage

ad4bd24

src: capitalize error message- Invalid handle scope usage

9bbfe39

neta-kedem added 3 commits November 20, 2017 23:41

Merge branch 'napi_handle_scope_mismatch' of github.com:neta-kedem/no…

3ec5489

…de into napi_handle_scope_mismatch

set email and username

0c70186

set email and username

cd89655

addaleax approved these changes Nov 20, 2017

View reviewed changes

cjihrig approved these changes Nov 21, 2017

View reviewed changes

jasnell approved these changes Nov 21, 2017

View reviewed changes

mhdawson approved these changes Nov 21, 2017

View reviewed changes

tools: undid the change in the index file

774ddd1

neta-kedem closed this Nov 21, 2017

neta-kedem reopened this Nov 21, 2017

aqrln approved these changes Nov 22, 2017

View reviewed changes

aqrln added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 22, 2017

fhinkel approved these changes Nov 22, 2017

View reviewed changes

mhdawson self-assigned this Nov 22, 2017

mhdawson closed this Nov 22, 2017

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 28, 2017

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

MylesBorins added the dont-land-on-v6.x label Dec 19, 2017

gibfahn added the land-on-v8.x label Dec 19, 2017

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17772

Closed

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17774

Merged

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Napi handle scope mismatch #17161

Napi handle scope mismatch #17161

neta-kedem commented Nov 20, 2017

TimothyGu left a comment

TimothyGu Nov 20, 2017

addaleax Nov 20, 2017

benjamingr Nov 21, 2017

fhinkel Nov 21, 2017

TimothyGu Nov 20, 2017

benjamingr left a comment

refack commented Nov 20, 2017

refack commented Nov 20, 2017

neta-kedem commented Nov 20, 2017

TimothyGu commented Nov 20, 2017

cjihrig left a comment

mhdawson left a comment

neta-kedem commented Nov 21, 2017

aqrln commented Nov 22, 2017

mhdawson commented Nov 22, 2017 •

edited

Loading

mhdawson commented Nov 22, 2017

mhdawson commented Nov 22, 2017


		module.exports = require(realJSYaml);
		module.exports = yaml;

Napi handle scope mismatch #17161

Napi handle scope mismatch #17161

Conversation

neta-kedem commented Nov 20, 2017

Checklist

Affected core subsystem(s)

TimothyGu left a comment

Choose a reason for hiding this comment

TimothyGu Nov 20, 2017

Choose a reason for hiding this comment

addaleax Nov 20, 2017

Choose a reason for hiding this comment

benjamingr Nov 21, 2017

Choose a reason for hiding this comment

fhinkel Nov 21, 2017

Choose a reason for hiding this comment

TimothyGu Nov 20, 2017

Choose a reason for hiding this comment

benjamingr left a comment

Choose a reason for hiding this comment

refack commented Nov 20, 2017

refack commented Nov 20, 2017

neta-kedem commented Nov 20, 2017

TimothyGu commented Nov 20, 2017

cjihrig left a comment

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

neta-kedem commented Nov 21, 2017

aqrln commented Nov 22, 2017

mhdawson commented Nov 22, 2017 • edited Loading

mhdawson commented Nov 22, 2017

mhdawson commented Nov 22, 2017

mhdawson commented Nov 22, 2017 •

edited

Loading