Merge nodejs/node master #245

kunalspathak · 2017-05-16T21:16:17Z

Merge 11918c4 as of 2017-05-16.

Interesting commits to review:

f7b8a7b Build errors fix.
a878d71 Fix N-API JSRT bugs found by new tests. @jasongin helped fixed this. Despite fixing there are still 3 outstanding test failures in test-addons-napi. Jason will fix it, but it will be a while when the fix comes to node-chakracore. I have intentionally not skipping these tests.
f43248f chakrashim, Build errors fix. There were several test cases related to promise and async that I had to mark flaky for now. 😞

Checklist

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

Affected core subsystem(s)

core, chakrashim, test

Removed common.PORT from test-regress-GH-5051 and test-regress-GH-5727 in order to eliminate the possibility of port collision. Refs: nodejs/node#12376 PR-URL: nodejs/node#12639 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>

As indicated by the FIXME comment, this macro guard is no longer needed. PR-URL: nodejs/node#12638 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

PR-URL: nodejs/node#12629 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

This test would currently create HTTP requests to localhost:80 and would time out on machines that actually had an server listening there. To address that, `end()` the requests that are generated. PR-URL: nodejs/node#12627 Ref: nodejs/node#12494 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Removed common.PORT from test-dgram-close-in-listening, test-dgram-close-is-not-callback, test-dgram-close, test-dgram-exclusive-implicit-bind and test-dgram-oob-buffer in order to eliminate the possibility of port collision. Refs: nodejs/node#12376 PR-URL: nodejs/node#12623 Reviewed-By: Anna Henningsen <[email protected]>

We should not use hardcoded string to warn users about file was generated by configure script. Since we already have do_not_edit variable we can use it PR-URL: nodejs/node#12610 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>

Replaced constructor with regular expression for assert.throw(). PR-URL: nodejs/node#12602 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Ref: nodejs/node#12442 PR-URL: nodejs/node#12489 Reviewed-By: Matthew Loring <[email protected]> Reviewed-By: Julien Gilli <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>

Expose `node::AddPromiseHook`, which wraps V8’s `SetPromiseHook` in a way that allows multiple hooks to be set up. PR-URL: nodejs/node#12489 Reviewed-By: Matthew Loring <[email protected]> Reviewed-By: Julien Gilli <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fixes: nodejs/node#10724 PR-URL: nodejs/node#12489 Reviewed-By: Matthew Loring <[email protected]> Reviewed-By: Julien Gilli <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Commit de168b4 ("src: guard bundled_ca/openssl_ca with HAVE_OPENSSL") included an incorrect end macro comment which this commit fixes. PR-URL: nodejs/node#12688 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

- Add a test project to addons-napi that covers the N-API reference and external APIs - Fix a bug in napi_typeof that was found by the new tests PR-URL: nodejs/node#12551 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Use of assert must be lazy to allow errors to be used early before the process is completely set up PR-URL: nodejs/node#11300 Ref: nodejs/node#11273 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

PR-URL: nodejs/node#11300 Ref: nodejs/node#11273 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

When test-cli-node-options is run it uses the --trace-events-enabled option which generates a file named node_trace.1.log. This commit changes the working directory to the test tmp directory to avoid this file being created in the project root. PR-URL: nodejs/node#12660 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Background: To enable N-API support for node versions back to v4, the N-API code can also be built as an external addon. To make maintenance easier, a single codebase needs to support both built-in and external scenarios, along with Node versions >= 4 (and corresponding V8 versions). This change includes several minor fixes to avoid using node internal APIs and support older V8 versions: - Expand node::arraysize - In the CHECK_ENV() macro, return an error code instead of calling node::FatalError(). This is more consistent with how other invalid arguments to N-API functions are handled. - In v8impl::SetterCallbackWrapper::SetReturnValue(), do nothing instead of calling node::FatalError(). This is more consistent with JavaScript setter callbacks, where any returned value is silently ignored. - When queueing async work items, get the uv default loop instead of getting the loop from node::Environment::GetCurrent(). Currently that returns the same loop anyway. If/when node supports multiple environments, it should have a public API for getting the environment & event loop, and we can update this implementation then. - Use v8::Maybe::FromJust() instead of the newer alias ToChecked() PR-URL: nodejs/node#12674 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

PR-URL: nodejs/node#12668 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: James M Snell <[email protected]>

Replace uses of the pronouns `you` and `your` throughout the docs + other minor style nits PR-URL: nodejs/node#12673 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>

The fs watch test was not run on AIX till now because of the known issue, nodejs/node#5085. Now that it is completed, this guard can be removed. PR-URL: nodejs/node#12687 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]>

This commit refactors test-whatwg-url-tojson.js to remove ESLint comments. PR-URL: nodejs/node#12669 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

PR-URL: nodejs/node#12669 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Add specifics as to the level of hardware supported for power platforms. Power 8 is the first processor level to support ppc little endian. Set the minimum level for AIX to Power7 for Node version 8 and later. This will allow the potential to leverage new instructions and optimizations going forward. We have spoken to the AIX team and they agree this makes sense. Earlier processor levels will continue to be supported for version 4.x and 6.x on AIX. PR-URL: nodejs/node#12679 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Richard Lau <[email protected]>

PR-URL: nodejs/node#12683 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Add the initial documentation for the N-API This PR is a result of work in the abi-stable-node repo: https://github.com/nodejs/abi-stable-node/tree/doc, with this PR being the cumulative work on the documentation sections in that repo with the following contributors in alphabetical order: Author: Arunesh Chandra <[email protected]> Author: Gabriel Schulhof <[email protected]> Author: Hitesh Kanwathirtha <[email protected]> Author: Jason Ginchereau <[email protected]> Author: Michael Dawson <[email protected]> Author: Sampson Gao <[email protected]> Author: Taylor Woll <[email protected]> PR-URL: nodejs/node#12549 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

PR-URL: nodejs/node#12681 Reviewed-By: Vse Mozhet Byt <[email protected]>

Use the more efficient module.exports = {} pattern, restructure imports from bindings, requires. PR-URL: nodejs/node#12681 Reviewed-By: Vse Mozhet Byt <[email protected]>

Found that libuv had a path character limit of 108. Used path.relative() to set prefix with relative path. Also added comments explaining the use of a relative path. PR-URL: nodejs/node#12601 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>

* Update an example according to an actual REPL session. * Replace an arrow function with a common function to hold `this`. PR-URL: nodejs/node#12684 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Delete unused method call. PR-URL: nodejs/node#12685 Refs: nodejs/node#6171 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Fix c0bde73, which inadvertently introduced a use of strcmp() without correctly comparing its return to zero. Caught by coverity: >>> CID 169223: Integer handling issues (CONSTANT_EXPRESSION_RESULT) >>> The "or" condition "strcmp(arg, "--abort-on-uncaught-exception") || strcmp(arg, "--abort_on_uncaught_exception")" will always be true because "arg" cannot be equal to two different values at the same time, so it must be not equal to at least one of them. 3909 } else if (strcmp(arg, "--abort-on-uncaught-exception") || 3910 strcmp(arg, "--abort_on_uncaught_exception")) { 3911 abort_on_uncaught_exception = true; 3912 // Also a V8 option. Pass through as-is. 3913 new_v8_argv[new_v8_argc] = arg; 3914 new_v8_argc += 1; PR-URL: nodejs/node#13004 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Richard Lau <[email protected]>

kfarnung · 2017-05-16T21:31:43Z

Just a nit for the new shims:

f43248f#diff-913fb686e1d16cb72858e61d72d46b8cR998

The approach that seems to be standard in the codebase is that continuations from the previous line are indented to either:

line up with the parameters on the previous line
indent 4 spaces

kfarnung

LGTM with minor nits

jasongin · 2017-05-16T23:21:50Z

The addons-napi/test_async failure appears to be a chakrashim issue that just happens to have been uncovered by the test; it is not actually N-API-specific.

The problem seems to be that node::EmitBeforeExit() tries to allocate a string while in an exception state. JsCreateString() returns JsErrorInExceptionState, and that leads to a fatal error: "V8::ToLocalChecked: Empty MaybeLocal". Does node::EmitBeforeExit() need to clear the JSRT exception state before doing more work?

This test case is checking the behavior of an unhandled JS exception thrown from a libuv async callback. See addons-napi/test_async/test.js:13.

Here is the call stack where the error originated:

>	chakracore.dll!ContextAPINoScriptWrapper_Core<_JsErrorCode <lambda>(Js::ScriptContext *) >(ContextAPINoScriptWrapper::__l2::_JsErrorCode <lambda>(Js::ScriptContext *) fn, bool allowInObjectBeforeCollectCallback, bool scriptExceptionAllowed) Line 242	C++
 	chakracore.dll!ContextAPINoScriptWrapper<_JsErrorCode <lambda>(Js::ScriptContext *, TTD::TTDJsRTActionResultAutoRecorder &) >(JsPointerToString::__l2::_JsErrorCode <lambda>(Js::ScriptContext *, TTD::TTDJsRTActionResultAutoRecorder &) fn, bool allowInObjectBeforeCollectCallback, bool scriptExceptionAllowed) Line 291	C++
 	chakracore.dll!JsPointerToString(const wchar_t * stringValue, unsigned __int64 stringLength, void * * string) Line 1189	C++
 	chakracore.dll!JsCreateString(const char * content, unsigned __int64 length, void * * value) Line 4105	C++
 	node.exe!v8::String::NewFromOneByte(v8::Isolate * isolate, const unsigned char * data, v8::NewStringType type, int length) Line 218	C++
 	node.exe!node::OneByteString(v8::Isolate * isolate, const char * data, int length) Line 205	C++
 	node.exe!node::EmitBeforeExit(node::Environment * env) Line 4498	C++
 	node.exe!node::Start(v8::Isolate * isolate, void * isolate_context, int argc, const char * const * argv, int exec_argc, const char * const * exec_argv) Line 4639	C++
 	node.exe!node::Start(uv_loop_s * event_loop, int argc, const char * const * argv, int exec_argc, const char * const * exec_argv) Line 4732	C++
 	node.exe!node::Start(int argc, char * * argv) Line 4971	C++
 	node.exe!wmain(int argc, wchar_t * * wargv) Line 71	C++

kunalspathak · 2017-05-16T23:40:55Z

@jasongin - Thanks for looking. I will investigate and fix this one.

The immediate.js benchmark with `type` set to `depth` measures the same thing as set-immediate-depth.js. Remove the redundancy.` PR-URL: nodejs/node#13009 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Currently when running make targets the directory is printed on some operating systems (Linux for example): $ make lint make[1]: Entering directory '/work/node' Running JS linter... ./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules --ext=.js,.md \ benchmark doc lib test tools make[1]: Leaving directory '/work/node' make[1]: Entering directory '/work/node' Running C++ linter... On other operating systems the directory is not printed. This commit suggests adding a flag to make this consistent for GNUMake by not printing the directory. PR-URL: nodejs/node#13042 Reviewed-By: Gibson Fahnestock <[email protected]>

Currently when building --without-ssl this test will report the following error: internal/util.js:82 throw new Error('Node.js is not compiled with openssl crypto support'); This commit adds a check for crypto and skips this test if node was built without ssl support. PR-URL: nodejs/node#13041 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

PR-URL: nodejs/node#13034 Ref: nodejs/node#13031 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rajaram Gaunker <[email protected]>

kunalspathak · 2017-05-17T19:10:06Z

Spoke offline with @jasongin . The problem is GetAndClearException() today happens either when Scope is popped or in dtor of v8::TryCatch. The napi jsrt APIs has to implement TryCatch like model where it will throw unhandled exception and clear it from context.

kunalspathak · 2017-05-17T22:12:33Z

Rebased. CI - https://ci.nodejs.org/job/chakracore-test-pull-request/37/

jasongin · 2017-05-17T22:54:16Z

All the test failures are fixed here: jasongin@a32568a

If you don't want to take that commit now, I'll submit it as a PR separately.

kunalspathak · 2017-05-17T22:58:41Z

Once my PR is merged, could you please send a separate PR? Also re-enable the test that i will be skipping in my PR.

jasongin · 2017-05-17T22:59:32Z

OK

kunalspathak · 2017-05-18T00:21:58Z

New CI - https://ci.nodejs.org/job/chakracore-test-pull-request/39/

Merge ef16319 as of 2017-05-16 PR-URL: nodejs#245 Reviewed-By: Kyle Farnung <[email protected]>

* Added shim for `Promise::Resolver` and some minor additions. * Skipped several unit test around promises and async hooks PR-URL: nodejs#245 Reviewed-By: Kyle Farnung <[email protected]>

Merge 11918c4 as of 2017-05-16 PR-URL: nodejs#245 Reviewed-By: Kyle Farnung <[email protected]>

PR-URL: nodejs#245 Reviewed-By: Kyle Farnung <[email protected]>

* Added jsEngine to newly added test * Skipped napi test PR-URL: nodejs#245 Reviewed-By: Kyle Farnung <[email protected]>

Merge '6342988053' as of 2017-05-17 into xplat PR-URL: nodejs#245 Reviewed-By: Kyle Farnung <[email protected]>

* Added shim for `Promise::Resolver` and some minor additions. * Skipped several unit test around promises and async hooks PR-URL: nodejs#245 Reviewed-By: Kyle Farnung <[email protected]>

PR-URL: nodejs#245 Reviewed-By: Kyle Farnung <[email protected]>

Sebastian Plesciuc and others added 30 commits April 27, 2017 17:09

errors, child_process: migrate to using internal/errors

7632761

PR-URL: nodejs/node#11300 Ref: nodejs/node#11273 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

test: use common.mustCall in test-https-strict

f11d4a1

PR-URL: nodejs/node#12668 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: James M Snell <[email protected]>

doc: document url.domainTo* methods separately

3a0058c

PR-URL: nodejs/node#12683 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

doc: add Added-in metadata for WHATWG URL

9d4bb55

PR-URL: nodejs/node#12683 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

benchmark: add benchmark for v8.getHeap*Statistics

bed4612

PR-URL: nodejs/node#12681 Reviewed-By: Vse Mozhet Byt <[email protected]>

v8: refactor struture of v8 module

1052383

Use the more efficient module.exports = {} pattern, restructure imports from bindings, requires. PR-URL: nodejs/node#12681 Reviewed-By: Vse Mozhet Byt <[email protected]>

doc: update example in repl.md

d8f8096

Delete unused method call. PR-URL: nodejs/node#12685 Refs: nodejs/node#6171 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

kunalspathak requested review from digitalinfinity and kfarnung May 16, 2017 21:16

kfarnung approved these changes May 16, 2017

View reviewed changes

Trott and others added 4 commits May 16, 2017 21:38

kunalspathak force-pushed the FI branch from 7108d5a to ed9ec1f Compare May 17, 2017 22:11

kunalspathak and others added 7 commits May 17, 2017 18:09

Merge nodejs/node into xplat

ca32671

Merge ef16319 as of 2017-05-16 PR-URL: nodejs#245 Reviewed-By: Kyle Farnung <[email protected]>

chakrashim, test: Build failures and skipped test

bfbf736

* Added shim for `Promise::Resolver` and some minor additions. * Skipped several unit test around promises and async hooks PR-URL: nodejs#245 Reviewed-By: Kyle Farnung <[email protected]>

Merge nodejs/node into xplat

297b176

Merge 11918c4 as of 2017-05-16 PR-URL: nodejs#245 Reviewed-By: Kyle Farnung <[email protected]>

Fix N-API JSRT bugs found by new tests

16922b4

PR-URL: nodejs#245 Reviewed-By: Kyle Farnung <[email protected]>

chakrashim: Fixed build errors

75c91ca

PR-URL: nodejs#245 Reviewed-By: Kyle Farnung <[email protected]>

build,test: Fixed test and build issue

a8d0006

* Added jsEngine to newly added test * Skipped napi test PR-URL: nodejs#245 Reviewed-By: Kyle Farnung <[email protected]>

Merge nodejs/master to xplat

d07825c

Merge '6342988053' as of 2017-05-17 into xplat PR-URL: nodejs#245 Reviewed-By: Kyle Farnung <[email protected]>

kunalspathak force-pushed the FI branch from caebc05 to d07825c Compare May 18, 2017 01:15

kunalspathak merged commit d07825c into nodejs:xplat May 18, 2017

kunalspathak deleted the FI branch May 18, 2017 01:16

kfarnung pushed a commit to kfarnung/node-chakracore that referenced this pull request May 23, 2017

Fix N-API JSRT bugs found by new tests

fa0f01d

PR-URL: nodejs#245 Reviewed-By: Kyle Farnung <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge nodejs/node master #245

Merge nodejs/node master #245

kunalspathak commented May 16, 2017

kfarnung commented May 16, 2017

kfarnung left a comment

jasongin commented May 16, 2017

kunalspathak commented May 16, 2017

kunalspathak commented May 17, 2017

kunalspathak commented May 17, 2017

jasongin commented May 17, 2017

kunalspathak commented May 17, 2017

jasongin commented May 17, 2017

kunalspathak commented May 18, 2017

Merge nodejs/node master #245

Merge nodejs/node master #245

Conversation

kunalspathak commented May 16, 2017

Checklist

Affected core subsystem(s)

kfarnung commented May 16, 2017

kfarnung left a comment

Choose a reason for hiding this comment

jasongin commented May 16, 2017

kunalspathak commented May 16, 2017

kunalspathak commented May 17, 2017

kunalspathak commented May 17, 2017

jasongin commented May 17, 2017

kunalspathak commented May 17, 2017

jasongin commented May 17, 2017

kunalspathak commented May 18, 2017