Bug 98580 – Change the range of m_functionQueueLock in RunLoop::dispatch()

• Component: WebCore Misc.
• Status: NEW

[Byungwoo Lee]

Runloop dispaching sequence can make deadlock.

[Byungwoo Lee]

Created attachment 167451 [details]
Patch

[Byungwoo Lee]

RunLoop::wakeUp() function also need some protection by a mutex locker because RunLoop::dispatch() can be invoked on multiple thread at one time.
So I added mutex locker for the RunLoop::wakeUp() also.

[Byungwoo Lee]

Created attachment 167494 [details]
Patch

[Byungwoo Lee]

I tested layout test and unit test for efl(webkit2) and there was no regression with this patch.

I'll upload same patch again for updating ews status on mac(merge conflict)

[Byungwoo Lee]

Created attachment 167724 [details]
Patch

[Build Bot]

Comment on attachment 167724 [details]
Patch

Attachment 167724 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14207896

New failing tests:
http/tests/inspector/search/resources-search-match-index.html

[Darin Adler]

Comment on attachment 167724 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167724&action=review

> Source/WebCore/ChangeLog:25
> +        And RunLoop::wakeUp() can be invoked on multiple thread at one time,
> +        so new mutex locker for the function is added(m_wakeUpLock).

As far as I can tell, there’s no problem running the CF version of wakeUp on multiple threads at once, so it doesn’t seem correct to add unneeded locking just because other platforms need it. Unless I’m mistaken and there is a need for locking.

[Byungwoo Lee]

(In reply to comment #7)
> (From update of attachment 167724 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167724&action=review
> 
> > Source/WebCore/ChangeLog:25
> > +        And RunLoop::wakeUp() can be invoked on multiple thread at one time,
> > +        so new mutex locker for the function is added(m_wakeUpLock).
> 
> As far as I can tell, there’s no problem running the CF version of wakeUp on multiple threads at once, so it doesn’t seem correct to add unneeded locking just because other platforms need it. Unless I’m mistaken and there is a need for locking.

Thanks for the review. :-)

As you pointed, the locking should be added only to the port that needs it.

I'll remove the locking from the CF version and check this issue on other ports also.

[Jim Roskind]

Drive by comment (since someone added me to the cc list... maybe because I did a lot of work on the message loop in Chrome):

Locks are best used to protect data, not code. When doing so, it is nicest to hold the lock as briefly as possible.

As a result, the call to a function (WakeUp()), while holding the lock, was suspicious/dangerous (and indeed, based on the bug, the source of a deadlock).  It is a nice change to *only* use the lock to protect the queue during the append (and that is part of the recent CL https://bugs.webkit.org/show_bug.cgi#attach_167724).

That CL also creates a new lock m_wakeUpLock, and acquires that lock within WakeUp().   That lock also appears to be held during more substantial calls... so it is potentially dangerous/problematic.  It would probably be nicer to push such a lock acquisition as low in the stack as possible, and only use it for a minimal set of data access (and not for larger calls... which might block... or need another lock, etc.).

YMMV.... but I'm guessing I was cc'ed to toss in this drive by.

[Eric Seidel (no email)]

Thank you Jim.  :)  Just trying to make sure all the Lock/RunLoop folks are kept in the loop.

[Michael Brüning]

afaics, we don't need the locking for the Qt port either. However, I am not a reviewer, so I added Simon so he can take a look.

[Byungwoo Lee]

Thanks again for your comments :-)

I checked with GTK and Window.

1) GTK doesn't need lock because g_source_attach and g_main_context_wakeup uses locking internally.

2) Window also does't need lock : it uses ::PostMessage() function that is thread-safe.

If the QT is clear, I'll update the patch.

(Maybe the EFL is the only port that needs additional locking)

[Byungwoo Lee]

I checked with QT and it seems that additional locking is not needed for wakeUp.
(wakeUp uses QMetaMethod::invoke() with Qt::QueuedConnection parameter. With that parameter, event will be sent by QCoreApplication::postEvent() which is thread-safe)

I'll use the additional locking in wakeUp() only for EFL port.

I tried to check carefully, but there can be a missed point on the QT, GTK, WIN port.
Please comment if there is some missed point about those ports and also about the other ports.

[Michael Brüning]

Discussed this issue with Jocelyn Turcotte and we agreed that the wakeup lock is not needed for Qt either as we use QMetaMethod.invoke with a QueuedConenction and that is itself thread-safe and protected by the Qt event loops internally.

[Byungwoo Lee]

(In reply to comment #14)
> Discussed this issue with Jocelyn Turcotte and we agreed that the wakeup lock is not needed for Qt either as we use QMetaMethod.invoke with a QueuedConenction and that is itself thread-safe and protected by the Qt event loops internally.

Thank you for the checking. I'll upload new patch. :-)

[Byungwoo Lee]

Created attachment 168184 [details]
Final patch that uses m_functionQueueLock only for accessing m_functionQueue and adds additional locker for wakeUp function only for efl port.

[Byungwoo Lee]

Created attachment 168328 [details]
incorrectly uploaded

[Byungwoo Lee]

Comment on attachment 168184 [details]
Final patch that uses m_functionQueueLock only for accessing m_functionQueue and adds additional locker for wakeUp function only for efl port.

Oops, uploaded incorrect patch. reopen this again.

[Byungwoo Lee]

I tested layout test and unit test on EFL, and there was no regressions.

[Yael]

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

[Anders Carlsson]

Comment on attachment 168184 [details]
Final patch that uses m_functionQueueLock only for accessing m_functionQueue and adds additional locker for wakeUp function only for efl port.

View in context: https://bugs.webkit.org/attachment.cgi?id=168184&action=review

> Source/WebCore/platform/RunLoop.cpp:111
> -    MutexLocker locker(m_functionQueueLock);
> -    m_functionQueue.append(function);
> +    {
> +        MutexLocker locker(m_functionQueueLock);
> +        m_functionQueue.append(function);
> +    }

Why is this still needed?

[Byungwoo Lee]

(In reply to comment #21)
> (From update of attachment 168184 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168184&action=review
> 
> > Source/WebCore/platform/RunLoop.cpp:111
> > -    MutexLocker locker(m_functionQueueLock);
> > -    m_functionQueue.append(function);
> > +    {
> > +        MutexLocker locker(m_functionQueueLock);
> > +        m_functionQueue.append(function);
> > +    }
> 
> Why is this still needed?

Because the mutex is for the m_functionQueue, and calling wakeUp doesn't need to be protected by m_functionQueueLock.

RunLoop::wakeUp() also be used in the Connection::wakeUpRunLoop() without acquiring any lock.

wakeUpRunLoop() is called in the ScrollingThread::dispatch(), and in the function, locking is released like below.

 void ScrollingThread::dispatch(const Function<void()>& function)
 {
     shared().createThreadIfNeeded();
 
     {
         MutexLocker locker(shared().m_functionsMutex);
         shared().m_functions.append(function);
     }
 
     shared().wakeUpRunLoop();
 }

This function also release lock right after the shared resource is accessed.
and call the wakeUp without locking.

I thought that this is more safer than the previous.

[Byungwoo Lee]

I made two bug for the EFL port issue : Bug 101132, Bug 101135
It will be clear to handle those in the separate issue.

[Andreas Kling]

Comment on attachment 168184 [details]
Final patch that uses m_functionQueueLock only for accessing m_functionQueue and adds additional locker for wakeUp function only for efl port.

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.