[embind] Drop the try/finally around stack-framed invokers

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) {

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' : '';

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
 }

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({