wxGLCanvasEGL dangling surface on unrealize (Issue #26340)
Florian Larysch
florolf created an issue (wxWidgets/wxWidgets#26340)Description
Bug description:
The KiCad Zone Manager dialog triggers a BadWindow error and associated exit() when using a wxWidgets build with wxUSE_GLCANVAS_EGL=1. I think this is a state management issue in wxWidgets.
See this bug report for a reproducer and more details.
Quoting my own TL;DR from there:
The wxNotebook::InsertPage call triggers gtk_widget_unparent
(https://github.com/wxWidgets/wxWidgets/blob/3.2/src/gtk/notebook.cpp#L465-L467)
which unrealizes the whole widget hierarchy, but doesn't invalidate the EGL
surface that is attached to the corresponding X11 windows. Once everything is
restored, the surface doesn't get recreated with the new window handle which
causes the abort described above.
Doing something hacky like:
diff --git a/include/wx/unix/glegl.h b/include/wx/unix/glegl.h
index 09bcb27ce2..3e17a3b842 100644
--- a/include/wx/unix/glegl.h
+++ b/include/wx/unix/glegl.h
@@ -67,6 +67,8 @@ public:
// creates EGLSurface
bool CreateSurface();
+ void DestroySurface();
+
virtual ~wxGLCanvasEGL();
// implement wxGLCanvasBase methods
diff --git a/src/gtk/glcanvas.cpp b/src/gtk/glcanvas.cpp
index b3251cdee9..ec71d013cf 100644
--- a/src/gtk/glcanvas.cpp
+++ b/src/gtk/glcanvas.cpp
@@ -43,6 +43,16 @@ static gboolean draw(GtkWidget* widget, cairo_t* cr, wxGLCanvas* win)
// emission hook for "parent-set"
//-----------------------------------------------------------------------------
+#if wxUSE_GLCANVAS_EGL
+extern "C" {
+static void
+glcanvas_egl_unrealize(GtkWidget*, wxGLCanvas* win)
+{
+ win->DestroySurface();
+}
+}
+#endif
+
#if !wxUSE_GLCANVAS_EGL
extern "C" {
static gboolean
@@ -253,6 +263,11 @@ bool wxGLCanvas::Create(wxWindow *parent,
gtk_widget_set_double_buffered(m_wxwindow, false);
+#if wxUSE_GLCANVAS_EGL
+ g_signal_connect(m_wxwindow, "unrealize",
+ G_CALLBACK(glcanvas_egl_unrealize), this);
+#endif
+
return true;
}
diff --git a/src/unix/glegl.cpp b/src/unix/glegl.cpp
index baee0da5b8..4ed7fc0ce9 100644
--- a/src/unix/glegl.cpp
+++ b/src/unix/glegl.cpp
@@ -705,6 +705,17 @@ bool wxGLCanvasEGL::CreateSurface()
return true;
}
+void wxGLCanvasEGL::DestroySurface()
+{
+#ifdef GDK_WINDOWING_X11
+ if ( m_surface != EGL_NO_SURFACE )
+ {
+ eglDestroySurface(m_display, m_surface);
+ m_surface = EGL_NO_SURFACE;
+ }
+#endif // GDK_WINDOWING_X11
+}
+
wxGLCanvasEGL::~wxGLCanvasEGL()
{
if ( m_config && m_config != ms_glEGLConfig )
Fixes the problem for me but I'm not familiar enough with wxWidgets (or any
other part of the Linux graphics stack) to be sure doing this doesn't break
some other invariants.
Platform and version information
- wxWidgets version you use: 3.2.10
- wxWidgets port you use: wxGTK with wxUSE_GLCANVAS_EGL=1
- OS and its version: Arch Linux
- GTK version: 3.24.51
- Which GDK backend is used: X11
- Desktop environment : Notion
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26340)Thanks, the patch looks good to me, it seems logical that we must do something similar to what we do under Wayland, where we call DestroyWaylandSubsurface() from "unmap" signal handler. And while I am not sure if it matters whether we do it from "unmap" or "unrealize", we must destroy the surface somewhere.
Just one nitpick: if we should do this in "unrealize", maybe it would be slightly better to make wxWindow::GTKHandleUnrealize() virtual as well (as GTKHandleRealized() already is) and override it in wxGLCanvas.
Finally, you mentioned that KiCad bug report includes a reproducer but I haven't found it there, somehow, could you please point it to me? Or do you mean that this can only be reproduced in KiCad itself currently?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Florian Larysch
florolf left a comment (wxWidgets/wxWidgets#26340)Just one nitpick: if we should do this in "unrealize", maybe it would be slightly better to make
wxWindow::GTKHandleUnrealize()virtual as well (asGTKHandleRealized()already is) and override it inwxGLCanvas.
Yeah that sounds like a cleaner solution. Are you going to implement the fix yourself or should I send a PR?
Finally, you mentioned that KiCad bug report includes a reproducer but I haven't found it there, somehow, could you please point it to me? Or do you mean that this can only be reproduced in KiCad itself currently?
Yeah that's what I meant - the refactoring in https://gitlab.com/kicad/code/kicad/-/commit/e4997f40ea1faf2060a59196f855d2729f82785b introduced the problem and it's somehow related to how things get sequenced there. I haven't attempted to minimize this into a standalone reproducer, but it can be triggered fairly easily from within KiCad (version 10, which includes said commit).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26340)Yeah that sounds like a cleaner solution. Are you going to implement the fix yourself or should I send a PR?
If you can make a PR tested with KiCad, i.e. known to fix the problem there, it would be great. I could make the PR myself too, it's simple enough, but it's the testing part that is difficult for me, I still haven't ever built KiCad myself (I really should do this one of these days...).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Florian Larysch
florolf left a comment (wxWidgets/wxWidgets#26340)If you can make a PR tested with KiCad, i.e. known to fix the problem there, it would be great.
Okay sure, I'll do that tomorrow.
Another question: I just saw that you marked this down for inclusion in 3.3.3. I'm not sure how the wxWidgets release train policy works, but at least Arch (which is what everybody who is hitting this seems to be on) seems to be tracking 3.2.x currently so it would be great if this fix could also get backported there after it lands in the main branch.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.