Bug 91179 - [EFL][WK2] WorkQueue::dispatchAfterDelay() doesn't work properly.
Summary: [EFL][WK2] WorkQueue::dispatchAfterDelay() doesn't work properly.
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: Byungwoo Lee
URL:
Keywords:
Depends on:
Blocks: 61838
  Show dependency treegraph
 
Reported: 2012-07-12 18:17 PDT by Byungwoo Lee
Modified: 2012-09-12 03:19 PDT (History)
11 users (show)

See Also:


Attachments
Patch (8.75 KB, patch)
2012-07-16 05:14 PDT, Byungwoo Lee
no flags Details | Formatted Diff | Diff
Patch (9.93 KB, patch)
2012-08-13 22:13 PDT, Byungwoo Lee
no flags Details | Formatted Diff | Diff
Patch (8.47 KB, patch)
2012-08-15 04:15 PDT, Byungwoo Lee
no flags Details | Formatted Diff | Diff
Patch (8.54 KB, patch)
2012-08-15 09:32 PDT, Byungwoo Lee
no flags Details | Formatted Diff | Diff
Patch (9.35 KB, patch)
2012-08-28 19:46 PDT, Byungwoo Lee
no flags Details | Formatted Diff | Diff
Patch (9.37 KB, patch)
2012-08-31 04:10 PDT, Byungwoo Lee
no flags Details | Formatted Diff | Diff
Patch (9.05 KB, patch)
2012-09-11 02:30 PDT, Byungwoo Lee
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Byungwoo Lee 2012-07-12 18:17:45 PDT
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.
Comment 1 Byungwoo Lee 2012-07-16 05:14:26 PDT
Created attachment 152514 [details]
Patch
Comment 2 Gyuyoung Kim 2012-07-24 23:37:18 PDT
It looks I'm not an expert for this patch yet. So, I would like to listen youngtaeck's opinion about this patch.
Comment 3 Kangil Han 2012-08-09 23:48:28 PDT
@Youngtaeck: Could you please review this patch?
Comment 4 Byungwoo Lee 2012-08-13 22:13:20 PDT
Created attachment 158212 [details]
Patch
Comment 5 Byungwoo Lee 2012-08-13 22:23:41 PDT
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.
Comment 6 YoungTaeck Song 2012-08-14 00:50:54 PDT
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.
Comment 7 Byungwoo Lee 2012-08-14 02:53:52 PDT
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 :)
Comment 8 Byungwoo Lee 2012-08-15 04:15:44 PDT
Created attachment 158544 [details]
Patch
Comment 9 Gyuyoung Kim 2012-08-15 06:49:07 PDT
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.
Comment 10 Byungwoo Lee 2012-08-15 09:26:13 PDT
(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());
Comment 11 Byungwoo Lee 2012-08-15 09:32:56 PDT
Created attachment 158579 [details]
Patch
Comment 12 Kangil Han 2012-08-26 22:43:42 PDT
gyuyoung, youngtaeck: review please?
Comment 13 Gyuyoung Kim 2012-08-27 19:58:28 PDT
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.
Comment 14 Byungwoo Lee 2012-08-28 19:44:57 PDT
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.
Comment 15 Byungwoo Lee 2012-08-28 19:46:02 PDT
Created attachment 161120 [details]
Patch
Comment 16 WebKit Review Bot 2012-08-28 23:11:01 PDT
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
Comment 17 Byungwoo Lee 2012-08-31 04:10:10 PDT
Created attachment 161659 [details]
Patch
Comment 18 Byungwoo Lee 2012-09-06 18:52:19 PDT
@Youngtaeck: Could you please review this patch?
Comment 19 YoungTaeck Song 2012-09-11 00:30:30 PDT
(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?
Comment 20 Byungwoo Lee 2012-09-11 00:39:50 PDT
(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 :)
Comment 21 Byungwoo Lee 2012-09-11 02:30:11 PDT
Created attachment 163311 [details]
Patch
Comment 22 YoungTaeck Song 2012-09-12 00:38:46 PDT
(In reply to comment #21)
> Created an attachment (id=163311) [details]
> Patch

LGTM!! :)
Comment 23 Gyuyoung Kim 2012-09-12 02:35:06 PDT
Comment on attachment 163311 [details]
Patch

Set r+ based on informal review.
Comment 24 WebKit Review Bot 2012-09-12 03:19:36 PDT
Comment on attachment 163311 [details]
Patch

Clearing flags on attachment: 163311

Committed r128289: <http://trac.webkit.org/changeset/128289>
Comment 25 WebKit Review Bot 2012-09-12 03:19:42 PDT
All reviewed patches have been landed.  Closing bug.