[Webkit-unassigned] [Bug 172167] [Threaded Compositor] SHOULD NEVER BE REACHED in WebKit::CompositingRunLoop::updateCompleted

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=172167

--- Comment #12 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to Zan Dobersek from comment #11)
> Comment on attachment 310260 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310260&action=review
> 
> >>>>> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:292
> >>>>> +            return;
> >>>> 
> >>>> m_coordinateUpdateCompletion should be set here, but to false if m_inForceRepaint is false as well.
> >>> 
> >>> That happens, because it doesn't return early.
> >> 
> >> I'm saying it should be set regardless of the value of m_inForceRepaint. But if m_inForceRepaint is false, it should be set to false.
> >> 
> >>     bool coordinateUpdate = m_inForceRepaint && std::any_of(...
> > 
> > That's what caused the timeout, and I suspect bug #172027 too. If we change coordinateUpdate for the force repaint, the frame is never completed. If the state is PendingAfterCompletion, we won't process more updates. So, what we do here is handling the force update like it didn't happen, to ensure frame updates continue working as expected.
> 
> When do you expect a forced updated to be finished? And do you expect the
> DisplayRefreshMonitor callback to be scheduled for forced updates?
> 
> You also don't have any guarantee that there is a scene state update pending
> that would be executed during a forced repaint, meaning
> m_clientRendersNextFrame isn't necessarily flipped to true, which would end
> up not scheduling a DisplayRefreshMonitor callback in sceneUpdateFinished()
> that would complete the update.

A fore repaint is always synchronous, it calls renderLayerTree directly so it's not expected to call updateCompleted(). I don't know if we should do anything special with the refresh monitor in that case. These are the situations that can happen when a forceRepaint is scheduled.

 1) Previous update is complete. If updateCompleted() is called, it will crash in SHOULD NEVER BE REACHED.

 2) Previous frame is InProgress and m_coordinateUpdateCompletionWithClient is false. If the frame renderLayerTree happens before the force repaint, then updateCompleted() will be called, so it would be like previous case. If it happens after, the forceRepaint one will not calle updateCompleted() and the InProgress frame will. At some point the scene is committed and the update lambda is called. If the force repaint happens before the update would do nothing, leaving m_coordinateUpdateCompletionWithClient as false. When completeCoordinatedUpdateIfNeeded is called, m_coordinateUpdateCompletionWithClient is still false as expected and updateCompleted() is not called (it was already called once for this frame).

 3) Previous frame is InProgress and m_coordinateUpdateCompletionWithClient is true. In this case updateCompleted() won't be called no matter if one renderLayerTree is called before the other. But at some point the scene is committed and the update lambda is called. If the force repaint happens before the update would do nothing, leaving the m_coordinateUpdateCompletionWithClient as true. When completeCoordinatedUpdateIfNeeded is called, m_coordinateUpdateCompletionWithClient is still true as expected and updateCompleted() is called.

 4) Previous frame if in PendingAfterCompletion. So this is like 2) or 3) but another frame has been scheduled and should be fired after previous one finishes.

1) was causing SHOULD NEVER BE REACHED, and it was fixed in r216182. 

In 2) and 3) the value of m_coordinateUpdateCompletionWithClient might be changed in an unexpected way by the forceRepaint commit, causing updateCompleted() to be called twice and then crashing in SHOULD NEVER BE REACHED. This was fixed by this bug.

In 4) the value of m_coordinateUpdateCompletionWithClient might be changed in an unexpected way by the forceRepaint commit, causing updateCompleted() to never be called, causing the timeout in imported/blink/compositing/layer-creation/incremental-destruction.html and maybe bug #172027 too. This was fixed by this bug.

Maybe there are still cases not considered or we need to do something else with the refresh monitor, but at least after r216970 all tests crashing in SHOULD NEVER BE REACHED stopped crashing, test imported/blink/compositing/layer-creation/incremental-destruction.html is no longer timing out, and Adri can use github again. I admit I don't completely understand the whole frame update mechanism and refresh monitor, but it's a lot more stable now. Let's improve it on top of this if needed.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170518/7fd56b0b/attachment.html>