| Summary: | [GTK] Crash in WebCore::TextureMapperLayer::paintSelf | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||
| Component: | WebKitGTK | Assignee: | Adrian Perez <aperez> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | aperez, bugs-noreply, Hironori.Fujii, loic.yhuel, magomez, mcatanzaro, olivier.blin, zdobersek | ||||
| Priority: | P2 | ||||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | PC | ||||||
| OS: | Linux | ||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=221789 | ||||||
| Attachments: |
|
||||||
|
Description
Michael Catanzaro
2022-05-10 11:24:02 PDT
https://trac.webkit.org/browser/webkit/releases/WebKitGTK/webkit-2.36.1/Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp#L201 > contentsLayer->paintToTextureMapper(options.textureMapper, m_state.contentsRect, transform, options.opacity); I have hit this one today while smoke testing the 2.36.3 release, while
watching a YouTube video with MiniBrowser. Sadly it was a RelWithDebInfo
build so the backtrace is not much more complete. It does show the step
into ::paintToTextureMapper() pointed out by Fujii:
#0 0x00007f2ed9288ca4 in WebCore::TextureMapperPlatformLayerBuffer::paintToTextureMapper(WebCore::TextureMapper&, WebCore::FloatRect const&, WebCore::TransformationMatrix const&, float) ()
at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#1 0x00007f2ed9278a0e in WebCore::TextureMapperLayer::paintSelf(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#2 0x00007f2ed927c42b in WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#3 0x00007f2ed927c25d in WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#4 0x00007f2ed927c57f in WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#5 0x00007f2ed927c25d in WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#6 0x00007f2ed927c57f in WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#7 0x00007f2ed927c25d in WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#8 0x00007f2ed927c57f in WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#9 0x00007f2ed927c25d in WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#10 0x00007f2ed927c57f in WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#11 0x00007f2ed927c25d in WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#12 0x00007f2ed927c57f in WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#13 0x00007f2ed927c25d in WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#14 0x00007f2ed927c57f in WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#15 0x00007f2ed927c25d in WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#16 0x00007f2ed927c57f in WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#17 0x00007f2ed927c25d in WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#18 0x00007f2ed927c57f in WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#19 0x00007f2ed927c25d in WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#20 0x00007f2ed927c57f in WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#21 0x00007f2ed927c25d in WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#22 0x00007f2ed927c57f in WebCore::TextureMapperLayer::paintSelfAndChildren(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#23 0x00007f2ed927c25d in WebCore::TextureMapperLayer::paintRecursive(WebCore::TextureMapperPaintOptions&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#24 0x00007f2ed927c329 in WebCore::TextureMapperLayer::paint(WebCore::TextureMapper&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#25 0x00007f2ed8ad4f83 in WebKit::CoordinatedGraphicsScene::paintToCurrentGLContext(WebCore::TransformationMatrix const&, WebCore::FloatRect const&, unsigned int) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#26 0x00007f2ed8ad52f0 in WebKit::ThreadedCompositor::renderLayerTree() () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#27 0x00007f2ed5e48285 in WTF::RunLoop::TimerBase::TimerBase(WTF::RunLoop&)::{lambda(void*)#1}::_FUN(void*) () at /app/webkit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.1.so.0
#28 0x00007f2ed5e4884f in WTF::RunLoop::{lambda(_GSource*, int (*)(void*), void*)#1}::_FUN(_GSource*, int (*)(void*), void*) () at /app/webkit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.1.so.0
#29 0x00007f2ed210f294 in g_main_dispatch (context=0x7f2e28000b60) at ../glib/gmain.c:3381
#30 g_main_context_dispatch (context=0x7f2e28000b60) at ../glib/gmain.c:4099
#31 0x00007f2ed210f638 in g_main_context_iterate (context=0x7f2e28000b60, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ../glib/gmain.c:4175
#32 0x00007f2ed210f943 in g_main_loop_run (loop=0x7f2e28001b10) at ../glib/gmain.c:4373
#33 0x00007f2ed5e48990 in WTF::RunLoop::run() () at /app/webkit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.1.so.0
#34 0x00007f2ed5dd80aa in WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) () at /app/webkit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.1.so.0
#35 0x00007f2ed5e4b4c9 in WTF::wtfThreadEntryPoint(void*) () at /app/webkit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.1.so.0
#36 0x00007f2ed209e3ba in start_thread (arg=0x7f2e3dbfd640) at pthread_create.c:481
#37 0x00007f2ed1b35953 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
I'm going to make a debug build and see what I can get.
Quick observation: the only change in “main” on top of what's in TextureMapperLayer.cpp from the 2.36 release branch is r290575 and that one definitely is not a fix for this issue. (In reply to Adrian Perez from comment #3) > Quick observation: the only change in “main” on top of what's in > TextureMapperLayer.cpp from the 2.36 release branch is r290575 and > that one definitely is not a fix for this issue. There's https://github.com/WebKit/WebKit/commit/fb8ed3d7e9868de82621015783d1f0cc1080b4e4 as well, that was added for 2.36.1, that can be related, despite I'm not sure how the crash situation can be achieved. According to the bt, the TextureMapperPlatformLayerBuffer associated to the TextureMapperLayer has been destroyed before calling paintToTextureMapper on it. But for the TextureMapperPlatformLayerBuffer to be destroyed, the TextureMapperPlatformLayerProxy should have been invalidated, which should have removed removed the reference to the TextureMapperPlatformLayerBuffer in the TextureMapperLayer, so in theory this could not happen. But it's happening, so there's some situation where the proxy may not be properly invalidated. I managed to get this to happen with a debug build, so I can dump variables and inspect things; but I am not sure what to try to look at -- hints welcome! What I did to hit this was opening https://www.youtube.com/watch?v=fmfR0XI5czI then pressing “t” to put the video in theatre mode, and started playing it. While the video was playing, I switched two or three times between fullscreen and windowed mode using the “f” key shortcut. Some seconds after switching back from fullscreen to windowed mode the crash happened. #0 0x00007f53ef220a4a in WebCore::TextureMapperLayer::paintSelf (this=0x7f528fbfe000, options=...) at ../Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:201 201 contentsLayer->paintToTextureMapper(options.textureMapper, m_state.contentsRect, transform, options.opacity); I cannot print “contentsLayer”, GDB says: (gdb) p contentsLayer $7 = <optimized out> But I can see that the %rdi register is non-NULL (0x7f5296a073f0), and IIRC that is the pointer value of what's to the left of “->” in a “foo->bar()” call because the target instance is passed as implicit first argument to functions and the first argument gets passed in %rdi in the SysV ABI. The “options.textureMapper” is valid, too: (gdb) p options.textureMapper $8 = (WebCore::TextureMapperGL &) @0x7f53d83e4000: { <WebCore::TextureMapper> = { _vptr$TextureMapper = 0x7f53f111e888 <vtable for WebCore::TextureMapperGL+16>, m_texturePool = std::unique_ptr<WebCore::BitmapTexturePool> = { ...lots of stuff... I have a theory about what could be happening here.
I know that it can happen that, during a layerFlush, a proxy is reassigned from one GraphicsLayer to a different one. In the cases I found about this, the former layer is destroyed during the flush as well. Due to this, during the composition stage, the proxy is invalidated in response to the deletion of the first layer and then it's activated in the second one, and this works fine.
My theory is that it's possible for the first layer to be kept alive while the proxy is assigned to the second one. This would cause that, on the composition stage, the proxy is not invalidated (because the first layer is still alive), and it gets activated on the second layer, but the first layer still has a reference to the proxy's currentBuffer as its contentLayer. After this there will be a swapBuffers call on the proxy, that will replace the currentBuffer, and will update the contentLayer of the second layer, but the first one will still hold a pointer to the released buffer. Then, when painting, we would get a crash like this, as the first layer will try to paint a buffer that already been freed.
I need to do some debugging to check whether this scenario is possible (specially the possibility of the two layers being kept alive), but if that's the case, the fix would be something simple like:
diff --git a/Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyGL.cpp b/Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyGL.cpp
index 8d60d142089f..7c69c96aa365 100644
--- a/Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyGL.cpp
+++ b/Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyGL.cpp
@@ -65,6 +65,9 @@ void TextureMapperPlatformLayerProxyGL::activateOnCompositingThread(Compositor*
{
Locker locker { m_lock };
m_compositor = compositor;
+ // If the proxy is already active on another layer, remove the layer's reference to the current buffer.
+ if (m_targetLayer)
+ m_targetLayer->setContentsLayer(nullptr);
m_targetLayer = targetLayer;
if (m_targetLayer && m_currentBuffer)
m_targetLayer->setContentsLayer(m_currentBuffer.get());
Ah! and another theory, a bit simpler and that would explain why this stared happening on 2.36.2, after https://github.com/WebKit/WebKit/commit/fb8ed3d7e9868de82621015783d1f0cc1080b4e4 was added. This would involve a proxy being disassociated from a layer during the layerFlush, but the layer being kept alive. Before the mentioned commit, during composition, invalidate wouldn't be called on the proxy, and the layer would keep painting the proxy's currentBuffer, which lead to use-after-free when the proxy was destroyed. With the commit, invalidate is now called on the proxy as we detect that it's not being used anymore. But even with that, we're not removing the layer's reference to the proxy's currentBuffer. And as the currentBuffer is freed when invalidate is called, trying to paint the layer would cause a crash like this. To fix this we would need to remove the targetLayer's reference to the currentBuffer during invalidate (which makes a lot of sense IMO). diff --git a/Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyGL.cpp b/Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyGL.cpp index 8d60d142089f..89dec13a6c28 100644 --- a/Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyGL.cpp +++ b/Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyGL.cpp @@ -94,7 +94,10 @@ void TextureMapperPlatformLayerProxyGL::invalidate() { Locker locker { m_lock }; m_compositor = nullptr; - m_targetLayer = nullptr; + if (m_targetLayer) { + m_targetLayer->setContentsLayer(nullptr); + m_targetLayer = nullptr; + } m_currentBuffer = nullptr; m_pendingBuffer = nullptr; So, recapitulating (sorry for the noise, I'm using this to order my ideas): when we have a proxy assigned to a layer, there are 4 cases that can happen after there has been a layerFlush and we're adopting the new state in the composition stage: 1- The layer is removed from the tree and the proxy is not assigned to any other layer: the deletion of the layer causes an invalidation of the proxy and both are destroyed afterwards. This works fine. 2- The layer is removed from the tree and the proxy is reassigned to a new layer: the deletion of the first layer causes the invalidation of the proxy, which is then activated on the second layer. As the first layer is destroyed, we don't have to worry about dangling references from it to the proxy's currentBuffer. This works fine. 3- The layer is kept in the tree and the proxy gets disassociated from it and not used by any other layer: we detect that the proxy is not used anymore and call invalidate on it, but the layer keeps a reference to the proxy's currentBuffer, which has been deleted during invalidate, which leads to a crash when trying to render the layer. This would require the fix in comment 7. 4- The layer is kept in the tree and the proxy gets associated to a new layer: as we detect that the proxy is still being used it's not invalidated, but it gets activated on the second layer. The first layer keeps a reference to the proxy's currentBuffer, which will be destroyed a bit later when swapBuffers is called on the proxy. This leads to a crash when trying to render the first layer. This would require the fix in comment 6. I'm pretty sure that this crash is happening due to situations 3 or 4 (I suspect it's 3, as that would explain why started happening on 2.36.2), but I can't confirm which one cause I haven't been able to reproduce the issue. But I suspect that if one of those situations can happen, the other can too, so we probably need to fix both cases. (In reply to Miguel Gomez from comment #7) > [...] > > diff --git > a/Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyGL. > cpp > b/Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyGL. > cpp > index 8d60d142089f..89dec13a6c28 100644 > --- > a/Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyGL. > cpp > +++ > b/Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxyGL. > cpp > @@ -94,7 +94,10 @@ void TextureMapperPlatformLayerProxyGL::invalidate() > { > Locker locker { m_lock }; > m_compositor = nullptr; > - m_targetLayer = nullptr; > + if (m_targetLayer) { > + m_targetLayer->setContentsLayer(nullptr); > + m_targetLayer = nullptr; > + } > > m_currentBuffer = nullptr; > m_pendingBuffer = nullptr; I did a local build with this patch, but I still managed to trigger the issue. It seems to be a bit easier to trigger by switching quality settings using the gear icon in YouTube videos *after* having entered fullscreen. I am doing now one more build with both this and your other suggested patch to see what happens :) (In reply to Miguel Gomez from comment #7) > Ah! and another theory, a bit simpler and that would explain why this stared > happening on 2.36.2, after > https://github.com/WebKit/WebKit/commit/ > fb8ed3d7e9868de82621015783d1f0cc1080b4e4 was added. Careful: the first backtrace here is from 2.36.1! (In reply to Adrian Perez from comment #9) > (In reply to Miguel Gomez from comment #7) > > > [...] > > [...] > > I am doing now one more build with both this and your other suggested > patch to see what happens :) Well, I have had videos playing from YouTube and Dailymotion (yes, it still exists) for 4+ hours with *BOTH* diffs suggested by Miguel applied, switching quality settings and fullscreen/windows now and then: so far, no crashes. (In reply to Adrian Perez from comment #11) > (In reply to Adrian Perez from comment #9) > > (In reply to Miguel Gomez from comment #7) > > > > > [...] > > > > [...] > > > > I am doing now one more build with both this and your other suggested > > patch to see what happens :) > > Well, I have had videos playing from YouTube and Dailymotion (yes, it > still exists) for 4+ hours with *BOTH* diffs suggested by Miguel applied, > switching quality settings and fullscreen/windows now and then: so far, > no crashes. I left a long 12h YouTube video playing overnight, without the adblocker (which stresses MSE more, as it inserts alterante streams at intervals), with a script sending keystrokes using “xdotool” to switch between window and fullscreen rapidly every few minutes. It ran the whole night without a single issue on a build from the 2.36 branch with the *two* patches suggested by Miguel applied 🙌️ I have been talking with Miguel and he won't have time soon to submit the patch, so we have agreed that I will do it on his behalf :) Pull request: https://github.com/WebKit/WebKit/pull/1046 Committed r295749 (251754@main): <https://commits.webkit.org/251754@main> Reviewed commits have been landed. Closing PR #1046 and removing active labels. This crash fix breaks the holepunch feature for me, after changing the video source. This occurs in YouTube TV after skipping an ad: the holepunch is not rendered anymore. I have opened a new bug to track this: https://bugs.webkit.org/show_bug.cgi?id=245135 |