From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Akira Kyle Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] Interferences between xwidgets and async processes? Date: Thu, 12 Nov 2020 12:25:21 -0700 Message-ID: <86h7putm1q.fsf@akirakyle.com> References: <87mu06wyfb.fsf@gnus.org> <83tuuecrl5.fsf@gnu.org> <83v9etaz6z.fsf@gnu.org> <86sg9nsg5a.fsf@akirakyle.com> <87pn4mmybt.fsf@gnus.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="36032"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: mu4e 1.4.13; emacs 28.0.50 Cc: JIANG Shaojian , Eli Zaretskii , emacs-devel@gnu.org To: Lars Ingebrigtsen Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Thu Nov 12 20:26:56 2020 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kdIFB-0009B9-QG for ged-emacs-devel@m.gmane-mx.org; Thu, 12 Nov 2020 20:26:53 +0100 Original-Received: from localhost ([::1]:39476 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kdIFA-0002xm-P6 for ged-emacs-devel@m.gmane-mx.org; Thu, 12 Nov 2020 14:26:52 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:59092) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kdIDn-0002Jx-SB for emacs-devel@gnu.org; Thu, 12 Nov 2020 14:25:27 -0500 Original-Received: from mail-oi1-f196.google.com ([209.85.167.196]:41560) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kdIDl-0006R4-PY; Thu, 12 Nov 2020 14:25:27 -0500 Original-Received: by mail-oi1-f196.google.com with SMTP id m13so7646454oih.8; Thu, 12 Nov 2020 11:25:24 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:references:user-agent:from:to:cc:subject :in-reply-to:date:message-id:mime-version; bh=/xWGPTOGRNXiuOMGkpfEH+jMtWgCstV2dG7keZ9NDps=; b=Jj8IDJXIRi07KM3T9AeNd+dH6v3puuX3Gx/tozN7mWKl40pjJHZ9T7HXcQ+cTJkL1+ tWKMgoKA+uhPL8z2euzo8Gh416JR/EBOuon5jLHx/rQ3LCs7L1/QQIIEUyX786VeBhL7 bwUV9JCjmgKkLkqhJ5vA4+V1LNaFSY/6cdsQP1H7jEnaHEnZth+EungKckJ8tqKm7FVh I4FkE3c424J+kxgqFIAUY3vyTn3S5Av0lgorAjNZIR01kbOyruLHb6TGR23glrVwxfIi HWH2kxhqDkaA4/Zr2dKB9e5hI4FWKxH/faKz+sCdI5OeyxxBaJ82bNFurX45AWg4/Z74 kifA== X-Gm-Message-State: AOAM532ipM/2mgm9NZMXFwQWDUYfjBLw1A+8/2pq6Wj+C3Qmywubow7i 53/N4Zo57G4nADtybb51bwsQXInu+vIHA4um X-Google-Smtp-Source: ABdhPJze1iA8t5AN5bymfvSBX68FJdNB+hGE52ZPyTyZ0+q19QEmNBOYBpPRE9VKtN14X0/jnERTig== X-Received: by 2002:aca:1e09:: with SMTP id m9mr893007oic.60.1605209123604; Thu, 12 Nov 2020 11:25:23 -0800 (PST) Original-Received: from data (c-67-162-131-131.hsd1.co.comcast.net. [67.162.131.131]) by smtp.gmail.com with ESMTPSA id 132sm1368228oid.54.2020.11.12.11.25.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 12 Nov 2020 11:25:22 -0800 (PST) In-reply-to: <87pn4mmybt.fsf@gnus.org> Received-SPF: pass client-ip=209.85.167.196; envelope-from=aikokyle@gmail.com; helo=mail-oi1-f196.google.com X-detected-operating-system: by eggs.gnu.org: First seen = 2020/11/12 14:25:24 X-ACL-Warn: Detected OS = Linux 2.2.x-3.x [generic] [fuzzy] X-Spam_score_int: -13 X-Spam_score: -1.4 X-Spam_bar: - X-Spam_report: (-1.4 / 5.0 requ) BAYES_00=-1.9, FREEMAIL_FORGED_FROMDOMAIN=0.249, FREEMAIL_FROM=0.001, HEADER_FROM_DIFFERENT_DOMAINS=0.25, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=no autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:259100 Archived-At: --=-=-= Content-Type: text/plain; format=flowed On Mon, Nov 09, 2020 at 06:52 AM, Lars Ingebrigtsen wrote: >> * src/xwidget.c (make-xwidget): Save and restore Emacs SIGCHLD >> signal >> handler since glib doesn't (but should) do this. > > Thanks for the analysis and the patch for this problem. I've > now > applied it to Emacs 28, and it fixes the issue here. It seems I jumped the gun on this one. While it does fix the reported issue, it leaves WebKit process zombies. The issue is deeper than I previously thought and it seems that even disregarding the xwidget feature, the current interplay between Emacs' and glib's signal handling is fundamentally broken since glib commit 2e471acf. The relevant glib issue that resulted in that commit is here [1]. Essentially Emacs tries to capture glib's signal handler, g_unix_signal_handler, into lib_child_handler in process.c so that when Emacs installs its own signal handler, deliver_child_signal, overwriting glib's, after deliver_child_signal handles waiting on any process Emacs starts, it calls g_unix_signal_handler so glib has the chance to wait on any process it may be handling. The comment before defining lib_child_handler in process.c explains this intent. However since glib 2e471acf breaks all this since glib now may install its signal handler multiple times instead of just once and will reset the signal handler to SIG_DFL if it doesn't have any watchers on that signal. I'm somewhat surprised this hasn't caused other issues so far, but I guess Emacs doesn't use glib for any process handling and own its own gtk isn't spawning any of its own processes on behalf of Emacs. The attached patch fixes this but only in a temporary way. Any Emacs code which uses gtk or glib to spawn a process will suffer the same bugs as the reported xwidget bug unless they are also fixed in a similar way to the patch I previously sent, however even then my previous patch has potential race conditions as an Emacs managed subprocess may throw SIGCHLD between glib installing g_unix_signal_handler in ref_unix_signal_handler_unlocked and Emacs calling sigaction to restore deliver_child_signal which will leave the process zombied so Emacs will need to block SIGCHLD during this, something my previous patch failed to consider. I think that this should ultimately be fixed in glib by making it be a better signal handling citizen and do what Emacs does by saving any existing signal handler and call it from its own signal handler. I suppose I might be able to send them a patch if that seems like the best course of action here. [1] https://gitlab.gnome.org/GNOME/glib/-/issues/733 --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0001-Work-around-glib-messing-with-signal-handlers-more-t.patch >From 3d9406f7a320c241c1105ab8c8051afa682907b7 Mon Sep 17 00:00:00 2001 From: Akira Kyle Date: Thu, 12 Nov 2020 11:47:14 -0700 Subject: [PATCH] Work around glib messing with signal handlers more than it should * src/process.c (init_process_emacs): force glib's g_unix_signal handler into lib_child_handler where it should belong. --- src/process.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/src/process.c b/src/process.c index 5c3c82c97f..69d71bf11d 100644 --- a/src/process.c +++ b/src/process.c @@ -8221,13 +8221,30 @@ init_process_emacs (int sockfd) if (!will_dump_with_unexec_p ()) { #if defined HAVE_GLIB && !defined WINDOWSNT - /* Tickle glib's child-handling code. Ask glib to wait for Emacs itself; - this should always fail, but is enough to initialize glib's + /* Tickle glib's child-handling code. Ask glib to install a + watch source for Emacs itself which will initialize glib's private SIGCHLD handler, allowing catch_child_signal to copy - it into lib_child_handler. */ - g_source_unref (g_child_watch_source_new (getpid ())); -#endif + it into lib_child_handler. + + Unfortunatly in glib commit 2e471acf, the behavior changed to + always install a signal handler when g_child_watch_source_new + is called and not just the first time it's called. Glib also + now resets signal handlers to SIG_DFL when it no longer has a + watcher on that signal. This is a hackey work around to get + glib's g_unix_signal_handler into lib_child_handler. + */ + GSource *source = g_child_watch_source_new (getpid ()); + catch_child_signal (); + g_source_unref (source); + + eassert (lib_child_handler != dummy_handler); + signal_handler_t lib_child_handler_glib = lib_child_handler; catch_child_signal (); + eassert (lib_child_handler == dummy_handler); + lib_child_handler = lib_child_handler_glib; +#else + catch_child_signal (); +#endif } #ifdef HAVE_SETRLIMIT -- 2.29.0 --=-=-=--