| Summary: | [EFL][WK2][AC] Regression(132392) infinite loop when displaying certain animations. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yael <yael> | ||||||||
| Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | cdumez, gyuyoung.kim, kenneth, noam, rakuco, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Yael
2012-10-24 14:18:50 PDT
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... 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) .
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? (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 :) Created attachment 170628 [details] Patch Rebase the patch and take comment #3 into account. 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? (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 . 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 (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() :) Created attachment 170643 [details]
Patch for landing.
Clear all the rects at once.
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 Comment on attachment 170643 [details] Patch for landing. Clearing flags on attachment: 170643 Committed r132483: <http://trac.webkit.org/changeset/132483> All reviewed patches have been landed. Closing bug. 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. (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. (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. |