[CoordinatedGraphics] Crash in TextureMapperLayer::setBackingStore()

[Chris Dumez]

I sometimes get a crash in TextureMapperLayer::setBackingStore() when running the compositing layout tests. It is not very easy to reproduce but I basically do the following:
Tools/Scripts/run-webkit-tests --debug --efl -2 --repeat-each=5 compositing

I'm not quite sure what the source of the issue is yet. Sadly, I'm busy with something else at the moment so I'm filing a bug in hope someone has some time to investigate this issue before I do.

The backtrace looks like:
crash log for WebKitTestRunner (pid 19564):
STDOUT: <empty>
STDERR: 1   0x7f03ab762b67
STDERR: 2   0x7f03a3cd6cb0
STDERR: 3   0x4215a2 WTF::RefCountedBase::derefBase()
STDERR: 4   0x7f03a7f988d4 WTF::RefCounted<WebCore::TextureMapperBackingStore>::deref()
STDERR: 5   0x7f03a7f98308 void WTF::derefIfNotNull<WebCore::TextureMapperBackingStore>(WebCore::TextureMapperBackingStore*)
STDERR: 6   0x7f03a896f5a2 WTF::RefPtr<WebCore::TextureMapperBackingStore>::operator=(WTF::PassRefPtr<WebCore::TextureMapperBackingStore> const&)
STDERR: 7   0x7f03a896f290 WebCore::TextureMapperLayer::setBackingStore(WTF::PassRefPtr<WebCore::TextureMapperBackingStore>)
STDERR: 8   0x7f03a4a5d6b9 WebKit::LayerTreeRenderer::commitPendingBackingStoreOperations()
STDERR: 9   0x7f03a4a5d7aa WebKit::LayerTreeRenderer::flushLayerChanges()
STDERR: 10  0x7f03a4a5a91c WTF::FunctionWrapper<void (WebKit::LayerTreeRenderer::*)()>::operator()(WebKit::LayerTreeRenderer*)
STDERR: 11  0x7f03a4a59f3c WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (WebKit::LayerTreeRenderer::*)()>, void (WebKit::LayerTreeRenderer*)>::operator()()
STDERR: 12  0x7f03ab797472 WTF::Function<void ()>::operator()() const
STDERR: 13  0x7f03a4a5dabe WebKit::LayerTreeRenderer::syncRemoteContent()
STDERR: 14  0x7f03a4a5b243 WebKit::LayerTreeRenderer::paintToCurrentGLContext(WebCore::TransformationMatrix const&, float, WebCore::FloatRect const&, unsigned int)
STDERR: 15  0x7f03a4b8f236 EwkViewImpl::displayTimerFired(WebCore::Timer<EwkViewImpl>*)
STDERR: 16  0x7f03a4b96884 WebCore::Timer<EwkViewImpl>::fired()
STDERR: 17  0x7f03a7f0ab1e WebCore::ThreadTimers::sharedTimerFiredInternal()
STDERR: 18  0x7f03a7f0aa3f WebCore::ThreadTimers::sharedTimerFired()
STDERR: 19  0x7f03a891dd25
STDERR: 20  0x7f03abd0146e _ecore_timer_expired_call
STDERR: 21  0x7f03abd0163b _ecore_timer_expired_timers_call
STDERR: 22  0x7f03abcfe551
STDERR: 23  0x7f03abcfebe7 ecore_main_loop_begin
STDERR: 24  0x433a69 WTR::TestController::platformRunUntil(bool&, double)
STDERR: 25  0x41eb90 WTR::TestController::runUntil(bool&, WTR::TestController::TimeoutDuration)
STDERR: 26  0x41e0ad WTR::TestController::resetStateToConsistentValues()
STDERR: 27  0x425d06 WTR::TestInvocation::invoke()
STDERR: 28  0x41e8c8 WTR::TestController::runTest(char const*)
STDERR: 29  0x41ea01 WTR::TestController::runTestingServerLoop()
STDERR: 30  0x41ea9b WTR::TestController::run()
STDERR: 31  0x41c4b1 WTR::TestController::TestController(int, char const**)
STDERR: LEAK: 1 WebPageProxy
STDERR: LEAK: 1 WebContext
STDERR: LEAK: 4 WebCoreNode

[Noam Rosenthal]

Created attachment 176968 [details]
Patch

[Chris Dumez]

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:462
> +    if (m_pendingSyncBackingStores.contains(layer)) {

Why did you remove those lines?  The difference seems to be that you are removing pending backing store removals as well, not just pending addition? The Changelog does not seem to explain this change.

[Jocelyn Turcotte]

Comment on attachment 176968 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:463
> +    if (m_pendingSyncBackingStores.contains(layer)) {
> +        m_pendingSyncBackingStores.remove(layer);

What about the case where m_pendingSyncBackingStores[layer] == 0? That would mean that you remove the removal.
So basically calling removeBackingStoreIfNeeded twice with the same layer would end up not removing it.

[Noam Rosenthal]

(In reply to comment #3)
> (From update of attachment 176968 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176968&action=review
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:463
> > +    if (m_pendingSyncBackingStores.contains(layer)) {
> > +        m_pendingSyncBackingStores.remove(layer);
> 
> What about the case where m_pendingSyncBackingStores[layer] == 0? That would mean that you remove the removal.
> So basically calling removeBackingStoreIfNeeded twice with the same layer would end up not removing it.

You're right, I'll remove those lines.

[Noam Rosenthal]

Created attachment 176973 [details]
Patch

[Chris Dumez]

Comment on attachment 176973 [details]
Patch

Looks good.

[WebKit Review Bot]

Comment on attachment 176973 [details]
Patch

Clearing flags on attachment: 176973

Committed r136322: <http://trac.webkit.org/changeset/136322>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.