Delay detached_cb to between messages

Without this it's possible for the following to happen:

- The debug transport gets broken by e.g. a syntax error during message
  handling.

- The detached_cb is called when marking transport bad.  The detached
  callback immediately reattaches.

- The debug command processing continues, potentially reading from and
  writing to the *new connection* which causes the connection to go
  out of sync.

Delaying the detached_cb call to the message loop, right after one message
has been fully processed, avoids this issue and keeps the connections
cleanly separated.

The solution is a bit awkward: duk_debug_do_detach() is split into two.
Outside callers (duk_debugger_detach(), duk_heap_free()) still have a single
function helper to hide the split.

src/duk_api_debug.c

@@ -53,6 +53,10 @@ DUK_EXTERNAL void duk_debugger_attach(duk_context *ctx,
 	const char *str;
 	duk_size_t len;
 
+	/* XXX: should there be an error or an automatic detach if
+	 * already attached?
+	 */
+
 	DUK_ASSERT_CTX_VALID(ctx);
 	DUK_ASSERT(read_cb != NULL);
 	DUK_ASSERT(write_cb != NULL);
@@ -104,7 +108,7 @@ DUK_EXTERNAL void duk_debugger_detach(duk_context *ctx) {
 	DUK_ASSERT(thr != NULL);
 	DUK_ASSERT(thr->heap != NULL);
 
-	/* Can be called muliple times with no harm. */
+	/* Can be called multiple times with no harm. */
 	duk_debug_do_detach(thr->heap);
 }
 

src/duk_debugger.c

@@ -25,25 +25,27 @@ typedef union {
 
 #define DUK__SET_CONN_BROKEN(thr) do { \
 		/* For now shared handler is fine. */ \
-		duk_debug_do_detach((thr)->heap); \
+		duk__debug_do_detach1((thr)->heap); \
 	} while (0)
 
-DUK_INTERNAL void duk_debug_do_detach(duk_heap *heap) {
-	duk_debug_detached_function detach_cb;
-	void *detach_udata;
-
-	/* Can be called multiple times with no harm. */
+DUK_LOCAL void duk__debug_do_detach1(duk_heap *heap) {
+	/* Can be called multiple times with no harm.  Mark the transport
+	 * bad (dbg_read_cb == NULL) and clear state except for the detached
+	 * callback and the udata field.  The detached callback is delayed
+	 * to the message loop so that it can be called between messages;
+	 * this avoids corner cases related to immediate debugger reattach
+	 * inside the detached callback.
+	 */
 
-	detach_cb = heap->dbg_detached_cb;
-	detach_udata = heap->dbg_udata;
+	DUK_D(DUK_DPRINT("debugger transport detaching, marking transport broken"));
 
 	heap->dbg_read_cb = NULL;
 	heap->dbg_write_cb = NULL;
 	heap->dbg_peek_cb = NULL;
 	heap->dbg_read_flush_cb = NULL;
 	heap->dbg_write_flush_cb = NULL;
-	heap->dbg_detached_cb = NULL;
-	heap->dbg_udata = NULL;
+	/* heap->dbg_detached_cb: keep */
+	/* heap->dbg_udata: keep */
 	heap->dbg_processing = 0;
 	heap->dbg_paused = 0;
 	heap->dbg_state_dirty = 0;
@@ -62,16 +64,34 @@ DUK_INTERNAL void duk_debug_do_detach(duk_heap *heap) {
 	 * XXX: clear breakpoint on either attach or detach?
 	 */
 	heap->dbg_breakpoints_active[0] = (duk_breakpoint *) NULL;
+}
 
-	/* Call the detached callback last, so that if it immediately
-	 * reattaches, the debugger state will be correct.  The udata
-	 * pointer used must be prior to NULLing heap->dbg_udata.
-	 */
-	if (detach_cb) {
-		detach_cb(detach_udata);
+DUK_LOCAL void duk__debug_do_detach2(duk_heap *heap) {
+	duk_debug_detached_function detached_cb;
+	void *detached_udata;
+
+	/* Safe to call multiple times. */
+
+	detached_cb = heap->dbg_detached_cb;
+	detached_udata = heap->dbg_udata;
+	heap->dbg_detached_cb = NULL;
+	heap->dbg_udata = NULL;
+
+	if (detached_cb) {
+		/* Careful here: state must be wiped before the call
+		 * so that we can cleanly handle a re-attach from
+		 * inside the callback.
+		 */
+		DUK_D(DUK_DPRINT("detached during message loop, delayed call to detached_cb"));
+		detached_cb(detached_udata);
 	}
 }
 
+DUK_INTERNAL void duk_debug_do_detach(duk_heap *heap) {
+	duk__debug_do_detach1(heap);
+	duk__debug_do_detach2(heap);
+}
+
 /*
  *  Debug connection peek and flush primitives
  */
@@ -1427,7 +1447,7 @@ DUK_LOCAL void duk__debug_handle_detach(duk_hthread *thr, duk_heap *heap) {
 	duk_debug_write_eom(thr);
 
 	DUK_D(DUK_DPRINT("debug connection detached, mark broken"));
-	DUK__SET_CONN_BROKEN(thr);  /* calls detached callback */
+	DUK__SET_CONN_BROKEN(thr);
 }
 
 #if defined(DUK_USE_DEBUGGER_DUMPHEAP)
@@ -1725,15 +1745,12 @@ DUK_LOCAL void duk__debug_process_message(duk_hthread *thr) {
 			break;
 		}
 		case DUK_DBG_CMD_DETACH: {
-			/* Important: when the detach handler returns,
-			 * user code may have re-attached so we must not
-			 * skip to EOM because it would happen in the new
-			 * connection.  Instead, return to message loop
-			 * and start parsing messages.
+			/* The actual detached_cb call is postponed to message loop so
+			 * we don't need any special precautions here (just skip to EOM
+			 * on the already closed connection).
 			 */
 			duk__debug_handle_detach(thr, heap);
-			DUK_D(DUK_DPRINT("returning to message loop after detach, don't skip to EOM"));
-			return;
+			break;
 		}
 #if defined(DUK_USE_DEBUGGER_DUMPHEAP)
 		case DUK_DBG_CMD_DUMPHEAP: {
@@ -1888,6 +1905,17 @@ DUK_INTERNAL duk_bool_t duk_debug_process_messages(duk_hthread *thr, duk_bool_t
 		}
 
 		duk__debug_process_message(thr);
+
+		if (thr->heap->dbg_read_cb == NULL) {
+			/* Became detached during message handling (perhaps because
+			 * of an error or by an explicit Detach).  Call detached
+			 * callback here, between messages, to avoid confusing the
+			 * broken connection and a possible replacement (which may
+			 * be provided by an instant reattach inside the detached
+			 * callback).
+			 */
+			duk__debug_do_detach2(thr->heap);
+		}
 		if (thr->heap->dbg_state_dirty) {
 			/* Executed something that may have affected status,
 			 * resend.