Bug 36832

Summary:

[Qt] crash in debug mode just before exit

Product:

WebKit

Reporter:

Balazs Kelemen <kbalazs>

Component:

WebKit Qt

Assignee:

Nobody <webkit-unassigned>

Status:

CLOSED FIXED

Severity:

Normal

CC:

agbakken, commit-queue, hausmann, kenneth, koivisto, koshuin, laszlo.gombos, yael

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

Linux

Bug Depends on:

Bug Blocks:

35784

Attachments:

Description	Flags
backtrace	none
proposed patch	none
proposed patch	none
proposed patch (hopefully final)	none
proposed patch	none

Balazs Kelemen

Reported 2010-03-30 09:32:36 PDT

My test case was the following: starting QtLauncher (a debug build), navigating to wave.google.com, sign in, wait for a little but do not wait until it could load, and close the window (it can be necessary because in that case there are living timers that has not been fired). Unfortunately, it is not easily reproducible. Actually, I started QtLauncher inside gdb, but I cannot say it is always reproducible even with gdb. If I remember correctly I also saw this problem inside valgrind before (what made me angry since valgrind is unusable for leak hunting when the application do not exits normally). From the backtrace I realized that isMainThread() is called during the destruction of the QCoreApplication inside an ASSERT. In that case it crashes since it tries to get the thread id of the QCoreApplication (not the ASSERT crashed but the execution of it).

Attachments
backtrace (1.37 KB, application/octet-stream) 2010-03-30 09:34 PDT, Balazs Kelemen	no flags	Details
proposed patch (2.55 KB, patch) 2010-03-30 09:57 PDT, Balazs Kelemen	no flags	Details Formatted Diff Diff
proposed patch (2.04 KB, patch) 2010-03-30 16:52 PDT, Balazs Kelemen	no flags	Details Formatted Diff Diff
proposed patch (hopefully final) (1.51 KB, patch) 2010-04-02 05:15 PDT, Balazs Kelemen	no flags	Details Formatted Diff Diff
proposed patch (2.49 KB, patch) 2010-04-19 07:38 PDT, Balazs Kelemen	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Balazs Kelemen

Comment 1 2010-03-30 09:34:06 PDT

Created attachment 52048 [details] backtrace

Balazs Kelemen

Comment 2 2010-03-30 09:57:43 PDT

Created attachment 52049 [details] proposed patch

Eric Seidel (no email)

Comment 3 2010-03-30 16:09:45 PDT

Comment on attachment 52049 [details] proposed patch Why don't we save off the thread id of the main thread in a global static instead? I would look at other implementations including the posix threading implementation before making this change.

Eric Seidel (no email)

Comment 4 2010-03-30 16:15:40 PDT

Balazs Kelemen

Comment 5 2010-03-30 16:20:30 PDT

Ok, what have I done is horribly overcomplicated, so I remove review flag and start to thinking about a more appropriate solution.

Balazs Kelemen

Comment 6 2010-03-30 16:52:11 PDT

Created attachment 52100 [details] proposed patch

Kenneth Rohde Christiansen

Comment 7 2010-03-31 05:43:51 PDT

Comment on attachment 52100 [details] proposed patch I would suggest adding at least a comment.

Balazs Kelemen

Comment 8 2010-03-31 08:14:25 PDT

Actually this will be the most straightforward: bool isMainThread() { static QThread* mainThread = QCoreApplication::instance()->thread(); return QThread::currentThread() == mainThread; } , but I am not sure that using a static is thread safe at all platforms. What do you think?

Eric Seidel (no email)

Comment 9 2010-04-01 16:44:52 PDT

Comment on attachment 52100 [details] proposed patch This is definitely better. Why do we need both mainThread and mainThreadIdentifier?

Yael

Comment 10 2010-04-01 17:08:14 PDT

This seems like the same issue I was talking about on IRC and in https://bugs.webkit.org/show_bug.cgi?id=35251#c5. All you need to do to get this crash is load www.cnn.com and exit QtLauncher. The cause of the crash is that SharedTimerQt 's parent is QCoreApplication and I think this is wrong. (dll depends on the application that loaded it)

Balazs Kelemen

Comment 11 2010-04-02 05:10:50 PDT

(In reply to comment #10) > This seems like the same issue I was talking about on IRC and in > https://bugs.webkit.org/show_bug.cgi?id=35251#c5. > All you need to do to get this crash is load www.cnn.com and exit QtLauncher. > The cause of the crash is that SharedTimerQt 's parent is QCoreApplication and > I think this is wrong. (dll depends on the application that loaded it) As I understand the 2 bug are not related to each other. QCoreApplication is needed to be the parent of SharedTimerQt if we want to do final cleanup (not just memory deallocation, but flushing to disk, etc).

Balazs Kelemen

Comment 12 2010-04-02 05:15:08 PDT

Created attachment 52408 [details] proposed patch (hopefully final) So, let's trust in c++ (and all of it's implementation) and solve this with a static :)

Yael

Comment 13 2010-04-02 06:15:08 PDT

(In reply to comment #11) > (In reply to comment #10) > As I understand the 2 bug are not related to each other. > QCoreApplication is needed to be the parent of SharedTimerQt if we want to do > final cleanup (not just memory deallocation, but flushing to disk, etc). The bugs are not related. Only comment #5 is.

Janne Koskinen

Comment 14 2010-04-07 05:41:17 PDT

I fixed similar issue on Symbian builds few weeks back https://bugs.webkit.org/show_bug.cgi?id=34081 The patch is ok from my PoV if it is only the ASSERT that this gets away with. what worries me is this line: WebCore::JSDOMWindowBase::commonJSGlobalData () Reason why you are seeing this crash is TLS getting destroyed at main() exit and then getting reconstructed at the time of timer trigger. I remember we had same issue with Symbian builds at some point. My fix at then was to cancel all timers at d'tor instead of firing them (the right thing to do), but there was some reason why this wasn't accepted. Problem I think still exists in Symbian, but there is now no use-case that would bring it to surface.

Simon Hausmann

Comment 15 2010-04-16 17:25:18 PDT

See also bug 37521 for an alternate solution.

Balazs Kelemen

Comment 16 2010-04-19 07:38:36 PDT

Created attachment 53678 [details] proposed patch Destruct the shared timer instance and firing all timers last time before ~QApplication, as suggested by Yael and Zoltan (Zoltan did that in the other bug).

Kenneth Rohde Christiansen

Comment 17 2010-04-19 07:47:59 PDT

Comment on attachment 53678 [details] proposed patch Looks good to me, but why was the parent removed? You could add a comment about that in the ChangeLog. Also I would keeop the changelog lines shorter

Simon Hausmann

Comment 18 2010-04-19 08:01:36 PDT

I like this patch, too! :)

Simon Hausmann

Comment 19 2010-04-19 08:03:46 PDT

(In reply to comment #17) > (From update of attachment 53678 [details]) > Looks good to me, but why was the parent removed? You could add a comment about > that in the ChangeLog. Also I would keeop the changelog lines shorter The connection achieves what the parent relationship tried to achieve earlier. But when the QApplication destructor ran it was too late.

WebKit Commit Bot

Comment 20 2010-04-19 11:01:07 PDT

Comment on attachment 53678 [details] proposed patch Clearing flags on attachment: 53678 Committed r57818: <http://trac.webkit.org/changeset/57818>

WebKit Commit Bot

Comment 21 2010-04-19 11:01:13 PDT

All reviewed patches have been landed. Closing bug.

Simon Hausmann

Comment 22 2010-04-19 17:45:45 PDT

*** Bug 37521 has been marked as a duplicate of this bug. ***

Simon Hausmann

Comment 23 2010-04-20 12:15:19 PDT

Revision r57818 cherry-picked into qtwebkit-2.0 with commit 95c4d11167cff1a8fd1e124e4655fedc0b00944a

Simon Hausmann

Comment 24 2010-06-04 00:52:35 PDT

Revision r57818 cherry-picked into qtwebkit-4.6 with commit d6d6c3821ed111b214a753f1ce8fa969c1a94dc3

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