src: return bool on object freeze and seal (nodejs#991)

These operations can call into JavaScript by using Proxy handlers, hence JavaScript exceptions might pending at the end of the operations.
deepakrkris · Sep 23, 2021 · 5920f57 · 5920f57
1 parent 61236ef
commit 5920f57
Show file tree

Hide file tree

Showing 5 changed files with 40 additions and 14 deletions.
diff --git a/napi-inl.h b/napi-inl.h
@@ -1389,14 +1389,16 @@ inline void Object::AddFinalizer(Finalizer finalizeCallback,
 }
 
 #if NAPI_VERSION >= 8
-inline void Object::Freeze() {
+inline bool Object::Freeze() {
   napi_status status = napi_object_freeze(_env, _value);
-  NAPI_THROW_IF_FAILED_VOID(_env, status);
+  NAPI_THROW_IF_FAILED(_env, status, false);
+  return true;
 }
 
-inline void Object::Seal() {
+inline bool Object::Seal() {
   napi_status status = napi_object_seal(_env, _value);
-  NAPI_THROW_IF_FAILED_VOID(_env, status);
+  NAPI_THROW_IF_FAILED(_env, status, false);
+  return true;
 }
 #endif  // NAPI_VERSION >= 8
 

diff --git a/napi.h b/napi.h
@@ -773,8 +773,8 @@ namespace Napi {
                              T* data,
                              Hint* finalizeHint);
 #if NAPI_VERSION >= 8
-    void Freeze();
-    void Seal();
+    bool Freeze();
+    bool Seal();
 #endif  // NAPI_VERSION >= 8
   };
 

diff --git a/test/common/index.js b/test/common/index.js
@@ -76,7 +76,7 @@ exports.mustNotCall = function(msg) {
 };
 
 exports.runTest = async function(test, buildType) {
-  buildType = buildType || process.config.target_defaults.default_configuration;
+  buildType = buildType || process.config.target_defaults.default_configuration || 'Release';
 
   const bindings = [
     `../build/${buildType}/binding.node`,
@@ -90,7 +90,7 @@ exports.runTest = async function(test, buildType) {
 }
 
 exports.runTestWithBindingPath = async function(test, buildType) {
-  buildType = buildType || process.config.target_defaults.default_configuration;
+  buildType = buildType || process.config.target_defaults.default_configuration || 'Release';
 
   const bindings = [
     `../build/${buildType}/binding.node`,

diff --git a/test/object/object_freeze_seal.cc b/test/object/object_freeze_seal.cc
@@ -4,14 +4,14 @@
 
 using namespace Napi;
 
-void Freeze(const CallbackInfo& info) {
+Value Freeze(const CallbackInfo& info) {
   Object obj = info[0].As<Object>();
-  obj.Freeze();
+  return Boolean::New(info.Env(), obj.Freeze());
 }
 
-void Seal(const CallbackInfo& info) {
+Value Seal(const CallbackInfo& info) {
   Object obj = info[0].As<Object>();
-  obj.Seal();
+  return Boolean::New(info.Env(), obj.Seal());
 }
 
 Object InitObjectFreezeSeal(Env env) {

diff --git a/test/object/object_freeze_seal.js b/test/object/object_freeze_seal.js
@@ -7,7 +7,7 @@ module.exports = require('../common').runTest(test);
 function test(binding) {
     {
         const obj = { x: 'a', y: 'b', z: 'c' };
-        binding.object_freeze_seal.freeze(obj);
+        assert.strictEqual(binding.object_freeze_seal.freeze(obj), true);
         assert.strictEqual(Object.isFrozen(obj), true);
         assert.throws(() => {
           obj.x = 10;
@@ -20,9 +20,21 @@ function test(binding) {
         }, /Cannot delete property 'x' of #<Object>/);
     }
 
+    {
+        const obj = new Proxy({ x: 'a', y: 'b', z: 'c' }, {
+          preventExtensions() {
+            throw new Error('foo');
+          },
+        });
+
+        assert.throws(() => {
+          binding.object_freeze_seal.freeze(obj);
+        }, /foo/);
+    }
+
     {
         const obj = { x: 'a', y: 'b', z: 'c' };
-        binding.object_freeze_seal.seal(obj);
+        assert.strictEqual(binding.object_freeze_seal.seal(obj), true);
         assert.strictEqual(Object.isSealed(obj), true);
         assert.throws(() => {
           obj.w = 'd';
@@ -34,4 +46,16 @@ function test(binding) {
         // so this should not throw.
         obj.x = 'd';
     }
+
+    {
+        const obj = new Proxy({ x: 'a', y: 'b', z: 'c' }, {
+          preventExtensions() {
+            throw new Error('foo');
+          },
+        });
+
+        assert.throws(() => {
+          binding.object_freeze_seal.seal(obj);
+        }, /foo/);
+    }
 }