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

VERIFIED FIXED

Status

Core Graveyard
GFX: Gtk
--
major
VERIFIED FIXED
13 years ago
9 years ago

People

(Reporter: green, Assigned: Walter Meinl)

Tracking

({fixed1.8.1.8})

Trunk
x86
Linux
fixed1.8.1.8

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

13 years ago
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.
(Reporter)

Updated

13 years ago
Keywords: hang
(Reporter)

Comment 1

13 years ago
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.
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...

(Reporter)

Comment 3

13 years ago
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.
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/
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.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → EXPIRED
(Assignee)

Comment 6

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

Comment 7

10 years ago
Created attachment 266366 [details] [diff] [review]
the patch from the original poster updated to the tip

Updated

10 years ago
Attachment #266366 - Flags: review?(benjamin)
(Assignee)

Comment 8

10 years ago
Created attachment 266488 [details] [diff] [review]
patch for seamonkey xpfe Makefile.in and nsAppRunner.cpp

As per comment #2 the same patch for seamonkey
Attachment #266488 - Flags: review?(benjamin)

Updated

10 years ago
Status: RESOLVED → UNCONFIRMED
Resolution: EXPIRED → ---

Comment 9

10 years ago
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.
Attachment #266366 - Flags: review?(benjamin) → review?(roc)

Comment 10

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

xpfe is no more!
Attachment #266488 - Flags: review?(benjamin)
Attachment #266366 - Flags: superreview+
Attachment #266366 - Flags: review?(roc)
Attachment #266366 - Flags: review+

Updated

10 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [checkin needed]
(Assignee)

Comment 11

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

Comment 12

10 years ago
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?
Attachment #266488 - Attachment is obsolete: true
Attachment #266991 - Flags: superreview?
Attachment #266991 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #266991 - Flags: superreview?(roc)
Attachment #266991 - Flags: superreview?
Attachment #266991 - Flags: review?(roc)
Attachment #266991 - Flags: review?
Attachment #266991 - Flags: superreview?(roc)
Attachment #266991 - Flags: superreview+
Attachment #266991 - Flags: review?(roc)
Attachment #266991 - Flags: review+
(Assignee)

Comment 13

10 years ago
Sorry no permit for check-ins, could someone do it for me? thanks
(Assignee)

Comment 14

10 years ago
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
Attachment #267938 - Flags: review+
Should you use pkg-config instead of hard-coding -lgthread-2.0 ?
(Assignee)

Comment 16

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

Comment 17

10 years ago
Created attachment 268400 [details] [diff] [review]
bitrot of the static patch (removal of xprint) taking over r+
Attachment #268400 - Flags: review+

Comment 18

10 years ago
"bitrot of the static patch (removal of xprint)" patch checked in. Clearing checkin-needed status.
Whiteboard: [checkin needed]
(Assignee)

Comment 19

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

10 years ago
Okay, checked in the "updated first patch after removal of qt".
(Assignee)

Comment 21

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

Updated

10 years ago
Attachment #270473 - Flags: superreview?(roc)
Attachment #270473 - Flags: review?(roc)
Attachment #270473 - Flags: superreview?(roc)
Attachment #270473 - Flags: superreview+
Attachment #270473 - Flags: review?(roc)
Attachment #270473 - Flags: review+
(Assignee)

Updated

10 years ago
Attachment #270473 - Flags: approval1.8.1.5?
Assignee: blizzard → wuno
Resolving FIXED, given that this is fixed on the trunk.
Status: NEW → RESOLVED
Last Resolved: 12 years ago10 years ago
Resolution: --- → FIXED
Comment on attachment 270473 [details] [diff] [review]
corresponding patch for the branch

approved for 1.8.1.5, a=dveditz for release-drivers
Attachment #270473 - Flags: approval1.8.1.5? → approval1.8.1.5+
(Assignee)

Comment 24

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

Updated

10 years ago
Keywords: hang → checkin-needed
Whiteboard: 1.8.1_BRANCH
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.
Attachment #270473 - Flags: approval1.8.1.6?
Attachment #270473 - Flags: approval1.8.1.5-
Attachment #270473 - Flags: approval1.8.1.5+
(Assignee)

Comment 26

10 years ago
(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.
Severity: critical → major
OS: FreeBSD → Linux
Whiteboard: 1.8.1_BRANCH → 1.8_BRANCH
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.
Keywords: checkin-needed
Whiteboard: 1.8_BRANCH
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.
Attachment #270473 - Flags: approval1.8.1.7? → approval1.8.1.7+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Whiteboard: MOZILLA_1_8_BRANCH
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 
Keywords: checkin-needed → fixed1.8.1.7
(Assignee)

Comment 30

10 years ago
thanks for helping
Whiteboard: MOZILLA_1_8_BRANCH
(Assignee)

Updated

10 years ago
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.