bug#17510: 24.3.91; Problem with `emacs --daemon' in cygw32 build

bug#17510: 24.3.91; Problem with `emacs --daemon' in cygw32 build

[Ken Brown]

This bug was reported in the Cygwin mailing list:

   https://cygwin.com/ml/cygwin/2014-05/msg00303.html

In a Cygwin terminal, do the following, where "emacs" denotes the cygw32
build of emacs (--with-w32).

1. $ emacs --daemon -Q
2. $ emacsclient -c
3. `C-x 5 0' in the client window to exit the frame.
4. Repeat steps 2 and 3.
5. Attempt to carry out steps 2 and 3 a third time.  The message 
"Waiting for Emacs..." appears in the terminal, but no new frame opens.

This problem is specific to the cygw32 build; it does not happen with 
the X11 build of emacs on Cygwin.  It also doesn't happen if the server 
is started via `M-x server-start' in an existing emacs.

In GNU Emacs 24.3.91.1 (x86_64-unknown-cygwin) of 2014-05-16 on desktop-new
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
  `configure
 
--srcdir=/home/kbrown/src/cygemacs/emacs-24.3.91-1.x86_64/src/emacs-24.3.91
  --prefix=/usr --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin
  --libexecdir=/usr/libexec --datadir=/usr/share --localstatedir=/var
  --sysconfdir=/etc --libdir=/usr/lib --datarootdir=/usr/share
  --docdir=/usr/share/doc/emacs --htmldir=/usr/share/doc/emacs/html -C
  --with-w32 'CFLAGS=-ggdb -O2 -pipe -Wimplicit-function-declaration -O0
 
-fdebug-prefix-map=/home/kbrown/src/cygemacs/emacs-24.3.91-1.x86_64/build=/usr/src/debug/emacs-24.3.91-1
 
-fdebug-prefix-map=/home/kbrown/src/cygemacs/emacs-24.3.91-1.x86_64/src/emacs-24.3.91=/usr/src/debug/emacs-24.3.91-1'
  CPPFLAGS= LDFLAGS=-Wl,--stack,0x400000'

Important settings:
   value of $LANG: en_US.UTF-8
   locale-coding-system: utf-8-unix

bug#17510: 24.3.91; Problem with `emacs --daemon' in cygw32 build

[Ken Brown]

On 5/16/2014 1:50 PM, Ken Brown wrote:
> This bug was reported in the Cygwin mailing list:
>
>    https://cygwin.com/ml/cygwin/2014-05/msg00303.html
>
> In a Cygwin terminal, do the following, where "emacs" denotes the cygw32
> build of emacs (--with-w32).
>
> 1. $ emacs --daemon -Q
> 2. $ emacsclient -c
> 3. `C-x 5 0' in the client window to exit the frame.
> 4. Repeat steps 2 and 3.
> 5. Attempt to carry out steps 2 and 3 a third time.  The message
> "Waiting for Emacs..." appears in the terminal, but no new frame opens.
>
> This problem is specific to the cygw32 build; it does not happen with
> the X11 build of emacs on Cygwin.  It also doesn't happen if the server
> is started via `M-x server-start' in an existing emacs.

And it doesn't happen in emacs-24.3.  As soon as I have a chance, I'll 
do a bisection to see when it started.

Ken

bug#17510: 24.3.91; Problem with `emacs --daemon' in cygw32 build

[Ken Brown]

[-- Attachment #1: Type: text/plain, Size: 1759 bytes --]

On 5/16/2014 4:06 PM, Ken Brown wrote:
> On 5/16/2014 1:50 PM, Ken Brown wrote:
>> This bug was reported in the Cygwin mailing list:
>>
>>    https://cygwin.com/ml/cygwin/2014-05/msg00303.html
>>
>> In a Cygwin terminal, do the following, where "emacs" denotes the cygw32
>> build of emacs (--with-w32).
>>
>> 1. $ emacs --daemon -Q
>> 2. $ emacsclient -c
>> 3. `C-x 5 0' in the client window to exit the frame.
>> 4. Repeat steps 2 and 3.
>> 5. Attempt to carry out steps 2 and 3 a third time.  The message
>> "Waiting for Emacs..." appears in the terminal, but no new frame opens.
>>
>> This problem is specific to the cygw32 build; it does not happen with
>> the X11 build of emacs on Cygwin.  It also doesn't happen if the server
>> is started via `M-x server-start' in an existing emacs.
>
> And it doesn't happen in emacs-24.3.  As soon as I have a chance, I'll
> do a bisection to see when it started.

Here's the culprit:

revno: 114710
committer: Dmitry Antipov <dmantipov@yandex.ru>
branch nick: trunk
timestamp: Fri 2013-10-18 16:57:44 +0400
message:
   Remove port-specific display name lists to avoid extra
   complexity and data duplication with display info lists.
[...]
   * w32term.h (w32_display_name_list): Remove declaration.
   * w32term.c (w32_display_name_list): Remove.
   (w32_initialize_display_info, x_delete_display, syms_of_w32term):
   Adjust users.
   * w32fns.c (x_display_info_for_name, Fx_display_list):
   Likewise.  Use x_display_list where appropriate.
[...]

The attached patch applied to the emacs-24 branch reverts these changes 
and fixes the problem.  This is presumably no the "right" fix.  Note, 
however, that Dimity's commit introduced a "FIXME" into x_delete_display 
in w32term.c.  Maybe that's the issue.

Ken

[-- Attachment #2: display_name_list.patch --]
[-- Type: text/plain, Size: 4749 bytes --]

=== modified file 'src/w32fns.c'
--- src/w32fns.c	2014-05-06 16:00:30 +0000
+++ src/w32fns.c	2014-05-17 22:15:56 +0000
@@ -5184,13 +5184,20 @@
 struct w32_display_info *
 x_display_info_for_name (Lisp_Object name)
 {
+  Lisp_Object names;
   struct w32_display_info *dpyinfo;
 
   CHECK_STRING (name);
 
-  for (dpyinfo = &one_w32_display_info; dpyinfo; dpyinfo = dpyinfo->next)
-    if (!NILP (Fstring_equal (XCAR (dpyinfo->name_list_element), name)))
-      return dpyinfo;
+  for (dpyinfo = &one_w32_display_info, names = w32_display_name_list;
+       dpyinfo && !NILP (w32_display_name_list);
+       dpyinfo = dpyinfo->next, names = XCDR (names))
+    {
+      Lisp_Object tem;
+      tem = Fstring_equal (XCAR (XCAR (names)), name);
+      if (!NILP (tem))
+	return dpyinfo;
+    }
 
   /* Use this general default value to start with.  */
   Vx_resource_name = Vinvocation_name;
@@ -5325,11 +5332,11 @@
        doc: /* Return the list of display names that Emacs has connections to.  */)
   (void)
 {
-  Lisp_Object result = Qnil;
-  struct w32_display_info *wdi;
+  Lisp_Object tail, result;
 
-  for (wdi = x_display_list; wdi; wdi = wdi->next)
-    result = Fcons (XCAR (wdi->name_list_element), result);
+  result = Qnil;
+  for (tail = w32_display_name_list; CONSP (tail); tail = XCDR (tail))
+    result = Fcons (XCAR (XCAR (tail)), result);
 
   return result;
 }

=== modified file 'src/w32term.c'
--- src/w32term.c	2014-04-16 14:00:39 +0000
+++ src/w32term.c	2014-05-17 22:15:56 +0000
@@ -100,6 +100,13 @@
 struct w32_display_info one_w32_display_info;
 struct w32_display_info *x_display_list;
 
+/* This is a list of cons cells, each of the form (NAME . FONT-LIST-CACHE),
+   one for each element of w32_display_list and in the same order.
+   NAME is the name of the frame.
+   FONT-LIST-CACHE records previous values returned by x-list-fonts.  */
+Lisp_Object w32_display_name_list;
+
+
 #if _WIN32_WINNT < 0x0500 && !defined(_W64)
 /* Pre Windows 2000, this was not available, but define it here so
    that Emacs compiled on such a platform will run on newer versions.
@@ -6161,7 +6168,11 @@
 
   memset (dpyinfo, 0, sizeof (*dpyinfo));
 
-  dpyinfo->name_list_element = Fcons (display_name, Qnil);
+  /* Put it on w32_display_name_list.  */
+  w32_display_name_list = Fcons (Fcons (display_name, Qnil),
+                                 w32_display_name_list);
+  dpyinfo->name_list_element = XCAR (w32_display_name_list);
+
   dpyinfo->w32_id_name = xmalloc (SCHARS (Vinvocation_name)
 				  + SCHARS (Vsystem_name) + 2);
   sprintf (dpyinfo->w32_id_name, "%s@%s",
@@ -6410,7 +6421,27 @@
 void
 x_delete_display (struct w32_display_info *dpyinfo)
 {
-  /* FIXME: the only display info apparently can't be deleted.  */
+  /* Discard this display from w32_display_name_list and w32_display_list.
+     We can't use Fdelq because that can quit.  */
+  if (! NILP (w32_display_name_list)
+      && EQ (XCAR (w32_display_name_list), dpyinfo->name_list_element))
+    w32_display_name_list = XCDR (w32_display_name_list);
+  else
+    {
+      Lisp_Object tail;
+
+      tail = w32_display_name_list;
+      while (CONSP (tail) && CONSP (XCDR (tail)))
+	{
+	  if (EQ (XCAR (XCDR (tail)), dpyinfo->name_list_element))
+	    {
+	      XSETCDR (tail, XCDR (XCDR (tail)));
+	      break;
+	    }
+	  tail = XCDR (tail);
+	}
+    }
+
   /* free palette table */
   {
     struct w32_palette_entry * plist;
@@ -6547,6 +6578,9 @@
 void
 syms_of_w32term (void)
 {
+  staticpro (&w32_display_name_list);
+  w32_display_name_list = Qnil;
+
   DEFSYM (Qvendor_specific_keysyms, "vendor-specific-keysyms");
 
   DEFSYM (Qadded, "added");

=== modified file 'src/w32term.h'
--- src/w32term.h	2014-02-04 16:13:51 +0000
+++ src/w32term.h	2014-05-17 22:15:56 +0000
@@ -71,7 +71,8 @@
   /* The generic display parameters corresponding to this w32 display.  */
   struct terminal *terminal;
 
-  /* This is a cons cell of the form (NAME . FONT-LIST-CACHE).  */
+  /* This is a cons cell of the form (NAME . FONT-LIST-CACHE).
+     The same cons cell also appears in x_display_name_list.  */
   Lisp_Object name_list_element;
 
   /* Number of frames that are on this display.  */
@@ -200,6 +201,12 @@
 extern struct w32_display_info *x_display_list;
 extern struct w32_display_info one_w32_display_info;
 
+/* This is a list of cons cells, each of the form (NAME . FONT-LIST-CACHE),
+   one for each element of w32_display_list and in the same order.
+   NAME is the name of the frame.
+   FONT-LIST-CACHE records previous values returned by x-list-fonts.  */
+extern Lisp_Object w32_display_name_list;
+
 extern struct frame *x_window_to_frame (struct w32_display_info *, HWND);
 
 struct w32_display_info *x_display_info_for_name (Lisp_Object);

bug#17510: 24.3.91; Problem with `emacs --daemon' in cygw32 build

[Eli Zaretskii]

> Date: Sat, 17 May 2014 19:39:33 -0400
> From: Ken Brown <kbrown@cornell.edu>
> 
> On 5/16/2014 4:06 PM, Ken Brown wrote:
> > On 5/16/2014 1:50 PM, Ken Brown wrote:
> >> This bug was reported in the Cygwin mailing list:
> >>
> >>    https://cygwin.com/ml/cygwin/2014-05/msg00303.html
> >>
> >> In a Cygwin terminal, do the following, where "emacs" denotes the cygw32
> >> build of emacs (--with-w32).
> >>
> >> 1. $ emacs --daemon -Q
> >> 2. $ emacsclient -c
> >> 3. `C-x 5 0' in the client window to exit the frame.
> >> 4. Repeat steps 2 and 3.
> >> 5. Attempt to carry out steps 2 and 3 a third time.  The message
> >> "Waiting for Emacs..." appears in the terminal, but no new frame opens.
> >>
> >> This problem is specific to the cygw32 build; it does not happen with
> >> the X11 build of emacs on Cygwin.  It also doesn't happen if the server
> >> is started via `M-x server-start' in an existing emacs.
> >
> > And it doesn't happen in emacs-24.3.  As soon as I have a chance, I'll
> > do a bisection to see when it started.
> 
> Here's the culprit:
> 
> revno: 114710
> committer: Dmitry Antipov <dmantipov@yandex.ru>
> branch nick: trunk
> timestamp: Fri 2013-10-18 16:57:44 +0400
> message:
>    Remove port-specific display name lists to avoid extra
>    complexity and data duplication with display info lists.
> [...]
>    * w32term.h (w32_display_name_list): Remove declaration.
>    * w32term.c (w32_display_name_list): Remove.
>    (w32_initialize_display_info, x_delete_display, syms_of_w32term):
>    Adjust users.
>    * w32fns.c (x_display_info_for_name, Fx_display_list):
>    Likewise.  Use x_display_list where appropriate.
> [...]
> 
> The attached patch applied to the emacs-24 branch reverts these changes 
> and fixes the problem.  This is presumably no the "right" fix.  Note, 
> however, that Dimity's commit introduced a "FIXME" into x_delete_display 
> in w32term.c.  Maybe that's the issue.

Thanks, but you need to be more selective: which one of these changes
is the root cause, and why?

In general, everything that is related to one_w32_display_info is
specific to the WINDOWSNT port, so perhaps the problem is that the
Cygwin-w32 build is incorrectly treated the same.  But where exactly?

Once you point out the parts that are causing this bug, they should be
modified for __CYGWIN__, but left alone for WINDOWSNT.

Thanks.

bug#17510: 24.3.91; Problem with `emacs --daemon' in cygw32 build

[Ken Brown]

On 5/18/2014 12:32 AM, Eli Zaretskii wrote:
> Thanks, but you need to be more selective: which one of these changes
> is the root cause, and why?

It's not easy to be selective; these aren't independent changes.  There 
used to be a `w32_display_name_list', which Dmitry removed.  Along with 
this change, he removed the code that used to be in x_delete_display (to 
delete a display from the list) and replaced it by a FIXME.

> In general, everything that is related to one_w32_display_info is
> specific to the WINDOWSNT port, so perhaps the problem is that the
> Cygwin-w32 build is incorrectly treated the same.  But where exactly?

Looking at the code, I don't see why this problem is specific to the 
Cygwin-w32 build.  Can you reproduce it in the Windows build?

I *think* what must be happening in the recipe that I gave for this bug 
is that every time a client frame is closed, x_delete_display is called. 
  Before Dmitry's change, this would actually delete something from a 
list.  Now it doesn't, and the server gets messed up and ultimately dies 
on the third attempt to create a client frame.

Unless there's an obvious fix for this, it seems to me that we're far 
enough into the pretest that we should just revert to the old code, at 
least for emacs-24.

Ken

bug#17510: 24.3.91; Problem with `emacs --daemon' in cygw32 build

[Eli Zaretskii]

> Date: Sun, 18 May 2014 10:30:28 -0400
> From: Ken Brown <kbrown@cornell.edu>
> CC: 17510@debbugs.gnu.org, dmantipov@yandex.ru
> 
> On 5/18/2014 12:32 AM, Eli Zaretskii wrote:
> > Thanks, but you need to be more selective: which one of these changes
> > is the root cause, and why?
> 
> It's not easy to be selective; these aren't independent changes.  There 
> used to be a `w32_display_name_list', which Dmitry removed.  Along with 
> this change, he removed the code that used to be in x_delete_display (to 
> delete a display from the list) and replaced it by a FIXME.

Perhaps that FIXME is not relevant to Cygwin, and the removed code
should be retained in the Cygwin build.

> > In general, everything that is related to one_w32_display_info is
> > specific to the WINDOWSNT port, so perhaps the problem is that the
> > Cygwin-w32 build is incorrectly treated the same.  But where exactly?
> 
> Looking at the code, I don't see why this problem is specific to the 
> Cygwin-w32 build.

If the Cygwin-w32 build wants more (or less) than one display_info
object, then that part _is_ specific to Cygwin, because the native
Windows build has only one such object that is never deleted.

> Can you reproduce it in the Windows build?

The native Windows build doesn't support --daemon, so no, I can't.

> I *think* what must be happening in the recipe that I gave for this bug 
> is that every time a client frame is closed, x_delete_display is called. 
>   Before Dmitry's change, this would actually delete something from a 
> list.  Now it doesn't, and the server gets messed up and ultimately dies 
> on the third attempt to create a client frame.

See above: try restoring that code for Cygwin only.

> Unless there's an obvious fix for this, it seems to me that we're far 
> enough into the pretest that we should just revert to the old code, at 
> least for emacs-24.

That would revert a useful cleanup, which I'm not sure is a good idea
at this point.

bug#17510: 24.3.91; Problem with `emacs --daemon' in cygw32 build

[Ken Brown]

On 5/18/2014 11:11 AM, Eli Zaretskii wrote:
>> Date: Sun, 18 May 2014 10:30:28 -0400
>> From: Ken Brown <kbrown@cornell.edu>
>> CC: 17510@debbugs.gnu.org, dmantipov@yandex.ru
>>
>> On 5/18/2014 12:32 AM, Eli Zaretskii wrote:
>>> Thanks, but you need to be more selective: which one of these changes
>>> is the root cause, and why?
>>
>> It's not easy to be selective; these aren't independent changes.  There
>> used to be a `w32_display_name_list', which Dmitry removed.  Along with
>> this change, he removed the code that used to be in x_delete_display (to
>> delete a display from the list) and replaced it by a FIXME.
> 
> Perhaps that FIXME is not relevant to Cygwin, and the removed code
> should be retained in the Cygwin build.
> 
>>> In general, everything that is related to one_w32_display_info is
>>> specific to the WINDOWSNT port, so perhaps the problem is that the
>>> Cygwin-w32 build is incorrectly treated the same.  But where exactly?
>>
>> Looking at the code, I don't see why this problem is specific to the
>> Cygwin-w32 build.
> 
> If the Cygwin-w32 build wants more (or less) than one display_info
> object, then that part _is_ specific to Cygwin, because the native
> Windows build has only one such object that is never deleted.
> 
>> Can you reproduce it in the Windows build?
> 
> The native Windows build doesn't support --daemon, so no, I can't.
> 
>> I *think* what must be happening in the recipe that I gave for this bug
>> is that every time a client frame is closed, x_delete_display is called.
>>    Before Dmitry's change, this would actually delete something from a
>> list.  Now it doesn't, and the server gets messed up and ultimately dies
>> on the third attempt to create a client frame.
> 
> See above: try restoring that code for Cygwin only.
> 
>> Unless there's an obvious fix for this, it seems to me that we're far
>> enough into the pretest that we should just revert to the old code, at
>> least for emacs-24.
> 
> That would revert a useful cleanup, which I'm not sure is a good idea
> at this point.

How does this look?

=== modified file 'src/w32fns.c'
--- src/w32fns.c        2014-05-06 16:00:30 +0000
+++ src/w32fns.c        2014-05-18 19:21:41 +0000
@@ -5188,9 +5188,22 @@

   CHECK_STRING (name);

+#ifdef CYGWIN /* See comment in w32term.c */
+  Lisp_Object names;
+  for (dpyinfo = &one_w32_display_info, names = cygw32_display_name_list;
+       dpyinfo && !NILP (cygw32_display_name_list);
+       dpyinfo = dpyinfo->next, names = XCDR (names))
+    {
+      Lisp_Object tem;
+      tem = Fstring_equal (XCAR (XCAR (names)), name);
+      if (!NILP (tem))
+       return dpyinfo;
+    }
+#else  /* !CYGWIN */
   for (dpyinfo = &one_w32_display_info; dpyinfo; dpyinfo = dpyinfo->next)
     if (!NILP (Fstring_equal (XCAR (dpyinfo->name_list_element), name)))
       return dpyinfo;
+#endif /* !CYGWIN */

   /* Use this general default value to start with.  */
   Vx_resource_name = Vinvocation_name;
@@ -5326,10 +5339,17 @@
   (void)
 {
   Lisp_Object result = Qnil;
+
+#ifdef CYGWIN
+  Lisp_Object tail;
+  for (tail = cygw32_display_name_list; CONSP (tail); tail = XCDR (tail))
+    result = Fcons (XCAR (XCAR (tail)), result);
+#else
   struct w32_display_info *wdi;

   for (wdi = x_display_list; wdi; wdi = wdi->next)
     result = Fcons (XCAR (wdi->name_list_element), result);
+#endif

   return result;
 }

=== modified file 'src/w32term.c'
--- src/w32term.c       2014-04-16 14:00:39 +0000
+++ src/w32term.c       2014-05-18 19:24:17 +0000
@@ -100,6 +100,17 @@
 struct w32_display_info one_w32_display_info;
 struct w32_display_info *x_display_list;

+/* In order to support "emacs --daemon" on the Cygwin-w32 build, we
+   maintain a list of display names.  (This compensates for the fact
+   that the one display can't be deleted.  See Bug#17510 and the FIXME
+   in x_delete_display below.)  This is a list of cons cells, each of
+   the form (NAME . FONT-LIST-CACHE).  NAME is the name of the frame.
+   FONT-LIST-CACHE records previous values returned by
+   x-list-fonts.  */
+#ifdef CYGWIN
+Lisp_Object cygw32_display_name_list;
+#endif
+
 #if _WIN32_WINNT < 0x0500 && !defined(_W64)
 /* Pre Windows 2000, this was not available, but define it here so
    that Emacs compiled on such a platform will run on newer versions.
@@ -6162,6 +6173,10 @@
   memset (dpyinfo, 0, sizeof (*dpyinfo));

   dpyinfo->name_list_element = Fcons (display_name, Qnil);
+#ifdef CYGWIN
+  cygw32_display_name_list = Fcons (dpyinfo->name_list_element,
+                                   cygw32_display_name_list);
+#endif
   dpyinfo->w32_id_name = xmalloc (SCHARS (Vinvocation_name)
                                  + SCHARS (Vsystem_name) + 2);
   sprintf (dpyinfo->w32_id_name, "%s@%s",
@@ -6411,6 +6426,26 @@
 x_delete_display (struct w32_display_info *dpyinfo)
 {
   /* FIXME: the only display info apparently can't be deleted.  */
+#ifdef CYGWIN
+  if (! NILP (cygw32_display_name_list)
+      && EQ (XCAR (cygw32_display_name_list), dpyinfo->name_list_element))
+    cygw32_display_name_list = XCDR (cygw32_display_name_list);
+  else
+    {
+      Lisp_Object tail;
+
+      tail = cygw32_display_name_list;
+      while (CONSP (tail) && CONSP (XCDR (tail)))
+       {
+         if (EQ (XCAR (XCDR (tail)), dpyinfo->name_list_element))
+           {
+             XSETCDR (tail, XCDR (XCDR (tail)));
+             break;
+           }
+         tail = XCDR (tail);
+       }
+    }
+#endif /* CYGWIN */
   /* free palette table */
   {
     struct w32_palette_entry * plist;
@@ -6547,6 +6582,9 @@
 void
 syms_of_w32term (void)
 {
+  staticpro (&cygw32_display_name_list);
+  cygw32_display_name_list = Qnil;
+
   DEFSYM (Qvendor_specific_keysyms, "vendor-specific-keysyms");

   DEFSYM (Qadded, "added");

=== modified file 'src/w32term.h'
--- src/w32term.h       2014-02-04 16:13:51 +0000
+++ src/w32term.h       2014-05-18 19:13:35 +0000
@@ -200,6 +200,10 @@
 extern struct w32_display_info *x_display_list;
 extern struct w32_display_info one_w32_display_info;

+#ifdef CYGWIN
+extern Lisp_Object cygw32_display_name_list;
+#endif
+
 extern struct frame *x_window_to_frame (struct w32_display_info *, HWND);

 struct w32_display_info *x_display_info_for_name (Lisp_Object);

bug#17510: 24.3.91; Problem with `emacs --daemon' in cygw32 build

[Ken Brown]

On 5/18/2014 3:36 PM, Ken Brown wrote:
> +  staticpro (&cygw32_display_name_list);
> +  cygw32_display_name_list = Qnil;

I forgot the #ifdef CYGWIN for this part.

bug#17510: 24.3.91; Problem with `emacs --daemon' in cygw32 build

[Eli Zaretskii]

> Date: Sun, 18 May 2014 15:36:11 -0400
> From: Ken Brown <kbrown@cornell.edu>
> CC: 17510@debbugs.gnu.org, dmantipov@yandex.ru
> 
> > If the Cygwin-w32 build wants more (or less) than one display_info
> > object, then that part _is_ specific to Cygwin, because the native
> > Windows build has only one such object that is never deleted.
> > 
> >> Can you reproduce it in the Windows build?
> > 
> > The native Windows build doesn't support --daemon, so no, I can't.
> > 
> >> I *think* what must be happening in the recipe that I gave for this bug
> >> is that every time a client frame is closed, x_delete_display is called.
> >>    Before Dmitry's change, this would actually delete something from a
> >> list.  Now it doesn't, and the server gets messed up and ultimately dies
> >> on the third attempt to create a client frame.
> > 
> > See above: try restoring that code for Cygwin only.
> > 
> >> Unless there's an obvious fix for this, it seems to me that we're far
> >> enough into the pretest that we should just revert to the old code, at
> >> least for emacs-24.
> > 
> > That would revert a useful cleanup, which I'm not sure is a good idea
> > at this point.
> 
> How does this look?

I guess it's OK for the branch, thanks.  But it strikes me that simply
replacing the car of dpyinfo->name_list_element by something like
"!!!DELETED DISPLAY!!!", or even just an empty string, would serve the
same purpose, and save us the nuisance of an additional list in
cygw32_display_name_list.  After all, all you need is to mark a
display deleted without actually deleting it, right?  IOW, the main
problem is in x_delete_display, and all the rest is just the overhead
you needed to fix that, correct?

bug#17510: 24.3.91; Problem with `emacs --daemon' in cygw32 build

[Eli Zaretskii]

> Date: Mon, 19 May 2014 19:46:56 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 17510@debbugs.gnu.org, dmantipov@yandex.ru
> 
> it strikes me that simply replacing the car of
> dpyinfo->name_list_element by something like "!!!DELETED
> DISPLAY!!!", or even just an empty string

I meant doing the above when a display is deleted.

bug#17510: 24.3.91; Problem with `emacs --daemon' in cygw32 build

[Ken Brown]

On 5/19/2014 12:46 PM, Eli Zaretskii wrote:
> I guess it's OK for the branch, thanks.  But it strikes me that simply
> replacing the car of dpyinfo->name_list_element by something like
> "!!!DELETED DISPLAY!!!", or even just an empty string, would serve the
> same purpose, and save us the nuisance of an additional list in
> cygw32_display_name_list.  After all, all you need is to mark a
> display deleted without actually deleting it, right?  IOW, the main
> problem is in x_delete_display, and all the rest is just the overhead
> you needed to fix that, correct?

I think that's correct, and I agree that there should be a much simpler 
fix.  I'll have to look into the code and try to understand better 
exactly what happens when emacs is started as a daemon and then a client 
frame is opened and closed.

I'll hold off on installing my patch until I see if I can find a better 
solution that is still safe enough for the branch.

Ken

bug#17510: 24.3.91; Problem with `emacs --daemon' in cygw32 build

[Ken Brown]

On 5/19/2014 3:25 PM, Ken Brown wrote:
> On 5/19/2014 12:46 PM, Eli Zaretskii wrote:
>> I guess it's OK for the branch, thanks.  But it strikes me that simply
>> replacing the car of dpyinfo->name_list_element by something like
>> "!!!DELETED DISPLAY!!!", or even just an empty string, would serve the
>> same purpose, and save us the nuisance of an additional list in
>> cygw32_display_name_list.  After all, all you need is to mark a
>> display deleted without actually deleting it, right?  IOW, the main
>> problem is in x_delete_display, and all the rest is just the overhead
>> you needed to fix that, correct?
>
> I think that's correct, and I agree that there should be a much simpler
> fix.  I'll have to look into the code and try to understand better
> exactly what happens when emacs is started as a daemon and then a client
> frame is opened and closed.

My guess as to the cause of this bug was completely wrong.  What happens 
in my recipe is that the pointer dpyinfo->w32_id_name is freed twice. 
(This is done in x_delete_display each time the only existing client 
frame is deleted.)  An attempt to create a client frame for the third 
time then leads to a crash because of malloc corruption.

I have no idea why this problem only showed up after Dmitry's code 
cleanup.  The only thing I can think of is that maintaining a list of 
display names, with insertions and deletions, masked the malloc corruption.

I think the right fix here would be to really delete the display when 
x_delete_display is called, free all resources, and set things up so 
that everything will be re-initialized if a new frame is created.  But 
this seems like a lot of trouble, possibly with unintended consequences. 
  The following is a much simpler workaround:

=== modified file 'src/w32term.c'
--- src/w32term.c       2014-04-16 14:00:39 +0000
+++ src/w32term.c       2014-05-24 12:13:15 +0000
@@ -6426,7 +6426,9 @@
      if (dpyinfo->palette)
        DeleteObject (dpyinfo->palette);
    }
+#ifndef CYGWIN
    xfree (dpyinfo->w32_id_name);
+#endif

    w32_reset_fringes ();
  }

I would of course add a comment explaining this.  What do you think?

Ken

bug#17510: 24.3.91; Problem with `emacs --daemon' in cygw32 build

[Eli Zaretskii]

> Date: Sat, 24 May 2014 08:38:14 -0400
> From: Ken Brown <kbrown@cornell.edu>
> CC: 17510@debbugs.gnu.org, dmantipov@yandex.ru
> 
> My guess as to the cause of this bug was completely wrong.  What happens 
> in my recipe is that the pointer dpyinfo->w32_id_name is freed twice. 
> (This is done in x_delete_display each time the only existing client 
> frame is deleted.)  An attempt to create a client frame for the third 
> time then leads to a crash because of malloc corruption.
> 
> I have no idea why this problem only showed up after Dmitry's code 
> cleanup.  The only thing I can think of is that maintaining a list of 
> display names, with insertions and deletions, masked the malloc corruption.
> 
> I think the right fix here would be to really delete the display when 
> x_delete_display is called, free all resources, and set things up so 
> that everything will be re-initialized if a new frame is created.  But 
> this seems like a lot of trouble, possibly with unintended consequences. 
>   The following is a much simpler workaround:
> 
> === modified file 'src/w32term.c'
> --- src/w32term.c       2014-04-16 14:00:39 +0000
> +++ src/w32term.c       2014-05-24 12:13:15 +0000
> @@ -6426,7 +6426,9 @@
>       if (dpyinfo->palette)
>         DeleteObject (dpyinfo->palette);
>     }
> +#ifndef CYGWIN
>     xfree (dpyinfo->w32_id_name);
> +#endif
> 
>     w32_reset_fringes ();
>   }
> 
> I would of course add a comment explaining this.  What do you think?

This looks OK to me, but I wonder: is it really correct not to free
w32_id_name at all?  And if that is correct, why only on Cygwin?

Does the Cygwin-w32 build also use a single dpyinfo object, like the
native Windows build?  If so, perhaps we need not free this in both
these builds.  IOW, I think your suggested change is OK for the
emacs-24 branch, but on the trunk I'd suggest to remove the xfree line
altogether.

Thanks.

bug#17510: 24.3.91; Problem with `emacs --daemon' in cygw32 build

[Ken Brown]

On 5/24/2014 8:59 AM, Eli Zaretskii wrote:
> This looks OK to me, but I wonder: is it really correct not to free
> w32_id_name at all?  And if that is correct, why only on Cygwin?

I would say it's harmless not to free w32_id_name.  It's malloc'd once 
and never changed.  But I agree that it's harmless also on native Windows.

> Does the Cygwin-w32 build also use a single dpyinfo object, like the
> native Windows build?

Yes

> If so, perhaps we need not free this in both
> these builds.  IOW, I think your suggested change is OK for the
> emacs-24 branch, but on the trunk I'd suggest to remove the xfree line
> altogether.

OK, I've made the change on the emacs-24 branch as revision 117147. 
After this has been merged to the trunk, I'll remove the xfree line.

I'm not closing the bug yet because I forgot to retest my change after 
revision 117146 was made, and the latter is causing a problem with 
emacsclient (at least on Cygwin-w32).  I need to make sure that this 
isn't related to my change.

Ken

bug#17510: 24.3.91; Problem with `emacs --daemon' in cygw32 build

[Daniel Colascione]

[-- Attachment #1: Type: text/plain, Size: 1470 bytes --]

On 05/24/2014 05:38 AM, Ken Brown wrote:
> On 5/19/2014 3:25 PM, Ken Brown wrote:
>> On 5/19/2014 12:46 PM, Eli Zaretskii wrote:
>>> I guess it's OK for the branch, thanks.  But it strikes me that simply
>>> replacing the car of dpyinfo->name_list_element by something like
>>> "!!!DELETED DISPLAY!!!", or even just an empty string, would serve the
>>> same purpose, and save us the nuisance of an additional list in
>>> cygw32_display_name_list.  After all, all you need is to mark a
>>> display deleted without actually deleting it, right?  IOW, the main
>>> problem is in x_delete_display, and all the rest is just the overhead
>>> you needed to fix that, correct?
>>
>> I think that's correct, and I agree that there should be a much simpler
>> fix.  I'll have to look into the code and try to understand better
>> exactly what happens when emacs is started as a daemon and then a client
>> frame is opened and closed.
> 
> My guess as to the cause of this bug was completely wrong.  What happens
> in my recipe is that the pointer dpyinfo->w32_id_name is freed twice.
> (This is done in x_delete_display each time the only existing client
> frame is deleted.)  An attempt to create a client frame for the third
> time then leads to a crash because of malloc corruption.

Thanks for finding that. I wonder whether this double-free also has
something to do with random crashes people have been seeing in 64-bit
Cygwin cygw32 Emacs builds.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

bug#17510: 24.3.91; Problem with `emacs --daemon' in cygw32 build

[Ken Brown]

On 5/24/2014 3:28 PM, Daniel Colascione wrote:
> On 05/24/2014 05:38 AM, Ken Brown wrote:
>> On 5/19/2014 3:25 PM, Ken Brown wrote:
>>> On 5/19/2014 12:46 PM, Eli Zaretskii wrote:
>>>> I guess it's OK for the branch, thanks.  But it strikes me that simply
>>>> replacing the car of dpyinfo->name_list_element by something like
>>>> "!!!DELETED DISPLAY!!!", or even just an empty string, would serve the
>>>> same purpose, and save us the nuisance of an additional list in
>>>> cygw32_display_name_list.  After all, all you need is to mark a
>>>> display deleted without actually deleting it, right?  IOW, the main
>>>> problem is in x_delete_display, and all the rest is just the overhead
>>>> you needed to fix that, correct?
>>>
>>> I think that's correct, and I agree that there should be a much simpler
>>> fix.  I'll have to look into the code and try to understand better
>>> exactly what happens when emacs is started as a daemon and then a client
>>> frame is opened and closed.
>>
>> My guess as to the cause of this bug was completely wrong.  What happens
>> in my recipe is that the pointer dpyinfo->w32_id_name is freed twice.
>> (This is done in x_delete_display each time the only existing client
>> frame is deleted.)  An attempt to create a client frame for the third
>> time then leads to a crash because of malloc corruption.
>
> Thanks for finding that. I wonder whether this double-free also has
> something to do with random crashes people have been seeing in 64-bit
> Cygwin cygw32 Emacs builds.

I doubt it, because this double-free occurs in both 64-bit and 32-bit 
Cygwin.  Also, I think it can only be triggered by running emacs as a 
daemon, and none of the people reporting crashes mentioned doing that.

Ken

bug#17510: 24.3.91; Problem with `emacs --daemon' in cygw32 build

[Ken Brown]

Version: 24.4

Closing.