Bug 100288 – [EFL][WK2][AC] Regression(132392) infinite loop when displaying certain animations.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Yael]

In EFL, we do the painting in the main loop, not in a thread, and r132392 seems to introduce an assumption that there is a second thread.
This infinite loop happens when loading e.g. http://trac.webkit.org/browser/trunk/Websites/webkit.org/blog-files/3d-transforms/poster-circle.html 

Bellow is a partial backtrace showing the problem:

41	ewk_view_display	ewk_view.cpp	1271	0x7ffff481f938	
42	WebKit::PageClientImpl::setViewNeedsDisplay	PageClientImpl.cpp	70	0x7ffff47fde0f	
43	WebKit::WebPageProxy::setViewNeedsDisplay	WebPageProxy.cpp	829	0x7ffff466e1c2	
44	WebKit::DrawingAreaProxy::updateViewport	DrawingAreaProxy.cpp	64	0x7ffff461a796	
45	WebKit::LayerTreeCoordinatorProxy::updateViewport	LayerTreeCoordinatorProxy.cpp	51	0x7ffff46cfacd	
46	WebKit::LayerTreeRenderer::updateViewport	LayerTreeRenderer.cpp	175	0x7ffff46d5a2b	
47	WTF::FunctionWrapper<void (WebKit::LayerTreeRenderer::*)()>::operator()	Functional.h	174	0x7ffff46d4d84	
48	WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (WebKit::LayerTreeRenderer::*)()>, void (WebKit::LayerTreeRenderer*)>::operator()()	Functional.h	406	0x7ffff46d4422	
49	WTF::Function<void ()>::operator()() const	Functional.h	614	0x7ffff46d7ee4	
50	WebKit::LayerTreeRenderer::dispatchOnMainThread(WTF::Function<void ()> const&)	LayerTreeRenderer.cpp	72	0x7ffff46d50a1	
51	WebKit::LayerTreeRenderer::paintToCurrentGLContext	LayerTreeRenderer.cpp	140	0x7ffff46d57cd	
52	WebKit::PageViewportControllerClientEfl::display	PageViewportControllerClientEfl.cpp	73	0x7ffff47fd374	
53	ewk_view_display	ewk_view.cpp	1271	0x7ffff481f938	
54	WebKit::PageClientImpl::setViewNeedsDisplay	PageClientImpl.cpp	70	0x7ffff47fde0f	
55	WebKit::WebPageProxy::setViewNeedsDisplay	WebPageProxy.cpp	829	0x7ffff466e1c2	
56	WebKit::DrawingAreaProxy::updateViewport	DrawingAreaProxy.cpp	64	0x7ffff461a796	
57	WebKit::LayerTreeCoordinatorProxy::updateViewport	LayerTreeCoordinatorProxy.cpp	51	0x7ffff46cfacd	
58	WebKit::LayerTreeRenderer::updateViewport	LayerTreeRenderer.cpp	175	0x7ffff46d5a2b	
59	WTF::FunctionWrapper<void (WebKit::LayerTreeRenderer::*)()>::operator()	Functional.h	174	0x7ffff46d4d84	
60	WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (WebKit::LayerTreeRenderer::*)()>, void (WebKit::LayerTreeRenderer*)>::operator()()	Functional.h	406	0x7ffff46d4422	
61	WTF::Function<void ()>::operator()() const	Functional.h	614	0x7ffff46d7ee4	
62	WebKit::LayerTreeRenderer::dispatchOnMainThread(WTF::Function<void ()> const&)	LayerTreeRenderer.cpp	72	0x7ffff46d50a1	
63	WebKit::LayerTreeRenderer::paintToCurrentGLContext	LayerTreeRenderer.cpp	140	0x7ffff46d57cd	
64	WebKit::PageViewportControllerClientEfl::display	PageViewportControllerClientEfl.cpp	73	0x7ffff47fd374	
65	ewk_view_display	ewk_view.cpp	1271	0x7ffff481f938

[Noam Rosenthal]

the assumption is not that there is a secondary thread - the assumption is that setViewNeedsDIsplay puts a message on the event loop rather than calling display directly...

[Yael]

Created attachment 170535 [details]
Patch

Set a 0 length timer when PageClientImpl::setViewNeedsDisplay is called. That makes calling ewk_view_display asynchronous and breaks the loop of ewk_view_display triggering another ewk_view_display (due to the animation) .

[Kenneth Rohde Christiansen]

Comment on attachment 170535 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:75
> +    DisplayData* displayData = new DisplayData;
> +    displayData->ewkView = m_viewWidget;
> +    displayData->viewRect = rect;
> +
> +    ecore_timer_add(0, ewk_view_display_async, static_cast<void*>(displayData));

Doesn't webcore have abstractions for these ecore timers already? Why not use webcore timers?

[Yael]

(In reply to comment #3)
> (From update of attachment 170535 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170535&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/PageClientImpl.cpp:75
> > +    DisplayData* displayData = new DisplayData;
> > +    displayData->ewkView = m_viewWidget;
> > +    displayData->viewRect = rect;
> > +
> > +    ecore_timer_add(0, ewk_view_display_async, static_cast<void*>(displayData));
> 
> Doesn't webcore have abstractions for these ecore timers already? Why not use webcore timers?
ok, it will look nicer :)

[Yael]

Created attachment 170628 [details]
Patch

Rebase the patch and take comment #3 into account.

[Kenneth Rohde Christiansen]

Comment on attachment 170628 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:265
> +    while (m_dirtyRects.size()) {
> +        dirtyRegion.unite(m_dirtyRects.first());
> +        m_dirtyRects.remove(0);
> +    }

con't you not just do m_dirtyRects.clear() or so afterward? Is remove really safe here?

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:268
> +    Vector<IntRect> rects = dirtyRegion.rects();
> +    Vector<IntRect>::iterator end = rects.end();

how are regions united? do you really get multiple regions afterward?

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:285
> +{
> +    if (!m_displayTimer.isActive())
> +        m_displayTimer.startOneShot(0);
> +    m_dirtyRects.append(rect);

does this actually happen?

[Yael]

(In reply to comment #6)
> (From update of attachment 170628 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170628&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:265
> > +    while (m_dirtyRects.size()) {
> > +        dirtyRegion.unite(m_dirtyRects.first());
> > +        m_dirtyRects.remove(0);
> > +    }
> 
> con't you not just do m_dirtyRects.clear() or so afterward? Is remove really safe here?
> 
Since I always take the first item, I think remove is safe here.

> > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:268
> > +    Vector<IntRect> rects = dirtyRegion.rects();
> > +    Vector<IntRect>::iterator end = rects.end();
> 
> how are regions united? do you really get multiple regions afterward?
> 
I get multiple rects before drawing, not regions. Using the region removes the need for redundant drawing.

> > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:285
> > +{
> > +    if (!m_displayTimer.isActive())
> > +        m_displayTimer.startOneShot(0);
> > +    m_dirtyRects.append(rect);
> 
> does this actually happen?
yes, with the test page listed in comment #0 .

[Kenneth Rohde Christiansen]

Comment on attachment 170628 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:265
>>> +    }
>> 
>> con't you not just do m_dirtyRects.clear() or so afterward? Is remove really safe here?
> 
> Since I always take the first item, I think remove is safe here.

Isn't thre a removeFirst() ? anyway this might not be very efficient. Why not just iterate and call clear() afterward

[Yael]

(In reply to comment #8)
> (From update of attachment 170628 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170628&action=review
> 
> >>> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:265
> >>> +    }
> >> 
> >> con't you not just do m_dirtyRects.clear() or so afterward? Is remove really safe here?
> > 
> > Since I always take the first item, I think remove is safe here.
> 
> Isn't thre a removeFirst() ? anyway this might not be very efficient. Why not just iterate and call clear() afterward

I was surprised to find that there is removeLast(), but no removeFirst() :)

[Yael]

Created attachment 170643 [details]
Patch for landing.

Clear all the rects at once.

[Kenneth Rohde Christiansen]

Comment on attachment 170643 [details]
Patch for landing.

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

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:262
> +    for (Vector<IntRect>::iterator it = m_dirtyRects.begin(); it != m_dirtyRects.end(); ++it)

I would do end out side

[WebKit Review Bot]

Comment on attachment 170643 [details]
Patch for landing.

Clearing flags on attachment: 170643

Committed r132483: <http://trac.webkit.org/changeset/132483>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Comment on attachment 170643 [details]
Patch for landing.

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

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:238
> +    WTF::Vector <WebCore::IntRect> m_dirtyRects;

Just a thought but why store the dirty rects in a Vector if we are going to construct a Region from them anyway?
We could have a "Region m_dirtyRegion;" member instead and we would call Region::unite() in redrawRegion(). This would avoid the iteration over m_dirtyRects in the timer callback.

[Yael]

(In reply to comment #14)
> (From update of attachment 170643 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170643&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:238
> > +    WTF::Vector <WebCore::IntRect> m_dirtyRects;
> 
> Just a thought but why store the dirty rects in a Vector if we are going to construct a Region from them anyway?
> We could have a "Region m_dirtyRegion;" member instead and we would call Region::unite() in redrawRegion(). This would avoid the iteration over m_dirtyRects in the timer callback.

The issue is that we are adding more rects while we are painting. We have to separate the already existing rects from the newly added, or we will end up with the same infinite loop I was trying to solve.

[Chris Dumez]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 170643 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=170643&action=review
> > 
> > > Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:238
> > > +    WTF::Vector <WebCore::IntRect> m_dirtyRects;
> > 
> > Just a thought but why store the dirty rects in a Vector if we are going to construct a Region from them anyway?
> > We could have a "Region m_dirtyRegion;" member instead and we would call Region::unite() in redrawRegion(). This would avoid the iteration over m_dirtyRects in the timer callback.
> 
> The issue is that we are adding more rects while we are painting. We have to separate the already existing rects from the newly added, or we will end up with the same infinite loop I was trying to solve.

I'm pretty sure we can achieve this by replacing the Vector with a Region. Maybe I did not explain it clearly enough so I will upload a patch for it at Bug 100736.