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
It is incorrect to call scheduleLayerFlush() inside of flushCompositingState().
(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?
(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 was thinking about something as simple as: http://pastebin.com/g9WBQXsb
(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.
(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.
(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 :)
(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.
Created attachment 213691 [details] Patch
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.
Created attachment 213768 [details] v2 Updated as per Noam's comments.
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.
Created attachment 213803 [details] Patch
Comment on attachment 213803 [details] Patch Clearing flags on attachment: 213803 Committed r157184: <http://trac.webkit.org/changeset/157184>
All reviewed patches have been landed. Closing bug.