[EH] Fix inconsistency of in/decrementing refcounts (#25988)

This fixes inconsistency of incrementing/decrementing refcounts between
Wasm EH and Emscripten EH.

Previously, in Emscripten EH, we incremented the refcount in
`__cxa_begin_catch`, while Wasm EH incremented it in `__cxa_throw`. This
PR moves the incrementing call from `__cxa_begin_catch` to `__cxa_throw`
as well. This also increments the refcount in `__cxa_rethrow`.

These incrementing calls are guarded with
`+#if !DISABLE_EXCEPTION_CATCHING`, because without that,
`std::terminate` will run:

https://github.com/emscripten-core/emscripten/blob/d1251798144df813c52934768964a1223504c440/system/lib/libcxxabi/src/cxa_noexception.cpp#L25-L35

Fixes #17115.

Author: aheejin

ChangeLog.md

@@ -20,6 +20,9 @@ See docs/process.md for more on how version tagging works.
 
 4.0.23 (in development)
 -----------------------
+- The inconsistency of incrementing / decrementing refcounts between Wasm EH and
+  Emscripten EH has been fixed. See `test_EXPORT_EXCEPTION_HANDLING_HELPERS` in
+  `test_core.py` to see the usage. (#25988)
 - The `select()` and `poll()` system calls can now block under certain
   circumstances.  Specifically, if they are called from a background thread and
   file descriptors include pipes.  (#25523, #25990)

site/source/docs/porting/exceptions.rst

@@ -155,16 +155,10 @@ leaked.
 
 .. note:: If you catch a Wasm exception and do not rethrow it, you need to free
    the storage associated with the exception in JS using
-   ``decrementExceptionRefcount`` method because the exception
-   catching code in Wasm does not have a chance to free it. But currently due to
-   an implementation issue that Wasm EH and Emscripten (JS-based) EH, you need
-   to call incrementExceptionRefcount additionally in case of Emscripten EH. See
-   https://github.com/emscripten-core/emscripten/issues/17115 for details and a
-   code example.
-
-.. todo:: Fix the above-mentinoed `inconsistency
-   <https://github.com/emscripten-core/emscripten/issues/17115>`_ between Wasm
-   EH and Emscripten EH, on the reference counting.
+   ``decrementExceptionRefcount`` method because the exception catching code in
+   Wasm does not have a chance to free it. See
+   ``test_EXPORT_EXCEPTION_HANDLING_HELPERS`` in test/test_core.py for an
+   example usage.
 
 
 Using Exceptions and setjmp-longjmp Together

site/source/docs/tools_reference/settings_reference.rst

@@ -1134,9 +1134,7 @@ the JS library:
 Setting this option also adds refcount increasing and decreasing functions
 ('incrementExceptionRefcount' and 'decrementExceptionRefcount') in the JS
 library because if you catch an exception from JS, you may need to manipulate
-the refcount manually not to leak memory. What you need to do is different
-depending on the kind of EH you use
-(https://github.com/emscripten-core/emscripten/issues/17115).
+the refcount manually not to leak memory.
 
 See test_EXPORT_EXCEPTION_HANDLING_HELPERS in test/test_core.py for an
 example usage.

src/lib/libexceptions.js

@@ -82,14 +82,21 @@ var LibraryExceptions = {
 
   // Here, we throw an exception after recording a couple of values that we need to remember
   // We also remember that it was the last exception thrown as we need to know that later.
-  __cxa_throw__deps: ['$ExceptionInfo', '$exceptionLast', '$uncaughtExceptionCount'],
+  __cxa_throw__deps: ['$ExceptionInfo', '$exceptionLast', '$uncaughtExceptionCount'
+#if !DISABLE_EXCEPTION_CATCHING
+  , '__cxa_increment_exception_refcount'
+#endif
+  ],
   __cxa_throw: (ptr, type, destructor) => {
 #if EXCEPTION_DEBUG
     dbg('__cxa_throw: ' + [ptrToString(ptr), type, ptrToString(destructor)]);
 #endif
     var info = new ExceptionInfo(ptr);
     // Initialize ExceptionInfo content after it was allocated in __cxa_allocate_exception.
     info.init(type, destructor);
+#if !DISABLE_EXCEPTION_CATCHING
+    ___cxa_increment_exception_refcount(ptr);
+#endif
     {{{ storeException('exceptionLast', 'ptr') }}}
     uncaughtExceptionCount++;
     {{{ makeThrow('exceptionLast') }}}
@@ -98,7 +105,11 @@ var LibraryExceptions = {
   // This exception will be caught twice, but while begin_catch runs twice,
   // we early-exit from end_catch when the exception has been rethrown, so
   // pop that here from the caught exceptions.
-  __cxa_rethrow__deps: ['$exceptionCaught', '$exceptionLast', '$uncaughtExceptionCount'],
+  __cxa_rethrow__deps: ['$exceptionCaught', '$exceptionLast', '$uncaughtExceptionCount'
+#if !DISABLE_EXCEPTION_CATCHING
+  , '__cxa_increment_exception_refcount'
+#endif
+  ],
   __cxa_rethrow: () => {
     var info = exceptionCaught.pop();
     if (!info) {
@@ -112,6 +123,9 @@ var LibraryExceptions = {
       info.set_caught(false);
       uncaughtExceptionCount++;
     }
+#if !DISABLE_EXCEPTION_CATCHING
+    ___cxa_increment_exception_refcount(ptr);
+#endif
 #if EXCEPTION_DEBUG
     dbg('__cxa_rethrow, popped ' +
       [ptrToString(ptr), exceptionLast, 'stack', exceptionCaught]);
@@ -122,8 +136,7 @@ var LibraryExceptions = {
 
   llvm_eh_typeid_for: (type) => type,
 
-  __cxa_begin_catch__deps: ['$exceptionCaught', '__cxa_increment_exception_refcount',
-                            '__cxa_get_exception_ptr',
+  __cxa_begin_catch__deps: ['$exceptionCaught', '__cxa_get_exception_ptr',
                             '$uncaughtExceptionCount'],
   __cxa_begin_catch: (ptr) => {
     var info = new ExceptionInfo(ptr);
@@ -136,7 +149,6 @@ var LibraryExceptions = {
 #if EXCEPTION_DEBUG
     dbg('__cxa_begin_catch ' + [ptrToString(ptr), 'stack', exceptionCaught]);
 #endif
-    ___cxa_increment_exception_refcount(ptr);
     return ___cxa_get_exception_ptr(ptr);
   },
 

src/settings.js

@@ -768,9 +768,7 @@ var DISABLE_EXCEPTION_THROWING = false;
 // Setting this option also adds refcount increasing and decreasing functions
 // ('incrementExceptionRefcount' and 'decrementExceptionRefcount') in the JS
 // library because if you catch an exception from JS, you may need to manipulate
-// the refcount manually not to leak memory. What you need to do is different
-// depending on the kind of EH you use
-// (https://github.com/emscripten-core/emscripten/issues/17115).
+// the refcount manually not to leak memory.
 //
 // See test_EXPORT_EXCEPTION_HANDLING_HELPERS in test/test_core.py for an
 // example usage.

test/codesize/test_codesize_cxx_except.json

@@ -1,10 +1,10 @@
 {
-  "a.out.js": 23309,
-  "a.out.js.gz": 9122,
+  "a.out.js": 23315,
+  "a.out.js.gz": 9125,
   "a.out.nodebug.wasm": 171263,
   "a.out.nodebug.wasm.gz": 57315,
-  "total": 194572,
-  "total_gz": 66437,
+  "total": 194578,
+  "total_gz": 66440,
   "sent": [
     "__cxa_begin_catch",
     "__cxa_end_catch",

test/codesize/test_codesize_cxx_mangle.json

@@ -1,10 +1,10 @@
 {
-  "a.out.js": 23359,
-  "a.out.js.gz": 9142,
+  "a.out.js": 23365,
+  "a.out.js.gz": 9145,
   "a.out.nodebug.wasm": 235290,
   "a.out.nodebug.wasm.gz": 78917,
-  "total": 258649,
-  "total_gz": 88059,
+  "total": 258655,
+  "total_gz": 88062,
   "sent": [
     "__cxa_begin_catch",
     "__cxa_end_catch",

test/test_core.py

@@ -1551,18 +1551,8 @@ class myexception : public exception {
               } catch(p) {
                   // Because we are catching and handling the exception in JS, the normal
                   // exception catching C++ code doesn't kick in, so we need to make sure we free
-                  // the exception, if necessary. By incrementing and decrementing the refcount
-                  // we trigger the free'ing of the exception if its refcount was zero.
-#ifdef __USING_EMSCRIPTEN_EXCEPTION__
-                  // FIXME Currently Wasm EH and Emscripten EH increases
-                  // refcounts in different places. Wasm EH sets the refcount to
-                  // 1 when throwing, and decrease it in __cxa_end_catch.
-                  // Emscripten EH sets the refcount to 0 when throwing, and
-                  // increase it in __cxa_begin_catch, and decrease it in
-                  // __cxa_end_catch. Fix this inconsistency later.
-                  // https://github.com/emscripten-core/emscripten/issues/17115
-                  incrementExceptionRefcount(p);
-#endif
+                  // the exception, if necessary. By decrementing the refcount we trigger the
+                  // free'ing of the exception.
                   out(getExceptionMessage(p).toString());
                   decrementExceptionRefcount(p);
               }