[Qt] crash in debug mode just before exit

• Component: WebKit Qt
• Status: CLOSED FIXED

[Balazs Kelemen]

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).

[Balazs Kelemen]

Created attachment 52048 [details]
backtrace

[Balazs Kelemen]

Created attachment 52049 [details]
proposed patch

[Eric Seidel (no email)]

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

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

[Balazs Kelemen]

Created attachment 52100 [details]
proposed patch

[Kenneth Rohde Christiansen]

Comment on attachment 52100 [details]
proposed patch

I would suggest adding at least a comment.

[Balazs Kelemen]

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 on attachment 52100 [details]
proposed patch

This is definitely better.  Why do we need both mainThread and mainThreadIdentifier?

[Yael]

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]

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

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]

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

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]

See also bug 37521 for an alternate solution.

[Balazs Kelemen]

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

I like this patch, too! :)

[Simon Hausmann]

(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 on attachment 53678 [details]
proposed patch

Clearing flags on attachment: 53678

Committed r57818: <http://trac.webkit.org/changeset/57818>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

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

[Simon Hausmann]

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

[Simon Hausmann]

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