Bug 37521

Summary: [Qt] QtWebKit crash on shutdown
Product: WebKit Reporter: Anders Bakken <agbakken>
Component: WebKit QtAssignee: Anders Bakken <agbakken>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: hausmann, kbalazs, koshuin, laszlo.gombos, yael, zherczeg
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
patch that fixes the crash hausmann: review-, hausmann: commit-queue-

Anders Bakken
Reported 2010-04-13 14:07:30 PDT
Created attachment 53278 [details] patch that fixes the crash I get the following crash on shutdown of a Qt webkit application: ==8808== Invalid read of size 8 ==8808== at 0x9AF2E90: QObject::thread() const (qobject.cpp:1409) ==8808== by 0x72801BD: WTF::isMainThread() (ThreadingQt.cpp:220) ==8808== by 0x66C7ED5: WebCore::JSDOMWindowBase::commonJSGlobalData() (JSDOMWindowBase.cpp:155) ==8808== by 0x66AF816: WebCore::collect(void*) (GCController.cpp:46) ==8808== by 0x66AFADF: WebCore::GCController::gcTimerFired(WebCore::Timer<WebCore::GCController>*) (GCController.cpp:69) ==8808== by 0x66AFC33: WebCore::Timer<WebCore::GCController>::fired() (Timer.h:98) ==8808== by 0x6CA0A53: WebCore::ThreadTimers::sharedTimerFiredInternal() (ThreadTimers.cpp:112) ==8808== by 0x6CA0986: WebCore::ThreadTimers::sharedTimerFired() (ThreadTimers.cpp:90) ==8808== by 0x6E73E36: WebCore::SharedTimerQt::~SharedTimerQt() (SharedTimerQt.cpp:68) ==8808== by 0x9AF59D6: QObjectPrivate::deleteChildren() (qobject.cpp:1972) ==8808== by 0x9AF8BFB: QObject::~QObject() (qobject.cpp:969) ==8808== by 0x8E7DCB3: QApplication::~QApplication() (qapplication.cpp:1138) ==8808== Address 0x8 is not stack'd, malloc'd or (recently) free'd ==8808== ==8808== ==8808== Process terminating with default action of signal 11 (SIGSEGV): dumping core ==8808== Access not within mapped region at address 0x8 ==8808== at 0x9AF2E90: QObject::thread() const (qobject.cpp:1409) ==8808== by 0x72801BD: WTF::isMainThread() (ThreadingQt.cpp:220) ==8808== by 0x66C7ED5: WebCore::JSDOMWindowBase::commonJSGlobalData() (JSDOMWindowBase.cpp:155) ==8808== by 0x66AF816: WebCore::collect(void*) (GCController.cpp:46) ==8808== by 0x66AFADF: WebCore::GCController::gcTimerFired(WebCore::Timer<WebCore::GCController>*) (GCController.cpp:69) ==8808== by 0x66AFC33: WebCore::Timer<WebCore::GCController>::fired() (Timer.h:98) ==8808== by 0x6CA0A53: WebCore::ThreadTimers::sharedTimerFiredInternal() (ThreadTimers.cpp:112) ==8808== by 0x6CA0986: WebCore::ThreadTimers::sharedTimerFired() (ThreadTimers.cpp:90) ==8808== by 0x6E73E36: WebCore::SharedTimerQt::~SharedTimerQt() (SharedTimerQt.cpp:68) ==8808== by 0x9AF59D6: QObjectPrivate::deleteChildren() (qobject.cpp:1972) ==8808== by 0x9AF8BFB: QObject::~QObject() (qobject.cpp:969) ==8808== by 0x8E7DCB3: QApplication::~QApplication() (qapplication.cpp:1138) It reproduces pretty much every time but the example is rather involved and includes adding some code to WebKit that may or may not be important for reproducing the problem. Let me know if you want to see the example. It seems like the problem is that isMainThread is called after QCoreApplication is deleted. I've attached a patch that fixes it but I am not 100% about the following: Could this thread be called in one thread while QCoreApplication is being deleted in another? If so, one would have to do some more thread-synchronization for this to be safe. Should it return true or false if QCoreApplication doesn't exist?
Attachments
patch that fixes the crash (1.06 KB, patch)
2010-04-13 14:07 PDT, Anders Bakken 		hausmann: review-
hausmann: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Zoltan Herczeg
Comment 1 2010-04-15 05:04:15 PDT
Is it a valgrind output? Am I see right that QApplication destructor fires the shared timer system of WebCore? I am wondering the possible side effects of such call. Perhaps it would be better to delete the SharedTimer just before the destructor of QApplication...
Anders Bakken
Comment 2 2010-04-15 10:34:12 PDT
(In reply to comment #1) > Is it a valgrind output? > > Am I see right that QApplication destructor fires the shared timer system of > WebCore? I am wondering the possible side effects of such call. Perhaps it > would be better to delete the SharedTimer just before the destructor of > QApplication... Hi Zoltan It is valgrind. I can't get gdb to run against a debug version of WebKit. I have to admit I wouldn't know about your proposed fix. I just saw the error and found a simple way to fix it. Maybe someone who knows the details better could comment. regards Anders
Simon Hausmann
Comment 3 2010-04-16 17:24:40 PDT
This is technically a duplicate of 36832, but I admit I like Anders' solution
Simon Hausmann
Comment 4 2010-04-16 17:24:59 PDT
I meant bug 36832
Zoltan Herczeg
Comment 5 2010-04-17 02:35:32 PDT
As far as I see this is an assertion fail, so only debug builds may crash. And the poblem is not there I think. Actually a half destroyed QApplication fires all timers, and a timer can do practically anything in WebKit, like it can also access/changes the members of this QApplication. This is unsafe. I feel this fix hides the issue, not solve it. Anyway it is just my opinion, nothing more.
Balazs Kelemen
Comment 6 2010-04-17 17:09:09 PDT
(In reply to comment #1) > Is it a valgrind output? > > Am I see right that QApplication destructor fires the shared timer system of > WebCore? I am wondering the possible side effects of such call. Perhaps it > would be better to delete the SharedTimer just before the destructor of > QApplication... Yep, this is exactly what happens here. I think we can achieve the behavior what you mentioned by installing a self-destroying slot for QApplication::aboutToQuit() into the SharedTimerQt instance (and make it independent from the QApplication). I will try it! :)
Simon Hausmann
Comment 7 2010-04-19 17:45:45 PDT
*** This bug has been marked as a duplicate of bug 36832 ***
Simon Hausmann
Comment 8 2010-04-19 17:49:10 PDT
Comment on attachment 53278 [details] patch that fixes the crash r- as http://trac.webkit.org/changeset/57818 was landed
Note You need to log in before you can comment on or make changes to this bug.