Bug 137195 - [WK2][EFL] Fix the problem that threads created by a DispatchQueueEfl are not destroyed even after the DispatchQueueEfl has been destructed
Summary: [WK2][EFL] Fix the problem that threads created by a DispatchQueueEfl are not...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jinwoo Song
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-28 09:50 PDT by Joonghun Park
Modified: 2014-10-14 17:53 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.56 KB, patch)
2014-09-28 09:56 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (3.04 KB, patch)
2014-09-28 10:47 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (3.24 KB, patch)
2014-09-28 11:13 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (3.37 KB, patch)
2014-10-06 22:34 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (3.71 KB, patch)
2014-10-08 05:02 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (2.14 KB, patch)
2014-10-13 23:17 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (2.14 KB, patch)
2014-10-13 23:35 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joonghun Park 2014-09-28 09:50:12 PDT
Linux and Unix System's pthread is created in joinable state by default.
So it is needed to detach the thread explicitly after dispatchQueue is destructed.
Currently, even after DispatchQueueEfl was destructed  the thread created by a DispatchQueueEfl is not destroyed 
because there is no explicit detachThread command.
It leads to maximum number of threads which is allowed by a process in platform 
in case iterating creation and deletion of plugin process many times and lock-up is occurred in conclusion.
Comment 1 Joonghun Park 2014-09-28 09:56:22 PDT
Created attachment 238815 [details]
Patch
Comment 2 Joonghun Park 2014-09-28 10:47:16 PDT
Created attachment 238817 [details]
Patch
Comment 3 Joonghun Park 2014-09-28 11:13:56 PDT
Created attachment 238818 [details]
Patch
Comment 4 Joonghun Park 2014-10-06 22:34:53 PDT
Created attachment 239390 [details]
Patch
Comment 5 Gyuyoung Kim 2014-10-08 03:26:09 PDT
Comment on attachment 239390 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        [WK2][EFL] Fix not destroyed thread created by DispatchQueueEfl even after the DispatchQueueEfl has been destructed

Not clear bug title.

> Source/WebKit2/ChangeLog:8
> +        Linux and Unix System's pthread is created in joinable state by default.

I don't get this patch description. Please improve it further.

> Source/WebKit2/ChangeLog:10
> +        The call to waitForThreadCompletion() in ~DispatchQueue() is for this purpose.

I don't read this sentence.

> Source/WebKit2/Platform/efl/DispatchQueueEfl.h:67
> +    ThreadIdentifier m_threadID;

If you declare this below bool m_isThreadRunning, you don't need to change m_isThreadRunning in DispatchQueue::DispatchQueue()
Comment 6 Joonghun Park 2014-10-08 05:02:21 PDT
Created attachment 239469 [details]
Patch
Comment 7 Gyuyoung Kim 2014-10-10 22:52:45 PDT
Comment on attachment 239469 [details]
Patch

Patch looks reasonable for me. However it would be great other folks also take a look this patch. Add byungwoo to CC.
Comment 8 Gyuyoung Kim 2014-10-12 20:35:02 PDT
Comment on attachment 239469 [details]
Patch

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

BTW I wonder whether there is any regression on EFL layout test. Did you run layout test with this patch ?

> Source/WebKit2/ChangeLog:8
> +        Linux and Unix System's pthread is created in joinable state by default.

pthread is -> pthreads are

> Source/WebKit2/ChangeLog:9
> +        It means that if the created threads are not destructed explicitly,

How about below sentence ?

"If threads aren't destructed explicitly, it will be still live until process which created the thread is dead. Besides it may cause out of capacity range of thread creation in a process. It is one of hidden defects. Thus we need to destroy the thread together when DispatchQueue's dtor is called. This patch calls waitForThreadCompletion() to destroy the thread in dtor of DispatchQueue class"
Comment 9 Gyuyoung Kim 2014-10-12 20:35:33 PDT
Comment on attachment 239469 [details]
Patch

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

> Source/WebKit2/Platform/efl/DispatchQueueEfl.cpp:74
> +    , m_threadID(0)

One more thing. 0 -> nullptr ?
Comment 10 Byungwoo Lee 2014-10-13 01:30:00 PDT
Do we have to join the created thread at the DispatchQueue destructor?
How about detaching after the thread was created? Is there any problem to do so?
Comment 11 Joonghun Park 2014-10-13 01:40:07 PDT
(In reply to comment #10)
> Do we have to join the created thread at the DispatchQueue destructor?
> How about detaching after the thread was created? Is there any problem to do so?

If the dispatchQueue object is destructed prior to the entry function of the dispatchQueue thread, dispatchQueue's m_workItems is valid no more.
If under the circumstances, DispatchQueue::performWork() is called, it leads to crash or locking up. That's why we should use pthread_join in dtor of DispatchQueue   class, I think.
Comment 12 Byungwoo Lee 2014-10-13 01:48:06 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > Do we have to join the created thread at the DispatchQueue destructor?
> > How about detaching after the thread was created? Is there any problem to do so?
> 
> If the dispatchQueue object is destructed prior to the entry function of the dispatchQueue thread, dispatchQueue's m_workItems is valid no more.
> If under the circumstances, DispatchQueue::performWork() is called, it leads to crash or locking up. That's why we should use pthread_join in dtor of DispatchQueue   class, I think.
How can it be possible? DispatchQueue::ThreadContext has the reference of DispatchQueue and the context will be passed to the thread function.
Comment 13 Byungwoo Lee 2014-10-13 01:59:33 PDT
Comment on attachment 239469 [details]
Patch

DispatchQueue can be destroyed at the ThreadContext::function because the DispatchQueue::ThreadContext has a reference of the DispatchQueue instance. Joining the thread at the DispatchQueue destructor can lead a infinite waiting of the created thread.
Comment 14 Joonghun Park 2014-10-13 03:14:06 PDT
(In reply to comment #13)
> (From update of attachment 239469 [details])
> DispatchQueue can be destroyed at the ThreadContext::function because the DispatchQueue::ThreadContext has a reference of the DispatchQueue instance. Joining the thread at the DispatchQueue destructor can lead a infinite waiting of the created thread.

I examined the code carefully again, and I found that you're right.
ThreadContext object has a RefPtr and passed to ThreadContext::function() as arguments. 
So I see that the destruction sequence is garanteed what which only after ThreadContext::function() is completed the DispatchQueue object can be destructed.
I will use detachThread() in ~DispatchQueue() instead of pthread_join.
Thank you for your advice~
Comment 15 Gyuyoung Kim 2014-10-13 03:17:06 PDT
(In reply to comment #13)
> (From update of attachment 239469 [details])
> DispatchQueue can be destroyed at the ThreadContext::function because the DispatchQueue::ThreadContext has a reference of the DispatchQueue instance. Joining the thread at the DispatchQueue destructor can lead a infinite waiting of the created thread.

Nice review Byungwoo ! I missed that DispatchQueue::ThreadContext has a reference of DispatchQueue though it looks very basic behavior.
Comment 16 Byungwoo Lee 2014-10-13 03:38:08 PDT
:)

And could you please check one more?
As I mentioned before, I think that it will be better to detach the thread right after the thread was created instead of ~DispatchQueue() if it is possible. To do so, we can remove redundant m_threadID.
Comment 17 Joonghun Park 2014-10-13 23:17:43 PDT
Created attachment 239780 [details]
Patch
Comment 18 Byungwoo Lee 2014-10-13 23:23:49 PDT
Comment on attachment 239780 [details]
Patch

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

> Source/WebKit2/ChangeLog:16
> +        Thus we need to destroy the thread together
> +        when DispatchQueue's dtor is called.
> +        This patch calls waitForThreadCompletion() to destroy the thread in dtor of DispatchQueue class.

These need to be updated.
Comment 19 Joonghun Park 2014-10-13 23:35:10 PDT
Created attachment 239782 [details]
Patch
Comment 20 Joonghun Park 2014-10-13 23:36:11 PDT
(In reply to comment #18)
> (From update of attachment 239780 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239780&action=review
> 
> > Source/WebKit2/ChangeLog:16
> > +        Thus we need to destroy the thread together
> > +        when DispatchQueue's dtor is called.
> > +        This patch calls waitForThreadCompletion() to destroy the thread in dtor of DispatchQueue class.
> 
> These need to be updated.

I revised the comment. Please check it once again.
Comment 21 Byungwoo Lee 2014-10-13 23:43:00 PDT
LGTM if you verified tests with this patch.
Comment 22 Joonghun Park 2014-10-13 23:45:00 PDT
(In reply to comment #21)
> LGTM if you verified tests with this patch.

I executed regression tests with this patch and there was no regression caused by this patch.
Comment 23 Joonghun Park 2014-10-13 23:47:24 PDT
(In reply to comment #22)
> (In reply to comment #21)
> > LGTM if you verified tests with this patch.
> 
> I executed regression tests with this patch and there was no regression caused by this patch.

I mean, layout test.
Comment 24 Gyuyoung Kim 2014-10-14 02:29:51 PDT
Comment on attachment 239782 [details]
Patch

r+ed based on Byungwoo internal review.
Comment 25 WebKit Commit Bot 2014-10-14 17:53:12 PDT
Comment on attachment 239782 [details]
Patch

Clearing flags on attachment: 239782

Committed r174719: <http://trac.webkit.org/changeset/174719>
Comment 26 WebKit Commit Bot 2014-10-14 17:53:17 PDT
All reviewed patches have been landed.  Closing bug.