Bug 91179 – [EFL][WK2] WorkQueue::dispatchAfterDelay() doesn't work properly.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Byungwoo Lee]

Sometimes, when UI Process is crashed, watchdocCallback() function in the ChildProcess.cpp doesn't triggered.
And found that, WorkQueue::dispatchAfterDelay() function uses ecore timer for getting timer event.

Ecore timer event will be processed by the ecore main loop,
and ecore main loop is in the main run loop, not in the WorkQueue thread.

When the UI Process is crashed, and WebProcess's ecore main loop is very busy or lockup also,
watchdogCallback() should be called to terminate WebProcess itself.
But WorkQueu::dispatchAfterDelay() uses ecore timer for timer event,
the watchdoc also cannot be trigerred.

The way to get timer event should be changed to use some other way like timerfd.

[Byungwoo Lee]

Created attachment 152514 [details]
Patch

[Gyuyoung Kim]

It looks I'm not an expert for this patch yet. So, I would like to listen youngtaeck's opinion about this patch.

[Kangil Han]

@Youngtaeck: Could you please review this patch?

[Byungwoo Lee]

Created attachment 158212 [details]
Patch

[Byungwoo Lee]

Applying offline suggestion from Youngtaeck about dispatching order of TimerWorkItems.
For the exact dispatching order between timer work items, new TimerWorkItem instance will be added to the m_timerWorkItems by order of expiration time.

[YoungTaeck Song]

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

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:54
> +    if ((timerFileDescriptor = timerfd_create(CLOCK_MONOTONIC, TFD_NONBLOCK)) < 0) {

One TimerWorkItem has one file descriptor. Right? If some system has many open files near system-max, creating timer will fail.
I think it is good that there is no dependency with timer and files.

[Byungwoo Lee]

Ok~ Instead of using file for every timer items, I'll use the last parameter of select() function(timeout) for expiration of timer items.
Thanks for the comments :)

[Byungwoo Lee]

Created attachment 158544 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 158544 [details]
Patch

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

> Source/WebKit2/ChangeLog:13
> +        For removing the dependency between the dispatchAfterDelay() and ecore

I would add a newline before that last line

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:110
> +    return static_cast<double>(timeValue.tv_sec) + (static_cast<double>(timeValue.tv_usec) / 1000000);

Is it enough to casting one of two operands ?

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

Add a space around *.

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:126
> +    }

Add a newline to here.

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:143
> +    for (size_t i = 0; i < timerWorkItems.size(); i++) {

WebKit prefers to use ++i.

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:215
> +    size_t position = 0;

Why do you define position variable to here instead of for (...) ?

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:216
> +    for (; position < m_timerWorkItems.size(); position++)

ditto.

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:218
> +            break;

Add a newline to here.

[Byungwoo Lee]

(In reply to comment #9)
Thanks for the comments. I'll apply all those.

> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:215
> > +    size_t position = 0;
> 
> Why do you define position variable to here instead of for (...) ?

This variable need to be used out of the for loop block.
If this is defined in the for loop statement, build will be failed.

Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:219
+    m_timerWorkItems.insert(position, timerWorkItem.release());

[Byungwoo Lee]

Created attachment 158579 [details]
Patch

[Kangil Han]

gyuyoung, youngtaeck: review please?

[Gyuyoung Kim]

Comment on attachment 158579 [details]
Patch

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

This patch is needed to review by youngtaeck first informally.

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:151
> +        timerWorkItems[i]->dispatch();

I think a comment is needed to explain this logic.

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:153
> +    }

It looks a new line is needed.

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:217
> +    for (; position < m_timerWorkItems.size(); position++)

WebKit prefers to use ++position.

[Byungwoo Lee]

Comment on attachment 158579 [details]
Patch

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

>> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:151
>> +        timerWorkItems[i]->dispatch();
> 
> I think a comment is needed to explain this logic.

Ok~ I'll add the comment

>> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:153
>> +    }
> 
> It looks a new line is needed.

Ok I'll apply it

>> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:217
>> +    for (; position < m_timerWorkItems.size(); position++)
> 
> WebKit prefers to use ++position.

Sorry for missing this again. will apply it.

[Byungwoo Lee]

Created attachment 161120 [details]
Patch

[WebKit Review Bot]

Comment on attachment 161120 [details]
Patch

Attachment 161120 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13695052

New failing tests:
CCLayerTreeHostTestScrollChildLayer.runMultiThread

[Byungwoo Lee]

Created attachment 161659 [details]
Patch

[Byungwoo Lee]

@Youngtaeck: Could you please review this patch?

[YoungTaeck Song]

(In reply to comment #18)
> @Youngtaeck: Could you please review this patch?

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

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:103
> +{

Why don't you use currentTime() in wtf/currentTime.cpp? It is same with this function.

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:120
> +    timeValue.tv_sec = 1;

If timeOut is 0, select() will wait 1 sec.
I think expired timer's callback have to be called right away.

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:156
> +            m_timerWorkItems.insert(position, timerWorkItems[i].release());

Line 151-156 are almost same with line 231-236. Why don't you use one function?

[Byungwoo Lee]

(In reply to comment #19)
> (In reply to comment #18)
> > @Youngtaeck: Could you please review this patch?
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=161659&action=review
> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:103
> > +{
> 
> Why don't you use currentTime() in wtf/currentTime.cpp? It is same with this function.
Ok I'll apply it.

> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:120
> > +    timeValue.tv_sec = 1;
> 
> If timeOut is 0, select() will wait 1 sec.
> I think expired timer's callback have to be called right away.
Yes, It's a terrible mistake. Thanks for pointing it.
Initially I was going to set usec = 1 for this case,
but as we shared in offline, set all 0 will be better.
I'll fix it.

> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:156
> > +            m_timerWorkItems.insert(position, timerWorkItems[i].release());
> 
> Line 151-156 are almost same with line 231-236. Why don't you use one function?
Ok, I'll apply it.

Thanks for the review :)

[Byungwoo Lee]

Created attachment 163311 [details]
Patch

[YoungTaeck Song]

(In reply to comment #21)
> Created an attachment (id=163311) [details]
> Patch

LGTM!! :)

[Gyuyoung Kim]

Comment on attachment 163311 [details]
Patch

Set r+ based on informal review.

[WebKit Review Bot]

Comment on attachment 163311 [details]
Patch

Clearing flags on attachment: 163311

Committed r128289: <http://trac.webkit.org/changeset/128289>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.