embind: Don't automatically destroy arguments passed into val calls. (#…

…21022)

Before PR #20383 val's `call` and `operator()` had different behaviors where
one would automatically destroy arguments after the call and the other
didn't. After that PR, they both called destroy. The automatic destruction
wasn't really documented anywhere and and led to some unexpected behavior.

This changes it so neither automatically destroys the arguments which I
think is more inline with the rest of embind where it's up to the user
to handle object lifetimes.

Fixes: #21016 and #20095

ChangeLog.md

@@ -50,6 +50,9 @@ See docs/process.md for more on how version tagging works.
   `MIN_CHROME_VERSION` will now result in build-time error.  All of these
   browser versions are at least 8 years old now so the hope is that nobody
   is intending to target them today.  (#20924)
+- C++ objects passed into embind's val via constructors, methods, and call
+  function will not be automatically destroyed after the function call. This
+  makes the behavior consistent for invocations. 
 
 3.1.51 - 12/13/23
 -----------------

src/embind/embind.js

@@ -678,9 +678,6 @@ var LibraryEmbind = {
       'argPackAdvance': GenericWireTypeSize,
       'readValueFromPointer': simpleReadValueFromPointer,
       destructorFunction: null, // This type does not need a destructor
-
-      // TODO: do we need a deleteObject here?  write a test where
-      // emval is passed into JS via an interface
     });
   },
 
@@ -1311,11 +1308,6 @@ var LibraryEmbind = {
       },
       'argPackAdvance': GenericWireTypeSize,
       'readValueFromPointer': readPointer,
-      'deleteObject'(handle) {
-        if (handle !== null) {
-          handle['delete']();
-        }
-      },
       'fromWireType': RegisteredPointer_fromWireType,
     });
   },

src/embind/emval.js

@@ -335,9 +335,6 @@ var LibraryEmVal = {
         offset += types[i]['argPackAdvance'];
       }
       var rv = kind === /* CONSTRUCTOR */ 1 ? reflectConstruct(func, argN) : func.apply(obj, argN);
-      for (var i = 0; i < argCount; ++i) {
-        types[i].deleteObject?.(argN[i]);
-      }
       return emval_returnValue(retType, destructorsRef, rv);
     };
 #else
@@ -362,12 +359,6 @@ var LibraryEmVal = {
     var invoker = kind === /* CONSTRUCTOR */ 1 ? 'new func' : 'func.call';
     functionBody +=
       `  var rv = ${invoker}(${argsList.join(", ")});\n`;
-    for (var i = 0; i < argCount; ++i) {
-      if (types[i]['deleteObject']) {
-        functionBody +=
-          `  argType${i}.deleteObject(arg${i});\n`;
-      }
-    }
     if (!retType.isVoid) {
       params.push("emval_returnValue");
       args.push(emval_returnValue);

test/code_size/embind_val_wasm.json

@@ -1,10 +1,10 @@
 {
   "a.html": 673,
   "a.html.gz": 431,
-  "a.js": 7426,
-  "a.js.gz": 3122,
+  "a.js": 7395,
+  "a.js.gz": 3109,
   "a.wasm": 11458,
   "a.wasm.gz": 5733,
-  "total": 19557,
-  "total_gz": 9286
+  "total": 19526,
+  "total_gz": 9273
 }

test/embind/embind.test.js

@@ -707,6 +707,10 @@ module({
             cm.emval_test_take_and_return_std_string_const_ref("foobar");
         });
 
+        test("val callback arguments are not destroyed", function() {
+            cm.emval_test_callback_arg_lifetime(function() {});
+        });
+
         test("can get global", function(){
             /*jshint evil:true*/
             assert.equal((new Function("return this;"))(), cm.embind_test_getglobal());

test/embind/embind_test.cpp

@@ -127,6 +127,22 @@ unsigned emval_test_sum(val v) {
     return rv;
 }
 
+struct DestructorCounter {
+    static int count;
+    ~DestructorCounter() {
+        count++;
+    };
+};
+
+int DestructorCounter::count = 0;
+
+void emval_test_callback_arg_lifetime(val callback) {
+    DestructorCounter dc;
+    int destructorCount = DestructorCounter::count;
+    callback(dc);
+    assert(destructorCount == DestructorCounter::count);
+}
+
 std::string get_non_ascii_string(bool embindStdStringUTF8Support) {
     if(embindStdStringUTF8Support) {
         //ASCII
@@ -1828,6 +1844,9 @@ EMSCRIPTEN_BINDINGS(tests) {
     function("const_ref_adder", &const_ref_adder);
     function("emval_test_sum", &emval_test_sum);
 
+    class_<DestructorCounter>("DestructorCounter");
+    function("emval_test_callback_arg_lifetime", &emval_test_callback_arg_lifetime);
+
     function("get_non_ascii_string", &get_non_ascii_string);
     function("get_non_ascii_wstring", &get_non_ascii_wstring);
     function("get_literal_wstring", &get_literal_wstring);