Skip to content

Commit 121cb8f

Browse files
Chad Austinwaywardmonkeys
authored andcommitted
Passing an argument from C++ into JavaScript has 'borrow' semantics rather than ownership semantics. That is, to keep a reference beyond the function call, you must call .clone(). This is necessary to avoid special-casing non-overridden virtual functions. (I don't know if this change will stick. It's possible it will have some problems.)
1 parent a33f6e6 commit 121cb8f

File tree

4 files changed

+95
-1
lines changed

4 files changed

+95
-1
lines changed

src/embind/embind.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -590,6 +590,9 @@ function __embind_register_emval(rawType, name) {
590590
'argPackAdvance': 8,
591591
'readValueFromPointer': simpleReadValueFromPointer,
592592
destructorFunction: null, // This type does not need a destructor
593+
594+
// TODO: do we need a deleteObject here? write a test where
595+
// emval is passed into JS via an interface
593596
});
594597
}
595598

@@ -1195,6 +1198,12 @@ RegisteredPointer.prototype.destructor = function destructor(ptr) {
11951198
RegisteredPointer.prototype['argPackAdvance'] = 8;
11961199
RegisteredPointer.prototype['readValueFromPointer'] = simpleReadValueFromPointer;
11971200

1201+
RegisteredPointer.prototype['deleteObject'] = function deleteObject(handle) {
1202+
if (handle !== null) {
1203+
handle['delete']();
1204+
}
1205+
};
1206+
11981207
RegisteredPointer.prototype['fromWireType'] = function fromWireType(ptr) {
11991208
// ptr is a raw pointer (or a raw smartpointer)
12001209

src/embind/emval.js

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -265,7 +265,14 @@ function __emval_get_method_caller(argCount, argTypes) {
265265
" args += argType" + i + ".argPackAdvance;\n";
266266
}
267267
functionBody +=
268-
" var rv = handle[name](" + argsList + ");\n" +
268+
" var rv = handle[name](" + argsList + ");\n";
269+
for (var i = 0; i < argCount - 1; ++i) {
270+
if (types[i + 1]['deleteObject']) {
271+
functionBody +=
272+
" argType" + i + ".deleteObject(arg" + i + ");\n";
273+
}
274+
}
275+
functionBody +=
269276
" return retType.toWireType(destructors, rv);\n" +
270277
"};\n";
271278

tests/embind/embind.test.js

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1953,6 +1953,53 @@ module({
19531953
rv.delete();
19541954
cm.flushPendingDeletes();
19551955
});
1956+
1957+
test("method arguments with pointer ownership semantics are cleaned up after call", function() {
1958+
var parent = cm.AbstractClass;
1959+
var C = parent.extend("C", {
1960+
abstractMethod: function() {
1961+
},
1962+
});
1963+
var impl = new C;
1964+
cm.passShared(impl);
1965+
impl.delete();
1966+
});
1967+
1968+
test("method arguments with pointer ownership semantics can be cloned", function() {
1969+
var parent = cm.AbstractClass;
1970+
var owned;
1971+
var C = parent.extend("C", {
1972+
abstractMethod: function() {
1973+
},
1974+
passShared: function(p) {
1975+
owned = p.clone();
1976+
}
1977+
});
1978+
var impl = new C;
1979+
cm.passShared(impl);
1980+
impl.delete();
1981+
1982+
assert.equal("Derived", owned.getClassName());
1983+
owned.delete();
1984+
});
1985+
1986+
test("emscripten::val method arguments don't leak", function() {
1987+
var parent = cm.AbstractClass;
1988+
var got;
1989+
var C = parent.extend("C", {
1990+
abstractMethod: function() {
1991+
},
1992+
passVal: function(g) {
1993+
got = g;
1994+
}
1995+
});
1996+
var impl = new C;
1997+
var v = {};
1998+
cm.passVal(impl, v);
1999+
impl.delete();
2000+
2001+
assert.equal(v, got);
2002+
});
19562003
});
19572004

19582005
BaseFixture.extend("registration order", function() {

tests/embind/embind_test.cpp

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1094,6 +1094,12 @@ class AbstractClass {
10941094
std::string concreteMethod() const {
10951095
return "concrete";
10961096
}
1097+
1098+
virtual void passShared(const std::shared_ptr<Derived>&) {
1099+
}
1100+
1101+
virtual void passVal(const val& v) {
1102+
}
10971103
};
10981104

10991105
EMSCRIPTEN_SYMBOL(optionalMethod);
@@ -1120,6 +1126,14 @@ class AbstractClassWrapper : public wrapper<AbstractClass> {
11201126
void differentArguments(int i, double d, unsigned char f, double q, std::string s) {
11211127
return call<void>("differentArguments", i, d, f, q, s);
11221128
}
1129+
1130+
virtual void passShared(const std::shared_ptr<Derived>& p) override {
1131+
return call<void>("passShared", p);
1132+
}
1133+
1134+
virtual void passVal(const val& v) override {
1135+
return call<void>("passVal", v);
1136+
}
11231137
};
11241138

11251139
class ConcreteClass : public AbstractClass {
@@ -1196,6 +1210,15 @@ std::shared_ptr<PolySecondBase> passHeldAbstractClass(std::shared_ptr<HeldAbstra
11961210
return p;
11971211
}
11981212

1213+
void passShared(AbstractClass& ac) {
1214+
auto p = std::make_shared<Derived>();
1215+
ac.passShared(p);
1216+
}
1217+
1218+
void passVal(AbstractClass& ac, val v) {
1219+
return ac.passVal(v);
1220+
}
1221+
11991222
EMSCRIPTEN_BINDINGS(interface_tests) {
12001223
class_<AbstractClass>("AbstractClass")
12011224
.smart_ptr<std::shared_ptr<AbstractClass>>("shared_ptr<AbstractClass>")
@@ -1209,13 +1232,21 @@ EMSCRIPTEN_BINDINGS(interface_tests) {
12091232
}
12101233
))
12111234
.function("concreteMethod", &AbstractClass::concreteMethod)
1235+
.function("passShared", select_overload<void(AbstractClass&, const std::shared_ptr<Derived>&)>([](AbstractClass& self, const std::shared_ptr<Derived>& derived) {
1236+
self.AbstractClass::passShared(derived);
1237+
}))
1238+
.function("passVal", select_overload<void(AbstractClass&, const val&)>([](AbstractClass& self, const val& v) {
1239+
self.AbstractClass::passVal(v);
1240+
}))
12121241
;
12131242

12141243
function("getAbstractClass", &getAbstractClass);
12151244
function("callAbstractMethod", &callAbstractMethod);
12161245
function("callOptionalMethod", &callOptionalMethod);
12171246
function("callReturnsSharedPtrMethod", &callReturnsSharedPtrMethod);
12181247
function("callDifferentArguments", &callDifferentArguments);
1248+
function("passShared", &passShared);
1249+
function("passVal", &passVal);
12191250

12201251
class_<AbstractClassWithConstructor>("AbstractClassWithConstructor")
12211252
.allow_subclass<AbstractClassWithConstructorWrapper>("AbstractClassWithConstructorWrapper", constructor<std::string>())

0 commit comments

Comments
 (0)