Skip to content

Commit

Permalink
embind: Don't automatically destroy arguments passed into val calls.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
brendandahl committed Jan 8, 2024
1 parent 67ce331 commit 99bb5ae
Show file tree
Hide file tree
Showing 5 changed files with 26 additions and 17 deletions.
3 changes: 3 additions & 0 deletions ChangeLog.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,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
-----------------
Expand Down
8 changes: 0 additions & 8 deletions src/embind/embind.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
});
},
Expand Down Expand Up @@ -1311,11 +1308,6 @@ var LibraryEmbind = {
},
'argPackAdvance': GenericWireTypeSize,
'readValueFromPointer': readPointer,
'deleteObject'(handle) {
if (handle !== null) {
handle['delete']();
}
},
'fromWireType': RegisteredPointer_fromWireType,
});
},
Expand Down
9 changes: 0 additions & 9 deletions src/embind/emval.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
Expand Down
4 changes: 4 additions & 0 deletions test/embind/embind.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
19 changes: 19 additions & 0 deletions test/embind/embind_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 99bb5ae

Please sign in to comment.