Bug 161605

Summary:

[GTK] Crash of WebProcess on the last WebView disconnect

Product:

WebKit

Reporter:

Milan Crha <mcrha>

Component:

WebKitGTK

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bugs-noreply, cgarcia, mcatanzaro

Priority:

Version:

WebKit Local Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	mcatanzaro: review+
Patch	none
valgrind log with applied patch	none

Milan Crha

Reported 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 {

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
View All Add attachment proposed patch, testcase, etc.

Milan Crha

Comment 1 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

Milan Crha

Comment 2 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.

Michael Catanzaro

Comment 3 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!

Michael Catanzaro

Comment 4 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

Michael Catanzaro

Comment 5 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.

Carlos Garcia Campos

Comment 6 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.

Carlos Garcia Campos

Comment 7 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.

Carlos Garcia Campos

Comment 8 2016-09-06 23:56:15 PDT

Created attachment 288112 [details] Patch

Carlos Garcia Campos

Comment 9 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.

Carlos Garcia Campos

Comment 10 2016-09-07 00:33:27 PDT

Created attachment 288115 [details] Patch Let's use this for now.

Milan Crha

Comment 11 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.

Carlos Garcia Campos

Comment 12 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.

Michael Catanzaro

Comment 13 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.

Michael Catanzaro

Comment 14 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

Michael Catanzaro

Comment 15 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?

Carlos Garcia Campos

Comment 16 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().

Carlos Garcia Campos

Comment 17 2016-09-07 06:49:16 PDT

Committed r205544: <http://trac.webkit.org/changeset/205544>

Note You need to log in before you can comment on or make changes to this bug.