[wpe-2.38] [regression] Crash while calling `TextureMapperPlatformLayerBuffer::addFenceSyncIfAvailable`

[Scony]

There is a regression causing a following crash in the https://sky.play.works/service/apps/puppy_rescue/index.html game (after few seconds - few minutes of actually playing):

Core was generated by `/usr/libexec/wpe-webkit-1.1/WPEWebProcess 11 43'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  WebCore::GLContext::version () at ../git/Source/WebCore/platform/graphics/GLContext.cpp:176
176	../git/Source/WebCore/platform/graphics/GLContext.cpp: No such file or directory.
[Current thread is 1 (Thread 0xae663250 (LWP 44))]
#0  WebCore::GLContext::version () at ../git/Source/WebCore/platform/graphics/GLContext.cpp:176
#1  0xb27036bc in WebCore::TextureMapperPlatformLayerBuffer::addFenceSyncIfAvailable () at ../git/Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:190
#2  0xb268b9cc in WebCore::ImageBufferCairoGLSurfaceBackend::swapBuffersIfNeeded () at ../git/Source/WebCore/platform/graphics/cairo/ImageBufferCairoGLSurfaceBackend.cpp:185
#3  0xb27140f0 in Nicosia::ContentLayerTextureMapperImpl::swapBuffersIfNeeded () at ../git/Source/WebCore/platform/graphics/nicosia/texmap/NicosiaContentLayerTextureMapperImpl.cpp:81
#4  0xb2707084 in WebCore::CoordinatedGraphicsLayer::updatePlatformLayer () at ../git/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:761
#5  WebCore::CoordinatedGraphicsLayer::updatePlatformLayer () at ../git/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:750
#6  0xb2708548 in WebCore::CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly () at ../git/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:862
#7  0xb2705950 in WebCore::CoordinatedGraphicsLayer::flushCompositingState () at ../git/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:727
#8  0xb270597c in WebCore::CoordinatedGraphicsLayer::flushCompositingState () at ../git/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:733
#9  0xb270597c in WebCore::CoordinatedGraphicsLayer::flushCompositingState () at ../git/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:733
#10 0xb270597c in WebCore::CoordinatedGraphicsLayer::flushCompositingState () at ../git/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:733
#11 0xb270597c in WebCore::CoordinatedGraphicsLayer::flushCompositingState () at ../git/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:733
#12 0xb270597c in WebCore::CoordinatedGraphicsLayer::flushCompositingState () at ../git/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:733
#13 0xb3553860 in WebCore::RenderLayerCompositor::flushPendingLayerChanges () at ../git/Source/WebCore/rendering/RenderLayerCompositor.cpp:620
#14 0xb32f7340 in WebCore::FrameView::flushCompositingStateForThisFrame () at ../git/Source/WebCore/page/FrameView.cpp:1037
#15 0xb32f7354 in WebCore::FrameView::flushCompositingStateIncludingSubframes () at ../git/Source/WebCore/page/FrameView.cpp:1203
#16 0xb331c016 in WebCore::Page::finalizeRenderingUpdate () at ../git/Source/WebCore/page/Page.cpp:1843
#17 0xb1ddd5b6 in WebKit::CompositingCoordinator::flushPendingLayerChanges () at ../git/Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.cpp:136
#18 0xb1dddeba in WebKit::LayerTreeHost::layerFlushTimerFired () at ../git/Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/LayerTreeHost.cpp:160
#19 0xb264c0fa in operator() () at ../git/Source/WTF/wtf/glib/RunLoopGLib.cpp:177
#20 _FUN () at ../git/Source/WTF/wtf/glib/RunLoopGLib.cpp:181
#21 0xb264c6c8 in operator() () at ../git/Source/WTF/wtf/glib/RunLoopGLib.cpp:53
#22 _FUN () at ../git/Source/WTF/wtf/glib/RunLoopGLib.cpp:56
#23 0xb15783b8 in g_main_dispatch (context=0x7f358db0) at ../glib-2.72.3/glib/gmain.c:3417
#24 g_main_context_dispatch (context=context@entry=0x7f358db0) at ../glib-2.72.3/glib/gmain.c:4135
#25 0xb1578522 in g_main_context_iterate (context=0x7f358db0, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ../glib-2.72.3/glib/gmain.c:4211
#26 0xb157884a in g_main_loop_run (loop=0x7f35a300) at ../glib-2.72.3/glib/gmain.c:4411
#27 0xb264c968 in WTF::RunLoop::run () at ../git/Source/WTF/wtf/glib/RunLoopGLib.cpp:108
#28 0xb1de1192 in WebKit::AuxiliaryProcessMainBase<WebKit::WebProcess, true>::run () at ../git/Source/WebKit/Shared/AuxiliaryProcessMain.h:71
#29 WebKit::AuxiliaryProcessMainBase<WebKit::WebProcess, true>::run () at ../git/Source/WebKit/Shared/AuxiliaryProcessMain.h:58
#30 WebKit::AuxiliaryProcessMain<WebKit::WebProcessMainWPE> () at ../git/Source/WebKit/Shared/AuxiliaryProcessMain.h:97
#31 0xb172998e in __libc_start_main (main=0x7d59a545 <main()>, argc=3, argv=0xbb92b4c4, init=<optimized out>, fini=0x7d59a699 <__libc_csu_fini>, rtld_fini=0xb3fa6099 <_dl_fini>, stack_end=0xbb92b4c4) at libc-start.c:308
#32 0x7d59a57c in _start () at start.S:112

The crash originates from the fact that GLContext::current() is null. It's being set to null via such flow:

#1  0xb33cf7ba in WebCore::ThreadGlobalGLContext::setContext () at ../git/Source/WebCore/platform/graphics/GLContext.cpp:52
#2  WebCore::GLContext::~GLContext () at ../git/Source/WebCore/platform/graphics/GLContext.cpp:147
#3  WebCore::GLContext::~GLContext () at ../git/Source/WebCore/platform/graphics/GLContext.cpp:144
#4  0xb33e68ca in WebCore::GLContextEGL::~GLContextEGL () at ../git/Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:386
#5  0xb33e68f0 in WebCore::GLContextEGL::~GLContextEGL () at ../git/Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:404
#6  0xb26e642e in std::default_delete<WebCore::GLContext>::operator() () at ../recipe-sysroot/usr/include/c++/9.3.0/bits/unique_ptr.h:81
#7  std::unique_ptr<WebCore::GLContext, std::default_delete<WebCore::GLContext> >::~unique_ptr () at ../recipe-sysroot/usr/include/c++/9.3.0/bits/unique_ptr.h:292
#8  Nicosia::GCGLLayer::~GCGLLayer () at ../git/Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGCGLLayer.cpp:82
#9  0xb26e6474 in Nicosia::GCGLLayer::~GCGLLayer () at ../git/Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGCGLLayer.cpp:93
#10 0xb2645852 in std::default_delete<Nicosia::GCGLLayer>::operator() () at ../recipe-sysroot/usr/include/c++/9.3.0/bits/unique_ptr.h:81
#11 std::unique_ptr<Nicosia::GCGLLayer, std::default_delete<Nicosia::GCGLLayer> >::~unique_ptr () at ../recipe-sysroot/usr/include/c++/9.3.0/bits/unique_ptr.h:292
#12 WebCore::GraphicsContextGLOpenGL::~GraphicsContextGLOpenGL () at ../git/Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.cpp:128
#13 0xb26cb7a8 in WebCore::GraphicsContextGLTextureMapper::~GraphicsContextGLTextureMapper () at ../git/Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapper.h:34
#14 0xb26cb7bc in WebCore::GraphicsContextGLTextureMapper::~GraphicsContextGLTextureMapper () at ../git/Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapper.h:34
#15 0xb3151fe8 in std::default_delete<WebCore::GraphicsContextGL>::operator() () at ../recipe-sysroot/usr/include/c++/9.3.0/bits/unique_ptr.h:81
#16 WTF::RefCounted<WebCore::GraphicsContextGL, std::default_delete<WebCore::GraphicsContextGL> >::deref () at WTF/Headers/wtf/RefCounted.h:190
#17 WTF::RefCounted<WebCore::GraphicsContextGL, std::default_delete<WebCore::GraphicsContextGL> >::deref () at WTF/Headers/wtf/RefCounted.h:187
#18 WTF::DefaultRefDerefTraits<WebCore::GraphicsContextGL>::derefIfNotNull () at WTF/Headers/wtf/RefPtr.h:42
#19 WTF::RefPtr<WebCore::GraphicsContextGL, WTF::RawPtrTraits<WebCore::GraphicsContextGL>, WTF::DefaultRefDerefTraits<WebCore::GraphicsContextGL> >::operator=(decltype(nullptr)) () at WTF/Headers/wtf/RefPtr.h:163
#20 WebCore::WebGLRenderingContextBase::destroyGraphicsContextGL () at ../git/Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1303
#21 0xb2fc587e in operator()<WebCore::ActiveDOMObject> () at ../git/Source/WebCore/dom/ScriptExecutionContext.cpp:341
#22 call () at WTF/Headers/wtf/Function.h:53
#23 0xb2fc64ce in WTF::Function<WebCore::ScriptExecutionContext::ShouldContinue (WebCore::ActiveDOMObject&)>::operator()(WebCore::ActiveDOMObject&) const () at WTF/Headers/wtf/Function.h:82
#24 WebCore::ScriptExecutionContext::forEachActiveDOMObject(WTF::Function<WebCore::ScriptExecutionContext::ShouldContinue (WebCore::ActiveDOMObject&)> const&) const () at ../git/Source/WebCore/dom/ScriptExecutionContext.cpp:275
#25 0xb2fc65c6 in WebCore::ScriptExecutionContext::stopActiveDOMObjects () at ../git/Source/WebCore/dom/ScriptExecutionContext.cpp:340
#26 WebCore::ScriptExecutionContext::stopActiveDOMObjects () at ../git/Source/WebCore/dom/ScriptExecutionContext.cpp:332
#27 0xb2f836bc in WebCore::Document::commonTeardown () at ../git/Source/WebCore/dom/Document.cpp:811
#28 0xb2f83d60 in WebCore::Document::willBeRemovedFromFrame () at ../git/Source/WebCore/dom/Document.cpp:2701
#29 0xb32c2330 in WebCore::Frame::setView () at ../git/Source/WebCore/page/Frame.cpp:259
#30 0xb32d53a2 in WebCore::Frame::createView () at ../git/Source/WebCore/page/Frame.cpp:889
#31 0xb1d88678 in WebKit::WebFrameLoaderClient::transitionToCommittedForNewPage () at ../git/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1552
#32 0xb324d60c in WebCore::FrameLoader::transitionToCommitted () at ../git/Source/WebCore/loader/FrameLoader.cpp:2253
#33 0xb324d6f6 in WebCore::FrameLoader::transitionToCommitted () at ../git/Source/WebCore/loader/FrameLoader.cpp:2172
#34 WebCore::FrameLoader::commitProvisionalLoad () at ../git/Source/WebCore/loader/FrameLoader.cpp:2061
#35 0xb322e57c in WebCore::DocumentLoader::commitIfReady () at ../git/Source/WebCore/loader/DocumentLoader.cpp:1201
#36 WebCore::DocumentLoader::commitLoad () at ../git/Source/WebCore/loader/DocumentLoader.cpp:1189
#37 0xb3292652 in WebCore::CachedRawResource::notifyClientsDataWasReceived () at ../git/Source/WebCore/loader/cache/CachedRawResource.cpp:145
#38 0xb329491a in WebCore::CachedRawResource::notifyClientsDataWasReceived () at ../git/Source/WebCore/loader/cache/CachedRawResource.cpp:139
#39 WebCore::CachedRawResource::updateBuffer () at ../git/Source/WebCore/loader/cache/CachedRawResource.cpp:81
#40 WebCore::CachedRawResource::updateBuffer () at ../git/Source/WebCore/loader/cache/CachedRawResource.cpp:59
#41 0xb326eabc in WebCore::SubresourceLoader::didReceiveBuffer () at ../git/Source/WebCore/loader/SubresourceLoader.cpp:559
#42 0xb1d5cbc2 in WebKit::WebResourceLoader::didReceiveData () at ../git/Source/WebKit/WebProcess/Network/WebResourceLoader.cpp:243
#43 0xb1afbe70 in IPC::callMemberFunctionImpl<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(IPC::SharedBufferReference&&, long long), std::tuple<IPC::SharedBufferReference, long long>, 0u, 1u> () at ../git/Source/WebKit/Platform/IPC/HandleMessage.h:131
#44 IPC::callMemberFunction<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(IPC::SharedBufferReference&&, long long), std::tuple<IPC::SharedBufferReference, long long>, std::integer_sequence<unsigned int, 0u, 1u> > () at ../git/Source/WebKit/Platform/IPC/HandleMessage.h:137
#45 IPC::handleMessage<Messages::WebResourceLoader::DidReceiveData, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(IPC::SharedBufferReference&&, long long)> () at ../git/Source/WebKit/Platform/IPC/HandleMessage.h:259
#46 WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage () at DerivedSources/WebKit/WebResourceLoaderMessageReceiver.cpp:76
#47 0xb1d64010 in WebKit::NetworkProcessConnection::didReceiveMessage () at ../git/Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp:102
#48 0xb1c035cc in IPC::Connection::dispatchMessage () at ../git/Source/WebKit/Platform/IPC/Connection.cpp:1105
#49 0xb1c042dc in IPC::Connection::dispatchMessage () at ../git/Source/WebKit/Platform/IPC/Connection.cpp:1150
#50 0xb1c052c2 in IPC::Connection::dispatchOneIncomingMessage () at ../git/Source/WebKit/Platform/IPC/Connection.cpp:1219
#51 0xb25e869a in WTF::Function<void ()>::operator()() const () at ../git/Source/WTF/wtf/Function.h:82
#52 WTF::RunLoop::performWork () at ../git/Source/WTF/wtf/RunLoop.cpp:134
#53 0xb261ee0e in operator() () at ../git/Source/WTF/wtf/glib/RunLoopGLib.cpp:80
#54 _FUN () at ../git/Source/WTF/wtf/glib/RunLoopGLib.cpp:82
#55 0xb261f658 in operator() () at ../git/Source/WTF/wtf/glib/RunLoopGLib.cpp:53
#56 _FUN () at ../git/Source/WTF/wtf/glib/RunLoopGLib.cpp:56
#57 0xb155b3b8 in g_main_dispatch (context=0x7e753db0) at ../glib-2.72.3/glib/gmain.c:3417
#58 g_main_context_dispatch (context=context@entry=0x7e753db0) at ../glib-2.72.3/glib/gmain.c:4135
#59 0xb155b522 in g_main_context_iterate (context=0x7e753db0, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ../glib-2.72.3/glib/gmain.c:4211
#60 0xb155b84a in g_main_loop_run (loop=0x7e755300) at ../glib-2.72.3/glib/gmain.c:4411
#61 0xb261f8f8 in WTF::RunLoop::run () at ../git/Source/WTF/wtf/glib/RunLoopGLib.cpp:108
#62 0xb1dc1e22 in WebKit::AuxiliaryProcessMainBase<WebKit::WebProcess, true>::run () at ../git/Source/WebKit/Shared/AuxiliaryProcessMain.h:71
#63 WebKit::AuxiliaryProcessMainBase<WebKit::WebProcess, true>::run () at ../git/Source/WebKit/Shared/AuxiliaryProcessMain.h:58
#64 WebKit::AuxiliaryProcessMain<WebKit::WebProcessMainWPE> () at ../git/Source/WebKit/Shared/AuxiliaryProcessMain.h:97
#65 0xb170c98e in __libc_start_main (main=0x7d61f545 <main()>, argc=3, argv=0xbbeed4d4, init=<optimized out>, fini=0x7d61f699 <__libc_csu_fini>, rtld_fini=0xb3f5a099 <_dl_fini>, stack_end=0xbbeed4d4) at libc-start.c:308
#66 0x7d61f57c in _start () at start.S:112

I haven't analyzed this isse further, but the regression looks to be introduced in 2deb416

The above was reproduced on arm32 platform using WPE WebKit at 51171f8

[magomez]

@Scony I've pushed 9f13ddc that should fix this. We need to ensure that there's a current glContext in order to get the GLES version that we're using, and use fences if they are available. Can you verify that the problem is fixed and close this if it is? Thanks!

[Scony]

Ok, that makes sense. Yes, I'll check in few hours.

[Scony]

I confirm that the crash is no longer occuring with 9f13ddc

[varumugam123]

Hi @magomez I have a query related to that, could you help clarify?

I see that the shared context is made current from ImageBufferCairoGLSurfaceBackend::swapBuffersIfNeeded() only when the second texture is not created. Post texture[1] creation, swapBuffersIfNeeded() uses whatever context is set already (could be null as seen in this crash case or some other context). Is this intended?

Because on all other cases where context is required like create() / ~ImageBufferCairoGLSurfaceBackend() / cairoDevice() the shared context is used.

[magomez]

Hi @magomez I have a query related to that, could you help clarify?

I see that the shared context is made current from ImageBufferCairoGLSurfaceBackend::swapBuffersIfNeeded() only when the second texture is not created. Post texture[1] creation, swapBuffersIfNeeded() uses whatever context is set already (could be null as seen in this crash case or some other context). Is this intended?

Because on all other cases where context is required like create() / ~ImageBufferCairoGLSurfaceBackend() / cairoDevice() the shared context is used.

When the cairo gl surface is created, cairo will store the current context and use it whenever it performs any rendering operation on the surface. That's why when there are cairo operations, there's no need to set a context current. I don't get which weird corner case is this bug hitting, but there should always be a current context after the cairo operations in swapBuffersIfNeeded.

[asurdej-comcast]

Looking at the traces it suggests that current context was destroyed by webgl destruction that happended somewhere in between and should not impact our case really as those are/should be separate contexts.

But ImageBufferCairoGLSurfaceBackend::swapBuffersIfNeeded() doesn't call makeContextCurrent() so it don't change ThreadSpecific* ThreadGlobalGLContext::staticGLContext; and probably it is left with nullptr from webgl destruction. So GLContext::current() returns nullptr still.

Another thing is eglContext set by eglMakeCurrent(). Cairo indeed sets the context for every operation but it seems to be reset after exiting from cairo function in some cases, not sure if it is applicable for our use-cases. Like cairo_gl_flush() sources (cairo-1.16.0):

static inline cairo_status_t
__cairo_surface_flush (cairo_surface_t *surface, unsigned flags)
{
    cairo_status_t status = CAIRO_STATUS_SUCCESS;
    if (surface->backend->flush)
    status = surface->backend->flush (surface, flags);
    return status;
}

static cairo_status_t
_cairo_gl_surface_flush (void *abstract_surface, unsigned flags)
{
...
status = _cairo_gl_context_acquire (surface->base.device, &ctx);
...
    return _cairo_gl_context_release (ctx, status);
}

_cairo_gl_context_acquire -> device->backend->lock (device); -> cairo-gl-device::_gl_lock() -> ctx->acquire (ctx);

That ends up here I believe:

static void
_egl_acquire (void *abstract_ctx)
{
    cairo_egl_context_t *ctx = abstract_ctx;
    EGLSurface current_surface = _egl_get_current_surface (ctx);

    _egl_query_current_state (ctx);
    if (!_context_acquisition_changed_egl_state (ctx, current_surface))
    return;

    eglMakeCurrent (ctx->display,
            current_surface, current_surface, ctx->context);
}

static void
_egl_release (void *abstract_ctx)
{
    cairo_egl_context_t *ctx = abstract_ctx;
    if (!ctx->base.thread_aware ||
    !_context_acquisition_changed_egl_state (ctx,
                         _egl_get_current_surface (ctx))) {
    return;
    }

    eglMakeCurrent (ctx->display,
            EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT);
}

So at the end it may restore egl context to EGL_NO_CONTEXT in some cases and glFenceSync is context specific and may not behave as correclty.

[magomez]

Yeah, I was absent minded yesterday. Cairo resets the appropriate context but just at the EGL level, not the one stored in GLContext::current(), and that's why inside addFenceSyncIfNeeded() there may not be a current context (in this case it was removed by the destruction of the prior WebGL context).
Making the sharingContext as current always inside swapBuffersIfNeeded() would have fixed this as well.

[asurdej-comcast]

Right, in fact I don't really care about GLContext::current() as it is used to get version only, until not crashing then it's fine.
I'm mainly wondering what EGL context is set at the time of crating glFenceSync as it can be EGL_NO_CONTEXT or sharingContext (I need to confirm what is the result of "if" cond in _egl_release) and if it makes any difference for glFenceSync behavior (like if glFenceSync waits for commands from current context only or so).

All in all this is some corner-case probably so should not impact us much