[PATCH] XCloseDisplay already calls XrmDestroyDatabase

[PATCH] XCloseDisplay already calls XrmDestroyDatabase

[İsmail Dönmez]

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

Hi,

Running on Ubuntu's upcoming Intrepid release I experienced X crashes
when I quit emacsclient. The gdb log shows XrmDestroyDatabase() is the
failing line. Looking at

src/xterm.c lines around about 10514:

#ifndef USE_X_TOOLKIT   /* I'm told Xt does this itself.  */
#ifndef AIX            /* On AIX, XCloseDisplay calls this.  */
  XrmDestroyDatabase (dpyinfo->xrdb);
#endif
#endif

So this code assumes only on AIX XCloseDisplay itself calls
XrmDestroyDatabase but this doesn't seem to be the case, looking at
libX11 1.1.4 source code,

src/OpenDis.c starting line 832:

    822     /* if RM database was allocated by XGetDefault() free it */
    823     if (dpy->db && (dpy->flags & XlibDisplayDfltRMDB))
    824         XrmDestroyDatabase(dpy->db);

this is from the _XFreeDisplayStructure() function and the function
documentation says:


 /* XFreeDisplayStructure frees all the storage associated with a
  * Display.  It is used by XOpenDisplay if it runs out of memory,
  * and also by XCloseDisplay.
....
*/

So looks like there is no need to manually call XrmDestroyDatabase()
anymore, attached patch removes it.

Regards,
ismail

-- 
Programmer Excuse #4: It's too complicated for you to understand.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: xrm.patch --]
[-- Type: text/x-diff; name=xrm.patch, Size: 444 bytes --]

diff --git a/src/xterm.c b/src/xterm.c
index a32f4e1..b5b3f19 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -10514,11 +10514,6 @@ x_delete_display (dpyinfo)
 	  tail->next = tail->next->next;
     }
 
-#ifndef USE_X_TOOLKIT   /* I'm told Xt does this itself.  */
-#ifndef AIX		/* On AIX, XCloseDisplay calls this.  */
-  XrmDestroyDatabase (dpyinfo->xrdb);
-#endif
-#endif
 #ifdef HAVE_X_I18N
   if (dpyinfo->xim)
     xim_close_dpy (dpyinfo);

Re: [PATCH] XCloseDisplay already calls XrmDestroyDatabase

[İsmail Dönmez]

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

Hi again,

On Tue, Aug 19, 2008 at 13:19, İsmail Dönmez <ismail@namtrac.org> wrote:
> Hi,
>
> Running on Ubuntu's upcoming Intrepid release I experienced X crashes
> when I quit emacsclient. The gdb log shows XrmDestroyDatabase() is the
> failing line. Looking at
>
> src/xterm.c lines around about 10514:
>
> #ifndef USE_X_TOOLKIT   /* I'm told Xt does this itself.  */
> #ifndef AIX            /* On AIX, XCloseDisplay calls this.  */
>  XrmDestroyDatabase (dpyinfo->xrdb);
> #endif
> #endif
>
> So this code assumes only on AIX XCloseDisplay itself calls
> XrmDestroyDatabase but this doesn't seem to be the case, looking at
> libX11 1.1.4 source code,
>
> src/OpenDis.c starting line 832:
>
>    822     /* if RM database was allocated by XGetDefault() free it */
>    823     if (dpy->db && (dpy->flags & XlibDisplayDfltRMDB))
>    824         XrmDestroyDatabase(dpy->db);
>
> this is from the _XFreeDisplayStructure() function and the function
> documentation says:
>
>
>  /* XFreeDisplayStructure frees all the storage associated with a
>  * Display.  It is used by XOpenDisplay if it runs out of memory,
>  * and also by XCloseDisplay.
> ....
> */
>
> So looks like there is no need to manually call XrmDestroyDatabase()
> anymore, attached patch removes it.

Actually correct solution is to disable XrmDestroyDatabase() call for
GTK+ as done for Xt. See attached patch.

Regards,
ismail

-- 
Programmer Excuse #4: It's too complicated for you to understand.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: xrm2.patch --]
[-- Type: text/x-diff; name=xrm2.patch, Size: 455 bytes --]

diff --git a/src/xterm.c b/src/xterm.c
index a32f4e1..9b9b976 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -10514,7 +10514,7 @@ x_delete_display (dpyinfo)
 	  tail->next = tail->next->next;
     }
 
-#ifndef USE_X_TOOLKIT   /* I'm told Xt does this itself.  */
+#if ! defined (USE_X_TOOLKIT) && ! defined (USE_GTK) /* Xt and GTK does this by itself */
 #ifndef AIX		/* On AIX, XCloseDisplay calls this.  */
   XrmDestroyDatabase (dpyinfo->xrdb);
 #endif

Re: [PATCH] XCloseDisplay already calls XrmDestroyDatabase

[İsmail Dönmez]

Hi,

On Wed, Aug 20, 2008 at 11:17, İsmail Dönmez <ismail@namtrac.org> wrote:
> Hi again,
>
> On Tue, Aug 19, 2008 at 13:19, İsmail Dönmez <ismail@namtrac.org> wrote:
>> Hi,
>>
>> Running on Ubuntu's upcoming Intrepid release I experienced X crashes
>> when I quit emacsclient. The gdb log shows XrmDestroyDatabase() is the
>> failing line. Looking at
>>
>> src/xterm.c lines around about 10514:
>>
>> #ifndef USE_X_TOOLKIT   /* I'm told Xt does this itself.  */
>> #ifndef AIX            /* On AIX, XCloseDisplay calls this.  */
>>  XrmDestroyDatabase (dpyinfo->xrdb);
>> #endif
>> #endif
>>
>> So this code assumes only on AIX XCloseDisplay itself calls
>> XrmDestroyDatabase but this doesn't seem to be the case, looking at
>> libX11 1.1.4 source code,
>>
>> src/OpenDis.c starting line 832:
>>
>>    822     /* if RM database was allocated by XGetDefault() free it */
>>    823     if (dpy->db && (dpy->flags & XlibDisplayDfltRMDB))
>>    824         XrmDestroyDatabase(dpy->db);
>>
>> this is from the _XFreeDisplayStructure() function and the function
>> documentation says:
>>
>>
>>  /* XFreeDisplayStructure frees all the storage associated with a
>>  * Display.  It is used by XOpenDisplay if it runs out of memory,
>>  * and also by XCloseDisplay.
>> ....
>> */
>>
>> So looks like there is no need to manually call XrmDestroyDatabase()
>> anymore, attached patch removes it.
>
> Actually correct solution is to disable XrmDestroyDatabase() call for
> GTK+ as done for Xt. See attached patch.

Any comments on this? It fixes a frequent crash for me.

Regards,
ismail

-- 
Programmer Excuse #4: It's too complicated for you to understand.

Re: [PATCH] XCloseDisplay already calls XrmDestroyDatabase

[Chong Yidong]

"İsmail Dönmez" <ismail@namtrac.org> writes:

>>> So looks like there is no need to manually call XrmDestroyDatabase()
>>> anymore, attached patch removes it.
>>
>> Actually correct solution is to disable XrmDestroyDatabase() call for
>> GTK+ as done for Xt. See attached patch.
>
> Any comments on this? It fixes a frequent crash for me.

I've checked in the patch.  Thanks.

Re: [PATCH] XCloseDisplay already calls XrmDestroyDatabase

[YAMAMOTO Mitsuharu]

>>>>> On Thu, 21 Aug 2008 15:43:21 -0400, Chong Yidong <cyd@stupidchicken.com> said:

> "İsmail Dönmez" <ismail@namtrac.org> writes:
>>>> So looks like there is no need to manually call
>>>> XrmDestroyDatabase() anymore, attached patch removes it.
>>> 
>>> Actually correct solution is to disable XrmDestroyDatabase() call
>>> for GTK+ as done for Xt. See attached patch.
>> 
>> Any comments on this? It fixes a frequent crash for me.

> I've checked in the patch.  Thanks.

I tried reverting it, but I couldn't reproduce the crash using the
procedure in Bug#581 with Ubuntu 9.04, GTK+ build.  I also tried
setting a breakpoint to XrmDestroyDatabase, but I couldn't observe its
call from XCloseDisplay on closing an X11 display.  On the other hand,
Bug#1812 says the similar crash happens even without X toolkit on Mac
OS X 10.4.  The same binary does not crash on Mac OS X 10.5.

I think necessity of the XrmDestroyDatabase call depends on which
version of libX11 is used rather than whether GTK+ is used or not.
The relevant change in libX11 would be:

  http://lists.freedesktop.org/archives/xorg-commit-diffs/2004-March/000239.html

I confirmed that the libX11 shipped with Mac OS X 10.4 doesn't contain
the above change.

  http://www.opensource.apple.com/source/X11/X11-0.46.4/xc/lib/X11/

If we remove the XrmDestroyDatabase call unconditionally, then it
causes a memory leak when used with a newer libX11.  But that would be
better than crashing when used with an older one.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: [PATCH] XCloseDisplay already calls XrmDestroyDatabase

[YAMAMOTO Mitsuharu]

>>>>> On Wed, 13 May 2009 12:27:01 +0900, YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> said:

> If we remove the XrmDestroyDatabase call unconditionally, then it
> causes a memory leak when used with a newer libX11.  But that would
> be better than crashing when used with an older one.

Or maybe we can dissociate the resource database from the display
before closing it.

  #ifdef HAVE_XRMSETDATABASE
    XrmSetDatabase (dpyinfo->display, NULL);
  #else
    dpyinfo->display->db = NULL;
  #endif

Then we can ignore the difference of libX11 at least for normal close.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: [PATCH] XCloseDisplay already calls XrmDestroyDatabase

[Stefan Monnier]

>> If we remove the XrmDestroyDatabase call unconditionally, then it
>> causes a memory leak when used with a newer libX11.  But that would
>> be better than crashing when used with an older one.

> Or maybe we can dissociate the resource database from the display
> before closing it.

>   #ifdef HAVE_XRMSETDATABASE
>     XrmSetDatabase (dpyinfo->display, NULL);
>   #else
dpyinfo-> display->db = NULL;
>   #endif

> Then we can ignore the difference of libX11 at least for normal close.

Sounds OK as well.


        Stefan

Re: [PATCH] XCloseDisplay already calls XrmDestroyDatabase

[YAMAMOTO Mitsuharu]

>>>>> On Sun, 17 May 2009 17:40:50 -0400, Stefan Monnier <monnier@iro.umontreal.ca> said:

>>> If we remove the XrmDestroyDatabase call unconditionally, then it
>>> causes a memory leak when used with a newer libX11.  But that would
>>> be better than crashing when used with an older one.

>> Or maybe we can dissociate the resource database from the display
>> before closing it.

>> #ifdef HAVE_XRMSETDATABASE
>> XrmSetDatabase (dpyinfo->display, NULL);
>> #else
>> dpyinfo->display->db = NULL;
>> #endif

>> Then we can ignore the difference of libX11 at least for normal close.

> Sounds OK as well.

Here is a patch.  If it's OK, I'd like to install it before the next
pretest.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Index: src/xterm.c
===================================================================
RCS file: /sources/emacs/emacs/src/xterm.c,v
retrieving revision 1.1025
diff -c -p -r1.1025 xterm.c
*** src/xterm.c	2 May 2009 20:16:58 -0000	1.1025
--- src/xterm.c	18 May 2009 08:00:51 -0000
*************** x_delete_display (dpyinfo)
*** 10613,10625 ****
  	  tail->next = tail->next->next;
      }
  
-   /* Xt and GTK do this themselves.  */
- #if ! defined (USE_X_TOOLKIT) && ! defined (USE_GTK)
- #ifndef AIX		/* On AIX, XCloseDisplay calls this.  */
-   XrmDestroyDatabase (dpyinfo->xrdb);
- #endif
- #endif
- 
    xfree (dpyinfo->x_id_name);
    xfree (dpyinfo->x_dnd_atoms);
    xfree (dpyinfo->color_cells);
--- 10613,10618 ----
*************** x_delete_terminal (struct terminal *term
*** 10740,10745 ****
--- 10733,10752 ----
        x_destroy_all_bitmaps (dpyinfo);
        XSetCloseDownMode (dpyinfo->display, DestroyAll);
  
+       /* Whether or not XCloseDisplay destroys the associated resource
+ 	 database depends on the version of libX11.  To avoid both
+ 	 crash and memory leak, we dissociate the database from the
+ 	 display and then destroy dpyinfo->xrdb ourselves.  */
+ #ifdef HAVE_XRMSETDATABASE
+       XrmSetDatabase (dpyinfo->display, NULL);
+ #else
+       dpyinfo->display->db = NULL;
+ #endif
+       /* We used to call XrmDestroyDatabase from x_delete_display, but
+ 	 some older versions of libX11 crash if we call it after
+ 	 closing all the displays.  */
+       XrmDestroyDatabase (dpyinfo->xrdb);
+ 
  #ifdef USE_GTK
        xg_display_close (dpyinfo->display);
  #else

Re: [PATCH] XCloseDisplay already calls XrmDestroyDatabase

[Chong Yidong]

YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> writes:

> Here is a patch.  If it's OK, I'd like to install it before the next
> pretest.

OK, please do it.

Re: [PATCH] XCloseDisplay already calls XrmDestroyDatabase

[Stefan Monnier]

> I confirmed that the libX11 shipped with Mac OS X 10.4 doesn't contain
> the above change.

>   http://www.opensource.apple.com/source/X11/X11-0.46.4/xc/lib/X11/

> If we remove the XrmDestroyDatabase call unconditionally, then it
> causes a memory leak when used with a newer libX11.  But that would be
> better than crashing when used with an older one.

OTOH, it's better to work correctly with the new code than with the old
code (better be future-proof than past-proof).  Seeing how Mac OS X 10.5
has been out for a while now, I expect most users of Emacs-23 will run
with code that would tend to leak rather than with code that
would tend to crash.

So, I'd rather just free XrmDestroyDatabase and add a note in PROBLEMS
that with older releases of libX11 there may be crashes when closing
the display.


        Stefan