72392 2011-11-15 09:45:22 -0800 [Qt] [WK2] Crash in Connection::readyReadHandler() on socket error 2011-11-16 08:37:51 -0800 1 1 1 Unclassified WebKit WebKit Qt 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED Qt P3 Normal --- 1 ostap73 webkit-unassigned hausmann kimmo.t.kinnunen laszlo.gombos webkit.review.bot oldest_to_newest 502984 0 ostap73 2011-11-15 09:45:22 -0800 Crash happens randomly on webprocess exit. Call stack: 0 QSocketNotifier::setEnabled qsocketnotifier.cpp 259 0xb2ff1b6d 1 CoreIPC::SocketNotifierResourceGuard::~SocketNotifierResourceGuard ConnectionUnix.cpp 170 0xb53d6803 2 CoreIPC::Connection::readyReadHandler ConnectionUnix.cpp 370 0xb53d754a 3 MemberFunctionWorkItem0<CoreIPC::Connection>::execute WorkItem.h 79 0xb53d6081 4 WorkQueue::WorkItemQt::execute WorkQueueQt.cpp 65 0xb53dd2e2 5 WorkQueue::WorkItemQt::qt_static_metacall WorkQueueQt.moc 50 0xb53dd97b 6 QMetaCallEvent::placeMetaCall qobject.cpp 529 0xb2fe4872 7 QObject::event qobject.cpp 1111 0xb2fe5493 8 QApplicationPrivate::notify_helper qapplication.cpp 4080 0xb3d98486 9 QApplication::notify qapplication.cpp 3497 0xb3d95e87 10 QCoreApplication::notifyInternal qcoreapplication.cpp 830 0xb2fc52ae 11 QCoreApplication::sendEvent qcoreapplication.h 205 0xb53583fb 12 QCoreApplicationPrivate::sendPostedEvents qcoreapplication.cpp 1472 0xb2fc6193 13 QEventDispatcherUNIX::processEvents qeventdispatcher_unix.cpp 908 0xb300e1cd 14 QEventLoop::processEvents qeventloop.cpp 149 0xb2fc2dd5 15 QEventLoop::exec qeventloop.cpp 225 0xb2fc3036 16 QThread::exec qthread.cpp 495 0xb2ea7b3b 17 QThread::run qthread.cpp 562 0xb2ea7cbb 18 QThreadPrivate::start qthread_unix.cpp 298 0xb2ea9db7 19 start_thread /lib/i386-linux-gnu/libpthread.so.0 0 0xb2d2bd31 20 clone /lib/i386-linux-gnu/libc.so.6 0 0xb4cb20ce What happens here: 1. socketNotifierEnabler keep hold on m_socketNotifier until Connection::readyReadHandler() runs and stores m_socketNotifier pointer internally. 2. If socket read error occurs connectionDidClose get called, which calls platformInvalidate and m_socketNotifier gets deleted. 3. On exit socketNotifierEnabler tries to call setEnabled on already deleted m_socketNotifier instance using internally stored pointer. Probably that the reason of crash Kimmo mentions here as race condition: https://bugs.webkit.org/show_bug.cgi?id=54938 503005 1 115188 ostap73 2011-11-15 10:08:05 -0800 Created attachment 115188 Move connectionDidClose() out of scope of socketNotifierEnabler. 503715 2 115188 hausmann 2011-11-16 01:51:36 -0800 Comment on attachment 115188 Move connectionDidClose() out of scope of socketNotifierEnabler. I'll say r+ because this fixes a real bug. But I would actually prefer a different solution to the problem: Instead of working around the fact that SocketNotifierResourceGuard would contain a dangling pointer by changing the scoping, I'd rather see a change that make SocketNotifierResourceGuard robust against this problem, by changing the QSocketNotifier pointer to be a QWeakPointer and checking it in the destructor. It's easy to introduce these kind of problems, and a more robust guard class makes it harder to do so. What do you think? 503732 3 hausmann 2011-11-16 02:38:06 -0800 This is what I have in mind: diff --git a/Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp b/Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp index 5a61ed6..29acf6c 100644 --- a/Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp +++ b/Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp @@ -40,6 +40,7 @@ #if PLATFORM(QT) #include <QSocketNotifier> +#include <QWeakPointer> #elif PLATFORM(GTK) #include <glib.h> #endif @@ -162,16 +163,17 @@ public: SocketNotifierResourceGuard(QSocketNotifier* socketNotifier) : m_socketNotifier(socketNotifier) { - m_socketNotifier->setEnabled(false); + m_socketNotifier.data()->setEnabled(false); } ~SocketNotifierResourceGuard() { - m_socketNotifier->setEnabled(true); + if (m_socketNotifier) + m_socketNotifier.data()->setEnabled(true); } private: - QSocketNotifier* const m_socketNotifier; + QWeakPointer<QSocketNotifier> const m_socketNotifier; }; #endif 503834 4 ostap73 2011-11-16 06:54:17 -0800 (In reply to comment #3) > This is what I have in mind: > > > diff --git a/Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp b/Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp > index 5a61ed6..29acf6c 100644 Yes, that's better. Thanks a lot. 503856 5 115377 ostap73 2011-11-16 07:20:37 -0800 Created attachment 115377 Add QWeakPointer 503857 6 115377 hausmann 2011-11-16 07:24:50 -0800 Comment on attachment 115377 Add QWeakPointer r=me 503917 7 115377 webkit.review.bot 2011-11-16 08:37:43 -0800 Comment on attachment 115377 Add QWeakPointer Clearing flags on attachment: 115377 Committed r100455: <http://trac.webkit.org/changeset/100455> 503918 8 webkit.review.bot 2011-11-16 08:37:51 -0800 All reviewed patches have been landed. Closing bug. 115188 2011-11-15 10:08:05 -0800 2011-11-16 07:20:37 -0800 Move connectionDidClose() out of scope of socketNotifierEnabler. readyReadHandler_fix.diff text/plain 3542 ostap73 ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCA2NjhjOTgyLi5hZGM2MDk1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUg QEAKKzIwMTEtMTEtMTUgIFZpYXRjaGVzbGF2IE9zdGFwZW5rbyAgPG9zdGFwZW5rby52aWF0Y2hl c2xhdkBub2tpYS5jb20+CisKKyAgICAgICAgW1F0XSBbV0syXSBDcmFzaCBpbiBDb25uZWN0aW9u OjpyZWFkeVJlYWRIYW5kbGVyKCkgb24gc29ja2V0IGVycm9yCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD03MjM5MgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE1vdmUgY29ubmVjdGlvbkRpZENsb3NlKCkgb3V0 IG9mIHNjb3BlIG9mIHNvY2tldE5vdGlmaWVyRW5hYmxlci4KKworICAgICAgICAqIFBsYXRmb3Jt L0NvcmVJUEMvdW5peC9Db25uZWN0aW9uVW5peC5jcHA6CisgICAgICAgIChDb3JlSVBDOjpDb25u ZWN0aW9uOjpyZWFkeVJlYWRIYW5kbGVyKToKKwogMjAxMS0xMS0xNCAgVmlhdGNoZXNsYXYgT3N0 YXBlbmtvICA8b3N0YXBlbmtvLnZpYXRjaGVzbGF2QG5va2lhLmNvbT4KIAogICAgICAgICBJbnRl cm1pdHRlbnQgYXNzZXJ0cyBpbiBUZXh0dXJlTWFwcGVyR0w6OmJpbmRTdXJmYWNlIG9uIE1pbmlC cm93c2VyIHN0YXJ0dXAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL1BsYXRmb3JtL0NvcmVJ UEMvdW5peC9Db25uZWN0aW9uVW5peC5jcHAgYi9Tb3VyY2UvV2ViS2l0Mi9QbGF0Zm9ybS9Db3Jl SVBDL3VuaXgvQ29ubmVjdGlvblVuaXguY3BwCmluZGV4IDVhNjFlZDYuLjYzNzI5NzEgMTAwNjQ0 Ci0tLSBhL1NvdXJjZS9XZWJLaXQyL1BsYXRmb3JtL0NvcmVJUEMvdW5peC9Db25uZWN0aW9uVW5p eC5jcHAKKysrIGIvU291cmNlL1dlYktpdDIvUGxhdGZvcm0vQ29yZUlQQy91bml4L0Nvbm5lY3Rp b25Vbml4LmNwcApAQCAtMzY2LDM5ICszNjYsNDQgQEAgc3RhdGljIHNzaXplX3QgcmVhZEJ5dGVz RnJvbVNvY2tldChpbnQgc29ja2V0RGVzY3JpcHRvciwgdWludDhfdCogYnVmZmVyLCBpbnQgY28K IAogdm9pZCBDb25uZWN0aW9uOjpyZWFkeVJlYWRIYW5kbGVyKCkKIHsKKyAgICBib29sIGNvbm5l Y3Rpb25EaWRDbG9zZUZsYWcgPSBmYWxzZTsKKyAgICB7CiAjaWYgUExBVEZPUk0oUVQpCi0gICAg U29ja2V0Tm90aWZpZXJSZXNvdXJjZUd1YXJkIHNvY2tldE5vdGlmaWVyRW5hYmxlcihtX3NvY2tl dE5vdGlmaWVyKTsKKyAgICAgICAgU29ja2V0Tm90aWZpZXJSZXNvdXJjZUd1YXJkIHNvY2tldE5v dGlmaWVyRW5hYmxlcihtX3NvY2tldE5vdGlmaWVyKTsKICNlbmRpZgogCi0gICAgd2hpbGUgKHRy dWUpIHsKLSAgICAgICAgc2l6ZV90IGZpbGVEZXNjcmlwdG9yc0NvdW50ID0gMDsKLSAgICAgICAg c2l6ZV90IGJ5dGVzVG9SZWFkID0gbV9yZWFkQnVmZmVyLnNpemUoKSAtIG1fcmVhZEJ1ZmZlclNp emU7Ci0gICAgICAgIHNzaXplX3QgYnl0ZXNSZWFkID0gcmVhZEJ5dGVzRnJvbVNvY2tldChtX3Nv Y2tldERlc2NyaXB0b3IsIG1fcmVhZEJ1ZmZlci5kYXRhKCkgKyBtX3JlYWRCdWZmZXJTaXplLCBi eXRlc1RvUmVhZCwKLSAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgIG1fZmlsZURlc2NyaXB0b3JzLmRhdGEoKSArIG1fZmlsZURlc2NyaXB0b3JzU2l6ZSwgJmZp bGVEZXNjcmlwdG9yc0NvdW50KTsKLQotICAgICAgICBpZiAoYnl0ZXNSZWFkIDwgMCkgewotICAg ICAgICAgICAgLy8gRUlOVFIgd2FzIGFscmVhZHkgaGFuZGxlZCBieSByZWFkQnl0ZXNGcm9tU29j a2V0LgotICAgICAgICAgICAgaWYgKGVycm5vID09IEVBR0FJTiB8fCBlcnJubyA9PSBFV09VTERC TE9DSykKLSAgICAgICAgICAgICAgICByZXR1cm47CisgICAgICAgIHdoaWxlICh0cnVlKSB7Cisg ICAgICAgICAgICBzaXplX3QgZmlsZURlc2NyaXB0b3JzQ291bnQgPSAwOworICAgICAgICAgICAg c2l6ZV90IGJ5dGVzVG9SZWFkID0gbV9yZWFkQnVmZmVyLnNpemUoKSAtIG1fcmVhZEJ1ZmZlclNp emU7CisgICAgICAgICAgICBzc2l6ZV90IGJ5dGVzUmVhZCA9IHJlYWRCeXRlc0Zyb21Tb2NrZXQo bV9zb2NrZXREZXNjcmlwdG9yLCBtX3JlYWRCdWZmZXIuZGF0YSgpICsgbV9yZWFkQnVmZmVyU2l6 ZSwgYnl0ZXNUb1JlYWQsCisgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgbV9maWxlRGVzY3JpcHRvcnMuZGF0YSgpICsgbV9maWxlRGVzY3JpcHRvcnNT aXplLCAmZmlsZURlc2NyaXB0b3JzQ291bnQpOwogCi0gICAgICAgICAgICAvLyBGSVhNRTogSGFu ZGxlIG90aGVyIGVycm9ycyBoZXJlPwotICAgICAgICAgICAgcmV0dXJuOwotICAgICAgICB9Cisg ICAgICAgICAgICBpZiAoYnl0ZXNSZWFkIDwgMCkgeworICAgICAgICAgICAgICAgIC8vIEVJTlRS IHdhcyBhbHJlYWR5IGhhbmRsZWQgYnkgcmVhZEJ5dGVzRnJvbVNvY2tldC4KKyAgICAgICAgICAg ICAgICBpZiAoZXJybm8gPT0gRUFHQUlOIHx8IGVycm5vID09IEVXT1VMREJMT0NLKQorICAgICAg ICAgICAgICAgICAgICByZXR1cm47CiAKLSAgICAgICAgbV9yZWFkQnVmZmVyU2l6ZSArPSBieXRl c1JlYWQ7Ci0gICAgICAgIG1fZmlsZURlc2NyaXB0b3JzU2l6ZSArPSBmaWxlRGVzY3JpcHRvcnND b3VudDsKKyAgICAgICAgICAgICAgICAvLyBGSVhNRTogSGFuZGxlIG90aGVyIGVycm9ycyBoZXJl PworICAgICAgICAgICAgICAgIHJldHVybjsKKyAgICAgICAgICAgIH0KIAotICAgICAgICBpZiAo IWJ5dGVzUmVhZCkgewotICAgICAgICAgICAgY29ubmVjdGlvbkRpZENsb3NlKCk7Ci0gICAgICAg ICAgICByZXR1cm47Ci0gICAgICAgIH0KKyAgICAgICAgICAgIG1fcmVhZEJ1ZmZlclNpemUgKz0g Ynl0ZXNSZWFkOworICAgICAgICAgICAgbV9maWxlRGVzY3JpcHRvcnNTaXplICs9IGZpbGVEZXNj cmlwdG9yc0NvdW50OwogCi0gICAgICAgIC8vIFByb2Nlc3MgbWVzc2FnZXMgZnJvbSBkYXRhIHJl Y2VpdmVkLgotICAgICAgICB3aGlsZSAodHJ1ZSkgewotICAgICAgICAgICAgaWYgKCFwcm9jZXNz TWVzc2FnZSgpKQorICAgICAgICAgICAgaWYgKCFieXRlc1JlYWQpIHsKKyAgICAgICAgICAgICAg ICBjb25uZWN0aW9uRGlkQ2xvc2VGbGFnID0gdHJ1ZTsKICAgICAgICAgICAgICAgICBicmVhazsK KyAgICAgICAgICAgIH0KKworICAgICAgICAgICAgLy8gUHJvY2VzcyBtZXNzYWdlcyBmcm9tIGRh dGEgcmVjZWl2ZWQuCisgICAgICAgICAgICB3aGlsZSAodHJ1ZSkgeworICAgICAgICAgICAgICAg IGlmICghcHJvY2Vzc01lc3NhZ2UoKSkKKyAgICAgICAgICAgICAgICAgICAgYnJlYWs7CisgICAg ICAgICAgICB9CiAgICAgICAgIH0KICAgICB9CisgICAgaWYgKGNvbm5lY3Rpb25EaWRDbG9zZUZs YWcpCisgICAgICAgIGNvbm5lY3Rpb25EaWRDbG9zZSgpOwogfQogCiBib29sIENvbm5lY3Rpb246 Om9wZW4oKQo= 115377 2011-11-16 07:20:37 -0800 2011-11-16 08:37:43 -0800 Add QWeakPointer readyReadHandler_fix02.diff text/plain 1905 ostap73 ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCA2NjhjOTgyLi4wZDNjNGNmIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcg QEAKKzIwMTEtMTEtMTYgIFZpYXRjaGVzbGF2IE9zdGFwZW5rbyAgPG9zdGFwZW5rby52aWF0Y2hl c2xhdkBub2tpYS5jb20+CisKKyAgICAgICAgW1F0XSBbV0syXSBDcmFzaCBpbiBDb25uZWN0aW9u OjpyZWFkeVJlYWRIYW5kbGVyKCkgb24gc29ja2V0IGVycm9yCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD03MjM5MgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEFkZCBRV2Vha1BvaW50ZXIgdG8gU29ja2V0Tm90 aWZpZXJSZXNvdXJjZUd1YXJkOjptX3NvY2tldE5vdGlmaWVyIHRvIHRyYWNrCisgICAgICAgIGRl YWxsb2NhdGlvbiBvZiByZWZlcmVuY2VkIG9iamVjdC4KKworICAgICAgICAqIFBsYXRmb3JtL0Nv cmVJUEMvdW5peC9Db25uZWN0aW9uVW5peC5jcHA6CisgICAgICAgIChDb3JlSVBDOjpTb2NrZXRO b3RpZmllclJlc291cmNlR3VhcmQ6OlNvY2tldE5vdGlmaWVyUmVzb3VyY2VHdWFyZCk6CisgICAg ICAgIChDb3JlSVBDOjpTb2NrZXROb3RpZmllclJlc291cmNlR3VhcmQ6On5Tb2NrZXROb3RpZmll clJlc291cmNlR3VhcmQpOgorCiAyMDExLTExLTE0ICBWaWF0Y2hlc2xhdiBPc3RhcGVua28gIDxv c3RhcGVua28udmlhdGNoZXNsYXZAbm9raWEuY29tPgogCiAgICAgICAgIEludGVybWl0dGVudCBh c3NlcnRzIGluIFRleHR1cmVNYXBwZXJHTDo6YmluZFN1cmZhY2Ugb24gTWluaUJyb3dzZXIgc3Rh cnR1cApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdDIvUGxhdGZvcm0vQ29yZUlQQy91bml4L0Nv bm5lY3Rpb25Vbml4LmNwcCBiL1NvdXJjZS9XZWJLaXQyL1BsYXRmb3JtL0NvcmVJUEMvdW5peC9D b25uZWN0aW9uVW5peC5jcHAKaW5kZXggNWE2MWVkNi4uMjlhY2Y2YyAxMDA2NDQKLS0tIGEvU291 cmNlL1dlYktpdDIvUGxhdGZvcm0vQ29yZUlQQy91bml4L0Nvbm5lY3Rpb25Vbml4LmNwcAorKysg Yi9Tb3VyY2UvV2ViS2l0Mi9QbGF0Zm9ybS9Db3JlSVBDL3VuaXgvQ29ubmVjdGlvblVuaXguY3Bw CkBAIC00MCw2ICs0MCw3IEBACiAKICNpZiBQTEFURk9STShRVCkKICNpbmNsdWRlIDxRU29ja2V0 Tm90aWZpZXI+CisjaW5jbHVkZSA8UVdlYWtQb2ludGVyPgogI2VsaWYgUExBVEZPUk0oR1RLKQog I2luY2x1ZGUgPGdsaWIuaD4KICNlbmRpZgpAQCAtMTYyLDE2ICsxNjMsMTcgQEAgcHVibGljOgog ICAgIFNvY2tldE5vdGlmaWVyUmVzb3VyY2VHdWFyZChRU29ja2V0Tm90aWZpZXIqIHNvY2tldE5v dGlmaWVyKQogICAgICAgICA6IG1fc29ja2V0Tm90aWZpZXIoc29ja2V0Tm90aWZpZXIpCiAgICAg ewotICAgICAgICBtX3NvY2tldE5vdGlmaWVyLT5zZXRFbmFibGVkKGZhbHNlKTsKKyAgICAgICAg bV9zb2NrZXROb3RpZmllci5kYXRhKCktPnNldEVuYWJsZWQoZmFsc2UpOwogICAgIH0KIAogICAg IH5Tb2NrZXROb3RpZmllclJlc291cmNlR3VhcmQoKQogICAgIHsKLSAgICAgICAgbV9zb2NrZXRO b3RpZmllci0+c2V0RW5hYmxlZCh0cnVlKTsKKyAgICAgICAgaWYgKG1fc29ja2V0Tm90aWZpZXIp CisgICAgICAgICAgICBtX3NvY2tldE5vdGlmaWVyLmRhdGEoKS0+c2V0RW5hYmxlZCh0cnVlKTsK ICAgICB9CiAKIHByaXZhdGU6Ci0gICAgUVNvY2tldE5vdGlmaWVyKiBjb25zdCBtX3NvY2tldE5v dGlmaWVyOworICAgIFFXZWFrUG9pbnRlcjxRU29ja2V0Tm90aWZpZXI+IGNvbnN0IG1fc29ja2V0 Tm90aWZpZXI7CiB9OwogI2VuZGlmCiAK