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.

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

src: make StreamBase prototype accessors more robust #16860

Closed
wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

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)

src

This problem showed up when I was stepping into Writable.prototype.write and hover over this from a console.log call, there was assertion like this:

see assertion
node[90480]: ../src/util-inl.h:243:TypeName *node::Unwrap(v8::Local<v8::Object>) [TypeName = node::LibuvStreamWrap]: Assertion `(object->InternalFieldCount()) > (0)' failed.
 1: node::Abort() [/Users/joyee/.tnvm/versions/node/v9.0.0/bin/node]
 2: node::(anonymous namespace)::DomainEnter(node::Environment*, v8::Local<v8::Object>) [/Users/joyee/.tnvm/versions/node/v9.0.0/bin/node]
 3: node::LibuvStreamWrap::~LibuvStreamWrap() [/Users/joyee/.tnvm/versions/node/v9.0.0/bin/node]
 4: void node::StreamBase::GetBytesRead<node::LibuvStreamWrap>(v8::Local<v8::String>, v8::PropertyCallbackInfo<v8::Value> const&) [/Users/joyee/.tnvm/versions/node/v9.0.0/bin/node]
 5: v8::internal::PropertyCallbackArguments::Call(void (*)(v8::Local<v8::Name>, v8::PropertyCallbackInfo<v8::Value> const&), v8::internal::Handle<v8::internal::Name>) [/Users/joyee/.tnvm/versions/node/v9.0.0/bin/node]
 6: v8::internal::Object::GetPropertyWithAccessor(v8::internal::LookupIterator*) [/Users/joyee/.tnvm/versions/node/v9.0.0/bin/node]
 7: v8::internal::JSReceiver::GetOwnPropertyDescriptor(v8::internal::LookupIterator*, v8::internal::PropertyDescriptor*) [/Users/joyee/.tnvm/versions/node/v9.0.0/bin/node]
 8: v8::internal::JSReceiver::GetOwnPropertyDescriptor(v8::internal::Isolate*, v8::internal::Handle<v8::internal::JSReceiver>, v8::internal::Handle<v8::internal::Object>, v8::internal::PropertyDescriptor*) [/Users/joyee/.tnvm/versions/node/v9.0.0/bin/node]
 9: v8::internal::Builtin_Impl_ObjectGetOwnPropertyDescriptor(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/Users/joyee/.tnvm/versions/node/v9.0.0/bin/node]
10: 0x3f95

because the inspector would wrap the inspected project's prototype when generating previews. This is also reproducible when util.inspect a prototype setup by StreamBase::AddMethods. This PR makes those accessors nonenumerable and check the signatures in the accessors so they throw instead of raise assertions.

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

// Should throw instead of raise assertions
{
const msg = /TypeError: Method \w+ called on incompatible receiver/;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not engine-agnostic but I am not sure what ChakraCore throws for this..

@@ -31,27 +32,33 @@ void StreamBase::AddMethods(Environment* env,
HandleScope scope(env->isolate());

enum PropertyAttribute attributes =
static_cast<PropertyAttribute>(v8::ReadOnly | v8::DontDelete);
static_cast<PropertyAttribute>(
v8::ReadOnly | v8::DontDelete | v8::DontEnum);
Copy link
Member

Choose a reason for hiding this comment

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

Four space indent here and two lines below.

@joyeecheung
Copy link
Member Author

@joyeecheung
Copy link
Member Author

Landed in 1ee3244, thanks!

joyeecheung added a commit that referenced this pull request Nov 11, 2017
This PR makes the prototype accessors added by StreamBase::AddMethods
nonenumerable and checks the signatures in the accessors so they
throw instead of raising assertions when called with incompatible
receivers. They could be enumerated when inspecting the prototype
with util.inspect or the inspector protocol.

PR-URL: #16860
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
This PR makes the prototype accessors added by StreamBase::AddMethods
nonenumerable and checks the signatures in the accessors so they
throw instead of raising assertions when called with incompatible
receivers. They could be enumerated when inspecting the prototype
with util.inspect or the inspector protocol.

PR-URL: #16860
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@evanlucas evanlucas mentioned this pull request Nov 13, 2017
MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
This PR makes the prototype accessors added by StreamBase::AddMethods
nonenumerable and checks the signatures in the accessors so they
throw instead of raising assertions when called with incompatible
receivers. They could be enumerated when inspecting the prototype
with util.inspect or the inspector protocol.

PR-URL: #16860
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins
Copy link
Contributor

I've landed this in both v6.x and v8.x staging

please let me know if this should bake longer before releasing

MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
This PR makes the prototype accessors added by StreamBase::AddMethods
nonenumerable and checks the signatures in the accessors so they
throw instead of raising assertions when called with incompatible
receivers. They could be enumerated when inspecting the prototype
with util.inspect or the inspector protocol.

PR-URL: #16860
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@gibfahn gibfahn mentioned this pull request Nov 21, 2017
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
This PR makes the prototype accessors added by StreamBase::AddMethods
nonenumerable and checks the signatures in the accessors so they
throw instead of raising assertions when called with incompatible
receivers. They could be enumerated when inspecting the prototype
with util.inspect or the inspector protocol.

PR-URL: #16860
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
This PR makes the prototype accessors added by StreamBase::AddMethods
nonenumerable and checks the signatures in the accessors so they
throw instead of raising assertions when called with incompatible
receivers. They could be enumerated when inspecting the prototype
with util.inspect or the inspector protocol.

PR-URL: #16860
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jure
Copy link
Contributor

jure commented Dec 8, 2017

As mentioned in #16949 (comment), isn't there a way to have it not assert, as was the case in 8.8.1 (just return undefined)?

We've been seeing some new test failures with the error thrown here, and the cause is hard to discover (and even harder to fix).

jure added a commit to jure/node that referenced this pull request Dec 13, 2017
apapirovski pushed a commit that referenced this pull request Dec 17, 2017
PR-URL: #17665
Fixes: #17636
Refs: #16482
Refs: #16860
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
PR-URL: #17665
Fixes: #17636
Refs: #16482
Refs: #16860
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
PR-URL: #17665
Fixes: #17636
Refs: #16482
Refs: #16860
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
boingoing pushed a commit to nodejs/node-chakracore that referenced this pull request Jan 18, 2018
PR-URL: nodejs/node#17665
Fixes: nodejs/node#17636
Refs: nodejs/node#16482
Refs: nodejs/node#16860
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
kjin pushed a commit to kjin/node that referenced this pull request Apr 28, 2018
PR-URL: nodejs#17665
Fixes: nodejs#17636
Refs: nodejs#16482
Refs: nodejs#16860
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Backport-PR-URL: #20456
PR-URL: #17665
Fixes: #17636
Refs: #16482
Refs: #16860
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
ckerr pushed a commit to electron/node that referenced this pull request Jun 14, 2018
PR-URL: nodejs/node#17665
Fixes: nodejs/node#17636
Refs: nodejs/node#16482
Refs: nodejs/node#16860
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
ckerr added a commit to electron/node that referenced this pull request Jun 15, 2018
* src: replace SetAccessor w/ SetAccessorProperty

PR-URL: nodejs/node#17665
Fixes: nodejs/node#17636
Refs: nodejs/node#16482
Refs: nodejs/node#16860
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

* test: make test-tls-external-accessor agnostic

Remove reliance on V8-specific error messages in
test/parallel/test-tls-external-accessor.js.

Check that the error is a `TypeError`.

The test should now be successful without modification using ChakraCore.

Backport-PR-URL: nodejs/node#20456
PR-URL: nodejs/node#16272
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants