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,stream: use SetAccessorProperty instead of SetAccessor #17665

Closed
wants to merge 16 commits into from

Conversation

jure
Copy link
Contributor

@jure jure commented Dec 13, 2017

Fixes: #17636
Refs: #16482
Refs: #16860

As per #17636's solution 1 (found in #17636 (comment)), this PR allows Object.getOwnPropertyDescriptor(process.stdin._handle.__proto__, 'bytesRead') to not throw anymore and return an object/property descriptor. However, accessing this property using process.stdin._handle.__proto__.bytesRead will still throw (as is the current case on 8.9.2+).

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, stream

@jure jure changed the title Change to using SetAccessorProperty instead of SetAccessor in StreamBase src,stream: use SetAccessorProperty instead of SetAccessor in StreamBase Dec 13, 2017

// Should not throw for Object.getOwnPropertyDescriptor
assert.strictEqual(
typeof (Object.getOwnPropertyDescriptor(TTY.prototype, 'bytesRead')),
Copy link
Member

Choose a reason for hiding this comment

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

You don’t need the parentheses after typeof here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Removed.

@addaleax addaleax 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 Dec 13, 2017
@addaleax
Copy link
Member

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

This PR should also update the following files. In all of them, look for PrototypeTemplate()->SetAccessor to find which ones to replace.

  • node_crypto.cc
  • udp_wrap.cc

Ideally, we shouldn't use SetAccessor on InstanceTemplate() either, other than under exceptional circumstances. Here're two files that are easier to fix (look for SetAccessor):

  • node_crypto.cc
  • node_perf.cc

signature);

Local<Signature> signature =
Signature::New(env->isolate(), t);
Copy link
Member

Choose a reason for hiding this comment

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

For the person landing this commit: these two lines could be merged into a single line, which might help with readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 This used to be AccessorSignature, and was over the limit. Fixed.

env->isolate(),
GetFD<Base>,
Local<Value>(),
signature);
Copy link
Member

Choose a reason for hiding this comment

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

For the person landing this commit: for consistency the indentation should look something like:

  Local<FunctionTemplate> get_fd_templ =
      FunctionTemplate::New(env->isolate(),
                            GetFD<Base>,
                            Local<Value>(),
                            signature);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Fixed, was struggling a bit with how to indent it before.

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

Choose a reason for hiding this comment

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

Even better, change all uses of msg below to TypeError, because the exact error message may differ depending on the JavaScript engine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Fixed.

@jure jure changed the title src,stream: use SetAccessorProperty instead of SetAccessor in StreamBase src,stream: use SetAccessorProperty instead of SetAccessor Dec 14, 2017
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

Almost there!

@@ -544,14 +545,18 @@ void SecureContext::Initialize(Environment* env, Local<Object> target) {
t->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "kTicketKeyIVIndex"),
Integer::NewFromUnsigned(env->isolate(), kTicketKeyIVIndex));

t->PrototypeTemplate()->SetAccessor(
Local<FunctionTemplate> ctx_getter_templ =
FunctionTemplate::New(env->isolate(),
Copy link
Member

@TimothyGu TimothyGu Dec 14, 2017

Choose a reason for hiding this comment

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

We indent by four spaces in line continuations :)

Here and also in other C++ files.

assert.strictEqual(
typeof Object.getOwnPropertyDescriptor(TTY.prototype, '_externalStream'),
'object'
);
Copy link
Member

@TimothyGu TimothyGu Dec 14, 2017

Choose a reason for hiding this comment

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

Similar tests should exist for Object.getOwnPropertyDescriptor(crypto.SecureContext.prototype, '_external') and Object.getOwnPropertyDescriptor(crypto.Connection.prototype, '_external') where crypto = process.binding('crypto'), and Object.getOwnPropertyDescriptor(process.binding('udp_wrap').UDP.prototype, 'fd').

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Added.

Local<FunctionTemplate> ctx_getter_templ =
FunctionTemplate::New(env->isolate(),
CtxGetter,
Local<Value>(),
Copy link
Member

Choose a reason for hiding this comment

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

We usually use env->as_external() for the parameter right here, so that we can get access to the Environment object in the callback itself. Again, here and below.

Copy link
Contributor Author

@jure jure Dec 14, 2017

Choose a reason for hiding this comment

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

You mean instead of Local<Value>()?

Copy link
Member

@TimothyGu TimothyGu Dec 14, 2017

Choose a reason for hiding this comment

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

Yes.The same also needs to be done for all other such instances, otherwise I'm fairly sure some tests will fail because of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests completed fine with just Local<Value>(), so that's a bit worrying, but it's perhaps out of the scope of this PR to add tests for that. I've now switched to env->as_external wherever it was previously used.

@jure
Copy link
Contributor Author

jure commented Dec 14, 2017

That should be that I think! Fun stuff :)

Local<FunctionTemplate> verify_error_getter_templ =
FunctionTemplate::New(env->isolate(),
DiffieHellman::VerifyErrorGetter,
Local<Value>(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would env->as_external() be appropriate here as well? Or better yet, is there a place where it isn't appropriate (there are 10 places still using it)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess anywhere where as_external was previously used. I'll get on it.

Copy link
Member

@TimothyGu TimothyGu Dec 14, 2017

Choose a reason for hiding this comment

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

From a quick look: it would be appropriate at all of those places. In fact if you look at the current SetAccessor()-based code, all of them have env->as_external() for the data parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Done.

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

LGTM. The comment is optional.

src/node_perf.cc Outdated
Local<FunctionTemplate> get_performance_entry_name_templ =
FunctionTemplate::New(env->isolate(),
GetPerformanceEntryName,
Local<Value>(),
Copy link
Member

Choose a reason for hiding this comment

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

I know they didn't use to get env->as_external(), so this should be fine. But for sake of consistency I'd rather still have env->as_external() here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that makes sense. Done 👍

@TimothyGu
Copy link
Member

@jure
Copy link
Contributor Author

jure commented Dec 14, 2017

What's with the failures on CI (Windows, Linux and SmartOS)? How does one look into a failed build?

@TimothyGu
Copy link
Member

The CI failures all seem to be infrastructural. I'll start a rebuild to see if the situation improves. If not we can land this as-is.

CI: https://ci.nodejs.org/job/node-test-commit/14820/

@apapirovski
Copy link
Member

Thank you @jure! This is an awesome 1st PR! 👍

@jure
Copy link
Contributor Author

jure commented Dec 15, 2017

Thanks @apapirovski :) Let me know if there's anything else. I've already put a bottle of bubbly into the fridge, just waiting for that merge notification!

@TimothyGu
Copy link
Member

@jure We usually tend to wait a few days before applying to make sure everyone who wants to take a look at this PR got a chance to. This should get applied in a few hours.

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>
@apapirovski
Copy link
Member

apapirovski commented Dec 17, 2017

Landed in efffcc2. Thanks @jure and congrats on becoming a Contributor!! 🎉

(Time to pop that bubbly!)

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>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
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>
@MylesBorins
Copy link
Contributor

Do we want to land this on LTS? It lands cleanly on v8.x and v6.x, just unsure if we should wait a bit before landing

@TimothyGu
Copy link
Member

@MylesBorins I’m +1 on backporting this.

@MylesBorins
Copy link
Contributor

MylesBorins commented Feb 27, 2018

This does not land cleanly on v6.x due to the performance API not existing on that release stream. Does it still make sense to backport?

edit: landing on v8.x also broke a test, need a backport to both branches

=== release test-tls-external-accessor ===
Path: parallel/test-tls-external-accessor
assert.js:649
      throw actual;
      ^

TypeError: Illegal invocation
    at assert.throws (/Users/mborins/code/node/v8.x/test/parallel/test-tls-external-accessor.js:14:28)
    at tryBlock (assert.js:619:5)
    at innerThrows (assert.js:638:18)
    at Function.throws (assert.js:662:3)
    at Object.<anonymous> (/Users/mborins/code/node/v8.x/test/parallel/test-tls-external-accessor.js:14:10)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
Command: out/Release/node /Users/mborins/code/node/v8.x/test/parallel/test-tls-external-accessor.js

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>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
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.

Object.getOwnPropertyDescriptor throws TypeError on process _handle
7 participants