Bug 238494 - REGRESSION(r290360): [GLX] Crash on process exit
Summary: REGRESSION(r290360): [GLX] Crash on process exit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2022-03-29 01:48 PDT by Carlos Garcia Campos
Modified: 2022-04-01 02:00 PDT (History)
4 users (show)

See Also:


Attachments
Patch (16.01 KB, patch)
2022-03-29 04:14 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (18.24 KB, patch)
2022-03-30 01:43 PDT, Carlos Garcia Campos
mcatanzaro: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (18.34 KB, patch)
2022-03-31 01:09 PDT, Carlos Garcia Campos
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2022-03-29 01:48:36 PDT
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 ()
Comment 1 Carlos Garcia Campos 2022-03-29 01:52:21 PDT
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.
Comment 2 Carlos Garcia Campos 2022-03-29 02:22:50 PDT
Or maybe the problem is that the shared gdk display is released before the PlatformDisplay destructor.
Comment 3 robert.nagy 2022-03-29 03:48:38 PDT
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.
Comment 4 Carlos Garcia Campos 2022-03-29 04:14:30 PDT
Created attachment 456013 [details]
Patch

Could you try this patch, please?
Comment 5 robert.nagy 2022-03-29 06:46:21 PDT
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)
Comment 6 Michael Catanzaro 2022-03-29 10:07:44 PDT
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....
Comment 7 Carlos Garcia Campos 2022-03-30 00:32:41 PDT
(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.
Comment 8 Carlos Garcia Campos 2022-03-30 00:34:21 PDT
(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.
Comment 9 Carlos Garcia Campos 2022-03-30 01:43:51 PDT
Created attachment 456099 [details]
Patch

Could you try again with this one, please?
Comment 10 robert.nagy 2022-03-30 04:21:24 PDT
That seems to not crash anymore.
Comment 11 Antoine Jacoutot 2022-03-30 05:30:40 PDT
(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.
Comment 12 Michael Catanzaro 2022-03-30 10:51:53 PDT
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]
Comment 13 Carlos Garcia Campos 2022-03-31 01:09:35 PDT
Created attachment 456211 [details]
Patch for landing
Comment 14 Carlos Garcia Campos 2022-04-01 02:00:45 PDT
Committed r292211 (?): <https://commits.webkit.org/r292211>