Skip to content
This repository was archived by the owner on Oct 15, 2020. It is now read-only.

Merge nodejs/master #321

Merged
kunalspathak merged 10 commits into from
Jun 29, 2017
Merged

Merge nodejs/master #321

kunalspathak merged 10 commits into from
Jun 29, 2017

Conversation

@kunalspathak
Copy link
Member

@kunalspathak kunalspathak commented Jun 29, 2017

Merge 3e17884 as of 2017-06-22.

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

core, n-api, test

Eugene Ostroukhov and others added 7 commits June 21, 2017 11:20
inspector, test: Fix test bug detected by Coverity
3d0ef56 
Error value was not checked. Turns out, uv_ip6_addr was actually called
on malformed IP (square brackets should not have been included).

PR-URL: nodejs/node#13799
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax
doc: mention rebasing of v?.x-staging post release
d102769 
Mention that the staging branch should be rebased on top of the release
branch after merging a release proposal.

PR-URL: nodejs/node#13742
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@addaleax
stream: fix undefined in Readable object mode
9a655e9 
Fixes `this.push(undefined)`.

Fixes: nodejs/node#13753
PR-URL: nodejs/node#13760
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Calvin Metcalf <calvin.metcalf@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@mhdawson
n-api: avoid crash in napi_escape_scope()
3e18c49 
V8 will crash if escape is called twice on the same
scope.

Add checks to avoid crashing if napi_escape_scope() is
called to try and do this.

Add test that tries to call napi_create_scope() twice.

PR-URL: nodejs/node#13651
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@vsemozhetbyt @jasnell
doc: fix nits in guides/using-internal-errors.md
259466c 
PR-URL: nodejs/node#13820
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: David Cai <davidcai1993@yahoo.com>
@Rob--W @jasnell
doc: add missing zlib link to stream API docs
2a46e57 
This seems to have been removed inadvertently by
330c8d7 in PR 12925.

PR-URL: nodejs/node#13838
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR @refack
errors: improve ERR_INVALID_ARG_TYPE
3e17884 
The error message might be misleading if an object property
was the issue and not the argument itself.

Fix this by checking if a argument or a property is passed
to the handler function.

PR-URL: nodejs/node#13730
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
kfarnung
kfarnung reviewed Jun 29, 2017
View reviewed changes
src/node_api_jsrt.cc

static_assert(
sizeof(error_messages) / sizeof(*error_messages) == napi_status_last,
node::arraysize(error_messages) == napi_escape_called_twice + 1,
Copy link
Contributor

@kfarnung kfarnung Jun 29, 2017

Choose a reason for hiding this comment

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

There's an upstream PR to eliminate node_internal.h from node_api.h, we should probably avoid taking any further dependency on it.

nodejs/node#13892

Copy link
Contributor

@kfarnung kfarnung Jun 29, 2017

Choose a reason for hiding this comment

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

Although taking another look it seems like there's some discussion ongoing, so we can leave this for now.

Copy link
Member Author

@kunalspathak kunalspathak Jun 29, 2017

Choose a reason for hiding this comment

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

Yeah, i just adopted this changes from node_api.cc.

Copy link
Contributor

@kfarnung kfarnung Jun 29, 2017

Choose a reason for hiding this comment

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

Yeah, that PR was closed now and we're going a different direction. LGTM as is.

kfarnung
kfarnung approved these changes Jun 29, 2017
View reviewed changes
Copy link
Contributor

@kfarnung kfarnung left a comment

Choose a reason for hiding this comment

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

LGTM with minor changes.

chakrabot and others added 3 commits June 29, 2017 11:51
@chakrabot @kunalspathak
Merge nodejs/master
3326b07 
Merge 3e17884 as of 2017-06-22.
This is an automatically created merge. For any problems please
contact @kunalspathak.

PR-URL: nodejs#321
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
@kunalspathak
n-api: removed napi_status_last usage
53b8790 
Removed usage of `napi_status_last` from `node_api_jsrt.cc` and replaced with
`node::array`

PR-URL: nodejs#321
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
@kunalspathak
test: skip a check in test_handle_scope
ffa055e 
jsrt napi doesn't implement `napi_open_escapable_handle_scope`.
When we escape handle scope twice, we should throw, but there is
no place to store the information.

Hence skipping the check for now.

PR-URL: nodejs#321
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
@kunalspathak kunalspathak merged commit ffa055e into nodejs:master Jun 29, 2017
kfarnung pushed a commit to kfarnung/node-chakracore that referenced this pull request Jul 5, 2017
@kunalspathak @kfarnung
n-api: removed napi_status_last usage
b324fa8 
Removed usage of `napi_status_last` from `node_api_jsrt.cc` and replaced with
`node::array`

PR-URL: nodejs#321
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
kfarnung pushed a commit to kfarnung/node-chakracore that referenced this pull request Jul 6, 2017
@kunalspathak @kfarnung
test: skip a check in test_handle_scope
fd8ca0d 
jsrt napi doesn't implement `napi_open_escapable_handle_scope`.
When we escape handle scope twice, we should throw, but there is
no place to store the information.

Hence skipping the check for now.

PR-URL: nodejs#321
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
kfarnung pushed a commit to kfarnung/node-chakracore that referenced this pull request Jul 6, 2017
@kunalspathak @kfarnung
n-api: removed napi_status_last usage
89800a5 
Removed usage of `napi_status_last` from `node_api_jsrt.cc` and replaced with
`node::array`

PR-URL: nodejs#321
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
kfarnung pushed a commit to kfarnung/node-chakracore that referenced this pull request Jul 6, 2017
@kunalspathak @kfarnung
test: skip a check in test_handle_scope
74c0740 
jsrt napi doesn't implement `napi_open_escapable_handle_scope`.
When we escape handle scope twice, we should throw, but there is
no place to store the information.

Hence skipping the check for now.

PR-URL: nodejs#321
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jasongin jasongin Awaiting requested review from jasongin

1 more reviewer

@kfarnung kfarnung kfarnung approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@kunalspathak @kfarnung @addaleax @mhdawson @vsemozhetbyt @Rob--W @BridgeAR @chakrabot