* [PATCH] XCloseDisplay already calls XrmDestroyDatabase
@ 2008-08-19 10:19 İsmail Dönmez
2008-08-20 8:17 ` İsmail Dönmez
0 siblings, 1 reply; 10+ messages in thread
From: İsmail Dönmez @ 2008-08-19 10:19 UTC (permalink / raw)
To: emacs- devel
[-- 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);
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] XCloseDisplay already calls XrmDestroyDatabase
2008-08-19 10:19 [PATCH] XCloseDisplay already calls XrmDestroyDatabase İsmail Dönmez
@ 2008-08-20 8:17 ` İsmail Dönmez
2008-08-21 18:40 ` İsmail Dönmez
0 siblings, 1 reply; 10+ messages in thread
From: İsmail Dönmez @ 2008-08-20 8:17 UTC (permalink / raw)
To: emacs- devel
[-- 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] XCloseDisplay already calls XrmDestroyDatabase
2008-08-20 8:17 ` İsmail Dönmez
@ 2008-08-21 18:40 ` İsmail Dönmez
2008-08-21 19:43 ` Chong Yidong
0 siblings, 1 reply; 10+ messages in thread
From: İsmail Dönmez @ 2008-08-21 18:40 UTC (permalink / raw)
To: emacs- devel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] XCloseDisplay already calls XrmDestroyDatabase
2008-08-21 18:40 ` İsmail Dönmez
@ 2008-08-21 19:43 ` Chong Yidong
2009-05-13 3:27 ` YAMAMOTO Mitsuharu
0 siblings, 1 reply; 10+ messages in thread
From: Chong Yidong @ 2008-08-21 19:43 UTC (permalink / raw)
To: İsmail Dönmez; +Cc: 581-done, emacs-devel
"İ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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] XCloseDisplay already calls XrmDestroyDatabase
2008-08-21 19:43 ` Chong Yidong
@ 2009-05-13 3:27 ` YAMAMOTO Mitsuharu
2009-05-13 23:48 ` YAMAMOTO Mitsuharu
2009-05-17 21:40 ` Stefan Monnier
0 siblings, 2 replies; 10+ messages in thread
From: YAMAMOTO Mitsuharu @ 2009-05-13 3:27 UTC (permalink / raw)
To: Chong Yidong; +Cc: İsmail Dönmez, emacs-devel
>>>>> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] XCloseDisplay already calls XrmDestroyDatabase
2009-05-13 3:27 ` YAMAMOTO Mitsuharu
@ 2009-05-13 23:48 ` YAMAMOTO Mitsuharu
2009-05-17 21:40 ` Stefan Monnier
2009-05-17 21:40 ` Stefan Monnier
1 sibling, 1 reply; 10+ messages in thread
From: YAMAMOTO Mitsuharu @ 2009-05-13 23:48 UTC (permalink / raw)
To: Chong Yidong; +Cc: İsmail Dönmez, emacs-devel
>>>>> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] XCloseDisplay already calls XrmDestroyDatabase
2009-05-13 23:48 ` YAMAMOTO Mitsuharu
@ 2009-05-17 21:40 ` Stefan Monnier
2009-05-18 8:09 ` YAMAMOTO Mitsuharu
0 siblings, 1 reply; 10+ messages in thread
From: Stefan Monnier @ 2009-05-17 21:40 UTC (permalink / raw)
To: YAMAMOTO Mitsuharu; +Cc: Chong Yidong, emacs-devel, ez
>> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] XCloseDisplay already calls XrmDestroyDatabase
2009-05-17 21:40 ` Stefan Monnier
@ 2009-05-18 8:09 ` YAMAMOTO Mitsuharu
2009-05-18 13:29 ` Chong Yidong
0 siblings, 1 reply; 10+ messages in thread
From: YAMAMOTO Mitsuharu @ 2009-05-18 8:09 UTC (permalink / raw)
To: Stefan Monnier; +Cc: Dönmez, Chong Yidong, emacs-devel
>>>>> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] XCloseDisplay already calls XrmDestroyDatabase
2009-05-13 3:27 ` YAMAMOTO Mitsuharu
2009-05-13 23:48 ` YAMAMOTO Mitsuharu
@ 2009-05-17 21:40 ` Stefan Monnier
1 sibling, 0 replies; 10+ messages in thread
From: Stefan Monnier @ 2009-05-17 21:40 UTC (permalink / raw)
To: YAMAMOTO Mitsuharu; +Cc: Chong Yidong, emacs-devel, ez
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2009-05-18 13:29 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-08-19 10:19 [PATCH] XCloseDisplay already calls XrmDestroyDatabase İsmail Dönmez
2008-08-20 8:17 ` İsmail Dönmez
2008-08-21 18:40 ` İsmail Dönmez
2008-08-21 19:43 ` Chong Yidong
2009-05-13 3:27 ` YAMAMOTO Mitsuharu
2009-05-13 23:48 ` YAMAMOTO Mitsuharu
2009-05-17 21:40 ` Stefan Monnier
2009-05-18 8:09 ` YAMAMOTO Mitsuharu
2009-05-18 13:29 ` Chong Yidong
2009-05-17 21:40 ` Stefan Monnier
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).