Last Comment Bug 247204 - lock up at start when using threaded GTK2 libraries (i.e. gconf)
: lock up at start when using threaded GTK2 libraries (i.e. gconf)
Status: VERIFIED FIXED
: fixed1.8.1.8
Product: Core Graveyard
Classification: Graveyard
Component: GFX: Gtk (show other bugs)
: Trunk
: x86 Linux
: -- major (vote)
: ---
Assigned To: Walter Meinl
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2004-06-16 16:37 PDT by green
Modified: 2009-01-22 10:17 PST (History)
13 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed patch to start libgthread correctly (633 bytes, patch)
2004-06-17 09:27 PDT, green
no flags Details | Diff | Review
the patch from the original poster updated to the tip (1.43 KB, patch)
2007-05-28 03:32 PDT, Walter Meinl
roc: review+
roc: superreview+
Details | Diff | Review
patch for seamonkey xpfe Makefile.in and nsAppRunner.cpp (1.73 KB, patch)
2007-05-29 11:03 PDT, Walter Meinl
no flags Details | Diff | Review
additional patch for static-config.mk fixes also startup-notification (1.01 KB, patch)
2007-06-02 02:01 PDT, Walter Meinl
roc: review+
roc: superreview+
Details | Diff | Review
updated first patch after removal of qt (1.48 KB, patch)
2007-06-11 00:09 PDT, Walter Meinl
wuno: review+
Details | Diff | Review
bitrot of the static patch (removal of xprint) taking over r+ (956 bytes, patch)
2007-06-14 12:22 PDT, Walter Meinl
wuno: review+
Details | Diff | Review
corresponding patch for the branch (4.30 KB, patch)
2007-07-01 00:43 PDT, Walter Meinl
roc: review+
roc: superreview+
dveditz: approval1.8.1.5-
dveditz: approval1.8.1.8+
Details | Diff | Review

Description green 2004-06-16 16:37:47 PDT
User-Agent:       Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.7) Gecko/20040616 Firefox/0.9
Build Identifier: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.7) Gecko/20040616 Firefox/0.9

When upgrading to Firefox 0.9 on FreeBSD 5, having no previous configuration
files lying around, I get lockups easily (before any windows are shown or all
components have finished initializing) due to what seems to be corruption of
thread-related data.  Using gdb I found that some per-thread data was corrupt
and causing the lock-ups inside the new gconf code.

I also used valgrind to check out the corruption and saw that lo and behold,
pthread_mutex_lock() was being called, from code that used the gthread support
library, and using a NULL mutex -- g_thread_init() was never called.  Fixing
this made things work.  In particular, I think the thread-local-storage calls
made by glib internals into gthread all do the wrong thing, and of course
mutexes never get allocated or used.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.




I was able to fix this by adding libglib specifically to the libraries that
firefox-bin linked with and (after including <glib.h#gt;), calling
g_thread_init() just before the gtk/etc. initialization calls in xre_main(). 
Note that this is only specifically necessary for GTK2, but this is worth
investigating against GTK1 as well.
Comment 1 green 2004-06-17 09:27:33 PDT
Created attachment 151037 [details] [diff] [review]
proposed patch to start libgthread correctly

The other part of this is that libgthread needs to be added to the list of
libraries linked in by {firefox-bin,mozilla-bin,whatever-bin}, just like glib
is now.
Comment 2 Christian :Biesinger (don't email me, ping me on IRC) 2004-06-17 12:29:55 PDT
can you also patch the mozilla version of this file?
(xpfe/bootstrap/nsAppRunner.cpp)

+#ifdef MOZ_WIDGET_GTK2
+#include <glib.h>
+#endif

I'm not sure that an ifdef here is needed; I mean, gtk1 also depends on xlib...

Comment 3 green 2004-06-18 18:25:31 PDT
Does gtk1 do any thread-related calls?  In any case, I don't think it has the
possibility of harming any platforms to initialize gthread if any form of
gtk/gconf stuff is included.
Comment 4 Gervase Markham [:gerv] 2005-09-27 02:07:08 PDT
This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/
Comment 5 Gervase Markham [:gerv] 2005-10-13 10:35:45 PDT
This bug has been automatically resolved after a period of inactivity (see above
comment). If anyone thinks this is incorrect, they should feel free to reopen it.
Comment 6 Walter Meinl 2007-05-28 03:31:06 PDT
Unfortunately this bug slept in before a commit. I want to reopen (but cannot) because new glib-2.13 (and what I found this won't be changed in 2.14) throws a warning
***MEMORY-WARNING***: firefox-bin[9414]: GSlice: g_thread_init() must be called before all other GLib functions; memory corruption due to late invocation of g_thread_init() has been detected; this program is likely to crash, leak or unexpectedly abort soon...
Now, following the thread on the gtk-devel mailing list
http://mail.gnome.org/archives/gtk-devel-list/2007-January/msg00005.html
there was a big discussion if the changes in gslice to throw this warning was breaking glib ABI. However, "this is not the case, because the requirement has
always been there, it just wasn't as strictly enforced." (glib developer Tim Janik) in the mailing-list thread mentioned above.
Further investigating this, I found this bug here and when I apply the patch (updated of course) the warning goes away.
Comment 7 Walter Meinl 2007-05-28 03:32:21 PDT
Created attachment 266366 [details] [diff] [review]
the patch from the original poster updated to the tip
Comment 8 Walter Meinl 2007-05-29 11:03:58 PDT
Created attachment 266488 [details] [diff] [review]
patch for seamonkey xpfe Makefile.in and nsAppRunner.cpp

As per comment #2 the same patch for seamonkey
Comment 9 Benjamin Smedberg [:bsmedberg] 2007-05-31 11:06:13 PDT
Comment on attachment 266366 [details] [diff] [review]
the patch from the original poster updated to the tip

Making a wild stab at an appropriate reviewer.
Comment 10 Benjamin Smedberg [:bsmedberg] 2007-05-31 11:06:41 PDT
Comment on attachment 266488 [details] [diff] [review]
patch for seamonkey xpfe Makefile.in and nsAppRunner.cpp

xpfe is no more!
Comment 11 Walter Meinl 2007-06-01 16:26:40 PDT
roc, benjamin, my bad the linkage won't work with static builds. When firefox-bin gets linked it fails due to an undefined reference to g_thread_init in nsAppRunner.o
I'll attach a better patch soon, so please don't check attachment 266366 [details] [diff] [review] in for now. Sorry to have bothered you
Comment 12 Walter Meinl 2007-06-02 02:01:24 PDT
Created attachment 266991 [details] [diff] [review]
additional patch for static-config.mk fixes also startup-notification

Tested all with enable-static and enable-shared, I figured out that  attachment 266366 [details] [diff] [review] is ok, but we need to patch also config/static-config.mk for enable-static.
roc, when I tried to build static I stumbled additionally over a build failure when I enabled startup-notification. It has also to be added to static-config.mk to build the static binary. The patch here would include this. As you were also reviewer in bug 223492 we could handle this trivial change w/o opening a new bug?
Comment 13 Walter Meinl 2007-06-06 23:15:01 PDT
Sorry no permit for check-ins, could someone do it for me? thanks
Comment 14 Walter Meinl 2007-06-11 00:09:59 PDT
Created attachment 267938 [details] [diff] [review]
updated first patch after removal of qt

the removal of qt made it necessary to update the patch of toolkit/library/Makefile.in
this changes also
-ifneq (,$(filter gtk2,$(MOZ_WIDGET_TOOLKIT)))
+ifeq (gtk2,$(MOZ_WIDGET_TOOLKIT))
as gtk2 is now the only toolkit we want to check at this place
I felt free to take over the r+ as substantially nothing was changed
Comment 15 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-06-11 00:15:08 PDT
Should you use pkg-config instead of hard-coding -lgthread-2.0 ?
Comment 16 Walter Meinl 2007-06-11 12:33:13 PDT
(In reply to comment #15)
> Should you use pkg-config instead of hard-coding -lgthread-2.0 ?
> 

That was my first thought, e.g. append it to the check for MOZ_ENABLE_GTK2 as we check there for gdk-x11-2.0 glib-2.0 gobject-2.0 anyway. I tried this and it works as well. But wouldn't it then be used in any linkage against the gtk2 libs?
gthread is also pulled in when you build with GNOME support. However that doesn't help as the glib memory warning comes also when you disable GNOME support, ie gnome-vfs and friends.
The reason I've chosen to hardcode -lgthread-2.0 here is that even if you disable GNOME support its needed to link libxul.so
Anyway, if any of the other alternatives is better, I can adjust the patch
Comment 17 Walter Meinl 2007-06-14 12:22:01 PDT
Created attachment 268400 [details] [diff] [review]
bitrot of the static patch (removal of xprint) taking over r+
Comment 18 Kenneth Herron 2007-06-17 06:02:52 PDT
"bitrot of the static patch (removal of xprint)" patch checked in. Clearing checkin-needed status.
Comment 19 Walter Meinl 2007-06-17 08:56:12 PDT
(In reply to comment #18)
> "bitrot of the static patch (removal of xprint)" patch checked in. Clearing
> checkin-needed status.
> 

Kenneth, could you please check in also
https://bugzilla.mozilla.org/attachment.cgi?id=267938
updated first patch after removal of qt
this is needed as well, thanks
Comment 20 Kenneth Herron 2007-06-17 09:02:20 PDT
Okay, checked in the "updated first patch after removal of qt".
Comment 21 Walter Meinl 2007-07-01 00:43:53 PDT
Created attachment 270473 [details] [diff] [review]
corresponding patch for the branch

The trunk patch sits now for some weeks w/o regression. Given the fact that the glib-2.14 release is coming closer (now at 2.13.6), I'd suggest to fix also the 1.8 Branch. Otherwise the memory warning (see comment #6) that's thrown in a terminal and also can be found in .xsession-errors will likely lead to several bug filings.
The situation in the branch is a little bit different as we build no libxul.so (except for xulrunner), so we have to fix the Makefiles for each application.
Comment 22 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-07-02 11:20:01 PDT
Resolving FIXED, given that this is fixed on the trunk.
Comment 23 Daniel Veditz [:dveditz] 2007-07-03 10:49:11 PDT
Comment on attachment 270473 [details] [diff] [review]
corresponding patch for the branch

approved for 1.8.1.5, a=dveditz for release-drivers
Comment 24 Walter Meinl 2007-07-04 08:36:18 PDT
Help is needed for check-in of the branch patch https://bugzilla.mozilla.org/attachment.cgi?id=270473
Thanks
Comment 25 Daniel Veditz [:dveditz] 2007-07-11 17:01:15 PDT
Comment on attachment 270473 [details] [diff] [review]
corresponding patch for the branch

tree closing early for firedrill, this will have to wait for the next release.

I recommend finding check-in buddies on irc.mozilla.org, relying on folks to notice bug comments amid the incoming deluge is not reliable.
Comment 26 Walter Meinl 2007-07-11 22:50:40 PDT
(In reply to comment #25)
> (From update of attachment 270473 [details] [diff] [review])
> tree closing early for firedrill, this will have to wait for the next release.
> 
> I recommend finding check-in buddies on irc.mozilla.org, relying on folks to
> notice bug comments amid the incoming deluge is not reliable.
> 
Daniel, I asked on irc.mozilla.org immedately after getting your mail that I should hurry to get this in.On #developers I was told to change the keywords to checkin-needed and a day later in the MDC news was posted
http://developer.mozilla.org/devnews/index.php/2007/07/10/getting-a-patch-checked-in/
I don't want to blame anybody here, cause this is not the most important bug, but I feel a little bit disappointed, because I did what I was told and was obviously to shy on getting this in.
Comment 27 Phil Ringnalda (:philor) 2007-08-16 22:34:58 PDT
Clearing checkin-needed until 1.8.1.7 triage gets around to re-approving.

Yeah, it can be a miserable pain trying to get patches landed: to get any action in #developers you have to very subtly get across that people have a responsibility to not discourage contributors, and that the whole "the Linux community should fix that" only works if they actually check in the Linux community's patches, but you have to avoid crossing over into being a nuisance. Hard enough to balance on that fine line in your first language. If you don't get any action once you get re-approved, feel free to email me directly to remind me.
Comment 28 Daniel Veditz [:dveditz] 2007-08-29 16:10:52 PDT
Comment on attachment 270473 [details] [diff] [review]
corresponding patch for the branch

approved for 1.8.1.7, a=dveditz for release-drivers

Please aim to land early in the cycle so this doesn't get cut off again.
Comment 29 Mats Palmgren (:mats) 2007-08-30 19:42:53 PDT
Landed the branch patch on MOZILLA_1_8_BRANCH

mozilla/toolkit/xre/nsAppRunner.cpp 	1.113.2.22
mozilla/browser/app/Makefile.in 	1.85.2.11
mozilla/calendar/sunbird/app/Makefile.in 	1.21.2.20
mozilla/mail/app/Makefile.in 	1.46.2.7
mozilla/toolkit/library/Makefile.in 	1.16.2.10
mozilla/xpfe/bootstrap/Makefile.in 	1.283.2.5
mozilla/xpfe/bootstrap/nsAppRunner.cpp 	1.442.2.10 
Comment 30 Walter Meinl 2007-09-03 08:00:32 PDT
thanks for helping

Note You need to log in before you can comment on or make changes to this bug.