Bug 72392 – [Qt] [WK2] Crash in Connection::readyReadHandler() on socket error

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Viatcheslav Ostapenko]

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

[Viatcheslav Ostapenko]

Created attachment 115188 [details]
Move connectionDidClose() out of scope of socketNotifierEnabler.

[Simon Hausmann]

Comment on attachment 115188 [details]
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?

[Simon Hausmann]

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

[Viatcheslav Ostapenko]

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

[Viatcheslav Ostapenko]

Created attachment 115377 [details]
Add QWeakPointer

[Simon Hausmann]

Comment on attachment 115377 [details]
Add QWeakPointer

r=me

[WebKit Review Bot]

Comment on attachment 115377 [details]
Add QWeakPointer

Clearing flags on attachment: 115377

Committed r100455: <http://trac.webkit.org/changeset/100455>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.