Disable NODEJS_CATCH_EXIT by default. NFC

It turns out that this setting is only needed in very specific
circumstances.  Specifically its only needed if native code calls
`exit` a callback that was not wrapped in callUserCallback.

This change removes the need for this setting from all our unit tests
and disables the setting by default.

Fixes: emscripten-core#17228

ChangeLog.md

@@ -20,6 +20,9 @@ See docs/process.md for more on how version tagging works.
 
 3.1.64 (in development)
 -----------------------
+- The `NODEJS_CATCH_EXIT` setting is now disabled by default.  This setting
+  is only useful under very specific circumstances, and has some downsides, so
+  disabling it by default makes sense.
 - Updated the SCons tool to not require the `EMSCRIPTEN_ROOT` environment
   variable, in which case it will assume that SCons will find the binaries in
   (its) `PATH`.

site/source/docs/tools_reference/settings_reference.rst

@@ -1185,7 +1185,7 @@ catch and handle ExitStatus exceptions.  However, this means all other
 uncaught exceptions are also caught and re-thrown, which is not always
 desirable.
 
-Default value: true
+Default value: false
 
 .. _nodejs_catch_rejection:
 

src/library_browser.js

@@ -770,9 +770,6 @@ var LibraryBrowser = {
   emscripten_async_load_script__deps: ['$UTF8ToString'],
   emscripten_async_load_script: (url, onload, onerror) => {
     url = UTF8ToString(url);
-    onload = {{{ makeDynCall('v', 'onload') }}};
-    onerror = {{{ makeDynCall('v', 'onerror') }}};
-
 #if PTHREADS
     if (ENVIRONMENT_IS_PTHREAD) {
       err(`emscripten_async_load_script("${url}") failed, emscripten_async_load_script is currently not available in pthreads!`);
@@ -787,17 +784,20 @@ var LibraryBrowser = {
     var loadDone = () => {
       {{{ runtimeKeepalivePop() }}}
       if (onload) {
+        var onloadCallback = () => callUserCallback({{{ makeDynCall('v', 'onload') }}});
         if (runDependencies > 0) {
-          dependenciesFulfilled = onload;
+          dependenciesFulfilled = onloadCallback;
         } else {
-          onload();
+          onloadCallback();
         }
       }
     }
 
     var loadError = () => {
       {{{ runtimeKeepalivePop() }}}
-      onerror?.();
+      if (onerror) {
+        callUserCallback({{{ makeDynCall('v', 'onerror') }}});
+      }
     };
 
 #if ENVIRONMENT_MAY_BE_NODE && DYNAMIC_EXECUTION

src/settings.js

@@ -795,7 +795,7 @@ var WASM_EXNREF = false;
 // desirable.
 //
 // [link]
-var NODEJS_CATCH_EXIT = true;
+var NODEJS_CATCH_EXIT = false;
 
 // Catch unhandled rejections in node. This only effect versions of node older
 // than 15.  Without this, old version node will print a warning, but exit

test/common.py

@@ -1366,7 +1366,7 @@ def run_js(self, filename, engine=None, args=None,
       if assert_returncode == NON_ZERO:
         self.fail('JS subprocess unexpectedly succeeded (%s):  Output:\n%s' % (error.cmd, ret))
       else:
-        self.fail('JS subprocess failed (%s): %s.  Output:\n%s' % (error.cmd, error.returncode, ret))
+        self.fail('JS subprocess failed (%s): %s (expected=%s).  Output:\n%s' % (error.cmd, error.returncode, assert_returncode, ret))
 
     #  We should pass all strict mode checks
     self.assertNotContained('strict warning:', ret)

test/test_core.py

@@ -5957,7 +5957,7 @@ def test_signals(self):
 
   @parameterized({
     'sigint': (EM_SIGINT, 128 + EM_SIGINT, True),
-    'sigabrt': (EM_SIGABRT, 7, False)
+    'sigabrt': (EM_SIGABRT, 1, False)
   })
   @crossplatform
   def test_sigaction_default(self, signal, exit_code, assert_identical):

test/test_fs_after_main.c

@@ -35,6 +35,8 @@ void looper() {
   fclose(f);
 }
 
+EM_JS_DEPS(deps, "$safeSetTimeout");
+
 int main() {
   EM_ASM({
     (function() {
@@ -60,10 +62,10 @@ int main() {
       counter++;
       if (counter < 5) {
         out('js queueing');
-        setTimeout(looper, 1);
+        safeSetTimeout(looper, 1);
       } else {
         out('js finishing');
-        setTimeout(Module['_finish'], 1);
+        safeSetTimeout(Module['_finish'], 1);
       }
     }
     looper();

test/test_other.py

@@ -3836,11 +3836,12 @@ def test_module_exports_with_closure(self):
 
   @requires_node
   def test_node_catch_exit(self):
-    # Test that in node.js exceptions are not caught if NODEJS_EXIT_CATCH=0
+    # Test that in top level JS exceptions are caught and rethrown when NODEJS_EXIT_CATCH=1
+    # is set but not by default.
     create_file('count.c', '''
       #include <string.h>
       int count(const char *str) {
-          return (int)strlen(str);
+        return (int)strlen(str);
       }
     ''')
 
@@ -3852,13 +3853,13 @@ def test_node_catch_exit(self):
 
     reference_error_text = 'console.log(xxx); //< here is the ReferenceError'
 
-    self.run_process([EMCC, 'count.c', '-o', 'count.js'])
+    self.run_process([EMCC, 'count.c', '-o', 'count.js', '-sNODEJS_CATCH_EXIT=1'])
 
     # Check that the ReferenceError is caught and rethrown and thus the original error line is masked
     self.assertNotContained(reference_error_text,
                             self.run_js('index.js', assert_returncode=NON_ZERO))
 
-    self.run_process([EMCC, 'count.c', '-o', 'count.js', '-sNODEJS_CATCH_EXIT=0'])
+    self.run_process([EMCC, 'count.c', '-o', 'count.js'])
 
     # Check that the ReferenceError is not caught, so we see the error properly
     self.assertContained(reference_error_text,
@@ -6171,16 +6172,17 @@ def test_embed_file_large(self):
 
   def test_force_exit(self):
     create_file('src.c', r'''
+#include <emscripten/console.h>
 #include <emscripten/emscripten.h>
 
-EMSCRIPTEN_KEEPALIVE void callback() {
-  EM_ASM({ out('callback pre()') });
+void callback() {
+  emscripten_out("callback pre()");
   emscripten_force_exit(42);
-  EM_ASM({ out('callback post()') });
+  emscripten_out("callback post()");
 }
 
 int main() {
-  EM_ASM({ setTimeout(() => { out("calling callback()"); _callback() }, 100) });
+  emscripten_async_call(callback, NULL, 100);
   emscripten_exit_with_live_runtime();
   return 123;
 }
@@ -11725,17 +11727,18 @@ def test_assertions_on_reject_promise(self):
       Promise.reject();
     ''')
     self.run_process([EMCC, test_file('hello_world.c'), '-sASSERTIONS', '--extern-post-js', 'test.js'])
-    # A return code of 7 is from the unhandled Promise rejection
-    self.run_js('a.out.js', assert_returncode=7)
+    # Node exits with 1 on Uncaught Fatal Exception (including unhandled rejections)
+    self.run_js('a.out.js', assert_returncode=1)
 
   def test_on_reject_promise(self):
     # Check that promise rejections give the correct error code
     create_file('test.js', r'''
       Promise.reject();
     ''')
     self.run_process([EMCC, test_file('hello_world.c'), '--extern-post-js', 'test.js'])
-    # A return code of 7 is from the unhandled Promise rejection
-    self.run_js('a.out.js', assert_returncode=7)
+    # Node exits with 1 on Uncaught Fatal Exception (including unhandled rejections)
+    err = self.run_js('a.out.js', assert_returncode=1)
+    self.assertContained('UnhandledPromiseRejection', err)
 
   def test_em_asm_duplicate_strings(self):
     # We had a regression where tow different EM_ASM strings from two diffferent