Bug 115332 – [EFL][WK2] Separate dispatch context from WorkQueue.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Byungwoo Lee]

With current implementation, deref() in the performWork() and performTimerWork() can make dangling workQueue pointer in WorkQueue::workQueueThread() at certain situation.

[Byungwoo Lee]

Created attachment 199969 [details]
Patch

[Byungwoo Lee]

Need some refactoring.

[Byungwoo Lee]

Created attachment 199980 [details]
Patch

[Chris Dumez]

Comment on attachment 199980 [details]
Patch

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

> Source/WebKit2/ChangeLog:19
> +        To make the code clear, WorkQueue::WorkQueueThreadPoller class is added.

What is the link between this bug (needing to ref/unref the work queue) and the WorkQueueThreadPoller? Maybe they should be split into 2 patches as one seems like a bug fix, and the other one seems to be refactoring. I don't think it is a good idea to do both at the same time.

> Source/WebKit2/Platform/WorkQueue.h:185
> +    class WorkQueueThreadPoller : public ThreadSafeRefCounted<WorkQueueThreadPoller> {

Does this really need to be ref counted?

> Source/WebKit2/Platform/WorkQueue.h:193
> +        bool socketActivated() { return m_socketActivated; }

Should be a const method.

> Source/WebKit2/Platform/WorkQueue.h:209
> +    RefPtr<WorkQueueThreadPoller> m_threadPoller;

It does not seem to be shared, is it? If so, we could use OwnPtr.

> Source/WebKit2/Platform/WorkQueue.h:217
> +    PassRefPtr<WorkQueueThreadPoller> threadPoller() { return m_threadPoller; }

I don't believe you want to pass ownership to the caller so this should return a raw pointer, not a PassRefPtr. Otherwise, this is confusing.

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:83
> +        char readBuf[threadMessageSize];

Not new but considering our message is always of size one. I think we could get rid of those arrays everywhere and use a simple char.

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:100
> +void WorkQueue::WorkQueueThreadPoller::registerSocketEventHandler(int fileDescriptor, const Function<void()>& function)

handler might be clearer than "function".

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:103
> +        LOG_ERROR("%d is already registerd.", fileDescriptor);

"registered"
Shouldn't we ASSERT instead?

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:115
>      m_socketDescriptor = invalidSocketDescriptor;

We should probably have an ASSERTION(m_socketDescriptor == fileDescriptor)?

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:261
> +    m_threadPoller->sendMessageToThread(wakupThreadMessage);

I know this is not new but "wakup" is a typo.

[Byungwoo Lee]

Comment on attachment 199980 [details]
Patch

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

>> Source/WebKit2/ChangeLog:19
>> +        To make the code clear, WorkQueue::WorkQueueThreadPoller class is added.
> 
> What is the link between this bug (needing to ref/unref the work queue) and the WorkQueueThreadPoller? Maybe they should be split into 2 patches as one seems like a bug fix, and the other one seems to be refactoring. I don't think it is a good idea to do both at the same time.

Hm.. During the bug fixing, I need to refactoring because I need to re-arrange performing work items and performing socket event handler to be placed under the WorkQueue keeper.
But if this refactoring alone is meeningful, I think separating this also ok.

>> Source/WebKit2/Platform/WorkQueue.h:185
>> +    class WorkQueueThreadPoller : public ThreadSafeRefCounted<WorkQueueThreadPoller> {
> 
> Does this really need to be ref counted?

Yes,
There is a reason that I use Ref-Counted object, because this can be shared with WorkQueue instance and workQueueThread function.
At the destructor of this instance, pipe fd is closed, so this instance should be alive when a WorkQueue instance is alive or workQueueThread function is not finished (or polling loop is finished).
As you can see, WorkQueue instance can be destroyed in the middle of the polling loop.

>> Source/WebKit2/Platform/WorkQueue.h:193
>> +        bool socketActivated() { return m_socketActivated; }
> 
> Should be a const method.

Ok, will change.

>> Source/WebKit2/Platform/WorkQueue.h:209
>> +    RefPtr<WorkQueueThreadPoller> m_threadPoller;
> 
> It does not seem to be shared, is it? If so, we could use OwnPtr.

Ditto.

>> Source/WebKit2/Platform/WorkQueue.h:217
>> +    PassRefPtr<WorkQueueThreadPoller> threadPoller() { return m_threadPoller; }
> 
> I don't believe you want to pass ownership to the caller so this should return a raw pointer, not a PassRefPtr. Otherwise, this is confusing.

I want to share the ownership so I used PassRefPtr.

>> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:83
>> +        char readBuf[threadMessageSize];
> 
> Not new but considering our message is always of size one. I think we could get rid of those arrays everywhere and use a simple char.

Yes, I used the previous code, but this can be more simpler. Will change this also :)

>> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:100
>> +void WorkQueue::WorkQueueThreadPoller::registerSocketEventHandler(int fileDescriptor, const Function<void()>& function)
> 
> handler might be clearer than "function".

I used the same function in the WorkQueue. Do I need to change the name?

>> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:103
>> +        LOG_ERROR("%d is already registerd.", fileDescriptor);
> 
> "registered"
> Shouldn't we ASSERT instead?

Yes assert might be better to find the problem easier.
This is the same code as previous also. I'll change this also.

>> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:115
>>      m_socketDescriptor = invalidSocketDescriptor;
> 
> We should probably have an ASSERTION(m_socketDescriptor == fileDescriptor)?

Yes it will be better. Same as previous, but will add it.

>> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:261
>> +    m_threadPoller->sendMessageToThread(wakupThreadMessage);
> 
> I know this is not new but "wakup" is a typo.

Yes should be changed.

[Byungwoo Lee]

Created attachment 200209 [details]
Patch

[Chris Dumez]

Comment on attachment 200209 [details]
Patch

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

> Source/WebKit2/Platform/WorkQueue.h:185
> +    class WorkQueueThread : public ThreadSafeRefCounted<WorkQueueThread> {

Does this really need to be ref counted?

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:142
> +    RefPtr<WorkQueueThread> thread = workQueue->thread();

I'm still not convinced workQueue->thread() should return a PassRefPtr. This is confusing as you don't pass ownership to the caller.
Also, I don't think the WorkQueueThread can go away as you ref the WorkQueue already.

[Mikhail Pozdnyakov]

Comment on attachment 200209 [details]
Patch

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

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:47
> +    , m_socketActivated(false)

where all the other class members?

[Byungwoo Lee]

Created attachment 201669 [details]
Patch

Did the API test and the crash about WorkQueue(reported at build.webkit.org) was fixed with this patch.

[Byungwoo Lee]

(In reply to comment #7)
> (From update of attachment 200209 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200209&action=review
> 
> > Source/WebKit2/Platform/WorkQueue.h:185
> > +    class WorkQueueThread : public ThreadSafeRefCounted<WorkQueueThread> {
> 
> Does this really need to be ref counted?
> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:142
> > +    RefPtr<WorkQueueThread> thread = workQueue->thread();
> 
> I'm still not convinced workQueue->thread() should return a PassRefPtr. This is confusing as you don't pass ownership to the caller.
> Also, I don't think the WorkQueueThread can go away as you ref the WorkQueue already.

WorkQueueThread contains information for polling thread.
(Means, it has pipe fds to implement work queue thread polling loop)

WorkQueue class intance can be deleted at any thread.
But WorkQueueThread:main() function can be terminated at a certain thread.

The pipe fds are accessed at the thread that WorkQueueThread:main() works.
And the function can work after WorkQueue instance is deleted at another thread.

Let's think about the followings.
WorkQueueThread:main() is working at Thread 1, and at any point while Thread 1 is working inside the polling loop, workQueue is deleted at Thread 2.

If workQueue only has the ownership of pipe fds, select or read might be
failed unintentionally.

So, pipe fds should be kept until the polling task is successfuly finished.

To this, pipe fds should be stored at Thread 1 that WorkQueueThread:main() is working, and passing ownership of WorkQueueThread is the way that I choose.

Please let me know if there are anything that I misunderstood.

[Byungwoo Lee]

(In reply to comment #8)
> (From update of attachment 200209 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200209&action=review
> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:47
> > +    , m_socketActivated(false)
> 
> where all the other class members?

Others are initialized at the body of the constructor.

Maybe diff align makes you confused.

[Chris Dumez]

Comment on attachment 201669 [details]
Patch

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

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:136
> +        RefPtr<WorkQueue> keep(workQueue);

Constructing a RefPtr from a raw pointer is dangerous. See m_adoptionIsRequired assertions in RefCountedBase.

[Byungwoo Lee]

(In reply to comment #12)
> (From update of attachment 201669 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201669&action=review
> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:136
> > +        RefPtr<WorkQueue> keep(workQueue);
> 
> Constructing a RefPtr from a raw pointer is dangerous. See m_adoptionIsRequired assertions in RefCountedBase.

Yes, this should be carefully considered, Thanks.

I think assertions will happen when trying to keep it before adopted or after deleted. (is it right?)

I'll update when I find a way to protect the danger.

If you have any suggestion, please let me know :)

[Byungwoo Lee]

Created attachment 201916 [details]
Patch

Minimize patch to fix the workqueue crash issue on the build bot first and make review easier. (Following the suggestion from Chris)

[Chris Dumez]

Comment on attachment 201916 [details]
Patch

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

We do have a problem but I don't think the approach to fix it is right. It seems to be that it does not solve the issue, just makes it less likely to happen. I'm still thinking about a better solution to this.
Waiting for the thread completion in the WorkQueue destructor would have done the trick but you did not like this proposal either :)

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:179
>      while (workQueue->m_threadLoop) {

workQueue may be destroyed already.

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:184
> +        if (workQueue->m_threadLoop)

Ditto.

[Byungwoo Lee]

(In reply to comment #15)
> (From update of attachment 201916 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201916&action=review
> 
> We do have a problem but I don't think the approach to fix it is right. It seems to be that it does not solve the issue, just makes it less likely to happen. I'm still thinking about a better solution to this.
> Waiting for the thread completion in the WorkQueue destructor would have done the trick but you did not like this proposal either :)
> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:179
> >      while (workQueue->m_threadLoop) {
> 
> workQueue may be destroyed already.
> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:184
> > +        if (workQueue->m_threadLoop)
> 
> Ditto.

Yes, your point is correct and to consider those, I had to do refactoring at the previous patch.

With your proposal to wait the thread termination, I have a concern about some situations that,Connection is being destroyed but workqueue is alive, and some lockup issue when workqueue is destroyed on the workqueue thread.

Both seems to be already mentioned on bug 114432.

[Byungwoo Lee]

Created attachment 203344 [details]
Refactoring patch to review

[Byungwoo Lee]

Comment on attachment 203344 [details]
Refactoring patch to review

I uploaded new patch that focuses refactoring.
I didn't consider naming and ordering to make review easy by minimizing difference.

[Rafael Brandao]

Comment on attachment 203344 [details]
Refactoring patch to review

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

Looks good! I think it would be nice to have DispatchQueue in a separate file so it will be easier to understand (both end with "queue", I think it's easy to get confused about what function is part of what). I think your approach is correct, I will look more into it later. :)

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:151
> +    dispatchQueue->dispatchRelease();

Awesome!!

[Byungwoo Lee]

Created attachment 203749 [details]
Patch

[Byungwoo Lee]

Uploaded new patch with separated file for DispatchQueue class according to the Rafael's suggestion. (Just moved the implementation)

[Byungwoo Lee]

Tested api test and LayoutTest, and there was no regression.

[Chris Dumez]

Comment on attachment 203749 [details]
Patch

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

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:28
> +    m_dispatchQueue = new DispatchQueue(name);

I fail to see who is freeing this allocated memory. Could you please explained? Looks to me that m_dispatchQueue is never deleted.

[Byungwoo Lee]

Comment on attachment 203749 [details]
Patch

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

>> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:28
>> +    m_dispatchQueue = new DispatchQueue(name);
> 
> I fail to see who is freeing this allocated memory. Could you please explained? Looks to me that m_dispatchQueue is never deleted.

The created DispatchQueue instance will be passed to the dispatch queue thread at DispatchQueue.cpp:90 and it will be deleted when the thread is terminated (DispatchQueue.cpp:237.
To make the thead termination, m_dispatchQueue->dispatchRelease() function is called in WorkQueue::platformInvalidate().

[Chris Dumez]

(In reply to comment #24)
> (From update of attachment 203749 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203749&action=review
> 
> >> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:28
> >> +    m_dispatchQueue = new DispatchQueue(name);
> > 
> > I fail to see who is freeing this allocated memory. Could you please explained? Looks to me that m_dispatchQueue is never deleted.
> 
> The created DispatchQueue instance will be passed to the dispatch queue thread at DispatchQueue.cpp:90 and it will be deleted when the thread is terminated (DispatchQueue.cpp:237.
> To make the thead termination, m_dispatchQueue->dispatchRelease() function is called in WorkQueue::platformInvalidate().

Right, I missed it. Thanks for the clarification.

[Sergio Correia (qrwteyrutiyoup)]

(In reply to comment #25)
> (In reply to comment #24)
> > (From update of attachment 203749 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=203749&action=review
> > 
> > >> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:28
> > >> +    m_dispatchQueue = new DispatchQueue(name);
> > > 
> > > I fail to see who is freeing this allocated memory. Could you please explained? Looks to me that m_dispatchQueue is never deleted.
> > 
> > The created DispatchQueue instance will be passed to the dispatch queue thread at DispatchQueue.cpp:90 and it will be deleted when the thread is terminated (DispatchQueue.cpp:237.
> > To make the thead termination, m_dispatchQueue->dispatchRelease() function is called in WorkQueue::platformInvalidate().
> 
> Right, I missed it. Thanks for the clarification.

One problem with the current implementation (being discussed at https://bugs.webkit.org/show_bug.cgi?id=114432, still without a working solution) is that the workqueue instance stays on forever, so we won't reach WorkQueue::platformInvalidate() to trigger this deletion path.

@Byungwoo: a few nits in your current patch:
In the changelog: typo: reference countable

DispatchQueue.cpp:129
> LOG_ERROR("%d is already registerd.", fileDescriptor);
Please fix this typo as well: registered

Source/WebKit2/Platform/efl/DispatchQueue.cpp:203
> LOG_ERROR("Failed to read from WorkQueueThread pipe");

Source/WebKit2/Platform/efl/DispatchQueue.cpp:245
> LOG_ERROR("Failed to wake up WorkQueue Thread");

You probably should update those error messages, since you changed it to be a DispatchQueue Thread. If you decide not to change it, at least make them both equal: "WorkQueue Thread" vs. "WorkQueueThread"

Anyway, I like the approach you are taking here, taking out (almost all) the EFL-specific bits from WorkQueue.h and managing the tasks dispatch with the DispatchQueue class. The code seems cleaner now, and once this has landed, we could start working again on https://bugs.webkit.org/show_bug.cgi?id=114432 and on possible race conditions that might to happen in the concurrent access of m_maxFileDescriptor/m_fileDescriptorSet in performFileDescriptorWork() and registerSocketEventHandler().

[Byungwoo Lee]

(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > (From update of attachment 203749 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=203749&action=review
> > > 
> > > >> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:28
> > > >> +    m_dispatchQueue = new DispatchQueue(name);
> > > > 
> > > > I fail to see who is freeing this allocated memory. Could you please explained? Looks to me that m_dispatchQueue is never deleted.
> > > 
> > > The created DispatchQueue instance will be passed to the dispatch queue thread at DispatchQueue.cpp:90 and it will be deleted when the thread is terminated (DispatchQueue.cpp:237.
> > > To make the thead termination, m_dispatchQueue->dispatchRelease() function is called in WorkQueue::platformInvalidate().
> > 
> > Right, I missed it. Thanks for the clarification.
> 
> One problem with the current implementation (being discussed at https://bugs.webkit.org/show_bug.cgi?id=114432, still without a working solution) is that the workqueue instance stays on forever, so we won't reach WorkQueue::platformInvalidate() to trigger this deletion path.
Yes, The issue can be handled separately at the bug 114432.

> @Byungwoo: a few nits in your current patch:
> In the changelog: typo: reference countable
> 
> DispatchQueue.cpp:129
> > LOG_ERROR("%d is already registerd.", fileDescriptor);
> Please fix this typo as well: registered
> 
> Source/WebKit2/Platform/efl/DispatchQueue.cpp:203
> > LOG_ERROR("Failed to read from WorkQueueThread pipe");
> 
> Source/WebKit2/Platform/efl/DispatchQueue.cpp:245
> > LOG_ERROR("Failed to wake up WorkQueue Thread");
> 
> You probably should update those error messages, since you changed it to be a DispatchQueue Thread. If you decide not to change it, at least make them both equal: "WorkQueue Thread" vs. "WorkQueueThread"
I missed the log messages. Thanks for pointing this. Will update it :)

[Nick Diego Yamane (diegoyam)]

I've just made some tests using the latest version of patch from https://bugs.webkit.org/show_bug.cgi?id=115332 + the WorkQueue's socketEventHandler cleanup at 'unregisterSocketEventHandler' function (proposed previously here).
I used this API test case (https://github.com/WebKitNix/webkitnix/blob/master/Tools/TestWebKitAPI/Tests/nix/WebViewWebProcessCrashed.cpp) from WebKitNix port, which forces WebProcess to crash/reload sequentially multiple times. Bellow some details about the results:

New implementation based on 'DispatchQueue' seems to be much more robust and easy to maintain/extend than the previous one, however at some point (usually after 400 crash/respawns) the test crashes with a ASSERTION fail at WTF::Mutex's destructor. More details here, gdb session log: http://paste.debian.net/8904/).

From that debug session I could see that a call to pthread_mutex_destroy() at Mutex's destructor was returning error code 16, which most probably means it was trying to destroy a locked mutex[1]. m_writeToPipeDescriptorLock was the mutex causing the problem. So, I propose (after the mentioned patch has been landed) to protect the DispatchQueue destruction using a MutexLocker on that mutex to prevent this concurrency issue between ~DispatchQueue and DispatchQueue::wakeup().

[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_mutex_init.html

[Nick Diego Yamane (diegoyam)]

(In reply to comment #28)

Sorry guys, this comment was meant to https://bugs.webkit.org/show_bug.cgi?id=114432 :)

[Byungwoo Lee]

Created attachment 204130 [details]
Patch

[Byungwoo Lee]

@Chris, could you please review this?

[Chris Dumez]

Comment on attachment 204130 [details]
Patch

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

Since you're touching the generic WK2 WorkQueue code, you'll likely need a WK2 owner to at least sign off on it.

> Source/WebKit2/ChangeLog:10
> +        Previously, WorkQueue class have all context about dispatch.

"the WorkQueue class had all the context..."

> Source/WebKit2/ChangeLog:12
> +        and those are accessed on the work queue thread through WorkQueue

"those were"

> Source/WebKit2/ChangeLog:16
> +        complicates handling workqueue ref-counting and makes dangling

"causes dangling"

> Source/WebKit2/Platform/WorkQueue.h:58
>  #include <Ecore.h>

Do we still need this include?

> Source/WebKit2/Platform/WorkQueue.h:172
> +    DispatchQueue* m_dispatchQueue;

Maybe a comment to explain why this is a raw pointer (and not a OwnPtr for e.g.)

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:2
> + * Copyright (C) 2012 Samsung Electronics. All rights reserved.

2012, 2013?

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:36
> +static const char wakeupThreadMessage = 'W';

"wakeUpThreadMessage"

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:57
> +PassOwnPtr<TimerWorkItem> TimerWorkItem::create(WorkQueue* workQueue, const Function<void()>& function, double delay)

Let's include the unit in the "delay" argument name for clarity. e.g. delaySeconds.

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:62
> +    double expireTime = currentTime() + delay;

expirationTime ?

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:63
> +    if (expireTime < 0)

How can this happen?

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:71
> +    , m_expireTime(expireTime)

m_expirationTime?

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:89
> +    m_threadLoop = true;

m_isThreadRunnning?
Could probably be initialized in the initializer list.

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:111
> +    if (!item)

Is this really normal? Maybe this can be replaced by an assertion?

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:118
> +void DispatchQueue::dispatchRelease()

Maybe something like "stopThread()" would be more easily understandable?

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:126
> +void DispatchQueue::registerSocketEventHandler(int fileDescriptor, const Function<void()>& function)

I would rename this to setSocketEventHandler() since only one can be set.

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:128
> +    if (m_socketDescriptor != invalidSocketDescriptor)

Can probably be an assertion.

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:139
> +void DispatchQueue::unregisterSocketEventHandler(int fileDescriptor)

I would rename this to clearSocketEventHandler() and remove the argument.

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:150
> +    while (true) {

Would while(m_threadLoop) make more sense here? (i.e. help exit earlier).

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:190
> +        // If a timer work item expired, dispatch the function of the work item.

This comment is not needed.

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:202
> +            if (read(m_readFromPipeDescriptor, &message, sizeof(char)) == -1)

char size is guaranteed to be 1 so we don't really need a sizeof.

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:215
> +    if (!item)

Why can this happen? Could this be replaced by an assertion?

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:221
> +    // m_timerWorkItems should be ordered by expire time.

"The items should be ordered by expiration time."

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:237
> +    delete dispatchQueue;

Maybe a comment to explain that the dispatchQueue needs to be destroyed in the thread?

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:241
> +void DispatchQueue::wakeup()

wakeUpThread?

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:248
> +struct timeval* DispatchQueue::getNextTimeOut()

This should probably be const.

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:254
> +    static struct timeval timeValue;

I don't think we need the "struct" since this is C++.

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:260
> +        timeValue.tv_usec = static_cast<long>((timeOut - timeValue.tv_sec) * 1000000);

1000000 should probably be a global static const variable for readability. e.g. "nanoSecondsPerSecond".

> Source/WebKit2/Platform/efl/DispatchQueue.h:52
> +    double expireTime() const { return m_expireTime; }

expirationTime

> Source/WebKit2/Platform/efl/DispatchQueue.h:53
> +    bool expired(double currentTime) const { return currentTime >= m_expireTime; }

"hasExpired"

> Source/WebKit2/Platform/efl/DispatchQueue.h:80
> +    struct timeval* getNextTimeOut();

struct probably not needed.

> Source/WebKit2/PlatformEfl.cmake:2
> +    Platform/efl/DispatchQueue.cpp

All the other files end with "Efl", why not this one?

[Byungwoo Lee]

Comment on attachment 204130 [details]
Patch

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

Thanks for quick review! :)
I'll apply those.

>> Source/WebKit2/Platform/efl/DispatchQueue.cpp:63
>> +    if (expireTime < 0)
> 
> How can this happen?

I just added this condition because 'delay' is signed value, but as you pointed, this looks redundant (and I cannot find the normal cases with negative delay).
Will remove this and use assertion for the 59, 60 line.

>> Source/WebKit2/Platform/efl/DispatchQueue.cpp:111
>> +    if (!item)
> 
> Is this really normal? Maybe this can be replaced by an assertion?

Your point is correct. item will be null when delay is negative, but I cannot find a normal cases with the negative delay.
Will change to use assertion.

>> Source/WebKit2/Platform/efl/DispatchQueue.cpp:150
>> +    while (true) {
> 
> Would while(m_threadLoop) make more sense here? (i.e. help exit earlier).

This loop will be finished after all work items are dispatched.
I think you thought the loop in DispatchQueue::dispatchQueueThread(), and the loop is checking m_threadLoop now.
If I misunderstood your comment, please let me know.

>> Source/WebKit2/PlatformEfl.cmake:2
>> +    Platform/efl/DispatchQueue.cpp
> 
> All the other files end with "Efl", why not this one?

I thought that 'Efl' post-fix is used for the port source files which contains port specific functions of common class. (e.g. WorkQueueEfl.cpp has EFL specific implementations of WorkQueue cleass)
There is no common class for 'DispatchQueue' so I didn't use that keyword.
But I also think that adding 'Efl' will be more clear. Will add it.

[Chris Dumez]

Comment on attachment 204130 [details]
Patch

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

>>> Source/WebKit2/Platform/efl/DispatchQueue.cpp:150
>>> +    while (true) {
>> 
>> Would while(m_threadLoop) make more sense here? (i.e. help exit earlier).
> 
> This loop will be finished after all work items are dispatched.
> I think you thought the loop in DispatchQueue::dispatchQueueThread(), and the loop is checking m_threadLoop now.
> If I misunderstood your comment, please let me know.

My point is that we may not want to process all the work items if the thread is being terminated.

[Byungwoo Lee]

Comment on attachment 204130 [details]
Patch

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

>>>> Source/WebKit2/Platform/efl/DispatchQueue.cpp:150
>>>> +    while (true) {
>>> 
>>> Would while(m_threadLoop) make more sense here? (i.e. help exit earlier).
>> 
>> This loop will be finished after all work items are dispatched.
>> I think you thought the loop in DispatchQueue::dispatchQueueThread(), and the loop is checking m_threadLoop now.
>> If I misunderstood your comment, please let me know.
> 
> My point is that we may not want to process all the work items if the thread is being terminated.

Ok, I can understand your point.
I think that m_threadLoop should not be false if there are work items in the dispatch queue, because each work item keeps WorkQueue reference, and the m_threadLoop is set as false at WorkQueue destructor.
(This means, m_threadLoop will be set as false only if there isn't any work item in the queue)
Do you think checking the condition still needed for the exceptional cases? How about adding ASSERT to check the status that m_threadLoop is false but there are still remained work items in the queue?

[Chris Dumez]

Comment on attachment 204130 [details]
Patch

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

>>>>> Source/WebKit2/Platform/efl/DispatchQueue.cpp:150
>>>>> +    while (true) {
>>>> 
>>>> Would while(m_threadLoop) make more sense here? (i.e. help exit earlier).
>>> 
>>> This loop will be finished after all work items are dispatched.
>>> I think you thought the loop in DispatchQueue::dispatchQueueThread(), and the loop is checking m_threadLoop now.
>>> If I misunderstood your comment, please let me know.
>> 
>> My point is that we may not want to process all the work items if the thread is being terminated.
> 
> Ok, I can understand your point.
> I think that m_threadLoop should not be false if there are work items in the dispatch queue, because each work item keeps WorkQueue reference, and the m_threadLoop is set as false at WorkQueue destructor.
> (This means, m_threadLoop will be set as false only if there isn't any work item in the queue)
> Do you think checking the condition still needed for the exceptional cases? How about adding ASSERT to check the status that m_threadLoop is false but there are still remained work items in the queue?

thanks for clarifying, you can leave as it is then.

[Byungwoo Lee]

Created attachment 204917 [details]
Patch

[Sergio Correia (qrwteyrutiyoup)]

Comment on attachment 204917 [details]
Patch

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

> Source/WebKit2/Platform/WorkQueue.h:172
> +    // The instance is passed to it's own thread function and deleted in it.

Nit: its own thread

> Source/WebKit2/Platform/efl/DispatchQueueEfl.cpp:35
> +static const int nanoSecondsPerSecond = 1000000;

I guess it should be "microSecondsPerSecond", no?

> Source/WebKit2/Platform/efl/DispatchQueueEfl.cpp:258
> +        timeValue.tv_usec = static_cast<long>((timeOut - timeValue.tv_sec) * nanoSecondsPerSecond);

Yes, this is in microseconds, not nanoseconds :)

[Byungwoo Lee]

Created attachment 204954 [details]
Patch

[Byungwoo Lee]

(In reply to comment #38)
> (From update of attachment 204917 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204917&action=review
> 
> > Source/WebKit2/Platform/WorkQueue.h:172
> > +    // The instance is passed to it's own thread function and deleted in it.
> 
> Nit: its own thread
Done.

> 
> > Source/WebKit2/Platform/efl/DispatchQueueEfl.cpp:35
> > +static const int nanoSecondsPerSecond = 1000000;
> 
> I guess it should be "microSecondsPerSecond", no?
> 
> > Source/WebKit2/Platform/efl/DispatchQueueEfl.cpp:258
> > +        timeValue.tv_usec = static_cast<long>((timeOut - timeValue.tv_sec) * nanoSecondsPerSecond);
> 
> Yes, this is in microseconds, not nanoseconds :)
Done. Thanks!

[Byungwoo Lee]

Created attachment 212951 [details]
Patch

Rebased

[Sergio Correia (qrwteyrutiyoup)]

Anders, could you take a look at this patch as a WK2 owner?

[Byungwoo Lee]

Created attachment 213339 [details]
Patch

Change for WorkQueue to contain the RefPtr<DispatchQueue> instead of DispatchQueue raw pointer to prevent concurrency issues.

[Sergio Correia (qrwteyrutiyoup)]

(In reply to comment #43)
> Created an attachment (id=213339) [details]
> Patch
> 
> Change for WorkQueue to contain the RefPtr<DispatchQueue> instead of DispatchQueue raw pointer to prevent concurrency issues.

Given recent patches (e.g. [1][2]) getting rid of OwnPtr/PassOwnPtr, it might be a good idea to do the same here.

[1] https://bugs.webkit.org/show_bug.cgi?id=122136
[2] https://bugs.webkit.org/show_bug.cgi?id=122086

[Byungwoo Lee]

(In reply to comment #44)
> (In reply to comment #43)
> > Created an attachment (id=213339) [details] [details]
> > Patch
> > 
> > Change for WorkQueue to contain the RefPtr<DispatchQueue> instead of DispatchQueue raw pointer to prevent concurrency issues.
> 
> Given recent patches (e.g. [1][2]) getting rid of OwnPtr/PassOwnPtr, it might be a good idea to do the same here.
> 
> [1] https://bugs.webkit.org/show_bug.cgi?id=122136
> [2] https://bugs.webkit.org/show_bug.cgi?id=122086

Thank you for the comment. Will apply it.

[Byungwoo Lee]

Created attachment 213373 [details]
Change to apply unique_ptr

I tried to apply std::unique_ptr to the patch, but it needs additional changes of WTF::Vector::insert() functions because it haven't been applied the unique_ptr.
The additional diff might be as attached, and I think this will be better to handle separately.
Sergio, Is it ok to apply the unique_ptr later?

[Sergio Correia (qrwteyrutiyoup)]

(In reply to comment #46)
> Created an attachment (id=213373) [details]
> Change to apply unique_ptr
> 
> I tried to apply std::unique_ptr to the patch, but it needs additional changes of WTF::Vector::insert() functions because it haven't been applied the unique_ptr.
> The additional diff might be as attached, and I think this will be better to handle separately.
> Sergio, Is it ok to apply the unique_ptr later?

Yes, I think it's fine if we handle the unique_ptr conversion separately, in a further patch. 

We just need a WK2 owner to review the patch now :)

[Byungwoo Lee]

Comment on attachment 213373 [details]
Change to apply unique_ptr

unique_ptr will be applied later.

[Byungwoo Lee]

Created attachment 213647 [details]
Patch

Applied unique_ptr - WTF::Vector::insert() can handle unique_ptr after r157074

[Anders Carlsson]

Comment on attachment 213647 [details]
Patch

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

> Source/WebKit2/Platform/efl/DispatchQueueEfl.cpp:49
> +    : m_dispatchQueue(dispatchQueue)

This should be indented.

> Source/WebKit2/Platform/efl/DispatchQueueEfl.cpp:73
> +    m_workQueue->ref();

Can't you just make m_workQueue a RefPtr?

[Byungwoo Lee]

Comment on attachment 213647 [details]
Patch

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

>> Source/WebKit2/Platform/efl/DispatchQueueEfl.cpp:49
>> +    : m_dispatchQueue(dispatchQueue)
> 
> This should be indented.

Oops, I'll fix it.

>> Source/WebKit2/Platform/efl/DispatchQueueEfl.cpp:73
>> +    m_workQueue->ref();
> 
> Can't you just make m_workQueue a RefPtr?

Yes, I can use RefPtr. I'll apply this with a separated header file to prevent circular header include.
Thanks to point this :)

[Byungwoo Lee]

Created attachment 213740 [details]
Patch

Applied the comment from Anders.

[WebKit Commit Bot]

Comment on attachment 213740 [details]
Patch

Clearing flags on attachment: 213740

Committed r157289: <http://trac.webkit.org/changeset/157289>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.