247204 - lock up at start when using threaded GTK2 libraries (i.e. gconf)

Status: VERIFIED FIXED

(Core Graveyard :: GFX: Gtk, defect)

Description

[green]

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]

Attachment: 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)]

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]

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]]

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]]

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]

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]

Attachment: the patch from the original poster updated to the tip

Comment 8

[Walter Meinl]

Attachment: patch for seamonkey xpfe Makefile.in and nsAppRunner.cpp

As per comment #2 the same patch for seamonkey

Comment 9

[Benjamin Smedberg]

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]

Comment on attachment 266488 [details] [diff] [review]
patch for seamonkey xpfe Makefile.in and nsAppRunner.cpp

xpfe is no more!

Comment 11

[Walter Meinl]

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]

Attachment: 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]

Sorry no permit for check-ins, could someone do it for me? thanks

Comment 14

[Walter Meinl]

Attachment: 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-4, no longer working on Mozilla)]

Should you use pkg-config instead of hard-coding -lgthread-2.0 ?

Comment 16

[Walter Meinl]

(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]

Attachment: bitrot of the static patch (removal of xprint) taking over r+

Comment 18

[Kenneth Herron]

"bitrot of the static patch (removal of xprint)" patch checked in. Clearing checkin-needed status.

Comment 19

[Walter Meinl]

(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]

Okay, checked in the "updated first patch after removal of qt".

Comment 21

[Walter Meinl]

Attachment: 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: [e-mail]]]

Resolving FIXED, given that this is fixed on the trunk.

Comment 23

[Daniel Veditz [:dveditz]]

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]

Help is needed for check-in of the branch patch https://bugzilla.mozilla.org/attachment.cgi?id=270473
Thanks

Comment 25

[Daniel Veditz [:dveditz]]

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]

(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)]

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]]

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 (inactive)]

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]

thanks for helping