Bug 238494 – REGRESSION(r290360): [GLX] Crash on process exit

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

After upgrading to webkitgtk 2.36.0 from 2.34 we have noticed a crash on ever exit
in all binaries using webkitgtk and reverting this commit fixes the issue.

I assume there is either a race or somethig gets called twice and it is trying
to destroy an already destroyed resource.

Backtrace:

#0  native () at /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/graphics/x11/PlatformDisplayX11.h:48
#1  sharedDisplay () at /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/graphics/x11/XUniqueResource.cpp:46
#2  deleteXResource () at /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/graphics/x11/XUniqueResource.cpp:79
#3  0x00000f5eea945e8c in reset () at /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/graphics/x11/XUniqueResource.h:86
#4  ~XUniqueResource () at /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/graphics/x11/XUniqueResource.h:77
#5  ~GLContextGLX () at /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:341
#6  0x00000f5eea945f3f in ~GLContextGLX () at /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:323
#7  0x00000f5eea955161 in operator() () at /usr/include/c++/v1/__memory/unique_ptr.h:57
#8  reset () at /usr/include/c++/v1/__memory/unique_ptr.h:318
#9  operator= () at /usr/include/c++/v1/__memory/unique_ptr.h:276
#10 ~PlatformDisplayX11 () at /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:76
#11 ~PlatformDisplayX11 () at /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:73
#12 0x00000f5ed8272df1 in _libc___cxa_finalize (dso=0x0) at /usr/src/lib/libc/stdlib/atexit.c:177
#13 0x00000f5ed826b1a1 in _libc_exit (status=0) at /usr/src/lib/libc/stdlib/exit.c:54
#14 0x00000f5c422ffa09 in _start ()

[Carlos Garcia Campos]

So, this is crashing while releasing the shared context in the PlatformDisplayX11 destructor, called by atexit. I think it should be safe to call PlatformDisplay::shared() from the destructor, and I can't reproduce the crash, but maybe it's not. We can try to find a better way to release the shared context.

[Carlos Garcia Campos]

Or maybe the problem is that the shared gdk display is released before the PlatformDisplay destructor.

I can test any patch easily. The problem is that you probably do not see this because of either glibc or wayland. Glibc is really good at hiding these kind of bugs.

[Carlos Garcia Campos]

Created attachment 456013 [details]
Patch

Could you try this patch, please?

New backtrace with the patch:

Core was generated by `WebKitWebProcess'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  native () at /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/graphics/x11/PlatformDisplayX11.h:50
50          ::Display* native() const { return m_display; }
[Current thread is 1 (process 281893)]
(gdb) bt
#0  native () at /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/graphics/x11/PlatformDisplayX11.h:50
#1  sharedDisplay () at /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/graphics/x11/XUniqueResource.cpp:46
#2  deleteXResource () at /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/graphics/x11/XUniqueResource.cpp:79
#3  0x000005cf5a2b40dc in reset () at /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/graphics/x11/XUniqueResource.h:86
#4  ~XUniqueResource () at /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/graphics/x11/XUniqueResource.h:77
#5  ~GLContextGLX () at /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:341
#6  0x000005cf5a2b418f in ~GLContextGLX () at /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:323
#7  0x000005cf5a2c3441 in operator() () at /usr/include/c++/v1/__memory/unique_ptr.h:57
#8  reset () at /usr/include/c++/v1/__memory/unique_ptr.h:318
#9  operator= () at /usr/include/c++/v1/__memory/unique_ptr.h:276
#10 ~PlatformDisplayX11 () at /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:90
#11 ~PlatformDisplayX11 () at /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:87
#12 0x000005d02ae80041 in _libc___cxa_finalize (dso=0x0) at /usr/src/lib/libc/stdlib/atexit.c:177
#13 0x000005d02ae5d401 in _libc_exit (status=0) at /usr/src/lib/libc/stdlib/exit.c:54
#14 0x000005cd39f65a09 in _start ()
(gdb)

[Michael Catanzaro]

Comment on attachment 456013 [details]
Patch

My $0.02: destroying the display is optional, and the exit handler is more trouble than it's worth....

[Carlos Garcia Campos]

(In reply to robert.nagy from comment #5)
> New backtrace with the patch:
> 
> Core was generated by `WebKitWebProcess'.
> Program terminated with signal SIGSEGV, Segmentation fault.
> #0  native () at
> /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/
> graphics/x11/PlatformDisplayX11.h:50
> 50          ::Display* native() const { return m_display; }
> [Current thread is 1 (process 281893)]
> (gdb) bt
> #0  native () at
> /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/
> graphics/x11/PlatformDisplayX11.h:50
> #1  sharedDisplay () at
> /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/
> graphics/x11/XUniqueResource.cpp:46
> #2  deleteXResource () at
> /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/
> graphics/x11/XUniqueResource.cpp:79
> #3  0x000005cf5a2b40dc in reset () at
> /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/
> graphics/x11/XUniqueResource.h:86
> #4  ~XUniqueResource () at
> /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/
> graphics/x11/XUniqueResource.h:77
> #5  ~GLContextGLX () at
> /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/
> graphics/glx/GLContextGLX.cpp:341
> #6  0x000005cf5a2b418f in ~GLContextGLX () at
> /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/
> graphics/glx/GLContextGLX.cpp:323
> #7  0x000005cf5a2c3441 in operator() () at
> /usr/include/c++/v1/__memory/unique_ptr.h:57
> #8  reset () at /usr/include/c++/v1/__memory/unique_ptr.h:318
> #9  operator= () at /usr/include/c++/v1/__memory/unique_ptr.h:276
> #10 ~PlatformDisplayX11 () at
> /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/
> graphics/x11/PlatformDisplayX11.cpp:90
> #11 ~PlatformDisplayX11 () at
> /home/pobj/webkitgtk4-2.36.0/webkitgtk-2.36.0/Source/WebCore/platform/
> graphics/x11/PlatformDisplayX11.cpp:87
> #12 0x000005d02ae80041 in _libc___cxa_finalize (dso=0x0) at
> /usr/src/lib/libc/stdlib/atexit.c:177
> #13 0x000005d02ae5d401 in _libc_exit (status=0) at
> /usr/src/lib/libc/stdlib/exit.c:54
> #14 0x000005cd39f65a09 in _start ()
> (gdb)

Ok, that confirms it's not that gtk closed the display. So, I guess the problem is that for whatever reason it's not ok to call PlatformDisplay::shared() from its destructor. The patch is still good, I think, so I'll try to find a solution on top of it.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #6)
> Comment on attachment 456013 [details]
> Patch
> 
> My $0.02: destroying the display is optional, and the exit handler is more
> trouble than it's worth....

Problem is that GL resources not released on process exit can be leaked or cause issues in GPU drivers, that's why we are avoiding abrupt exit on web process.

[Carlos Garcia Campos]

Created attachment 456099 [details]
Patch

Could you try again with this one, please?

That seems to not crash anymore.

[Antoine Jacoutot]

(In reply to robert.nagy from comment #10)
> That seems to not crash anymore.

I can confirm this does _not_ crash anymore for me as well.

[Michael Catanzaro]

Comment on attachment 456099 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        PlatformDisplay, which is always the shared one, we can just install an atexit handler in the constrcutor to

constructor

> Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:77
> +    // In X11 only one PlatformDisplay instance is allowed per process which is the sharedDisplay one.

Is there an assert for this already? I'd like to see an assert to enforce this, or else at least ensure the exit handler is registered called only once.

> Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:82
> +    std::atexit([] {
> +	PlatformDisplay::sharedDisplay().clearSharingGLContext();

Indentation is wrong:

ERROR: Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:82:  Tab found; better to use spaces  [whitespace/tab] [1]

[Carlos Garcia Campos]

Created attachment 456211 [details]
Patch for landing

[Carlos Garcia Campos]

Committed r292211 (?): <https://commits.webkit.org/r292211>