From a755bcc6361a8d392edd1f7887ea75dd52ce2be6 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Tue, 9 Jan 2024 15:14:36 -0800 Subject: [PATCH 1/4] [EH] Check if Exception.stack is writable Apparently `WebAssembly.Exception.stack` has been `readonly` all along and I wasn't aware. But I wonder why this has to be `readonly`, and currently writing to this property works in Chrome and Node and fails in Firefox. I'll ask in the EH repo whether this should be `readonly`, but in the meantime, we check if this is writable before writing to it. Temporarily fixes #21044. Not sure how to test this... Do we run Firefox tests? --- src/library_exceptions.js | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/library_exceptions.js b/src/library_exceptions.js index bfe919e890fe5..8cd82ab3ce0b7 100644 --- a/src/library_exceptions.js +++ b/src/library_exceptions.js @@ -337,9 +337,15 @@ var LibraryExceptions = { // Remove this JS function name, which is in the second line, from the stack // trace. Note that .stack does not yet exist in all browsers (see #18828). if (e.stack) { - var arr = e.stack.split('\n'); - arr.splice(1,1); - e.stack = arr.join('\n'); + // WebAssembly.Exception.stack is currently readonly, but this can change, + // and it is implemented as writable in Chrome and Node. Will check the + // 'writable' condition for now and later resolve this by either deleting + // this altogether or removing this condition check. + if (Object.getOwnPropertyDescriptor(e, 'stack').writable) { + var arr = e.stack.split('\n'); + arr.splice(1,1); + e.stack = arr.join('\n'); + } } throw e; }, From 7cf1c69bcd1d1346a5ef1385a158b55831b46713 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Wed, 10 Jan 2024 11:33:08 -0800 Subject: [PATCH 2/4] Add a browser test --- src/library_exceptions.js | 22 +++++++++++----------- test/test_browser.py | 19 +++++++++++++++++++ 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/src/library_exceptions.js b/src/library_exceptions.js index 8cd82ab3ce0b7..e99f1112fe49d 100644 --- a/src/library_exceptions.js +++ b/src/library_exceptions.js @@ -335,17 +335,17 @@ var LibraryExceptions = { // ... // // Remove this JS function name, which is in the second line, from the stack - // trace. Note that .stack does not yet exist in all browsers (see #18828). - if (e.stack) { - // WebAssembly.Exception.stack is currently readonly, but this can change, - // and it is implemented as writable in Chrome and Node. Will check the - // 'writable' condition for now and later resolve this by either deleting - // this altogether or removing this condition check. - if (Object.getOwnPropertyDescriptor(e, 'stack').writable) { - var arr = e.stack.split('\n'); - arr.splice(1,1); - e.stack = arr.join('\n'); - } + // trace. + // + // Note that .stack does not yet exist in all browsers (see #18828). + // Also, WebAssembly.Exception.stack is currently readonly, but this can + // change, and it is implemented as writable in Chrome and Node. Will check + // the 'writable' condition for now and later resolve this by either + // deleting this altogether or removing this condition check. + if (e.stack && Object.getOwnPropertyDescriptor(e, 'stack').writable) { + var arr = e.stack.split('\n'); + arr.splice(1,1); + e.stack = arr.join('\n'); } throw e; }, diff --git a/test/test_browser.py b/test/test_browser.py index d614a215d314f..476524ab5556c 100644 --- a/test/test_browser.py +++ b/test/test_browser.py @@ -5825,6 +5825,25 @@ def test_webpack(self): shutil.copyfile('webpack/src/hello.wasm', 'webpack/dist/hello.wasm') self.run_browser('webpack/dist/index.html', '/report_result?exit:0') + def test_exceptions_stack_trace_and_message_wasm_eh(self): + # Currently writing to WebAssembly.Exception.stack property is not allowed + # in Firefox, but we need to ensure generating stack traces runs on FireFox + # without erroring out, even though stack trace simplification will not be + # done. See https://github.com/emscripten-core/emscripten/pull/21050 + if not is_firefox(): + self.skipTest('https://github.com/emscripten-core/emscripten/pull/21050') + + create_file('main.c', r''' + #include + int main() { + throw std::runtime_error("my message"); + return 0; + } + ''') + self.emcc_args = ['-g', '-fwasm-exceptions'] + self.set_setting('EXCEPTION_STACK_TRACES', 1) + self.btest_exit('main.c') + class emrun(RunnerCore): def test_emrun_info(self): From b99361d58e9d7d555706b146a5852a94ffc5c114 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Wed, 10 Jan 2024 13:26:00 -0800 Subject: [PATCH 3/4] c->cpp --- test/test_browser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_browser.py b/test/test_browser.py index 50eeba89cea41..22fb435ceb915 100644 --- a/test/test_browser.py +++ b/test/test_browser.py @@ -5833,7 +5833,7 @@ def test_exceptions_stack_trace_and_message_wasm_eh(self): if not is_firefox(): self.skipTest('https://github.com/emscripten-core/emscripten/pull/21050') - create_file('main.c', r''' + create_file('main.cpp', r''' #include int main() { throw std::runtime_error("my message"); From 23a16f91eccb62e619c109023f249c58d6e0b5ba Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Wed, 10 Jan 2024 18:04:51 -0800 Subject: [PATCH 4/4] Address comments --- src/library_exceptions.js | 11 +++++++---- test/common.py | 18 ++++++++++++++++++ test/test_browser.py | 19 ------------------- test/test_core.py | 21 ++++++++++++++++++++- 4 files changed, 45 insertions(+), 24 deletions(-) diff --git a/src/library_exceptions.js b/src/library_exceptions.js index fa5b8938e7201..b55d72094d96e 100644 --- a/src/library_exceptions.js +++ b/src/library_exceptions.js @@ -342,10 +342,13 @@ var LibraryExceptions = { // change, and it is implemented as writable in Chrome and Node. Will check // the 'writable' condition for now and later resolve this by either // deleting this altogether or removing this condition check. - if (e.stack && Object.getOwnPropertyDescriptor(e, 'stack').writable) { - var arr = e.stack.split('\n'); - arr.splice(1,1); - e.stack = arr.join('\n'); + if (e.stack) { + var pd = Object.getOwnPropertyDescriptor(e, 'stack'); + if (pd && pd.writable) { + var arr = e.stack.split('\n'); + arr.splice(1,1); + e.stack = arr.join('\n'); + } } throw e; }, diff --git a/test/common.py b/test/common.py index 036c89886e5c4..511685934b89f 100644 --- a/test/common.py +++ b/test/common.py @@ -301,6 +301,16 @@ def decorated(self, *args, **kwargs): return decorated +def requires_spidermonkey(func): + assert callable(func) + + def decorated(self, *args, **kwargs): + self.require_spidermonkey() + return func(self, *args, **kwargs) + + return decorated + + def node_pthreads(f): @wraps(f) def decorated(self, *args, **kwargs): @@ -698,6 +708,14 @@ def require_node_canary(self): else: self.fail('node canary required to run this test. Use EMTEST_SKIP_NODE_CANARY to skip') + def require_spidermonkey(self): + if not config.SPIDERMONKEY_ENGINE or config.SPIDERMONKEY_ENGINE not in config.JS_ENGINES: + if 'EMTEST_SKIP_SPIDERMONKEY' in os.environ: + self.skipTest('test requires spidermonkey and EMTEST_SKIP_SPIDERMONKEY is set') + else: + self.fail('spidermonkey required to run this test. Use EMTEST_SKIP_SPIDERMONKEY to skip') + self.require_engine(config.SPIDERMONKEY_ENGINE) + def require_engine(self, engine): logger.debug(f'require_engine: {engine}') if self.required_engine and self.required_engine != engine: diff --git a/test/test_browser.py b/test/test_browser.py index 22fb435ceb915..f5b36a5ce0255 100644 --- a/test/test_browser.py +++ b/test/test_browser.py @@ -5825,25 +5825,6 @@ def test_webpack(self): shutil.copyfile('webpack/src/hello.wasm', 'webpack/dist/hello.wasm') self.run_browser('webpack/dist/index.html', '/report_result?exit:0') - def test_exceptions_stack_trace_and_message_wasm_eh(self): - # Currently writing to WebAssembly.Exception.stack property is not allowed - # in Firefox, but we need to ensure generating stack traces runs on FireFox - # without erroring out, even though stack trace simplification will not be - # done. See https://github.com/emscripten-core/emscripten/pull/21050 - if not is_firefox(): - self.skipTest('https://github.com/emscripten-core/emscripten/pull/21050') - - create_file('main.cpp', r''' - #include - int main() { - throw std::runtime_error("my message"); - return 0; - } - ''') - self.emcc_args = ['-g', '-fwasm-exceptions'] - self.set_setting('EXCEPTION_STACK_TRACES', 1) - self.btest_exit('main.c') - class emrun(RunnerCore): def test_emrun_info(self): diff --git a/test/test_core.py b/test/test_core.py index b92cc592834a9..cf14d73337383 100644 --- a/test/test_core.py +++ b/test/test_core.py @@ -27,7 +27,7 @@ from common import RunnerCore, path_from_root, requires_native_clang, test_file, create_file from common import skip_if, needs_dylink, no_windows, no_mac, is_slow_test, parameterized from common import env_modify, with_env_modify, disabled, flaky, node_pthreads, also_with_wasm_bigint -from common import read_file, read_binary, requires_v8, requires_node, requires_node_canary +from common import read_file, read_binary, requires_v8, requires_node, requires_node_canary, requires_spidermonkey from common import compiler_for, crossplatform, no_4gb, no_2gb from common import with_both_sjlj, also_with_standalone_wasm, can_do_standalone, no_wasm64 from common import NON_ZERO, WEBIDL_BINDER, EMBUILDER, PYTHON @@ -9641,6 +9641,25 @@ def test_wasm_worker_wait_async(self): self.prep_wasm_worker_in_node() self.do_runf('atomic/test_wait_async.c', emcc_args=['-sWASM_WORKERS']) + @requires_spidermonkey + def test_exceptions_stack_trace_and_message_wasm_eh(self): + # Currently writing to WebAssembly.Exception.stack property is not allowed + # in Firefox, but we need to ensure generating stack traces runs on FireFox + # without erroring out, even though stack trace simplification will not be + # done. See https://github.com/emscripten-core/emscripten/pull/21050 + src = r''' + #include + int main() { + throw std::runtime_error("my message"); + return 0; + } + ''' + self.emcc_args = ['--profiling-funcs', '-gsource-map'] + self.emcc_args = ['-g', '-fwasm-exceptions'] + self.set_setting('EXCEPTION_STACK_TRACES', 1) + err = self.do_run(src, assert_returncode=NON_ZERO) + self.assertNotContained('setting getter-only property "stack"', err) + # Generate tests for everything def make_run(name, emcc_args, settings=None, env=None,