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.
Created attachment 152514 [details] Patch
It looks I'm not an expert for this patch yet. So, I would like to listen youngtaeck's opinion about this patch.
@Youngtaeck: Could you please review this patch?
Created attachment 158212 [details] Patch
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.
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.
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 :)
Created attachment 158544 [details] Patch
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.
(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());
Created attachment 158579 [details] Patch
gyuyoung, youngtaeck: review please?
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 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.
Created attachment 161120 [details] Patch
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
Created attachment 161659 [details] Patch
(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?
(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 :)
Created attachment 163311 [details] Patch
(In reply to comment #21) > Created an attachment (id=163311) [details] > Patch LGTM!! :)
Comment on attachment 163311 [details] Patch Set r+ based on informal review.
Comment on attachment 163311 [details] Patch Clearing flags on attachment: 163311 Committed r128289: <http://trac.webkit.org/changeset/128289>
All reviewed patches have been landed. Closing bug.