Unify onerror handler used by test framework

sbc100 · sbc100 · commit 0ab97a018a2f · 2021-08-25T15:26:24.000-07:00
diff --git a/src/postamble_minimal.js b/src/postamble_minimal.js
@@ -61,6 +61,7 @@ function initRuntime(asm) {
   Module['_emscripten_tls_init'] = _emscripten_tls_init;
   Module['HEAPU32'] = HEAPU32;
   Module['__emscripten_thread_init'] = __emscripten_thread_init;
+  Module['__emscripten_thread_exit'] = __emscripten_thread_exit;
   Module['_pthread_self'] = _pthread_self;
 
   if (ENVIRONMENT_IS_PTHREAD) {
diff --git a/src/preamble.js b/src/preamble.js
@@ -305,17 +305,6 @@ assert(typeof Int32Array !== 'undefined' && typeof Float64Array !== 'undefined'
        'JS engine does not provide full typed array support');
 #endif
 
-#if IN_TEST_HARNESS
-// Test runs in browsers should always be free from uncaught exceptions. If an uncaught exception is thrown, we fail browser test execution in the REPORT_RESULT() macro to output an error value.
-if (ENVIRONMENT_IS_WEB) {
-  window.addEventListener('error', function(e) {
-    if (e.message.includes('unwind')) return;
-    console.error('Page threw an exception ' + e);
-    Module['pageThrewException'] = true;
-  });
-}
-#endif
-
 #if IMPORTED_MEMORY
 // In non-standalone/normal mode, we create the memory here.
 #include "runtime_init_memory.js"
diff --git a/tests/browser/async_bad_list.cpp b/tests/browser/async_bad_list.cpp
@@ -7,32 +7,10 @@
 #include <emscripten.h>
 
 int main() {
-  int x = EM_ASM_INT({
-    window.onerror = function(e) {
-      var message = e.toString();
-      var success = message.indexOf("unreachable") >= 0 || // firefox
-                    message.indexOf("Script error.") >= 0; // chrome
-      if (success && !Module.reported) {
-        Module.reported = true;
-        console.log("reporting success");
-        // manually REPORT_RESULT; we shouldn't call back into native code at this point
-        var xhr = new XMLHttpRequest();
-        xhr.open("GET", "http://localhost:8888/report_result?0");
-        xhr.onload = xhr.onerror = function() {
-          window.close();
-        };
-        xhr.send();
-      }
-    };
-    return 0;
-  });
-
   emscripten_sleep(1);
 
-  // We should not get here - the unwind will fail as we did now all the right
-  // functions - this function should be instrumented, but will not be.
+  // We should not get here - the unwind will fail as we did not list all the
+  // right functions - this function should be instrumented, but will not be.
   puts("We should not get here!");
-  REPORT_RESULT(1);
-
-  return 0;
+  return 1;
 }
diff --git a/tests/browser/async_returnvalue.cpp b/tests/browser/async_returnvalue.cpp
@@ -30,24 +30,6 @@ extern "C" int sync_tunnel_bool(bool);
 #endif
 
 int main() {
-#ifdef BAD
-  EM_ASM({
-    window.onerror = function(e) {
-      var success = e.toString().indexOf("import sync_tunnel was not in ASYNCIFY_IMPORTS, but changed the state") > 0;
-      if (success && !Module.reported) {
-        Module.reported = true;
-        console.log("reporting success");
-        // manually REPORT_RESULT; we shouldn't call back into native code at this point
-        var xhr = new XMLHttpRequest();
-        xhr.open("GET", "http://localhost:8888/report_result?0");
-        xhr.onload = xhr.onerror = function() {
-          window.close();
-        };
-        xhr.send();
-      }
-    };
-  });
-#endif
   int x;
   x = sync_tunnel(0);
   assert(x == 1);
@@ -68,15 +50,13 @@ int main() {
   y = sync_tunnel_bool(true);
   assert(y == false);
 
-
-
 #ifdef BAD
   // We should not get here.
   printf("We should not get here\n");
+  return 1;
 #else
   // Success!
   REPORT_RESULT(0);
-#endif
-
   return 0;
+#endif
 }
diff --git a/tests/browser_reporting.js b/tests/browser_reporting.js
@@ -1,21 +1,22 @@
 var hasModule = typeof Module === 'object' && Module;
+var hasWindow = typeof window === 'object' && window;
+var keepWindowAlive = false;
 
 /** @param {boolean=} sync
     @param {number=} port */
 function reportResultToServer(result, sync, port) {
   port = port || 8888;
   if (reportResultToServer.reported) {
     // Only report one result per test, even if the test misbehaves and tries to report more.
-    reportErrorToServer("excessive reported results, sending " + result + ", test will fail");
+    reportStderrToServer("excessive reported results, sending " + result + ", test will fail");
   }
   reportResultToServer.reported = true;
   var xhr = new XMLHttpRequest();
-  if (hasModule && Module['pageThrewException']) {
-    result = 'pageThrewException';
-  }
   xhr.open('GET', 'http://localhost:' + port + '/report_result?' + result, !sync);
   xhr.send();
-  if (typeof window === 'object' && window && hasModule && !Module['pageThrewException'] /* for easy debugging, don't close window on failure */) setTimeout(function() { window.close() }, 1000);
+  if (hasWindow && hasModule && !keepWindowAlive) {
+    setTimeout(function() { window.close() }, 1000);
+  }
 }
 
 /** @param {boolean=} sync
@@ -25,18 +26,35 @@ function maybeReportResultToServer(result, sync, port) {
   reportResultToServer(result, sync, port);
 }
 
-function reportErrorToServer(message) {
+function reportStderrToServer(message) {
   var xhr = new XMLHttpRequest();
   xhr.open('GET', encodeURI('http://localhost:8888?stderr=' + message));
   xhr.send();
 }
 
+function reportExceptionToServer(e) {
+  var xhr = new XMLHttpRequest();
+  xhr.open('GET', encodeURI('http://localhost:8888?exception=' + e.message + ' / ' + e.stack));
+  xhr.send();
+}
+
 if (typeof window === 'object' && window) {
   function report_error(e) {
+    var message = e.message || e;
+    if (e.error) {
+      message = e.error.message;
+    }
+    // MINIMAL_RUNTIME lets unwind exceptions remain uncaught, and these
+    // should not be considered actually errors.
+    if (message == 'unwind') {
+      return;
+    }
+    console.error("emtest: got top level error: " + message);
+    // We report aborts via the onAbort handler so can ignore them here
+    if (message.indexOf(' abort(') != -1)
+      return;
     // MINIMAL_RUNTIME doesn't handle exit or call the below onExit handler
     // so we detect the exit by parsing the uncaught exception message.
-    var message = e.message || e;
-    console.error("got top level error: " + message);
     var offset = message.indexOf('exit(');
     if (offset != -1) {
       var status = message.substring(offset + 5);
@@ -45,9 +63,14 @@ if (typeof window === 'object' && window) {
       console.error(status);
       maybeReportResultToServer('exit:' + status);
     } else {
-      var xhr = new XMLHttpRequest();
-      xhr.open('GET', encodeURI('http://localhost:8888?exception=' + e.message + ' / ' + e.stack));
-      xhr.send();
+      reportExceptionToServer(e);
+      /*
+       * Also report the exception as the result of the test if non has been
+       * reported yet
+       * For easy debugging, don't close window on failure.
+       */
+      keepWindowAlive = true;
+      maybeReportResultToServer('exception:' + e.message);
     }
   }
   window.addEventListener('error', report_error);
diff --git a/tests/common.py b/tests/common.py
@@ -1334,7 +1334,7 @@ def assert_out_queue_empty(self, who):
   #                     synchronously, so we have a timeout, which can be hit if the VM
   #                     we run on stalls temporarily), so we let each test try more than
   #                     once by default
-  def run_browser(self, html_file, message, expectedResult=None, timeout=None, extra_tries=1):
+  def run_browser(self, html_file, message, expectedResult=None, timeout=None, extra_tries=1, assert_all=False):
     if not has_browser():
       return
     if BrowserCore.unresponsive_tests >= BrowserCore.MAX_UNRESPONSIVE_TESTS:
@@ -1372,12 +1372,16 @@ def run_browser(self, html_file, message, expectedResult=None, timeout=None, ext
           # verify the result, and try again if we should do so
           output = unquote(output)
           try:
-            self.assertContained(expectedResult, output)
+            if assert_all:
+              for o in expectedResult:
+                self.assertContained(o, output)
+            else:
+              self.assertContained(expectedResult, output)
           except Exception as e:
             if extra_tries > 0:
               print('[test error (see below), automatically retrying]')
               print(e)
-              return self.run_browser(html_file, message, expectedResult, timeout, extra_tries - 1)
+              return self.run_browser(html_file, message, expectedResult, timeout, extra_tries - 1, assert_all=assert_all)
             else:
               raise e
       finally:
diff --git a/tests/emscripten_throw_number.c b/tests/emscripten_throw_number.c
@@ -3,7 +3,5 @@
 int main()
 {
 	emscripten_throw_number(42);
-#ifdef REPORT_RESULT
-	REPORT_RESULT(1); // failed
-#endif
+	__builtin_trap();
 }
diff --git a/tests/emscripten_throw_number_pre.js b/tests/emscripten_throw_number_pre.js
diff --git a/tests/emscripten_throw_string.c b/tests/emscripten_throw_string.c
@@ -3,7 +3,5 @@
 int main()
 {
 	emscripten_throw_string("Hello!");
-#ifdef REPORT_RESULT
-	REPORT_RESULT(1); // failed
-#endif
+	__builtin_trap();
 }
diff --git a/tests/emscripten_throw_string_pre.js b/tests/emscripten_throw_string_pre.js
diff --git a/tests/test_browser.py b/tests/test_browser.py
@@ -627,14 +627,6 @@ def setup(assetLocalization):
           <center><canvas id='canvas' width='256' height='256'></canvas></center>
           <hr><div id='output'></div><hr>
           <script type='text/javascript'>
-            window.onerror = function(error) {
-              window.onerror = null;
-              var result = error.indexOf("test.data") >= 0 ? 1 : 0;
-              var xhr = new XMLHttpRequest();
-              xhr.open('GET', 'http://localhost:8888/report_result?' + result, true);
-              xhr.send();
-              setTimeout(function() { window.close() }, 1000);
-            }
             var Module = {
               locateFile: function (path, prefix) {if (path.endsWith(".wasm")) {return prefix + path;} else {return "''' + assetLocalization + r'''" + path;}},
               print: (function() {
@@ -653,17 +645,17 @@ def test():
       setup("")
       self.compile_btest(['main.cpp', '--shell-file', 'on_window_error_shell.html', '--preload-file', 'data.txt', '-o', 'test.html'])
       shutil.move('test.data', 'missing.data')
-      self.run_browser('test.html', '', '/report_result?1')
+      self.run_browser('test.html', '', ['/report_result?exception:', 'test.data'], assert_all=True)
 
       # test unknown protocol should go through xhr.onerror
       setup("unknown_protocol://")
       self.compile_btest(['main.cpp', '--shell-file', 'on_window_error_shell.html', '--preload-file', 'data.txt', '-o', 'test.html'])
-      self.run_browser('test.html', '', '/report_result?1')
+      self.run_browser('test.html', '', ['/report_result?exception:', 'test.data'], assert_all=True)
 
       # test wrong protocol and port
       setup("https://localhost:8800/")
       self.compile_btest(['main.cpp', '--shell-file', 'on_window_error_shell.html', '--preload-file', 'data.txt', '-o', 'test.html'])
-      self.run_browser('test.html', '', '/report_result?1')
+      self.run_browser('test.html', '', ['/report_result?exception:', 'test.data'], assert_all=True)
 
     test()
 
@@ -3321,22 +3313,33 @@ def test_async_iostream(self):
   # ASYNCIFY_IMPORTS.
   # To make the test more precise we also use ASYNCIFY_IGNORE_INDIRECT here.
   @parameterized({
-    'normal': (['-s', 'ASYNCIFY_IMPORTS=[sync_tunnel, sync_tunnel_bool]'],), # noqa
-    'response': (['-s', 'ASYNCIFY_IMPORTS=@filey.txt'],), # noqa
-    'nothing': (['-DBAD'],), # noqa
-    'empty_list': (['-DBAD', '-s', 'ASYNCIFY_IMPORTS=[]'],), # noqa
-    'em_js_bad': (['-DBAD', '-DUSE_EM_JS'],), # noqa
+    'normal': (False, ['-s', 'ASYNCIFY_IMPORTS=[sync_tunnel, sync_tunnel_bool]'],), # noqa
+    'response': (False, ['-s', 'ASYNCIFY_IMPORTS=@filey.txt'],), # noqa
+    'nothing': (True, ['-DBAD'],), # noqa
+    'empty_list': (True, ['-DBAD', '-s', 'ASYNCIFY_IMPORTS=[]'],), # noqa
+    'em_js_bad': (True, ['-DBAD', '-DUSE_EM_JS'],), # noqa
   })
-  def test_async_returnvalue(self, args):
+  def test_async_returnvalue(self, bad, args):
+    if bad:
+      expected = [
+        'exception:Uncaught Error: import sync_tunnel was not in ASYNCIFY_IMPORTS, but changed the state', # firefox
+        'exception:Error: import sync_tunnel was not in ASYNCIFY_IMPORTS, but changed the state' # chrome
+      ]
+    else:
+      expected = "0"
     if '@' in str(args):
       create_file('filey.txt', 'sync_tunnel\nsync_tunnel_bool\n')
-    self.btest('browser/async_returnvalue.cpp', '0', args=['-s', 'ASYNCIFY', '-s', 'ASYNCIFY_IGNORE_INDIRECT', '--js-library', test_file('browser/async_returnvalue.js')] + args + ['-s', 'ASSERTIONS'])
+    self.btest('browser/async_returnvalue.cpp', expected, args=['-s', 'ASYNCIFY', '-s', 'ASYNCIFY_IGNORE_INDIRECT', '--js-library', test_file('browser/async_returnvalue.js')] + args + ['-s', 'ASSERTIONS'])
 
   def test_async_stack_overflow(self):
     self.btest('browser/async_stack_overflow.cpp', 'abort:RuntimeError: unreachable', args=['-s', 'ASYNCIFY', '-s', 'ASYNCIFY_STACK_SIZE=4'])
 
   def test_async_bad_list(self):
-    self.btest('browser/async_bad_list.cpp', '0', args=['-s', 'ASYNCIFY', '-s', 'ASYNCIFY_ONLY=[waka]', '--profiling'])
+    expected = [
+        'exception:RuntimeError: unreachable executed', # firefox
+        'exception:Uncaught RuntimeError: unreachable', # chrome
+    ]
+    self.btest('browser/async_bad_list.cpp', expected, args=['-s', 'ASYNCIFY', '-s', 'ASYNCIFY_ONLY=[waka]', '--profiling'])
 
   # Tests that when building with -s MINIMAL_RUNTIME=1, the build can use -s MODULARIZE=1 as well.
   def test_minimal_runtime_modularize(self):
@@ -4303,8 +4306,9 @@ def test_small_js_flags(self):
     # Check an absolute js code size, with some slack.
     size = os.path.getsize('test.js')
     print('size:', size)
-    # Note that this size includes test harness additions (for reporting the result, etc.).
-    self.assertLess(abs(size - 5629), 100)
+    # Note that this size includes test harness additions (so will change when, for example,
+    # browser_reporting.js changed)
+    self.assertLess(abs(size - 5496), 100)
 
   # Tests that it is possible to initialize and render WebGL content in a pthread by using OffscreenCanvas.
   # -DTEST_CHAINED_WEBGL_CONTEXT_PASSING: Tests that it is possible to transfer WebGL canvas in a chain from main thread -> thread 1 -> thread 2 and then init and render WebGL content there.
@@ -4984,10 +4988,10 @@ def test_emscripten_console_log(self):
     self.btest(test_file('emscripten_console_log.c'), '0', args=['--pre-js', test_file('emscripten_console_log_pre.js')])
 
   def test_emscripten_throw_number(self):
-    self.btest(test_file('emscripten_throw_number.c'), '0', args=['--pre-js', test_file('emscripten_throw_number_pre.js')])
+    self.btest(test_file('emscripten_throw_number.c'), 'exception:Uncaught 42')
 
   def test_emscripten_throw_string(self):
-    self.btest(test_file('emscripten_throw_string.c'), '0', args=['--pre-js', test_file('emscripten_throw_string_pre.js')])
+    self.btest(test_file('emscripten_throw_string.c'), 'exception:Uncaught Hello!')
 
   # Tests that Closure run in combination with -s ENVIRONMENT=web mode works with a minimal console.log() application
   def test_closure_in_web_only_target_environment_console_log(self):

Original file line number	Diff line number	Diff line change
`@@ -3,7 +3,5 @@`
`3`	`3`	`int main()`
`4`	`4`	`{`
`5`	`5`	`emscripten_throw_number(42);`
`6`		`-#ifdef REPORT_RESULT`
`7`		`- REPORT_RESULT(1); // failed`
`8`		`-#endif`
	`6`	`+ __builtin_trap();`
`9`	`7`	`}`
Original file line number	Diff line number	Diff line change
`@@ -3,7 +3,5 @@`
`3`	`3`	`int main()`
`4`	`4`	`{`
`5`	`5`	`emscripten_throw_string("Hello!");`
`6`		`-#ifdef REPORT_RESULT`
`7`		`- REPORT_RESULT(1); // failed`
`8`		`-#endif`
	`6`	`+ __builtin_trap();`
`9`	`7`	`}`