394466 - Call XCloseDisplay before exit and exec

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Karl Tomlinson (:karlt back Feb 10)]

Shutting down the display properly frees associated allocated memory from Xlib, and also from libraries (such as Xft and cairo) that register clean up routines with XESetCloseDisplay.  This makes real leaks easier to separate from the noise.

This may also help with bug 246313, where an exec without XCloseDisplay causes the Xserver to believe that second cient is wanting to use the same XDM authorization key before the first has finished.

Comment 1

[Karl Tomlinson (:karlt back Feb 10)]

Attachment: open and close the display ourselves (for gtk)

Comment 2

[Reed Loden [:reed]]

I was going to check this in, but I decided to check the review requirements first to make sure it had all the needed reviews. According to http://www.mozilla.org/projects/toolkit/review.html, toolkit/xre/ patches need review from an owner/peer or the sub-module owner/peer (bsmedberg/darin). roc's only listed on the toolkit page as sub-module owner for toolkit/components/gnome/, so I don't think he's a valid reviewer (unless :bs granted him reviewer power for this patch).

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 279115 [details] [diff] [review]
open and close the display ourselves (for gtk)

let's get review from bsmedberg as well, then

Comment 4

[Reed Loden [:reed]]

Checking in toolkit/xre/nsAppRunner.cpp;
/cvsroot/mozilla/toolkit/xre/nsAppRunner.cpp,v  <--  nsAppRunner.cpp
new revision: 1.190; previous revision: 1.189
done

Comment 5

[Reed Loden [:reed]]

Had to back this out due to the unknown orange it was causing for some of the Linux tinderboxen.

Comment 6

[Karl Tomlinson (:karlt back Feb 10)]

So, with this patch, the tinderboxen reftest and simpletest seg fault on exit and the XPCOM_MEM_BLOAT_LOG=1 <firefox-bin> -P default resource:///res/bloatcycle.html bloat test timed out after closing all dom windows but before outputing bloat information.

I can't reproduce but:
There is an issue in gtk (2.10.11 at least) where color_scheme_data_free calls g_hash_table_unref on data->color_hash but this is initialized to 0 in settings_update_color_scheme unless there is a screen setting.

However, this would only be a problem if G_DISABLE_CHECKS were defined when glib was built, which seems unlikely.  I'll keep looking.

Comment 7

[Karl Tomlinson (:karlt back Feb 10)]

That gtk bug is here:
http://bugzilla.gnome.org/show_bug.cgi?id=423916
Introduced in 2.10.10 and fixed in 2.10.12,
but centOS 5.0 uses gtk2-2.10.4-16.el5,
so that doesn't seem to be the tinderbox issue.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I saw a crash on shutdown with this patch with Fedora 7, but didn't have a chance to look into it before traveling (I just unapplied it from my tree).

Comment 9

[Karl Tomlinson (:karlt back Feb 10)]

David, if you are still able to reproduce a crash on shutdown and could provide a stack trace, please, that would be very helpful.

If your Fedora 7 still has gtk 2.10.11 then you can expect to see

  (Gecko:11933): GLib-CRITICAL **: g_hash_table_unref: assertion `hash_table != NULL' failed

(glib2-2.12.11-1.fc7 wasn't built with G_DISABLE_CHECKS defined.)

But if you are up-to-date with gtk2-2.10.14-3.fc7 then you shouldn't be seeing the above message.

I've had no success reproducing a crash, and can see nothing interesting under valgrind when running against CentOS 5.0's X-dependent shared libraries.
Other possible factors are gtk-engine, gnome-settings, fonts, ...

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'll try to look when I'm home next week.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: info about plugin-related crash this caused

So the crash I saw before was probably the one I just saw again -- and it seems related to plugin shutdown happening (after the XCloseDisplay done by this patch?).  Here's the stacks of the assertion preceding it and then the crash, plus a little more info from gdb.

Comment 12

[Karl Tomlinson (:karlt back Feb 10)]

Thank you David, but:

(In reply to comment #11)
> So the crash I saw before was probably the one I just saw again -- and it seems
> related to plugin shutdown happening (after the XCloseDisplay done by this
> patch?).

Actually this is happening _before_ XCloseDisplay or even gdk_display_close, which makes it hard to imagine how this could be related.

Is the crash reproducible enough to confirm that it is this patch causing it?

Also, I can't think why there should be any ns4xPluginInstance alive at ~ScopedXPCOMStartup (nsAppRunner.cpp:873).
(Normally they are Stopped when the frame is Destroyed.)

Nice stack though :) Thanks, fix-linux-stack.pl

Comment 13

[(not currently active) Ted Mielczarek]

dbaron: seems more likely that's a regression from bug 393845, see the deps list.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, makes sense.  I suppose it may not have been the crash I saw earlier, which I also thought I saw more reliably.  It could have been some library upgrades (on Fedora 7) that happened in the interim that fixed it...

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: info about crash this caused related to MIT-SCREEN-SAVER extension

Maybe this is the crash you were looking for.

(I'm using Fedora 7, with the X server from Fedora 6, and the ATI fglrx driver packaged from rpm.livna.org.)

Comment 16

[Karl Tomlinson (:karlt back Feb 10)]

I suspect this:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/widget/src/gtk2/nsIdleServiceGTK.cpp&rev=1.2&mark=108#103

Comment 17

[Karl Tomlinson (:karlt back Feb 10)]

Thanks very much David for your helpful debugging of XCloseDisplay.

Reproduced crash here by installing libXScrnSaver (libXss) and leaving running long enough for XScreenSaverQueryExtension to be called from places:

#0  0x00002abf0d0969b3 in XESetCloseDisplay () from /usr/lib/libX11.so.6
#1  0x00002abf0e256b54 in XextAddDisplay () from /usr/lib/libXext.so.6
#2  0x00002aaab5e2e1d5 in XScreenSaverQueryExtension ()
   from /usr/lib/libXss.so.1
#3  0x00002aaaaad77bbc in nsIdleServiceGTK::GetIdleTime (this=0x1632530, 
    aTimeDiff=0x7fffa00cc18c)
    at /home/karl/moz/mozilla/widget/src/gtk2/nsIdleServiceGTK.cpp:133
#4  0x00002aaab1e6eb06 in nsNavHistory::PerformVacuumIfIdle (this=0x6e19c0)
    at /home/karl/moz/mozilla/toolkit/components/places/src/nsNavHistory.cpp:3338
#5  0x00002aaab1e6ee37 in nsNavHistory::VacuumTimerCallback (aTimer=0x73bad0, 
    aClosure=0x6e19c0)
    at /home/karl/moz/mozilla/toolkit/components/places/src/nsNavHistory.cpp:3381

Comment 18

[Karl Tomlinson (:karlt back Feb 10)]

Attachment: patch for checkin

The only modification is a style correction removing the space from "gdk_display_manager_get ()".

Let's try this again now that bug 394466 is fixed.

Comment 19

[Karl Tomlinson (:karlt back Feb 10)]

s/bug 394466/bug 397607/

Comment 20

[Reed Loden [:reed]]

Checking in toolkit/xre/nsAppRunner.cpp;
/cvsroot/mozilla/toolkit/xre/nsAppRunner.cpp,v  <--  nsAppRunner.cpp
new revision: 1.193; previous revision: 1.192
done