WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
CLOSED FIXED
36832
[Qt] crash in debug mode just before exit
https://bugs.webkit.org/show_bug.cgi?id=36832
Summary
[Qt] crash in debug mode just before exit
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
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.
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.
Top of Page
Format For Printing
XML
Clone This Bug