Merge pull request #2132 from svaarala/fix-finalizer-resume-gh2030

Fix incorrect assertion in heap destruction finalizer runs

RELEASES.rst

@@ -3486,6 +3486,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)
 

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

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;

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) {

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);
 

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');

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');