n-api: define ECMAScript-compliant accessors on napi_define_properties

Fixes: #26551
nodejs · legendecas · May 24, 2019 · May 29, 2019 · May 29, 2019 · be84fae7c97cad80ec478a596c82ba528dccc3a2
commit be84fae7c97cad80ec478a596c82ba528dccc3a2
diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc
@@ -1171,26 +1171,45 @@ napi_status napi_define_properties(napi_env env,
       return napi_set_last_error(env, status);
     }
 
-    v8::PropertyAttribute attributes =
-        v8impl::V8PropertyAttributesFromDescriptor(p);
-
     if (p->getter != nullptr || p->setter != nullptr) {
-      v8::Local<v8::Value> cbdata = v8impl::CreateAccessorCallbackData(
-        env,
-        p->getter,
-        p->setter,
-        p->data);
+      v8::Local<v8::Value> local_getter;
+      v8::Local<v8::Value> local_setter;
 
-      auto set_maybe = obj->SetAccessor(
-        context,
-        property_name,
-        p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
-        p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
-        cbdata,
-        v8::AccessControl::DEFAULT,
-        attributes);
+      if (p->getter != nullptr) {
+        v8::Local<v8::Value> getter_data =
+            v8impl::CreateFunctionCallbackData(env, p->getter, p->data);
+        CHECK_MAYBE_EMPTY(env, getter_data, napi_generic_failure);
+
+        v8::MaybeLocal<v8::Function> maybe_getter =
+            v8::Function::New(context,
+                              v8impl::FunctionCallbackWrapper::Invoke,
+                              getter_data);
+        CHECK_MAYBE_EMPTY(env, maybe_getter, napi_generic_failure);
+
+        local_getter = maybe_getter.ToLocalChecked();
+      }
+      if (p->setter != nullptr) {
+        v8::Local<v8::Value> setter_data =
+            v8impl::CreateFunctionCallbackData(env, p->setter, p->data);
+        CHECK_MAYBE_EMPTY(env, setter_data, napi_generic_failure);
+
+        v8::MaybeLocal<v8::Function> maybe_setter =
+            v8::Function::New(context,
+                              v8impl::FunctionCallbackWrapper::Invoke,
+                              setter_data);
+        CHECK_MAYBE_EMPTY(env, maybe_setter, napi_generic_failure);
+        local_setter = maybe_setter.ToLocalChecked();
+      }
 
-      if (!set_maybe.FromMaybe(false)) {
+      v8::PropertyDescriptor descriptor(local_getter, local_setter);
+      descriptor.set_enumerable((p->attributes & napi_enumerable) != 0);
+      descriptor.set_configurable((p->attributes & napi_configurable) != 0);
+
+      auto define_maybe = obj->DefineProperty(context,
+                                              property_name,
+                                              descriptor);
+
+      if (!define_maybe.FromMaybe(false)) {
         return napi_set_last_error(env, napi_invalid_arg);
       }
     } else if (p->method != nullptr) {
@@ -1206,17 +1225,28 @@ napi_status napi_define_properties(napi_env env,
 
       CHECK_MAYBE_EMPTY(env, maybe_fn, napi_generic_failure);
 
-      auto define_maybe = obj->DefineOwnProperty(
-        context, property_name, maybe_fn.ToLocalChecked(), attributes);
+      v8::PropertyDescriptor descriptor(maybe_fn.ToLocalChecked(),
+                                        (p->attributes & napi_writable) != 0);
+      descriptor.set_enumerable((p->attributes & napi_enumerable) != 0);
+      descriptor.set_configurable((p->attributes & napi_configurable) != 0);
+
+      auto define_maybe = obj->DefineProperty(context,
+                                              property_name,
+                                              descriptor);
 
       if (!define_maybe.FromMaybe(false)) {
         return napi_set_last_error(env, napi_generic_failure);
       }
     } else {
       v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value);
 
+      v8::PropertyDescriptor descriptor(value,
+                                        (p->attributes & napi_writable) != 0);
+      descriptor.set_enumerable((p->attributes & napi_enumerable) != 0);
+      descriptor.set_configurable((p->attributes & napi_configurable) != 0);
+
       auto define_maybe =
-          obj->DefineOwnProperty(context, property_name, value, attributes);
+          obj->DefineProperty(context, property_name, descriptor);
 
       if (!define_maybe.FromMaybe(false)) {
         return napi_set_last_error(env, napi_invalid_arg);

diff --git a/test/js-native-api/test_properties/test.js b/test/js-native-api/test_properties/test.js
@@ -3,6 +3,8 @@ const common = require('../../common');
 const assert = require('assert');
 const readonlyErrorRE =
   /^TypeError: Cannot assign to read only property '.*' of object '#<Object>'$/;
+const getterOnlyErrorRE =
+  /^TypeError: Cannot set property .* of #<Object> which has only a getter$/;
 
 // Testing api calls for defining properties
 const test_object = require(`./build/${common.buildType}/test_properties`);
@@ -41,14 +43,24 @@ const symbolDescription =
 assert.strictEqual(symbolDescription, 'NameKeySymbol');
 
 // The napi_writable attribute should be ignored for accessors.
+const readwriteAccessor1Descriptor =
+  Object.getOwnPropertyDescriptor(test_object, 'readwriteAccessor1');
+const readonlyAccessor1Descriptor =
+  Object.getOwnPropertyDescriptor(test_object, 'readonlyAccessor1');
+assert.ok(readwriteAccessor1Descriptor.get != null);
+assert.ok(readwriteAccessor1Descriptor.set != null);
+assert.ok(readwriteAccessor1Descriptor.value === undefined);
+assert.ok(readonlyAccessor1Descriptor.get != null);
+assert.ok(readonlyAccessor1Descriptor.set === undefined);
+assert.ok(readonlyAccessor1Descriptor.value === undefined);
 test_object.readwriteAccessor1 = 1;
 assert.strictEqual(test_object.readwriteAccessor1, 1);
 assert.strictEqual(test_object.readonlyAccessor1, 1);
-assert.throws(() => { test_object.readonlyAccessor1 = 3; }, readonlyErrorRE);
+assert.throws(() => { test_object.readonlyAccessor1 = 3; }, getterOnlyErrorRE);
 test_object.readwriteAccessor2 = 2;
 assert.strictEqual(test_object.readwriteAccessor2, 2);
 assert.strictEqual(test_object.readonlyAccessor2, 2);
-assert.throws(() => { test_object.readonlyAccessor2 = 3; }, readonlyErrorRE);
+assert.throws(() => { test_object.readonlyAccessor2 = 3; }, getterOnlyErrorRE);
 
 assert.strictEqual(test_object.hasNamedProperty(test_object, 'echo'), true);
 assert.strictEqual(test_object.hasNamedProperty(test_object, 'hiddenValue'),