Bug 122016 – [CoordinatedGraphics] ASSERTION FAILED: !m_flushingLayers (after r156291)

• Component: New Bugs
• Status: RESOLVED FIXED

[Sergio Correia (qrwteyrutiyoup)]

After r156291, we are hitting the !m_flushingLayers assert in WebCore/rendering/RenderLayerCompositor.cpp(358), which was added in such revision.

The EFL trace follows:

./WebKitBuild/Debug/bin/MiniBrowser 
HTML5 local storage is enabled for this view.
GLib-GIO-Message: Using the 'memory' GSettings backend.  Your settings will not be saved or shared with other applications.
ASSERTION FAILED: !m_flushingLayers
/home/sergio/projects/webkit/Source/WebCore/rendering/RenderLayerCompositor.cpp(358) : void WebCore::RenderLayerCompositor::scheduleLayerFlush(bool)
1   0x7feeaa8d4bb7 WTFCrash
2   0x7feea68e7d1a WebCore::RenderLayerCompositor::scheduleLayerFlush(bool)
3   0x7feea68e7c7e WebCore::RenderLayerCompositor::notifyFlushRequired(WebCore::GraphicsLayer const*)
4   0x7feea66d9d7c WebCore::CoordinatedGraphicsLayer::didChangeLayerState()
5   0x7feea66dbf30 WebCore::CoordinatedGraphicsLayer::syncImageBacking()
6   0x7feea66dc7b0 WebCore::CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly()
7   0x7feea66dbb8c WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&)
8   0x7feea68e7f4b WebCore::RenderLayerCompositor::flushPendingLayerChanges(bool)
9   0x7feea65511b3 WebCore::FrameView::flushCompositingStateForThisFrame(WebCore::Frame*)
10  0x7feea65515f7 WebCore::FrameView::flushCompositingStateIncludingSubframes()
11  0x7feea66cb199 WebCore::CompositingCoordinator::flushPendingLayerChanges()
12  0x7feea2df3d59 WebKit::CoordinatedLayerTreeHost::forceRepaint()
13  0x7feea2db9586 WebKit::DrawingAreaImpl::sendDidUpdateBackingStoreState()
14  0x7feea2db9348 WebKit::DrawingAreaImpl::updateBackingStoreState(unsigned long, bool, float, WebCore::IntSize const&, WebCore::IntSize const&)
15  0x7feea2eac63f void CoreIPC::callMemberFunction<WebKit::DrawingArea, void (WebKit::DrawingArea::*)(unsigned long, bool, float, WebCore::IntSize const&, WebCore::IntSize const&), unsigned long, bool, float, WebCore::IntSize, WebCore::IntSize>(std::tuple<unsigned long, bool, float, WebCore::IntSize, WebCore::IntSize>&&, WebKit::DrawingArea*, void (WebKit::DrawingArea::*)(unsigned long, bool, float, WebCore::IntSize const&, WebCore::IntSize const&))
16  0x7feea2eac230 void CoreIPC::handleMessage<Messages::DrawingArea::UpdateBackingStoreState, WebKit::DrawingArea, void (WebKit::DrawingArea::*)(unsigned long, bool, float, WebCore::IntSize const&, WebCore::IntSize const&)>(CoreIPC::MessageDecoder&, WebKit::DrawingArea*, void (WebKit::DrawingArea::*)(unsigned long, bool, float, WebCore::IntSize const&, WebCore::IntSize const&))
17  0x7feea2eabe97 WebKit::DrawingArea::didReceiveDrawingAreaMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
18  0x7feea2ddc97a WebKit::WebPage::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
19  0x7feea2b85fd6 CoreIPC::MessageReceiverMap::dispatchMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
20  0x7feea2d0745d WebKit::WebProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
21  0x7feea2b73d8e CoreIPC::Connection::dispatchMessage(CoreIPC::MessageDecoder&)
22  0x7feea2b73e5a CoreIPC::Connection::dispatchMessage(std::unique_ptr<CoreIPC::MessageDecoder, std::default_delete<CoreIPC::MessageDecoder> >)
23  0x7feea2b7401b CoreIPC::Connection::dispatchOneMessage()
24  0x7feea2b852e3 WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>::operator()(CoreIPC::Connection*)
25  0x7feea2b84e6a WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>, void (CoreIPC::Connection*)>::operator()()
26  0x7feeaa8da4d7 WTF::Function<void ()>::operator()() const
27  0x7feea65ef658 WebCore::RunLoop::performWork()
28  0x7feea70d7b0a WebCore::RunLoop::wakeUpEvent(void*, void*, unsigned int)
29  0x7fee9e20b1cb
30  0x7fee9e20a271
31  0x7fee9e20a6b7 ecore_main_loop_begin
ERR<17489>:elementary els_tooltip.c:906 elm_object_tooltip_unset() Object does not have tooltip: obj

[Simon Fraser (smfr)]

It is incorrect to call scheduleLayerFlush() inside of flushCompositingState().

[Rob Płóciennik]

(In reply to comment #1)
> It is incorrect to call scheduleLayerFlush() inside of flushCompositingState().

Is any attempt to reenter RenderLayerCompositor considered incorrect at that point?
In other words, could a simple check whether a flush is underway inside any implementation of GraphicsLayerClient::notifyFlushRequired() (like in this very case) be considered a right step?

[Sergio Correia (qrwteyrutiyoup)]

(In reply to comment #2)
> (In reply to comment #1)
> > It is incorrect to call scheduleLayerFlush() inside of flushCompositingState().
> 
> Is any attempt to reenter RenderLayerCompositor considered incorrect at that point?
> In other words, could a simple check whether a flush is underway inside any implementation of GraphicsLayerClient::notifyFlushRequired() (like in this very case) be considered a right step?

Heh, I was going to attempt something like that: https://gist.github.com/qrwteyrutiyoup/647d9ca085b2dc13d1d7

To prevent calling scheduleLayerFlush() inside flushCompositingState(),         which is incorrect, we now only call m_client->notifyFlushRequired() --         which will trigger scheduleLayerFlush() -- if we are not already flushing         layer changes.

[Rob Płóciennik]

I was thinking about something as simple as:

http://pastebin.com/g9WBQXsb

[Noam Rosenthal]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > It is incorrect to call scheduleLayerFlush() inside of flushCompositingState().
> > 
> > Is any attempt to reenter RenderLayerCompositor considered incorrect at that point?
> > In other words, could a simple check whether a flush is underway inside any implementation of GraphicsLayerClient::notifyFlushRequired() (like in this very case) be considered a right step?
> 
> Heh, I was going to attempt something like that: https://gist.github.com/qrwteyrutiyoup/647d9ca085b2dc13d1d7
> 
> To prevent calling scheduleLayerFlush() inside flushCompositingState(),         which is incorrect, we now only call m_client->notifyFlushRequired() --         which will trigger scheduleLayerFlush() -- if we are not already flushing         layer changes.

I think this is a good approach.

[Simon Fraser (smfr)]

(In reply to comment #5)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (In reply to comment #1)
> > > > It is incorrect to call scheduleLayerFlush() inside of flushCompositingState().
> > > 
> > > Is any attempt to reenter RenderLayerCompositor considered incorrect at that point?
> > > In other words, could a simple check whether a flush is underway inside any implementation of GraphicsLayerClient::notifyFlushRequired() (like in this very case) be considered a right step?
> > 
> > Heh, I was going to attempt something like that: https://gist.github.com/qrwteyrutiyoup/647d9ca085b2dc13d1d7
> > 
> > To prevent calling scheduleLayerFlush() inside flushCompositingState(),         which is incorrect, we now only call m_client->notifyFlushRequired() --         which will trigger scheduleLayerFlush() -- if we are not already flushing         layer changes.
> 
> I think this is a good approach.

The fix should be in your GraphicsLayer implementation, not up in RLB/RLC, so I think that approach is OK.

The reason it's an error to do this is that there's ambiguity about whether doing this would trigger a later flush, or just get dropped.

[Noam Rosenthal]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > (In reply to comment #1)
> > > > > It is incorrect to call scheduleLayerFlush() inside of flushCompositingState().
> > > > 
> > > > Is any attempt to reenter RenderLayerCompositor considered incorrect at that point?
> > > > In other words, could a simple check whether a flush is underway inside any implementation of GraphicsLayerClient::notifyFlushRequired() (like in this very case) be considered a right step?
> > > 
> > > Heh, I was going to attempt something like that: https://gist.github.com/qrwteyrutiyoup/647d9ca085b2dc13d1d7
> > > 
> > > To prevent calling scheduleLayerFlush() inside flushCompositingState(),         which is incorrect, we now only call m_client->notifyFlushRequired() --         which will trigger scheduleLayerFlush() -- if we are not already flushing         layer changes.
> > 
> > I think this is a good approach.
> 
> The fix should be in your GraphicsLayer implementation, not up in RLB/RLC, so I think that approach is OK.
> 
> The reason it's an error to do this is that there's ambiguity about whether doing this would trigger a later flush, or just get dropped.

This ambiguity can maybe fixed by proper naming. I think if I see a patch with this approach with r? we can discuss the naming :)

[Noam Rosenthal]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #3)
> > > > (In reply to comment #2)
> > > > > (In reply to comment #1)
> > > > > > It is incorrect to call scheduleLayerFlush() inside of flushCompositingState().
> > > > > 
> > > > > Is any attempt to reenter RenderLayerCompositor considered incorrect at that point?
> > > > > In other words, could a simple check whether a flush is underway inside any implementation of GraphicsLayerClient::notifyFlushRequired() (like in this very case) be considered a right step?
> > > > 
> > > > Heh, I was going to attempt something like that: https://gist.github.com/qrwteyrutiyoup/647d9ca085b2dc13d1d7
> > > > 
> > > > To prevent calling scheduleLayerFlush() inside flushCompositingState(),         which is incorrect, we now only call m_client->notifyFlushRequired() --         which will trigger scheduleLayerFlush() -- if we are not already flushing         layer changes.
> > > 
> > > I think this is a good approach.
> > 
> > The fix should be in your GraphicsLayer implementation, not up in RLB/RLC, so I think that approach is OK.
> > 
> > The reason it's an error to do this is that there's ambiguity about whether doing this would trigger a later flush, or just get dropped.
> 
> This ambiguity can maybe fixed by proper naming. I think if I see a patch with this approach with r? we can discuss the naming :)
Though if you were referring to the RLC/RLB patch, I totally agree.

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 213691 [details]
Patch

[Noam Rosenthal]

Comment on attachment 213691 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        To prevent calling scheduleLayerFlush() inside flushCompositingState(),
> +        which is incorrect, we now only call m_client->notifyFlushRequired() --
> +        which will trigger scheduleLayerFlush() -- if we are not already flushing
> +        layer changes.

Please use - instead of --, and remove ", which is incorrect", which is unnecessary :)

> Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:93
> +    notifyFlushRequired(nullptr);

I think we should pass the layer here, even if it's currently not in use.
Having nullptr here is hard to read. Alternatively, have a nullptr default for CompositingCoordinator::notifyFlushRequired

> Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:275
> +    notifyFlushRequired(nullptr);

Ditto

> Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:356
> +    notifyFlushRequired(nullptr);

Ditto

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:54
> +    if (client() && m_coordinator && !m_coordinator->isFlushingLayerChanges()) {

Why do we need && m_coordinator? you already assert for this earlier.

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 213768 [details]
v2

Updated as per Noam's comments.

[Sergio Correia (qrwteyrutiyoup)]

Comment on attachment 213691 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        layer changes.
> 
> Please use - instead of --, and remove ", which is incorrect", which is unnecessary :)

Fixed in v2 :)

>> Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:93
>> +    notifyFlushRequired(nullptr);
> 
> I think we should pass the layer here, even if it's currently not in use.
> Having nullptr here is hard to read. Alternatively, have a nullptr default for CompositingCoordinator::notifyFlushRequired

I don't think a default value adds much to readability, so I chose to instead pass the layer obtained from mainContentsLayer().

>> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:54
>> +    if (client() && m_coordinator && !m_coordinator->isFlushingLayerChanges()) {
> 
> Why do we need && m_coordinator? you already assert for this earlier.

Removed this check in v2.

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 213803 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 213803 [details]
Patch

Clearing flags on attachment: 213803

Committed r157184: <http://trac.webkit.org/changeset/157184>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.