Skip to content

Commit 6dc6e87

Browse files
sbc100sbc100
committed
Fix race condition in checkMailbox
The `checkMailbox` callback can occur after the thread has terminated. In this case calling into native code can trigger the `makeAbortWrapper` wrapper that is put around each native function which then results in a "program has already aborted!" error being thrown. Once solution to this is to make sure that the function which are called do not have `makeAbortWrapper` applied to them. This was the technique I used in #18754, but the list of functions became stale when emscripten_proxy_execute_task_queue was removed in #18852. A better solution is to wrap to whole function in callUserCallback, which takes case of checking if the runtime is alive before calling into native code. Fixes: #20067
1 parent fe2e532 commit 6dc6e87

16 files changed

+71
-58
lines changed

‎src/lib/libcore.js

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2113,8 +2113,19 @@ addToLibrary({
21132113
},
21142114

21152115
#else // MINIMAL_RUNTIME
2116-
// MINIMAL_RUNTIME doesn't support the runtimeKeepalive stuff
2117-
$callUserCallback: (func) => func(),
2116+
$callUserCallback: (func) => {
2117+
// MINIMAL_RUNTIME doesn't support the runtimeKeepalive stuff, but under
2118+
// some circumstances it supportes `runtimeExited`
2119+
#if EXIT_RUNTIME
2120+
if (runtimeExited) {
2121+
#if ASSERTIONS
2122+
err('user callback triggered after runtime exited or application aborted. Ignoring.');
2123+
#endif
2124+
return;
2125+
}
2126+
#endif
2127+
func();
2128+
},
21182129
#endif // MINIMAL_RUNTIME
21192130

21202131
$asmjsMangle: (x) => {

‎src/lib/libpthread.js

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1215,19 +1215,23 @@ var LibraryPThread = {
12151215
'pthread_self',
12161216
'_emscripten_check_mailbox',
12171217
'_emscripten_thread_mailbox_await'],
1218-
$checkMailbox: () => {
1218+
$checkMailbox: () => callUserCallback(() => {
12191219
// Only check the mailbox if we have a live pthread runtime. We implement
12201220
// pthread_self to return 0 if there is no live runtime.
1221+
//
1222+
// TODO(sbc): Is this check still needed? `callUserCallback` is supposed to
1223+
// ensure the runtime is alive, and if `_pthread_self` is NULL then the
1224+
// runtime certainly is *not* alive, so this should be a redundant check.
12211225
var pthread_ptr = _pthread_self();
12221226
if (pthread_ptr) {
12231227
// If we are using Atomics.waitAsync as our notification mechanism, wait
12241228
// for a notification before processing the mailbox to avoid missing any
12251229
// work that could otherwise arrive after we've finished processing the
12261230
// mailbox and before we're ready for the next notification.
12271231
__emscripten_thread_mailbox_await(pthread_ptr);
1228-
callUserCallback(__emscripten_check_mailbox);
1232+
__emscripten_check_mailbox();
12291233
}
1230-
},
1234+
}),
12311235

12321236
_emscripten_thread_mailbox_await__deps: ['$checkMailbox'],
12331237
_emscripten_thread_mailbox_await: (pthread_ptr) => {

‎test/code_size/test_codesize_cxx_except.json

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
{
22
"a.out.js": 23414,
33
"a.out.js.gz": 9144,
4-
"a.out.nodebug.wasm": 171270,
5-
"a.out.nodebug.wasm.gz": 57331,
6-
"total": 194684,
7-
"total_gz": 66475,
4+
"a.out.nodebug.wasm": 171285,
5+
"a.out.nodebug.wasm.gz": 57342,
6+
"total": 194699,
7+
"total_gz": 66486,
88
"sent": [
99
"__cxa_begin_catch",
1010
"__cxa_end_catch",

‎test/code_size/test_codesize_cxx_except_wasm.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
{
22
"a.out.js": 19642,
33
"a.out.js.gz": 8109,
4-
"a.out.nodebug.wasm": 144629,
4+
"a.out.nodebug.wasm": 144643,
55
"a.out.nodebug.wasm.gz": 54892,
6-
"total": 164271,
6+
"total": 164285,
77
"total_gz": 63001,
88
"sent": [
99
"_abort_js",

‎test/code_size/test_codesize_cxx_except_wasm_legacy.json

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
{
22
"a.out.js": 19642,
33
"a.out.js.gz": 8109,
4-
"a.out.nodebug.wasm": 142218,
5-
"a.out.nodebug.wasm.gz": 54353,
6-
"total": 161860,
7-
"total_gz": 62462,
4+
"a.out.nodebug.wasm": 142232,
5+
"a.out.nodebug.wasm.gz": 54356,
6+
"total": 161874,
7+
"total_gz": 62465,
88
"sent": [
99
"_abort_js",
1010
"_tzset_js",

‎test/code_size/test_codesize_cxx_mangle.json

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
{
22
"a.out.js": 23465,
33
"a.out.js.gz": 9164,
4-
"a.out.nodebug.wasm": 235311,
5-
"a.out.nodebug.wasm.gz": 78929,
6-
"total": 258776,
7-
"total_gz": 88093,
4+
"a.out.nodebug.wasm": 235325,
5+
"a.out.nodebug.wasm.gz": 78943,
6+
"total": 258790,
7+
"total_gz": 88107,
88
"sent": [
99
"__cxa_begin_catch",
1010
"__cxa_end_catch",

‎test/code_size/test_codesize_hello_O0.json

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
{
22
"a.out.js": 22497,
33
"a.out.js.gz": 8329,
4-
"a.out.nodebug.wasm": 15127,
5-
"a.out.nodebug.wasm.gz": 7448,
6-
"total": 37624,
7-
"total_gz": 15777,
4+
"a.out.nodebug.wasm": 15143,
5+
"a.out.nodebug.wasm.gz": 7452,
6+
"total": 37640,
7+
"total_gz": 15781,
88
"sent": [
99
"fd_write"
1010
],

‎test/code_size/test_codesize_hello_dylink_all.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"a.out.js": 246640,
3-
"a.out.nodebug.wasm": 597702,
4-
"total": 844342,
3+
"a.out.nodebug.wasm": 597720,
4+
"total": 844360,
55
"sent": [
66
"IMG_Init",
77
"IMG_Load",

‎test/code_size/test_codesize_minimal_pthreads.json

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
{
2-
"a.out.js": 7649,
3-
"a.out.js.gz": 3768,
2+
"a.out.js": 7655,
3+
"a.out.js.gz": 3773,
44
"a.out.nodebug.wasm": 19588,
55
"a.out.nodebug.wasm.gz": 9025,
6-
"total": 27237,
7-
"total_gz": 12793,
6+
"total": 27243,
7+
"total_gz": 12798,
88
"sent": [
99
"a (memory)",
1010
"b (emscripten_get_now)",

‎test/code_size/test_codesize_minimal_pthreads_memgrowth.json

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
{
2-
"a.out.js": 8076,
3-
"a.out.js.gz": 3974,
2+
"a.out.js": 8082,
3+
"a.out.js.gz": 3978,
44
"a.out.nodebug.wasm": 19589,
55
"a.out.nodebug.wasm.gz": 9025,
6-
"total": 27665,
7-
"total_gz": 12999,
6+
"total": 27671,
7+
"total_gz": 13003,
88
"sent": [
99
"a (memory)",
1010
"b (emscripten_get_now)",

0 commit comments

Comments
 (0)