diff --git a/src/lib/libembind.js b/src/lib/libembind.js index d14f950295d05..640dc26da2a54 100644 --- a/src/lib/libembind.js +++ b/src/lib/libembind.js @@ -745,30 +745,27 @@ var LibraryEmbind = { sp = stackSave(); } var thisWired; - var rv; - // The frame must be released on every completion, including a throwing - // argument conversion or callee: a skipped stackRestore permanently - // leaks wasm stack. - try { - invokerFuncArgs.length = isClassMethodFunc ? 2 : 1; - invokerFuncArgs[0] = cppTargetFunc; - if (isClassMethodFunc) { - thisWired = argTypes[1].toWireType(destructors, this); - invokerFuncArgs[1] = thisWired; - } - for (var i = 0; i < expectedArgCount; ++i) { - var argType = argTypes[i + 2]; - // Stack-allocating types take the stack path only under a frame; a - // null destructors argument is that contract. - argsWired[i] = argType.toWireType(useStackFrame && argType.argStackAlloc ? null : destructors, args[i]); - invokerFuncArgs.push(argsWired[i]); - } + invokerFuncArgs.length = isClassMethodFunc ? 2 : 1; + invokerFuncArgs[0] = cppTargetFunc; + if (isClassMethodFunc) { + thisWired = argTypes[1].toWireType(destructors, this); + invokerFuncArgs[1] = thisWired; + } + for (var i = 0; i < expectedArgCount; ++i) { + var argType = argTypes[i + 2]; + // Stack-allocating types take the stack path only under a frame; a + // null destructors argument is that contract. + argsWired[i] = argType.toWireType(useStackFrame && argType.argStackAlloc ? null : destructors, args[i]); + invokerFuncArgs.push(argsWired[i]); + } - rv = cppInvokerFunc(...invokerFuncArgs); - } finally { - if (useStackFrame) { - stackRestore(sp); - } + var rv = cppInvokerFunc(...invokerFuncArgs); + if (useStackFrame) { + // The callee has consumed the stack-allocated argument temporaries. + // As everywhere else in emscripten, the frame is not restored on an + // exception path; a JS caller that catches and continues must + // stackSave/stackRestore around the call (see the exceptions docs). + stackRestore(sp); } function onDone(rv) { diff --git a/src/lib/libembind_shared.js b/src/lib/libembind_shared.js index 3a30a765123f0..5c1fd8881fbbb 100644 --- a/src/lib/libembind_shared.js +++ b/src/lib/libembind_shared.js @@ -277,10 +277,7 @@ var LibraryEmbindShared = { invokerFnBody += 'var destructors = [];\n'; } if (useStackFrame) { - // The frame must be released on every completion, including a throwing - // argument conversion or callee: a skipped stackRestore permanently - // leaks wasm stack. `var` declarations hoist out of the try block. - invokerFnBody += 'var sp = stackSave();\ntry {\n'; + invokerFnBody += 'var sp = stackSave();\n'; } var dtorStack = needsDestructorStack ? 'destructors' : 'null'; @@ -306,8 +303,10 @@ var LibraryEmbindShared = { invokerFnBody += (returns || isAsync ? 'var rv = ' : '') + `invoker(${argsListWired});\n`; if (useStackFrame) { // The callee has consumed the stack-allocated argument temporaries; - // release the frame before any post-call work. - invokerFnBody += '} finally {\nstackRestore(sp);\n}\n'; + // release the frame before any post-call work. Not restored on an + // exception path, matching the rest of emscripten (see the exceptions + // docs on stackSave/stackRestore at JS catch sites). + invokerFnBody += 'stackRestore(sp);\n'; } var returnVal = returns ? 'rv' : ''; diff --git a/test/codesize/test_minimal_runtime_code_size_hello_embind.json b/test/codesize/test_minimal_runtime_code_size_hello_embind.json index 4ee8bc01f4164..4443b82bc4b68 100644 --- a/test/codesize/test_minimal_runtime_code_size_hello_embind.json +++ b/test/codesize/test_minimal_runtime_code_size_hello_embind.json @@ -1,10 +1,10 @@ { "a.html": 548, "a.html.gz": 371, - "a.js": 7487, - "a.js.gz": 3437, + "a.js": 7464, + "a.js.gz": 3418, "a.wasm": 7201, "a.wasm.gz": 3303, - "total": 15236, - "total_gz": 7111 + "total": 15213, + "total_gz": 7092 } diff --git a/test/embind/test_embind_trivial_value_stack.cpp b/test/embind/test_embind_trivial_value_stack.cpp index df2a100526655..67dbcf7e47cde 100644 --- a/test/embind/test_embind_trivial_value_stack.cpp +++ b/test/embind/test_embind_trivial_value_stack.cpp @@ -163,26 +163,6 @@ int main() { printf("trivial allocs: %d frees: %d\n", allocCount, freeCount); assert(allocCount == 0 && freeCount == 0); - // A conversion that throws mid-call must unwind the stack frame; the - // stackSave comparison below catches any leaked frame. - EM_ASM({ - var trap = ({ index: 1, world: 2, get generation() { throw new Error('boom'); } }); - var sp = stackSave(); - for (var i = 0; i < 1000; i++) { - try { - Module['sumVec'](trap, [1, 2, 3]); - // No .message on this string, so the catch below rethrows it. - throw 'expected sumVec to throw'; - } catch (e) { - if (e.message !== 'boom') throw e; - } - } - if (stackSave() !== sp) throw 'stack leaked across throwing conversions'; - if (Module['sumId']({ index: 1, world: 2, generation: 3 }) !== 6) throw 'sumId broken after throws'; - }); - printf("throwing allocs: %d frees: %d\n", allocCount, freeCount); - assert(allocCount == 0 && freeCount == 0); - // The non-trivial type stays on the heap path, balances its allocations, // and runs its destructor. EM_ASM({