node-api: fix napi_get_all_property_names

src/js_native_api.h

@@ -562,6 +562,15 @@ NAPI_EXTERN napi_status napi_object_freeze(napi_env env, napi_value object);
 NAPI_EXTERN napi_status napi_object_seal(napi_env env, napi_value object);
 #endif  // NAPI_VERSION >= 8
 
+#ifdef NAPI_EXPERIMENTAL
+NAPI_EXTERN napi_status node_api_get_features(napi_env env,
+                                              node_api_features* features);
+NAPI_EXTERN napi_status node_api_add_features(napi_env env,
+                                              node_api_features features);
+NAPI_EXTERN napi_status node_api_remove_features(napi_env env,
+                                                 node_api_features features);
+#endif  // NAPI_EXPERIMENTAL
+
 EXTERN_C_END
 
 #endif  // SRC_JS_NATIVE_API_H_

src/js_native_api_types.h

@@ -100,6 +100,23 @@ typedef enum {
 //   * the definition of `napi_status` in doc/api/n-api.md to reflect the newly
 //     added value(s).
 
+#ifdef NAPI_EXPERIMENTAL
+// Feature bit flags allow to tweak internal behavior of the Node-API functions.
+// E.g. we can use it to fix old bugs or change result status codes.
+// The node_api_features_default defines the default set of features per
+// Node-API version. It is used during the native module initialization.
+// More specifically, NAPI_MODULE_X macro uses NAPI_DEFAULT_FEATURES that is set
+// to node_api_features_default. To override the default behavior define
+// NAPI_DEFAULT_FEATURES before the node_api.h is included.
+typedef enum {
+  node_api_features_none = 0,
+  // Fix use of napi_key_configurable in napi_get_all_property_names.
+  node_api_features_use_configurable_key_filter = 1 << 0,
+  // The default features per Node-API version.
+  node_api_features_default = node_api_features_use_configurable_key_filter
+} node_api_features;
+#endif
+
 typedef napi_value (*napi_callback)(napi_env env, napi_callback_info info);
 typedef void (*napi_finalize)(napi_env env,
                               void* finalize_data,

src/js_native_api_v8.cc

@@ -936,8 +936,14 @@ napi_status napi_get_all_property_names(napi_env env,
         filter | v8::PropertyFilter::ONLY_ENUMERABLE);
   }
   if (key_filter & napi_key_configurable) {
-    filter = static_cast<v8::PropertyFilter>(filter |
-                                             v8::PropertyFilter::ONLY_WRITABLE);
+    if (env->HasFeature(node_api_features_use_configurable_key_filter)) {
+      filter = static_cast<v8::PropertyFilter>(
+          filter | v8::PropertyFilter::ONLY_CONFIGURABLE);
+    } else {
+      // The code path before the bug fix
+      filter = static_cast<v8::PropertyFilter>(
+          filter | v8::PropertyFilter::ONLY_WRITABLE);
+    }
   }
   if (key_filter & napi_key_skip_strings) {
     filter = static_cast<v8::PropertyFilter>(filter |
@@ -3210,3 +3216,25 @@ napi_status napi_is_detached_arraybuffer(napi_env env,
 
   return napi_clear_last_error(env);
 }
+
+NAPI_EXTERN napi_status node_api_get_features(napi_env env,
+                                              node_api_features* features) {
+  CHECK_ENV(env);
+  CHECK_ARG(env, features);
+  *features = env->features;
+  return napi_clear_last_error(env);
+}
+
+NAPI_EXTERN napi_status node_api_add_features(napi_env env,
+                                              node_api_features features) {
+  CHECK_ENV(env);
+  env->features = static_cast<node_api_features>(env->features | features);
+  return napi_clear_last_error(env);
+}
+
+NAPI_EXTERN napi_status node_api_remove_features(napi_env env,
+                                                 node_api_features features) {
+  CHECK_ENV(env);
+  env->features = static_cast<node_api_features>(env->features & ~features);
+  return napi_clear_last_error(env);
+}

src/js_native_api_v8.h

@@ -52,8 +52,11 @@ class RefTracker {
 }  // end of namespace v8impl
 
 struct napi_env__ {
-  explicit napi_env__(v8::Local<v8::Context> context)
-      : isolate(context->GetIsolate()), context_persistent(isolate, context) {
+  explicit napi_env__(v8::Local<v8::Context> context,
+                      node_api_features features)
+      : isolate(context->GetIsolate()),
+        context_persistent(isolate, context),
+        features(features) {
     CHECK_EQ(isolate, context->GetIsolate());
     napi_clear_last_error(this);
   }
@@ -107,6 +110,10 @@ struct napi_env__ {
     CallIntoModule([&](napi_env env) { cb(env, data, hint); });
   }
 
+  bool HasFeature(node_api_features feature) {
+    return (features & feature) != 0;
+  }
+
   v8impl::Persistent<v8::Value> last_exception;
 
   // We store references in two different lists, depending on whether they have
@@ -118,9 +125,13 @@ struct napi_env__ {
   int open_handle_scopes = 0;
   int open_callback_scopes = 0;
   int refs = 1;
+  node_api_features features = node_api_features_default;
   void* instance_data = nullptr;
 };
 
+static_assert(node_api_features_default < INT_MAX,
+              "node_api_features must fit into an int32_t.");
+
 // This class is used to keep a napi_env live in a way that
 // is exception safe versus calling Ref/Unref directly
 class EnvRefHolder {

src/node_api.cc

@@ -18,8 +18,9 @@
 #include <memory>
 
 node_napi_env__::node_napi_env__(v8::Local<v8::Context> context,
-                                 const std::string& module_filename)
-    : napi_env__(context), filename(module_filename) {
+                                 const std::string& module_filename,
+                                 node_api_features features)
+    : napi_env__(context, features), filename(module_filename) {
   CHECK_NOT_NULL(node_env());
 }
 
@@ -84,10 +85,11 @@ class BufferFinalizer : private Finalizer {
 };
 
 static inline napi_env NewEnv(v8::Local<v8::Context> context,
-                              const std::string& module_filename) {
+                              const std::string& module_filename,
+                              node_api_features features) {
   node_napi_env result;
 
-  result = new node_napi_env__(context, module_filename);
+  result = new node_napi_env__(context, module_filename, features);
   // TODO(addaleax): There was previously code that tried to delete the
   // napi_env when its v8::Context was garbage collected;
   // However, as long as N-API addons using this napi_env are in place,
@@ -549,6 +551,13 @@ class AsyncContext {
 
 }  // end of namespace v8impl
 
+void napi_module_register_by_symbol_with_features(
+    v8::Local<v8::Object> exports,
+    v8::Local<v8::Value> module,
+    v8::Local<v8::Context> context,
+    napi_addon_register_func init,
+    node_api_features features);
+
 // Intercepts the Node-V8 module registration callback. Converts parameters
 // to NAPI equivalents and then calls the registration callback specified
 // by the NAPI module.
@@ -567,6 +576,16 @@ void napi_module_register_by_symbol(v8::Local<v8::Object> exports,
                                     v8::Local<v8::Value> module,
                                     v8::Local<v8::Context> context,
                                     napi_addon_register_func init) {
+  napi_module_register_by_symbol_with_features(
+      exports, module, context, init, node_api_features_none);
+}
+
+void napi_module_register_by_symbol_with_features(
+    v8::Local<v8::Object> exports,
+    v8::Local<v8::Value> module,
+    v8::Local<v8::Context> context,
+    napi_addon_register_func init,
+    node_api_features features) {
   node::Environment* node_env = node::Environment::GetCurrent(context);
   std::string module_filename = "";
   if (init == nullptr) {
@@ -594,7 +613,7 @@ void napi_module_register_by_symbol(v8::Local<v8::Object> exports,
   }
 
   // Create a new napi_env for this specific module.
-  napi_env env = v8impl::NewEnv(context, module_filename);
+  napi_env env = v8impl::NewEnv(context, module_filename, features);
 
   napi_value _exports;
   env->CallIntoModule([&](napi_env env) {

src/node_api.h

@@ -38,7 +38,8 @@ typedef struct napi_module {
   napi_addon_register_func nm_register_func;
   const char* nm_modname;
   void* nm_priv;
-  void* reserved[4];
+  uintptr_t nm_features;
+  void* reserved[3];
 } napi_module;
 
 #define NAPI_MODULE_VERSION 1
@@ -73,6 +74,14 @@ typedef struct napi_module {
   static void fn(void)
 #endif
 
+#ifndef NAPI_DEFAULT_FEATURES
+#ifdef NAPI_EXPERIMENTAL
+#define NAPI_DEFAULT_FEATURES (uintptr_t) node_api_features_default
+#else
+#define NAPI_DEFAULT_FEATURES (uintptr_t)0
+#endif
+#endif
+
 #define NAPI_MODULE_X(modname, regfunc, priv, flags)                           \
   EXTERN_C_START                                                               \
   static napi_module _module = {                                               \
@@ -82,6 +91,7 @@ typedef struct napi_module {
       regfunc,                                                                 \
       #modname,                                                                \
       priv,                                                                    \
+      NAPI_DEFAULT_FEATURES,                                                   \
       {0},                                                                     \
   };                                                                           \
   NAPI_C_CTOR(_register_##modname) { napi_module_register(&_module); }         \

src/node_api_internals.h

@@ -10,7 +10,8 @@
 
 struct node_napi_env__ : public napi_env__ {
   node_napi_env__(v8::Local<v8::Context> context,
-                  const std::string& module_filename);
+                  const std::string& module_filename,
+                  node_api_features features);
 
   bool can_call_into_js() const override;
   v8::Maybe<bool> mark_arraybuffer_as_untransferable(

test/js-native-api/entry_point.c

@@ -1,3 +1,4 @@
+#define NAPI_EXPERIMENTAL
 #include <node_api.h>
 
 EXTERN_C_START

test/js-native-api/test_object/test.js

@@ -238,15 +238,53 @@ assert.strictEqual(newObject.test_string, 'test string');
     value: 4,
     enumerable: false,
     writable: true,
+    configurable: false
+  });
+  Object.defineProperty(object, 'writable', {
+    value: 4,
+    enumerable: true,
+    writable: true,
+    configurable: false
+  });
+  Object.defineProperty(object, 'configurable', {
+    value: 4,
+    enumerable: true,
+    writable: false,
     configurable: true
   });
   object[5] = 5;
 
   assert.deepStrictEqual(test_object.GetPropertyNames(object),
-                         ['5', 'normal', 'inherited']);
+                         ['5',
+                          'normal',
+                          'writable',
+                          'configurable',
+                          'inherited']);
 
   assert.deepStrictEqual(test_object.GetSymbolNames(object),
                          [fooSymbol]);
+
+  assert.deepStrictEqual(test_object.GetWritableNames(object),
+                         ['5',
+                          'normal',
+                          'writable',
+                          fooSymbol,
+                          'inherited']);
+
+  // Previously Node-API used writable key instead of configurable
+  assert.deepStrictEqual(test_object.GetConfigurableNamesUnfixed(object),
+                         ['5',
+                          'normal',
+                          'writable',
+                          fooSymbol,
+                          'inherited']);
+
+  assert.deepStrictEqual(test_object.GetConfigurableNames(object),
+                         ['5',
+                          'normal',
+                          'configurable',
+                          fooSymbol,
+                          'inherited']);
 }
 
 // Verify that passing NULL to napi_set_property() results in the correct