8.43 KB, text/plain; charset=UTF-8
8.92 KB, text/plain; charset=UTF-8
3.85 KB, patch
|Details | Diff | Splinter Review|
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.
Created attachment 279115 [details] [diff] [review] open and close the display ourselves (for gtk)
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 on attachment 279115 [details] [diff] [review] open and close the display ourselves (for gtk) let's get review from bsmedberg as well, then
Checking in toolkit/xre/nsAppRunner.cpp; /cvsroot/mozilla/toolkit/xre/nsAppRunner.cpp,v <-- nsAppRunner.cpp new revision: 1.190; previous revision: 1.189 done
Had to back this out due to the unknown orange it was causing for some of the Linux tinderboxen.
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.
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.
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).
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, ...
I'll try to look when I'm home next week.
Created attachment 282216 [details] 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.
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
dbaron: seems more likely that's a regression from bug 393845, see the deps list.
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...
Created attachment 282325 [details] 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.)
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
Created attachment 282602 [details] [diff] [review] 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.
Checking in toolkit/xre/nsAppRunner.cpp; /cvsroot/mozilla/toolkit/xre/nsAppRunner.cpp,v <-- nsAppRunner.cpp new revision: 1.193; previous revision: 1.192 done