Change to using SetAccessorProperty instead of SetAccessor in StreamBase

Fixes: nodejs#17636 Refs: nodejs#16482 Refs: nodejs#16860
jure · Dec 13, 2017 · 329e162 · 329e162
1 parent 04ae486
commit 329e162
Show file tree

Hide file tree

Showing 3 changed files with 61 additions and 42 deletions.
diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h
@@ -11,7 +11,7 @@
 
 namespace node {
 
-using v8::AccessorSignature;
+using v8::Signature;
 using v8::External;
 using v8::FunctionCallbackInfo;
 using v8::FunctionTemplate;
@@ -34,31 +34,42 @@ void StreamBase::AddMethods(Environment* env,
   enum PropertyAttribute attributes =
       static_cast<PropertyAttribute>(
           v8::ReadOnly | v8::DontDelete | v8::DontEnum);
-  Local<AccessorSignature> signature =
-      AccessorSignature::New(env->isolate(), t);
-  t->PrototypeTemplate()->SetAccessor(env->fd_string(),
-                                      GetFD<Base>,
-                                      nullptr,
-                                      env->as_external(),
-                                      v8::DEFAULT,
-                                      attributes,
-                                      signature);
-
-  t->PrototypeTemplate()->SetAccessor(env->external_stream_string(),
-                                      GetExternal<Base>,
-                                      nullptr,
-                                      env->as_external(),
-                                      v8::DEFAULT,
-                                      attributes,
-                                      signature);
-
-  t->PrototypeTemplate()->SetAccessor(env->bytes_read_string(),
-                                      GetBytesRead<Base>,
-                                      nullptr,
-                                      env->as_external(),
-                                      v8::DEFAULT,
-                                      attributes,
-                                      signature);
+
+  Local<Signature> signature =
+      Signature::New(env->isolate(), t);
+
+  Local<FunctionTemplate> get_fd_templ = FunctionTemplate::New(
+    env->isolate(),
+    GetFD<Base>,
+    Local<Value>(),
+    signature);
+
+  Local<FunctionTemplate> get_external_templ = FunctionTemplate::New(
+    env->isolate(),
+    GetExternal<Base>,
+    Local<Value>(),
+    signature);
+
+  Local<FunctionTemplate> get_bytes_read = FunctionTemplate::New(
+    env->isolate(),
+    GetBytesRead<Base>,
+    Local<Value>(),
+    signature);
+
+  t->PrototypeTemplate()->SetAccessorProperty(env->fd_string(),
+                                              get_fd_templ,
+                                              Local<FunctionTemplate>(),
+                                              attributes);
+
+  t->PrototypeTemplate()->SetAccessorProperty(env->external_stream_string(),
+                                              get_external_templ,
+                                              Local<FunctionTemplate>(),
+                                              attributes);
+
+  t->PrototypeTemplate()->SetAccessorProperty(env->bytes_read_string(),
+                                              get_bytes_read,
+                                              Local<FunctionTemplate>(),
+                                              attributes);
 
   env->SetProtoMethod(t, "readStart", JSMethod<Base, &StreamBase::ReadStart>);
   env->SetProtoMethod(t, "readStop", JSMethod<Base, &StreamBase::ReadStop>);
@@ -85,8 +96,7 @@ void StreamBase::AddMethods(Environment* env,
 
 
 template <class Base>
-void StreamBase::GetFD(Local<String> key,
-                       const PropertyCallbackInfo<Value>& args) {
+void StreamBase::GetFD(const FunctionCallbackInfo<Value>& args) {
   // Mimic implementation of StreamBase::GetFD() and UDPWrap::GetFD().
   Base* handle;
   ASSIGN_OR_RETURN_UNWRAP(&handle,
@@ -100,10 +110,8 @@ void StreamBase::GetFD(Local<String> key,
   args.GetReturnValue().Set(wrap->GetFD());
 }
 
-
 template <class Base>
-void StreamBase::GetBytesRead(Local<String> key,
-                              const PropertyCallbackInfo<Value>& args) {
+void StreamBase::GetBytesRead(const FunctionCallbackInfo<Value>& args) {
   // The handle instance hasn't been set. So no bytes could have been read.
   Base* handle;
   ASSIGN_OR_RETURN_UNWRAP(&handle,
@@ -115,10 +123,8 @@ void StreamBase::GetBytesRead(Local<String> key,
   args.GetReturnValue().Set(static_cast<double>(wrap->bytes_read_));
 }
 
-
 template <class Base>
-void StreamBase::GetExternal(Local<String> key,
-                             const PropertyCallbackInfo<Value>& args) {
+void StreamBase::GetExternal(const FunctionCallbackInfo<Value>& args) {
   Base* handle;
   ASSIGN_OR_RETURN_UNWRAP(&handle, args.This());
 

diff --git a/src/stream_base.h b/src/stream_base.h
@@ -265,16 +265,13 @@ class StreamBase : public StreamResource {
   int WriteString(const v8::FunctionCallbackInfo<v8::Value>& args);
 
   template <class Base>
-  static void GetFD(v8::Local<v8::String> key,
-                    const v8::PropertyCallbackInfo<v8::Value>& args);
+  static void GetFD(const v8::FunctionCallbackInfo<v8::Value>& args);
 
   template <class Base>
-  static void GetExternal(v8::Local<v8::String> key,
-                          const v8::PropertyCallbackInfo<v8::Value>& args);
+  static void GetExternal(const v8::FunctionCallbackInfo<v8::Value>& args);
 
   template <class Base>
-  static void GetBytesRead(v8::Local<v8::String> key,
-                           const v8::PropertyCallbackInfo<v8::Value>& args);
+  static void GetBytesRead(const v8::FunctionCallbackInfo<v8::Value>& args);
 
   template <class Base,
             int (StreamBase::*Method)(

diff --git a/test/parallel/test-stream-base-prototype-accessors.js b/test/parallel/test-stream-base-prototype-accessors.js
@@ -10,9 +10,9 @@ const assert = require('assert');
 // Or anything that calls StreamBase::AddMethods when setting up its prototype
 const TTY = process.binding('tty_wrap').TTY;
 
-// Should throw instead of raise assertions
 {
-  const msg = /TypeError: Method \w+ called on incompatible receiver/;
+  // Should throw instead of raise assertions
+  const msg = /TypeError: Illegal invocation/;
   assert.throws(() => {
     TTY.prototype.bytesRead;
   }, msg);
@@ -24,4 +24,20 @@ const TTY = process.binding('tty_wrap').TTY;
   assert.throws(() => {
     TTY.prototype._externalStream;
   }, msg);
+
+  // Should not throw for Object.getOwnPropertyDescriptor
+  assert.strictEqual(
+    typeof (Object.getOwnPropertyDescriptor(TTY.prototype, 'bytesRead')),
+    'object'
+  );
+
+  assert.strictEqual(
+    typeof (Object.getOwnPropertyDescriptor(TTY.prototype, 'fd')),
+    'object'
+  );
+
+  assert.strictEqual(
+    typeof (Object.getOwnPropertyDescriptor(TTY.prototype, '_externalStream')),
+    'object'
+  );
 }