Bug 98505 – [EFL] Use ecore_main_loop_thread_safe_call_async() to wakeup main loop.

• Component: WebKit EFL
• Status: RESOLVED INVALID

[Byungwoo Lee]

ecore provide ecore_main_loop_thread_safe_call_async() function to avoid deadlock or race condition.
using this function will be more safe than using ecore_pipe_write().

[Byungwoo Lee]

Created attachment 167369 [details]
Patch

[Byungwoo Lee]

Comment on attachment 167369 [details]
Patch

ecore_main_loop_thread_safe_call_async() uses a lock internally, and this can make some dead lock status with the m_functionQueueLock in RunLoop.
This patch need more check.

[Byungwoo Lee]

There cannot be a dead lock between the two lock.

In the callback flush function for ecore_main_loop_thread_safe_call,
lock is used just for accessing callback item. (same for  m_functionQueueLock)
The accessing order of ecore internal lock and m_functionQueueLock will be always ok.

[Kenneth Rohde Christiansen]

Comment on attachment 167369 [details]
Patch

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

> Source/WTF/ChangeLog:13
> +        Instead of ecore_pipe_write(),
> +        use ecore_main_loop_thread_safe_call_async() to wakeup ecore main loop.
> +
> +        According to the EFL API document, this function is designed to dispatch
> +        a function on ecore main loop by avoiding dead lock or race condition. 
> +        With this function, webkit doesn't need to maintain ecore pipe also.

So apart from this being a nicer implementation, does it actually improve performance or avoid some deadlocks you have been running in to?

[Chris Dumez]

Comment on attachment 167369 [details]
Patch

Implementation seems nicer. Looks sane.

[Byungwoo Lee]

(In reply to comment #4)
> (From update of attachment 167369 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167369&action=review
> 
> > Source/WTF/ChangeLog:13
> > +        Instead of ecore_pipe_write(),
> > +        use ecore_main_loop_thread_safe_call_async() to wakeup ecore main loop.
> > +
> > +        According to the EFL API document, this function is designed to dispatch
> > +        a function on ecore main loop by avoiding dead lock or race condition. 
> > +        With this function, webkit doesn't need to maintain ecore pipe also.
> 
> So apart from this being a nicer implementation, does it actually improve performance or avoid some deadlocks you have been running in to?

Yes~. Actually, this has some complicated history.

I observed a lockup in UIProcess about the pipe write. (with WebKit2)
(You can see the detail in the patch comment on bug 98580.)

ecore_pipe_write() can be work as synchronously when pipe buffer is full.
(The function will be wait until the buffer becomes writable.)

Using ecore_pipe_write() can make some dead lock issue
even though the bug 98580 is applied,  because RunLoop::dispatch() can be invoked in the main thread run loop also.
 * When the pipe buffer is full,
 1. in the IPC thread, RunLoop::dispatch() is called, ecore_pipe_write() will wait until pipe buffer is available.
 2. in the Main thread, RunLoop::dispatch() can be invoked during the performWork() and ecore_pipe_write() will also wait.

When this happens, the process also becomes lockup status because there is no chance to read pipe buffer.

ecore_main_loop_thread_safe_call_async() also uses ecore_pipe_write() but it has a check logic for calling ecore_pipe_write().
(when ecore_pipe_write() for a pipe is called and the callback is not invoked yet, it will not call ecore_pipe_write() again)

so with this function, the above issue can be prevented.

But I want to ask your opinion about this.

The root cause of this issue is, there can be some unnecessary pipe write.
and the above API prevent the problem.

But webkit also maintain the status whether the pipe callback is invoked or not. and this will be more clear than using ecore_main_loop_thread_safe_call_async()

multiple calling for the function doesn't have a multiple pipe write, but it also has multiple callback invoke.

With maintaining the status for callback invoked, redundant callback call can be removed.

[Byungwoo Lee]

(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 167369 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=167369&action=review
> > 
> > > Source/WTF/ChangeLog:13
> > > +        Instead of ecore_pipe_write(),
> > > +        use ecore_main_loop_thread_safe_call_async() to wakeup ecore main loop.
> > > +
> > > +        According to the EFL API document, this function is designed to dispatch
> > > +        a function on ecore main loop by avoiding dead lock or race condition. 
> > > +        With this function, webkit doesn't need to maintain ecore pipe also.
> > 
> > So apart from this being a nicer implementation, does it actually improve performance or avoid some deadlocks you have been running in to?
> 
> Yes~. Actually, this has some complicated history.
> 
> I observed a lockup in UIProcess about the pipe write. (with WebKit2)
> (You can see the detail in the patch comment on bug 98580.)
> 
> ecore_pipe_write() can be work as synchronously when pipe buffer is full.
> (The function will be wait until the buffer becomes writable.)
> 
> Using ecore_pipe_write() can make some dead lock issue
> even though the bug 98580 is applied,  because RunLoop::dispatch() can be invoked in the main thread run loop also.
>  * When the pipe buffer is full,
>  1. in the IPC thread, RunLoop::dispatch() is called, ecore_pipe_write() will wait until pipe buffer is available.
>  2. in the Main thread, RunLoop::dispatch() can be invoked during the performWork() and ecore_pipe_write() will also wait.
> 
> When this happens, the process also becomes lockup status because there is no chance to read pipe buffer.
> 
> ecore_main_loop_thread_safe_call_async() also uses ecore_pipe_write() but it has a check logic for calling ecore_pipe_write().
> (when ecore_pipe_write() for a pipe is called and the callback is not invoked yet, it will not call ecore_pipe_write() again)
> 
> so with this function, the above issue can be prevented.
> 
> But I want to ask your opinion about this.
> 
> The root cause of this issue is, there can be some unnecessary pipe write.
> and the above API prevent the problem.
> 
> But webkit also maintain the status whether the pipe callback is invoked or not. and this will be more clear than using ecore_main_loop_thread_safe_call_async()
> 
> multiple calling for the function doesn't have a multiple pipe write, but it also has multiple callback invoke.
> 
> With maintaining the status for callback invoked, redundant callback call can be removed.

The code block for the above will be as below.

Source/WebCore/platform/RunLoop.cpp
@@ -101,12 +101,23 @@ void RunLoop::performWork()
         
         function();
     }
+
+    {
+        MutexLocker locker(m_functionQueueLock);
+        if (m_functionQueue.isEmpty())
+            return;
+    }
+
+    wakeUp();
 }
 
 void RunLoop::dispatch(const Function<void()>& function)
 {
     MutexLocker locker(m_functionQueueLock);
+    bool needWakeUp = m_functionQueue.isEmpty()
     m_functionQueue.append(function);
+    if (!needWakeUp)
+        return;
 
     wakeUp();
 }

And if this is ok, I think this can be merged with bug 98580 or can be a seperate patch or replace this patch.

[WebKit Review Bot]

Comment on attachment 167369 [details]
Patch

Clearing flags on attachment: 167369

Committed r130619: <http://trac.webkit.org/changeset/130619>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

This patch caused major crashing on the bots:
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/6863

Please roll out.

[Chris Dumez]

Backtraces:

crash log for DumpRenderTree (pid 6382):
STDOUT: <empty>
STDERR: 1   0x7f6d83a206df
STDERR: 2   0x7f6d7bf8ecb0
STDERR: 3   0x7f6d800e8848 WebCore::ResourceLoader::identifier() const
STDERR: 4   0x7f6d80166ef4 WebCore::CachedRawResource::setResponse(WebCore::ResourceResponse const&)
STDERR: 5   0x7f6d8012ae3b WebCore::SubresourceLoader::didReceiveResponse(WebCore::ResourceResponse const&)
STDERR: 6   0x7f6d8012663b WebCore::ResourceLoader::didReceiveResponse(WebCore::ResourceHandle*, WebCore::ResourceResponse const&)
STDERR: 7   0x7f6d8032e4cb WebCore::BlobResourceHandle::notifyResponseOnSuccess()
STDERR: 8   0x7f6d8032e2be WebCore::BlobResourceHandle::notifyResponse()
STDERR: 9   0x7f6d8032d11a WebCore::BlobResourceHandle::getSizeForNext()
STDERR: 10  0x7f6d8032d3aa WebCore::BlobResourceHandle::didGetSize(long long)
STDERR: 11  0x7f6d8032d1b5 WebCore::BlobResourceHandle::getSizeForNext()
STDERR: 12  0x7f6d8032cfee WebCore::BlobResourceHandle::doStart()
STDERR: 13  0x7f6d8032ce86 WebCore::delayedStartBlobResourceHandle(void*)
STDERR: 14  0x7f6d83a51871 WTF::dispatchFunctionsFromMainThread()
STDERR: 15  0x7f6d83a5bbcd
STDERR: 16  0x7f6d83a5bbea WTF::scheduleDispatchFunctionsOnMainThread()
STDERR: 17  0x7f6d83a5199d WTF::callOnMainThread(void (*)(void*), void*)
STDERR: 18  0x7f6d8032ced2 WebCore::BlobResourceHandle::start()
STDERR: 19  0x7f6d8032962e WebCore::BlobRegistryImpl::createResourceHandle(WebCore::ResourceRequest const&, WebCore::ResourceHandleClient*)
STDERR: 20  0x7f6d803294a5
STDERR: 21  0x7f6d8033f8d4 WebCore::ResourceHandle::create(WebCore::NetworkingContext*, WebCore::ResourceRequest const&, WebCore::ResourceHandleClient*, bool, bool)
STDERR: 22  0x7f6d801257fe WebCore::ResourceLoader::start()
STDERR: 23  0x7f6d8012013e WebCore::ResourceLoadScheduler::servePendingRequests(WebCore::ResourceLoadScheduler::HostInformation*, WebCore::ResourceLoadPriority)
STDERR: 24  0x7f6d8011fb7e WebCore::ResourceLoadScheduler::scheduleLoad(WebCore::ResourceLoader*, WebCore::ResourceLoadPriority)
STDERR: 25  0x7f6d8011f8b3 WebCore::ResourceLoadScheduler::scheduleSubresourceLoad(WebCore::Frame*, WebCore::CachedResource*, WebCore::ResourceRequest const&, WebCore::ResourceLoadPriority, WebCore::ResourceLoaderOptions const&)
STDERR: 26  0x7f6d80168540 WebCore::CachedResource::load(WebCore::CachedResourceLoader*, WebCore::ResourceLoaderOptions const&)
STDERR: 27  0x7f6d80175881 WebCore::CachedResourceLoader::requestResource(WebCore::CachedResource::Type, WebCore::ResourceRequest&, WTF::String const&, WebCore::ResourceLoaderOptions const&, WebCore::ResourceLoadPriority, bool, WebCore::CachedResourceLoader::DeferOption)
STDERR: 28  0x7f6d80174e49 WebCore::CachedResourceLoader::requestRawResource(WebCore::ResourceRequest&, WebCore::ResourceLoaderOptions const&)
STDERR: 29  0x7f6d800e7df2 WebCore::DocumentThreadableLoader::loadRequest(WebCore::ResourceRequest const&, WebCore::SecurityCheckPolicy)
STDERR: 30  0x7f6d800e5f0f WebCore::DocumentThreadableLoader::DocumentThreadableLoader(WebCore::Document*, WebCore::ThreadableLoaderClient*, WebCore::DocumentThreadableLoader::BlockingBehavior, WebCore::ResourceRequest const&, WebCore::ThreadableLoaderOptions const&)
STDERR: 31  0x7f6d800e5c16 WebCore::DocumentThreadableLoader::create(WebCore::Document*, WebCore::ThreadableLoaderClient*, WebCore::ResourceRequest const&, WebCore::ThreadableLoaderOptions const&)
STDERR: LEAK: 5 RenderObject
STDERR: LEAK: 1 Page
STDERR: LEAK: 1 Frame
STDERR: LEAK: 4 CachedResource
STDERR: LEAK: 2 SubresourceLoader
STDERR: LEAK: 58 WebCoreNode

------

crash log for DumpRenderTree (pid 3766):
STDOUT: CONSOLE MESSAGE: Application Cache download process was aborted.
STDOUT: This tests that download process was aborted after downloading event although resource was not found.
STDOUT: SUCCESS
STDERR: ASSERTION FAILED: m_table
STDERR: /home/buildslave-1/webkit-buildslave/efl-linux-64-debug/build/Source/WTF/wtf/HashTable.h(210) : void WTF::HashTableConstIterator<Key, Value, Extractor, HashFunctions, Traits, KeyTraits>::checkValidity() const [with Key = WebCore::DocumentLoader*, Value = WebCore::DocumentLoader*, Extractor = WTF::IdentityExtractor, HashFunctions = WTF::PtrHash<WebCore::DocumentLoader*>, Traits = WTF::HashTraits<WebCore::DocumentLoader*>, KeyTraits = WTF::HashTraits<WebCore::DocumentLoader*>]
STDERR: 1   0x7f52434650a9 WTF::HashTableConstIterator<WebCore::DocumentLoader*, WebCore::DocumentLoader*, WTF::IdentityExtractor, WTF::PtrHash<WebCore::DocumentLoader*>, WTF::HashTraits<WebCore::DocumentLoader*>, WTF::HashTraits<WebCore::DocumentLoader*> >::checkValidity() const
STDERR: 2   0x7f5243463b04 WTF::HashTableConstIterator<WebCore::DocumentLoader*, WebCore::DocumentLoader*, WTF::IdentityExtractor, WTF::PtrHash<WebCore::DocumentLoader*>, WTF::HashTraits<WebCore::DocumentLoader*>, WTF::HashTraits<WebCore::DocumentLoader*> >::operator++()
STDERR: 3   0x7f52434628b2 WTF::HashTableConstIteratorAdapter<WTF::HashTable<WebCore::DocumentLoader*, WebCore::DocumentLoader*, WTF::IdentityExtractor, WTF::PtrHash<WebCore::DocumentLoader*>, WTF::HashTraits<WebCore::DocumentLoader*>, WTF::HashTraits<WebCore::DocumentLoader*> >, WebCore::DocumentLoader*>::operator++()
STDERR: 4   0x7f524346128c WebCore::ApplicationCacheGroup::postListenerTask(WebCore::ApplicationCacheHost::EventID, int, int, WTF::HashSet<WebCore::DocumentLoader*, WTF::PtrHash<WebCore::DocumentLoader*>, WTF::HashTraits<WebCore::DocumentLoader*> > const&)
STDERR: 5   0x7f52434615ab WebCore::ApplicationCacheGroup::postListenerTask(WebCore::ApplicationCacheHost::EventID, WTF::HashSet<WebCore::DocumentLoader*, WTF::PtrHash<WebCore::DocumentLoader*>, WTF::HashTraits<WebCore::DocumentLoader*> > const&)
STDERR: 6   0x7f524345f70d WebCore::ApplicationCacheGroup::didFinishLoadingManifest()
STDERR: 7   0x7f524345e4d1 WebCore::ApplicationCacheGroup::didFinishLoading(WebCore::ResourceHandle*, double)
STDERR: 8   0x7f5243f76a1c
STDERR: 9   0x7f523bb78765
STDERR: 10  0x7f523bb8d8cd g_simple_async_result_complete
STDERR: 11  0x7f523bb8d9fc
STDERR: 12  0x7f523bebfe53 g_main_context_dispatch
STDERR: 13  0x7f524034323e
STDERR: 14  0x7f524033d7b1
STDERR: 15  0x7f524033e245
STDERR: 16  0x7f524033e547 ecore_main_loop_begin
STDERR: 17  0x47a82d
STDERR: 18  0x47aa0b
STDERR: 19  0x47b049 main
STDERR: 20  0x7f523e9ed76d __libc_start_main
STDERR: 21  0x468349

-------

crash log for DumpRenderTree (pid 6264):
STDOUT: <empty>
STDERR: ASSERTION FAILED: m_downloadingPendingMasterResourceLoadersCount > 0
STDERR: /home/buildslave-1/webkit-buildslave/efl-linux-64-debug/build/Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp(275) : void WebCore::ApplicationCacheGroup::finishedLoadingMainResource(WebCore::DocumentLoader*)
STDERR: 1   0x7f88e3be2209 WebCore::ApplicationCacheGroup::finishedLoadingMainResource(WebCore::DocumentLoader*)
STDERR: 2   0x7f88e3be6cdd WebCore::ApplicationCacheGroup::deliverDelayedMainResources()
STDERR: 3   0x7f88e3be5c63 WebCore::ApplicationCacheGroup::cacheUpdateFailed()
STDERR: 4   0x7f88e3be2dff WebCore::ApplicationCacheGroup::stopLoadingInFrame(WebCore::Frame*)
STDERR: 5   0x7f88e3bee020 WebCore::ApplicationCacheHost::stopLoadingInFrame(WebCore::Frame*)
STDERR: 6   0x7f88e3b7cfe6 WebCore::DocumentLoader::stopLoading()
STDERR: 7   0x7f88e3ba1659 WebCore::FrameLoader::stopAllLoaders(WebCore::ClearProvisionalItemPolicy)
STDERR: 8   0x7f88e3ba4d2b WebCore::FrameLoader::frameDetached()
STDERR: 9   0x7f88e3997d7c WebCore::HTMLFrameOwnerElement::disconnectContentFrame()
STDERR: 10  0x7f88e3759c78 WebCore::ChildFrameDisconnector::Target::disconnect()
STDERR: 11  0x7f88e3754cf4 WebCore::ChildFrameDisconnector::disconnect()
STDERR: 12  0x7f88e3750f03
STDERR: 13  0x7f88e3751241 WebCore::ContainerNode::removeChild(WebCore::Node*, int&)
STDERR: 14  0x7f88e3750abd WebCore::ContainerNode::replaceChild(WTF::PassRefPtr<WebCore::Node>, WebCore::Node*, int&, bool)
STDERR: 15  0x7f88e381aed5 WebCore::Node::replaceChild(WTF::PassRefPtr<WebCore::Node>, WebCore::Node*, int&, bool)
STDERR: 16  0x7f88e4285d93 WebCore::JSNode::replaceChild(JSC::ExecState*)
STDERR: 17  0x7f88e43ae025 WebCore::jsNodePrototypeFunctionReplaceChild(JSC::ExecState*)
STDERR: 18  0x7f889564d265

[Byungwoo Lee]

Sorry for the regression and thanks for checking and rolling out.

I'm checking the crash issue, and will update.

[Byungwoo Lee]

(In reply to comment #12)
> Sorry for the regression and thanks for checking and rolling out.
> I'm checking the crash issue, and will update.

ecore_main_loop_thread_safe_call_async() has a logic that it can call the callback function synchronously in the main thread.
(If current thread is main thread, ecore_main_loop_thread_safe_call_async() will call the callback function directly.)

I sent a mail to the EFL maintainer whether this is a bug or not.
If this cannot be fixed, ecore_main_loop_thread_safe_call_async() cannot use for the wakeup purpose.

[Byungwoo Lee]

(In reply to comment #13)
> (In reply to comment #12)
> > Sorry for the regression and thanks for checking and rolling out.
> > I'm checking the crash issue, and will update.
> 
> ecore_main_loop_thread_safe_call_async() has a logic that it can call the callback function synchronously in the main thread.
> (If current thread is main thread, ecore_main_loop_thread_safe_call_async() will call the callback function directly.)
> 
> I sent a mail to the EFL maintainer whether this is a bug or not.
> If this cannot be fixed, ecore_main_loop_thread_safe_call_async() cannot use for the wakeup purpose.

I received answer that the function was designed like the above.
(in the main thread, it calls a callback function directly)

According to the reply, it is recommended to use ecore idler or some other mechanism like ecore event.

But using other mechanism can make some timing issue.
(caused by a dispatching order of loop items (timer, pipe, idler...) in ecore main loop)

Using ecore_pipe_write() will be the best way not to make side effects.

To prevent lockup issue on pipe buffer full status,
RunLoop needs additional check logic to prevent redundant pipe write.