From d0f104dec0e22b74180dde1f7d471696a347b2e9 Mon Sep 17 00:00:00 2001 From: Sami Vaarala Date: Mon, 24 Jun 2019 22:06:30 +0300 Subject: [PATCH 1/3] Add bug testcases for GH-2030 --- ...bug-finalizer-coroutine-resume-gh2030-1.js | 17 ++++++++++++++ ...bug-finalizer-coroutine-resume-gh2030-2.js | 22 +++++++++++++++++++ 2 files changed, 39 insertions(+) create mode 100644 tests/ecmascript/test-bug-finalizer-coroutine-resume-gh2030-1.js create mode 100644 tests/ecmascript/test-bug-finalizer-coroutine-resume-gh2030-2.js diff --git a/tests/ecmascript/test-bug-finalizer-coroutine-resume-gh2030-1.js b/tests/ecmascript/test-bug-finalizer-coroutine-resume-gh2030-1.js new file mode 100644 index 00000000..6db9a741 --- /dev/null +++ b/tests/ecmascript/test-bug-finalizer-coroutine-resume-gh2030-1.js @@ -0,0 +1,17 @@ +/* + * https://github.com/svaarala/duktape/issues/2030 + */ + +/*=== +done +===*/ + +Duktape.fin( + Proxy, function( ) { + function f( ) { + performance ('g called', isFinite(Error), TypeError(g)); + } + Duktape.Thread.resume(new Duktape.Thread(f)); +}); + +print('done'); diff --git a/tests/ecmascript/test-bug-finalizer-coroutine-resume-gh2030-2.js b/tests/ecmascript/test-bug-finalizer-coroutine-resume-gh2030-2.js new file mode 100644 index 00000000..8be09613 --- /dev/null +++ b/tests/ecmascript/test-bug-finalizer-coroutine-resume-gh2030-2.js @@ -0,0 +1,22 @@ +/* + * https://github.com/svaarala/duktape/issues/2030 + * + * Testcase adapted a bit in this version. + */ + +/*=== +done +finalizer called +f called +===*/ + +Duktape.fin(Proxy, function( ) { + function f() { + print('f called'); + performance(); // Throws. + } + print('finalizer called'); + Duktape.Thread.resume(new Duktape.Thread(f)); +}); + +print('done'); From 4c516cbfeee1b845f14af8ee5b80d0198ab532e4 Mon Sep 17 00:00:00 2001 From: Sami Vaarala Date: Sun, 9 Jun 2019 16:22:16 +0300 Subject: [PATCH 2/3] Don't assert thr == heap_thread in finalizer run If we're destroying the heap, ms_running == 1 and finalizers are executed. If a finalizer resumes a coroutine, the assert doesn't hold (GH-2030). --- src-input/duk_bi_thread.c | 12 ++++++++++++ src-input/duk_heap_alloc.c | 10 ++++++++-- src-input/duk_heap_refcount.c | 10 ++++++---- src-input/duk_js_call.c | 2 ++ 4 files changed, 28 insertions(+), 6 deletions(-) diff --git a/src-input/duk_bi_thread.c b/src-input/duk_bi_thread.c index 1761e0a8..69f51d83 100644 --- a/src-input/duk_bi_thread.c +++ b/src-input/duk_bi_thread.c @@ -135,6 +135,18 @@ DUK_INTERNAL duk_ret_t duk_bi_thread_resume(duk_hthread *ctx) { duk_pop(thr); } +#if 0 + /* This check would prevent a heap destruction time finalizer from + * launching a coroutine, which would ensure that during finalization + * 'thr' would always equal heap_thread. Normal runtime finalizers + * run with ms_running == 0, i.e. outside mark-and-sweep. See GH-2030. + */ + if (thr->heap->ms_running) { + DUK_D(DUK_DPRINT("refuse Duktape.Thread.resume() when ms_running == 1")); + goto state_error; + } +#endif + /* * The error object has been augmented with a traceback and other * info from its creation point -- usually another thread. The diff --git a/src-input/duk_heap_alloc.c b/src-input/duk_heap_alloc.c index 5b2c3f66..3c4cffa0 100644 --- a/src-input/duk_heap_alloc.c +++ b/src-input/duk_heap_alloc.c @@ -257,8 +257,14 @@ DUK_LOCAL void duk__free_run_finalizers(duk_heap *heap) { /* Prevent finalize_list processing and mark-and-sweep entirely. * Setting ms_running = 1 also prevents refzero handling from moving - * objects away from the heap_allocated list (the flag name is a bit - * misleading here). + * objects away from the heap_allocated list. The flag name is a bit + * misleading here. + * + * We're running finalizers with ms_running == 1 here, something that + * doesn't happen with normal mark-and-sweep. This is unfortunate, as + * it caused GH-2030 and makes runtime assertions weaker. It would be + * better to use a different flag or mark heap destruction state + * specially so the asserts could be stronger. */ DUK_ASSERT(heap->pf_prevent_count == 0); heap->pf_prevent_count = 1; diff --git a/src-input/duk_heap_refcount.c b/src-input/duk_heap_refcount.c index fa6e871a..36afe7b6 100644 --- a/src-input/duk_heap_refcount.c +++ b/src-input/duk_heap_refcount.c @@ -521,11 +521,12 @@ DUK_LOCAL DUK_INLINE void duk__refcount_refzero_hbuffer(duk_heap *heap, duk_hbuf DUK_ASSERT(thr->heap != NULL); \ /* When mark-and-sweep runs, heap_thread must exist. */ \ DUK_ASSERT(thr->heap->ms_running == 0 || thr->heap->heap_thread != NULL); \ - /* When mark-and-sweep runs, the 'thr' argument always matches heap_thread. \ - * This could be used to e.g. suppress check against 'thr' directly (and \ - * knowing it would be heap_thread); not really used now. \ + /* In normal operation finalizers are executed with ms_running == 0. \ + * However, in heap destruction ms_running is set to 1 and remaining \ + * finalizers then executed. A finalizer may resume a coroutine, in \ + * which case we'd have ms_running == 1 and thr != heap_thread (GH-2030). \ */ \ - DUK_ASSERT(thr->heap->ms_running == 0 || thr == thr->heap->heap_thread); \ + /* DUK_ASSERT(thr->heap->ms_running == 0 || thr == thr->heap->heap_thread); */ \ /* We may be called when the heap is initializing and we process \ * refzeros normally, but mark-and-sweep and finalizers are prevented \ * if that's the case. \ @@ -613,6 +614,7 @@ DUK_LOCAL DUK__RZ_INLINE void duk__heaphdr_refzero_helper(duk_hthread *thr, duk_ heap = thr->heap; htype = (duk_small_uint_t) DUK_HEAPHDR_GET_TYPE(h); + DUK_DDD(DUK_DDDPRINT("ms_running=%ld, heap_thread=%p", (long) thr->heap->ms_running, thr->heap->heap_thread)); DUK__RZ_SUPPRESS_CHECK(); switch (htype) { diff --git a/src-input/duk_js_call.c b/src-input/duk_js_call.c index 747319c3..b1789993 100644 --- a/src-input/duk_js_call.c +++ b/src-input/duk_js_call.c @@ -1202,6 +1202,8 @@ DUK_LOCAL duk_hobject *duk__resolve_target_func_and_this_binding(duk_hthread *th tv_func = DUK_GET_TVAL_POSIDX(thr, idx_func); DUK_ASSERT(tv_func != NULL); + DUK_DD(DUK_DDPRINT("target func: %!iT", tv_func)); + if (DUK_TVAL_IS_OBJECT(tv_func)) { func = DUK_TVAL_GET_OBJECT(tv_func); From 97f8e1597a7778df6e6d592352e0fdf4979ef4eb Mon Sep 17 00:00:00 2001 From: Sami Vaarala Date: Mon, 24 Jun 2019 23:23:28 +0300 Subject: [PATCH 3/3] RELEASES: finalizer coroutine resume (GH-2030) --- RELEASES.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/RELEASES.rst b/RELEASES.rst index 52f4ad9a..e4c92e18 100644 --- a/RELEASES.rst +++ b/RELEASES.rst @@ -3479,6 +3479,11 @@ Planned function chains with a Proxy object (rather than a plain function) as the final non-bound function (GH-2049, GH-2103) +* Fix incorrect assertion with no underlying bug for "thr == heap_thread" + during heap destruction finalizer runs; the assert is untrue when a + finalizer (executed during heap destruction) resumes a coroutine (GH-2030, + GH-2132) + * Fix compile error for extras/eventloop due to missing a header file (c_eventloop.h) in the dist package (GH-2090)