Bug 161605 - [GTK] Crash of WebProcess on the last WebView disconnect
Summary: [GTK] Crash of WebProcess on the last WebView disconnect
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-05 08:09 PDT by Milan Crha
Modified: 2016-09-07 06:49 PDT (History)
3 users (show)

See Also:


Attachments
Patch (3.83 KB, patch)
2016-09-06 23:56 PDT, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff
Patch (8.34 KB, patch)
2016-09-07 00:33 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
valgrind log with applied patch (327.54 KB, text/x-log)
2016-09-07 01:08 PDT, Milan Crha
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Milan Crha 2016-09-05 08:09:25 PDT
I'm running WebKitGTK+ 2.13.90 and the WebProcess crashes here when the last WebView "disconnects" from it. The console (terminal) doesn't show any messages.

Thread 1 "WebKitWebProces" received signal SIGSEGV, Segmentation fault.
0x00007fa103de78c6 in std::default_delete<WebCore::GLContext>::operator() (this=0x17d6e10, __ptr=0x7fa0781a68c0) at /usr/include/c++/6.0.0/bits/unique_ptr.h:76
76		delete __ptr;
(gdb) bt
#0  0x00007fa103de78c6 in std::default_delete<WebCore::GLContext>::operator()(WebCore::GLContext*) const (this=0x17d6e10, __ptr=0x7fa0781a68c0) at /usr/include/c++/6.0.0/bits/unique_ptr.h:76
#1  0x00007fa103de6867 in std::unique_ptr<WebCore::GLContext, std::default_delete<WebCore::GLContext> >::~unique_ptr() (this=0x17d6e10, __in_chrg=<optimized out>) at /usr/include/c++/6.0.0/bits/unique_ptr.h:236
#2  0x00007fa105603cf7 in WebCore::PlatformDisplay::~PlatformDisplay() (this=0x17d6df0, __in_chrg=<optimized out>) at /data/develop/local/webkitgtk-2.13.90/Source/WebCore/platform/graphics/PlatformDisplay.cpp:128
#3  0x00007fa1054e943c in WebCore::PlatformDisplayX11::~PlatformDisplayX11() (this=0x17d6df0, __in_chrg=<optimized out>) at /data/develop/local/webkitgtk-2.13.90/Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:54
#4  0x00007fa1054e9458 in WebCore::PlatformDisplayX11::~PlatformDisplayX11() (this=0x17d6df0, __in_chrg=<optimized out>) at /data/develop/local/webkitgtk-2.13.90/Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:58
#5  0x00007fa105604c5c in std::default_delete<WebCore::PlatformDisplay>::operator()(WebCore::PlatformDisplay*) const (this=0x7fa109b31d48 <WebCore::PlatformDisplay::sharedDisplay()::display>, __ptr=0x17d6df0)
    at /usr/include/c++/6.0.0/bits/unique_ptr.h:76
#6  0x00007fa1056046f1 in std::unique_ptr<WebCore::PlatformDisplay, std::default_delete<WebCore::PlatformDisplay> >::~unique_ptr() (this=0x7fa109b31d48 <WebCore::PlatformDisplay::sharedDisplay()::display>, __in_chrg=<optimized out>)
    at /usr/include/c++/6.0.0/bits/unique_ptr.h:236
#7  0x00007fa102de6c08 in __run_exit_handlers () at /lib64/libc.so.6
#8  0x00007fa102de6c55 in  () at /lib64/libc.so.6
#9  0x00007fa1038054ef in IPC::Connection::didFailToSendSyncMessage() (this=0x15d6af0) at /data/develop/local/webkitgtk-2.13.90/Source/WebKit2/Platform/IPC/Connection.cpp:814
#10 0x00007fa103803395 in IPC::Connection::sendSyncMessage(unsigned long, std::unique_ptr<IPC::Encoder, std::default_delete<IPC::Encoder> >, std::chrono::duration<long, std::ratio<1l, 1000l> >, WTF::OptionSet<IPC::SendSyncOption>) (this=0x15d6af0, syncRequestID=6, encoder=std::unique_ptr<IPC::Encoder> containing 0x1946c40, timeout=..., sendSyncOptions=...) at /data/develop/local/webkitgtk-2.13.90/Source/WebKit2/Platform/IPC/Connection.cpp:466
#11 0x00007fa103a978e6 in IPC::Connection::sendSync<Messages::WebProcessProxy::ShouldTerminate>(Messages::WebProcessProxy::ShouldTerminate&&, Messages::WebProcessProxy::ShouldTerminate::Reply&&, unsigned long, std::chrono::duration<long, std::ratio<1l, 1000l> >, WTF::OptionSet<IPC::SendSyncOption>) (this=0x15d6af0, message=<unknown type in /build/local/lib/libwebkit2gtk-4.0.so.37, CU 0x57ceb6a, DIE 0x59c3244>, reply=<unknown type in /build/local/lib/libwebkit2gtk-4.0.so.37, CU 0x57ceb6a, DIE 0x59c3250>, destinationID=0, timeout=..., sendSyncOptions=...) at /data/develop/local/webkitgtk-2.13.90/Source/WebKit2/Platform/IPC/Connection.h:353
#12 0x00007fa103a8e2c2 in WebKit::WebProcess::shouldTerminate() (this=0x15d4ce0) at /data/develop/local/webkitgtk-2.13.90/Source/WebKit2/WebProcess/WebProcess.cpp:616
#13 0x00007fa103836865 in WebKit::ChildProcess::terminationTimerFired() (this=0x15d4ce0) at /data/develop/local/webkitgtk-2.13.90/Source/WebKit2/Shared/ChildProcess.cpp:160
#14 0x00007fa1038367e0 in WebKit::ChildProcess::enableTermination() (this=0x15d4ce0) at /data/develop/local/webkitgtk-2.13.90/Source/WebKit2/Shared/ChildProcess.cpp:141
#15 0x00007fa103a8e1fe in WebKit::WebProcess::removeWebPage(unsigned long) (this=0x15d4ce0, pageID=2) at /data/develop/local/webkitgtk-2.13.90/Source/WebKit2/WebProcess/WebProcess.cpp:607
#16 0x00007fa103bfd19e in WebKit::WebPage::close() (this=0x15fd2b0) at /data/develop/local/webkitgtk-2.13.90/Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1113
#17 0x00007fa103a8e68c in WebKit::WebProcess::didClose(IPC::Connection&) (this=0x15d4ce0) at /data/develop/local/webkitgtk-2.13.90/Source/WebKit2/WebProcess/WebProcess.cpp:674
#18 0x00007fa103804c72 in IPC::Connection::<lambda()>::operator()(void) (__closure=0x7fa09c005a78) at /data/develop/local/webkitgtk-2.13.90/Source/WebKit2/Platform/IPC/Connection.cpp:735
#19 0x00007fa10380a504 in WTF::Function<void()>::CallableWrapper<IPC::Connection::connectionDidClose()::<lambda()> >::call(void) (this=0x7fa09c005a70) at /data/develop/local/webkitgtk-2.13.90/Source/WTF/wtf/Function.h:101
#20 0x00007fa0ffabc2e3 in WTF::Function<void ()>::operator()() const (this=0x7ffeb8f7f7b0) at /data/develop/local/webkitgtk-2.13.90/Source/WTF/wtf/Function.h:50
#21 0x00007fa0ffacf50e in WTF::RunLoop::performWork() (this=0x15d4aa0) at /data/develop/local/webkitgtk-2.13.90/Source/WTF/wtf/RunLoop.cpp:105
#22 0x00007fa0ffb12198 in WTF::RunLoop::<lambda(gpointer)>::operator()(gpointer) const (__closure=0x0, userData=0x15d4aa0) at /data/develop/local/webkitgtk-2.13.90/Source/WTF/wtf/glib/RunLoopGLib.cpp:66
#23 0x00007fa0ffb121bc in WTF::RunLoop::<lambda(gpointer)>::_FUN(gpointer) () at /data/develop/local/webkitgtk-2.13.90/Source/WTF/wtf/glib/RunLoopGLib.cpp:68
#24 0x00007fa0ffb12138 in WTF::<lambda(GSource*, GSourceFunc, gpointer)>::operator()(GSource *, GSourceFunc, gpointer) const (__closure=0x0, source=0x15d4c00, callback=0x7fa0ffb1219f <WTF::RunLoop::<lambda(gpointer)>::_FUN(gpointer)>, userData=0x15d4aa0) at /data/develop/local/webkitgtk-2.13.90/Source/WTF/wtf/glib/RunLoopGLib.cpp:44
#25 0x00007fa0ffb12167 in WTF::<lambda(GSource*, GSourceFunc, gpointer)>::_FUN(GSource *, GSourceFunc, gpointer) () at /data/develop/local/webkitgtk-2.13.90/Source/WTF/wtf/glib/RunLoopGLib.cpp:45
#26 0x00007fa100d94803 in g_main_context_dispatch () at /lib64/libglib-2.0.so.0
#27 0x00007fa100d94bb0 in g_main_context_iterate.isra () at /lib64/libglib-2.0.so.0
#28 0x00007fa100d94ed2 in g_main_loop_run () at /lib64/libglib-2.0.so.0
#29 0x00007fa0ffb12718 in WTF::RunLoop::run() () at /data/develop/local/webkitgtk-2.13.90/Source/WTF/wtf/glib/RunLoopGLib.cpp:94
#30 0x00007fa103db8e33 in WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain>(int, char**) (argc=2, argv=0x7ffeb8f7fbb8) at /data/develop/local/webkitgtk-2.13.90/Source/WebKit2/Shared/unix/ChildProcessMain.h:61
#31 0x00007fa103db8ce6 in WebKit::WebProcessMainUnix(int, char**) (argc=2, argv=0x7ffeb8f7fbb8) at /data/develop/local/webkitgtk-2.13.90/Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:69
#32 0x000000000040089a in main(int, char**) (argc=2, argv=0x7ffeb8f7fbb8) at /data/develop/local/webkitgtk-2.13.90/Source/WebKit2/WebProcess/EntryPoint/unix/WebProcessMain.cpp:44
(gdb) f 2
#2  0x00007fa105603cf7 in WebCore::PlatformDisplay::~PlatformDisplay (this=0x17d6df0, __in_chrg=<optimized out>) at /data/develop/local/webkitgtk-2.13.90/Source/WebCore/platform/graphics/PlatformDisplay.cpp:128
128	PlatformDisplay::~PlatformDisplay()
(gdb) l
123	    : m_eglDisplay(EGL_NO_DISPLAY)
124	#endif
125	{
126	}
127	
128	PlatformDisplay::~PlatformDisplay()
129	{
130	#if USE(EGL)
131	    ASSERT(m_eglDisplay == EGL_NO_DISPLAY);
132	#endif
(gdb) 
133	}
134	
135	#if !PLATFORM(EFL)
136	GLContext* PlatformDisplay::sharingGLContext()
137	{
138	    if (!m_sharingGLContext)
139	        m_sharingGLContext = GLContext::createSharingContext(*this);
140	    return m_sharingGLContext.get();
141	}
142	#endif
(gdb) f 3
#3  0x00007fa1054e943c in WebCore::PlatformDisplayX11::~PlatformDisplayX11 (this=0x17d6df0, __in_chrg=<optimized out>) at /data/develop/local/webkitgtk-2.13.90/Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:54
54	PlatformDisplayX11::~PlatformDisplayX11()
(gdb) l
49	    : m_display(display)
50	    , m_ownedDisplay(false)
51	{
52	}
53	
54	PlatformDisplayX11::~PlatformDisplayX11()
55	{
56	    if (m_ownedDisplay)
57	        XCloseDisplay(m_display);
58	}
(gdb) p this
$1 = (WebCore::PlatformDisplayX11 * const) 0x17d6df0
(gdb) p *this
$2 = {<WebCore::PlatformDisplay> = {_vptr.PlatformDisplay = 0x7fa109a503c0 <vtable for WebCore::PlatformDisplay+16>, m_eglDisplay = 0x0, m_eglDisplayInitialized = false, m_eglMajorVersion = 0, m_eglMinorVersion = 0, 
    m_sharingGLContext = std::unique_ptr<WebCore::GLContext> containing 0x7fa0781a68c0}, m_display = 0x1580960, m_ownedDisplay = false, m_supportsXComposite = {m_isEngaged = false, m_value = {__data = "\001", 
      __align = {<No data fields>}}}, m_supportsXDamage = {m_isEngaged = false, m_value = {__data = "", __align = {<No data fields>}}}, m_damageEventBase = {m_isEngaged = false, m_value = {__data = "\000\000\000", 
      __align = {<No data fields>}}}}
(gdb) f 4
#4  0x00007fa1054e9458 in WebCore::PlatformDisplayX11::~PlatformDisplayX11 (this=0x17d6df0, __in_chrg=<optimized out>) at /data/develop/local/webkitgtk-2.13.90/Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:58
58	}
(gdb) l
53	
54	PlatformDisplayX11::~PlatformDisplayX11()
55	{
56	    if (m_ownedDisplay)
57	        XCloseDisplay(m_display);
58	}
59	
60	#if USE(EGL)
61	void PlatformDisplayX11::initializeEGLDisplay()
62	{
(gdb) p this
$3 = (WebCore::PlatformDisplayX11 * const) 0x17d6df0
(gdb) p *this
$4 = {<WebCore::PlatformDisplay> = {_vptr.PlatformDisplay = 0x7fa109a503c0 <vtable for WebCore::PlatformDisplay+16>, m_eglDisplay = 0x0, m_eglDisplayInitialized = false, m_eglMajorVersion = 0, m_eglMinorVersion = 0, 
    m_sharingGLContext = std::unique_ptr<WebCore::GLContext> containing 0x7fa0781a68c0}, m_display = 0x1580960, m_ownedDisplay = false, m_supportsXComposite = {m_isEngaged = false, m_value = {__data = "\001", 
      __align = {<No data fields>}}}, m_supportsXDamage = {m_isEngaged = false, m_value = {__data = "", __align = {<No data fields>}}}, m_damageEventBase = {m_isEngaged = false, m_value = {__data = "\000\000\000", 
      __align = {<No data fields>}}}}
(gdb) f 5
#5  0x00007fa105604c5c in std::default_delete<WebCore::PlatformDisplay>::operator() (this=0x7fa109b31d48 <WebCore::PlatformDisplay::sharedDisplay()::display>, __ptr=0x17d6df0) at /usr/include/c++/6.0.0/bits/unique_ptr.h:76
76		delete __ptr;
(gdb) f 9
#9  0x00007fa1038054ef in IPC::Connection::didFailToSendSyncMessage (this=0x15d6af0) at /data/develop/local/webkitgtk-2.13.90/Source/WebKit2/Platform/IPC/Connection.cpp:814
814	    exit(0);
(gdb) l
809	void Connection::didFailToSendSyncMessage()
810	{
811	    if (!m_shouldExitOnSyncMessageSendFailure)
812	        return;
813	
814	    exit(0);
815	}
816	
817	void Connection::enqueueIncomingMessage(std::unique_ptr<Decoder> incomingMessage)
818	{
Comment 1 Milan Crha 2016-09-06 09:50:37 PDT
valgrind output (without detailed debuginfo, because of having issues with it):

==16976== Thread 1:
==16976== Invalid read of size 8
==16976==    at 0x66581C7: WebCore::PlatformDisplay::~PlatformDisplay() (in /build/local/lib/libwebkit2gtk-4.0.so.37.14.4)
==16976==    by 0x6575F68: WebCore::PlatformDisplayX11::~PlatformDisplayX11() (in /build/local/lib/libwebkit2gtk-4.0.so.37.14.4)
==16976==    by 0x764FC07: __run_exit_handlers (in /usr/lib64/libc-2.23.so)
==16976==    by 0x764FC54: exit (in /usr/lib64/libc-2.23.so)
==16976==    by 0x7635727: (below main) (in /usr/lib64/libc-2.23.so)
==16976==  Address 0x31788f70 is 0 bytes inside a block of size 64 free'd
==16976==    at 0x4C2CD5A: free (vg_replace_malloc.c:530)
==16976==    by 0x65539C5: WebCore::GLContext::cleanupActiveContextsAtExit() (in /build/local/lib/libwebkit2gtk-4.0.so.37.14.4)
==16976==    by 0x764FC07: __run_exit_handlers (in /usr/lib64/libc-2.23.so)
==16976==    by 0x764FC54: exit (in /usr/lib64/libc-2.23.so)
==16976==    by 0x7635727: (below main) (in /usr/lib64/libc-2.23.so)
==16976==  Block was alloc'd at
==16976==    at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
==16976==    by 0xA8EB868: WTF::fastMalloc(unsigned long) (in /build/local/lib/libjavascriptcoregtk-4.0.so.18.4.4)
==16976==    by 0x6563F1D: WebCore::GLContextGLX::createPbufferContext(WebCore::PlatformDisplay&, __GLXcontextRec*) (in /build/local/lib/libwebkit2gtk-4.0.so.37.14.4)
==16976==    by 0x6564272: WebCore::GLContextGLX::createSharingContext(WebCore::PlatformDisplay&) (in /build/local/lib/libwebkit2gtk-4.0.so.37.14.4)
==16976==    by 0x6553ED6: WebCore::GLContext::createSharingContext(WebCore::PlatformDisplay&) (in /build/local/lib/libwebkit2gtk-4.0.so.37.14.4)
==16976==    by 0x6658222: WebCore::PlatformDisplay::sharingGLContext() (in /build/local/lib/libwebkit2gtk-4.0.so.37.14.4)
==16976==    by 0x656418D: WebCore::GLContextGLX::createContext(unsigned long, WebCore::PlatformDisplay&) (in /build/local/lib/libwebkit2gtk-4.0.so.37.14.4)
==16976==    by 0x6553D6F: WebCore::GLContext::createContextForWindow(unsigned long, WebCore::PlatformDisplay*) (in /build/local/lib/libwebkit2gtk-4.0.so.37.14.4)
==16976==    by 0x562DBD5: WebKit::ThreadedCompositor::makeContextCurrent() (in /build/local/lib/libwebkit2gtk-4.0.so.37.14.4)
==16976==    by 0x562DCD4: WebKit::ThreadedCompositor::renderLayerTree() (in /build/local/lib/libwebkit2gtk-4.0.so.37.14.4)
==16976==    by 0x562AB44: WebKit::CompositingRunLoop::updateTimerFired() (in /build/local/lib/libwebkit2gtk-4.0.so.37.14.4)
==16976==    by 0xA92C78C: WTF::RunLoop::TimerBase::TimerBase(WTF::RunLoop&)::{lambda(void*)#1}::_FUN(void*) (in /build/local/lib/libjavascriptcoregtk-4.0.so.18.4.4)
==16976== 
==16976== Jump to the invalid address stated on the next line
==16976==    at 0x0: ???
==16976==    by 0x6575F68: WebCore::PlatformDisplayX11::~PlatformDisplayX11() (in /build/local/lib/libwebkit2gtk-4.0.so.37.14.4)
==16976==    by 0x764FC07: __run_exit_handlers (in /usr/lib64/libc-2.23.so)
==16976==    by 0x764FC54: exit (in /usr/lib64/libc-2.23.so)
==16976==    by 0x7635727: (below main) (in /usr/lib64/libc-2.23.so)
==16976==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
Comment 2 Milan Crha 2016-09-06 09:51:47 PDT
The valgrind claims more issues here, maybe consider giving it a try too. The claims are on usage of uninitialized memory.
Comment 3 Michael Catanzaro 2016-09-06 12:01:15 PDT
(In reply to comment #1)
> valgrind output (without detailed debuginfo, because of having issues with
> it):
> 
> ==16976== Thread 1:
> ==16976== Invalid read of size 8
> ==16976==    at 0x66581C7: WebCore::PlatformDisplay::~PlatformDisplay() (in
> /build/local/lib/libwebkit2gtk-4.0.so.37.14.4)
> ==16976==    by 0x6575F68:
> WebCore::PlatformDisplayX11::~PlatformDisplayX11() (in
> /build/local/lib/libwebkit2gtk-4.0.so.37.14.4)
> ==16976==    by 0x764FC07: __run_exit_handlers (in /usr/lib64/libc-2.23.so)
> ==16976==    by 0x764FC54: exit (in /usr/lib64/libc-2.23.so)
> ==16976==    by 0x7635727: (below main) (in /usr/lib64/libc-2.23.so)
> ==16976==  Address 0x31788f70 is 0 bytes inside a block of size 64 free'd
> ==16976==    at 0x4C2CD5A: free (vg_replace_malloc.c:530)
> ==16976==    by 0x65539C5: WebCore::GLContext::cleanupActiveContextsAtExit()
> (in /build/local/lib/libwebkit2gtk-4.0.so.37.14.4)
> ==16976==    by 0x764FC07: __run_exit_handlers (in /usr/lib64/libc-2.23.so)
> ==16976==    by 0x764FC54: exit (in /usr/lib64/libc-2.23.so)
> ==16976==    by 0x7635727: (below main) (in /usr/lib64/libc-2.23.so)

OK that shows the problem, good job getting the valgrind output!
Comment 4 Michael Catanzaro 2016-09-06 12:03:20 PDT
(In reply to comment #2)
> The valgrind claims more issues here, maybe consider giving it a try too.
> The claims are on usage of uninitialized memory.

Consider filing a new bug if it's not bug #146729
Comment 5 Michael Catanzaro 2016-09-06 13:56:33 PDT
OK, so... there are a couple different ways to fix this, but fundamentally the problem is the GLContext class hands off ownership of new GLContexts to callers in its create functions, returning std::unique_ptrs. Then it goes behind the callers' backs and deletes them all in an exit handler if they haven't already been deleted. That fails here, as PlatformDisplay also gets deleted in an exit handler, which runs after GLContext's exit handler, causing the same GLContext to be deleted twice.

There are many ways to fix this... remove the GLContext exit handler (seems like the correct solution, but it carries a warning of possibly crashing the X server when using nvidia proprietary driver? do we care?), leak the PlatformDisplay's GLContext by calling release() on the unique_ptr and rely on the GLContext exit handler to free it, create a throwaway GLContext the first time PlatformDisplay::initializeEGLDisplay is called to ensure the GLContext exit handler gets registered first (and therefore executed last at exit time)... let's see if Carlos Garcia has an opinion on what's best.
Comment 6 Carlos Garcia Campos 2016-09-06 23:10:33 PDT
(In reply to comment #5)
> OK, so... there are a couple different ways to fix this, but fundamentally
> the problem is the GLContext class hands off ownership of new GLContexts to
> callers in its create functions, returning std::unique_ptrs. Then it goes
> behind the callers' backs and deletes them all in an exit handler if they
> haven't already been deleted. That fails here, as PlatformDisplay also gets
> deleted in an exit handler, which runs after GLContext's exit handler,
> causing the same GLContext to be deleted twice.

This is not accurate. PlatformDisplay is not deleted in an exit handler, it's a static std::unique_ptr that is deleted after all exit handlers.

> There are many ways to fix this... remove the GLContext exit handler (seems
> like the correct solution, but it carries a warning of possibly crashing the
> X server when using nvidia proprietary driver? do we care?),

Never liked that, TBH, I was tempted to remove it when reworked the GLContext. It's probably a workaround for a bug in nvidia drivers that might have already been fixed. In any case, all contexts created now except the sharing context that is owned by the PlatformDisplay should be freed already on exit.

> leak the
> PlatformDisplay's GLContext by calling release() on the unique_ptr and rely
> on the GLContext exit handler to free it,

This is not possible because the GLContext exit handler is a workaround used on for X11 contexts, and other classes shouldn't rely on that.

> create a throwaway GLContext the
> first time PlatformDisplay::initializeEGLDisplay is called to ensure the
> GLContext exit handler gets registered first (and therefore executed last at
> exit time)... let's see if Carlos Garcia has an opinion on what's best.

This has nothing to do with EGLDisplay, the crash is happening on ~PlatformDisplay and the exit handler of the EGLDisplay doesn't delete the PlatformDisplay.

So, the solution is either removing the GLContext active contents handling assuming the nvidia issue is now fixed, or the conservative solution could be to not handle the sharing context as active context, since we are sure PlatformDisplay destructor is always called.
Comment 7 Carlos Garcia Campos 2016-09-06 23:38:48 PDT
The original code deleted the GraphicsContext3D instead of the GLContext, so it seems to me this was a hack to workaround both a bug in nvidia drivers, and a leak in WebKit. I'll remove that code and if we get reports about X server crashes we can look for a better solution.
Comment 8 Carlos Garcia Campos 2016-09-06 23:56:15 PDT
Created attachment 288112 [details]
Patch
Comment 9 Carlos Garcia Campos 2016-09-07 00:00:58 PDT
Comment on attachment 288112 [details]
Patch

This is probably too risky at this point of the cycle, I'll use a conservative approach instead.
Comment 10 Carlos Garcia Campos 2016-09-07 00:33:27 PDT
Created attachment 288115 [details]
Patch

Let's use this for now.
Comment 11 Milan Crha 2016-09-07 01:08:28 PDT
Created attachment 288117 [details]
valgrind log with applied patch

(In reply to comment #4)
> Consider filing a new bug if it's not bug #146729

Okay, I updated that bug report.

(In reply to comment #10)
> Let's use this for now.

Thanks, it fixes the crash for me. Valgrind claims some leaked memory, though some of it goes down to the intel drivers which may or may not be the driver issue. See the attached valgrind log.
Comment 12 Carlos Garcia Campos 2016-09-07 02:18:50 PDT
(In reply to comment #9)
> Comment on attachment 288112 [details]
> Patch
> 
> This is probably too risky at this point of the cycle, I'll use a
> conservative approach instead.

Maybe we can land this one in trunk and the other one in the stable branch.
Comment 13 Michael Catanzaro 2016-09-07 05:59:27 PDT
(In reply to comment #6)
> This is not accurate. PlatformDisplay is not deleted in an exit handler,
> it's a static std::unique_ptr that is deleted after all exit handlers.

Where do you think static objects get deleted. :)

> Never liked that, TBH, I was tempted to remove it when reworked the
> GLContext. It's probably a workaround for a bug in nvidia drivers that might
> have already been fixed. In any case, all contexts created now except the
> sharing context that is owned by the PlatformDisplay should be freed already
> on exit.

Agreed.

> This is not possible because the GLContext exit handler is a workaround used
> on for X11 contexts, and other classes shouldn't rely on that.

Good point.

> > create a throwaway GLContext the
> > first time PlatformDisplay::initializeEGLDisplay is called to ensure the
> > GLContext exit handler gets registered first (and therefore executed last at
> > exit time)... let's see if Carlos Garcia has an opinion on what's best.
> 
> This has nothing to do with EGLDisplay, the crash is happening on
> ~PlatformDisplay and the exit handler of the EGLDisplay doesn't delete the
> PlatformDisplay.

Yes, I was completely wrong; for some reason I thought we were deleting the PlatformDisplay in the exit handler registered in that function, but clearly that's not the case.
Comment 14 Michael Catanzaro 2016-09-07 06:04:00 PDT
Comment on attachment 288112 [details]
Patch

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

I prefer to use this patch in trunk.

> Source/WebCore/ChangeLog:8
> +        Stop tracking X11 GL contexts to be cleanered on an exit handler. This was added to work around bugs on drivers,

cleared
Comment 15 Michael Catanzaro 2016-09-07 06:10:16 PDT
Comment on attachment 288115 [details]
Patch

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

rs=me for 2.14, but I prefer just removing the code in trunk.

> Source/WebCore/ChangeLog:13
> +        pointer ownership. Since this is specific to GLX, I've moed the code from GLContext to GLContextGLX and

moved

> Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:209
> +    clear();
> +    activeContexts().remove(this);

Good idea.

> Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:225
> +    glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, 0);
> +    glXMakeCurrent(downcast<PlatformDisplayX11>(m_display).native(), None, None);

Is this new?
Comment 16 Carlos Garcia Campos 2016-09-07 06:13:00 PDT
(In reply to comment #15)
> Comment on attachment 288115 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288115&action=review
> 
> rs=me for 2.14, but I prefer just removing the code in trunk.

Agree.

> > Source/WebCore/ChangeLog:13
> > +        pointer ownership. Since this is specific to GLX, I've moed the code from GLContext to GLContextGLX and
> 
> moved
> 
> > Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:209
> > +    clear();
> > +    activeContexts().remove(this);
> 
> Good idea.
> 
> > Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:225
> > +    glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, 0);
> > +    glXMakeCurrent(downcast<PlatformDisplayX11>(m_display).native(), None, None);
> 
> Is this new?

No, it was in the destructor and now in clear().
Comment 17 Carlos Garcia Campos 2016-09-07 06:49:16 PDT
Committed r205544: <http://trac.webkit.org/changeset/205544>