The default bug view has changed. See this FAQ.

Call XCloseDisplay before exit and exec

RESOLVED FIXED in mozilla1.9beta1

Status

()

Toolkit
Startup and Profile System
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

(Blocks: 1 bug, {mlk})

Trunk
mozilla1.9beta1
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
Created attachment 279115 [details] [diff] [review]
open and close the display ourselves (for gtk)
Assignee: nobody → mozbugz
Status: NEW → ASSIGNED
Attachment #279115 - Flags: review?(benjamin)
(Assignee)

Updated

10 years ago
Attachment #279115 - Flags: review?(benjamin) → review?(roc)
Attachment #279115 - Flags: review?(roc) → review+
(Assignee)

Updated

10 years ago
Attachment #279115 - Flags: approval1.9?
Attachment #279115 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
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
Attachment #279115 - Flags: review?(benjamin)
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Flags: blocking1.9?
Attachment #279115 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
Checking in toolkit/xre/nsAppRunner.cpp;
/cvsroot/mozilla/toolkit/xre/nsAppRunner.cpp,v  <--  nsAppRunner.cpp
new revision: 1.190; previous revision: 1.189
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Flags: blocking1.9?
Had to back this out due to the unknown orange it was causing for some of the Linux tinderboxen.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: blocking1.9?
(Assignee)

Comment 6

10 years ago
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.
(Assignee)

Comment 7

10 years ago
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).
(Assignee)

Comment 9

10 years ago
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.
(Assignee)

Comment 12

10 years ago
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.)
(Assignee)

Comment 16

10 years ago
I suspect this:

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

Comment 17

10 years ago
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
(Assignee)

Updated

10 years ago
Depends on: 397607
(Assignee)

Comment 18

10 years ago
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.
Attachment #279115 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
(Assignee)

Comment 19

10 years ago
s/bug 394466/bug 397607/
Checking in toolkit/xre/nsAppRunner.cpp;
/cvsroot/mozilla/toolkit/xre/nsAppRunner.cpp,v  <--  nsAppRunner.cpp
new revision: 1.193; previous revision: 1.192
done
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M9

Updated

10 years ago
Depends on: 398057
(Assignee)

Updated

10 years ago
Blocks: 293007
Duplicate of this bug: 28986
(Assignee)

Updated

10 years ago
Depends on: 398364
(Assignee)

Updated

10 years ago
Depends on: 398512

Updated

9 years ago
Component: XRE Startup → Startup and Profile System
QA Contact: xre.startup → startup
You need to log in before you can comment on or make changes to this bug.