Skip to content

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-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 7, 2017
@joyeecheungjoyeecheung 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
{
constmsg=/TypeError:Method\w+calledonincompatiblereceiver/;
Copy link
MemberAuthor

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..

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
MemberAuthor

@joyeecheung
Copy link
MemberAuthor

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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@evanlucasevanlucas 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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@gibfahngibfahn mentioned this pull request Nov 21, 2017
@MylesBorinsMylesBorins 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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@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).

apapirovski pushed a commit that referenced this pull request Dec 17, 2017
PR-URL: #17665Fixes: #17636 Refs: #16482 Refs: #16860 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
PR-URL: #17665Fixes: #17636 Refs: #16482 Refs: #16860 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
PR-URL: #17665Fixes: #17636 Refs: #16482 Refs: #16860 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
boingoing pushed a commit to nodejs/node-chakracore that referenced this pull request Jan 18, 2018
PR-URL: nodejs/node#17665Fixes: nodejs/node#17636 Refs: nodejs/node#16482 Refs: nodejs/node#16860 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Apr 28, 2018
PR-URL: nodejs#17665Fixes: nodejs#17636 Refs: nodejs#16482 Refs: nodejs#16860 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Backport-PR-URL: #20456 PR-URL: #17665Fixes: #17636 Refs: #16482 Refs: #16860 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
ckerr pushed a commit to electron/node that referenced this pull request Jun 14, 2018
PR-URL: nodejs/node#17665Fixes: nodejs/node#17636 Refs: nodejs/node#16482 Refs: nodejs/node#16860 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
ckerr added a commit to electron/node that referenced this pull request Jun 15, 2018
* src: replace SetAccessor w/ SetAccessorProperty PR-URL: nodejs/node#17665Fixes: nodejs/node#17636 Refs: nodejs/node#16482 Refs: nodejs/node#16860 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> * 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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
Sign up for freeto 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 / srcIssues 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

@joyeecheung@MylesBorins@jure@bnoordhuis@jasnell@addaleax@cjihrig@nodejs-github-bot