Created attachment 265293 [details] Backtraces for the UI and Web processes (Release build) At least since WebKitGTK+ 2.10.0, the Web Process does very often die when the WebView gets reparented from one container to another (removing + adding, NOT using gtk_widget_reparent()) while using Accelerated Compositing, which seems to happen due to a X BadDrawable error, which eventually makes the UI process crash as well. More specifically, the error that is being reported from the X.org server is like this: (WebKitWebProcess:8500): Gdk-ERROR **: The program 'WebKitWebProcess' received an X Window System error. This probably reflects a bug in the program. The error was 'BadDrawable (invalid Pixmap or Window parameter)'. (Details: serial 690 error_code 9 request_code 152 (DRI2) minor_code 8) (Note to programmers: normally, X errors are reported asynchronously; that is, you will receive the error a while after causing it. To debug your program, run it with the GDK_SYNCHRONIZE environment variable to change this behavior. You can then get a meaningful backtrace from your debugger if you break on the gdk_x_error() function.) Please see attached a dump of the two backtraces as they are generated by the UI and the Web processes. Those were taken with a release build of WebKit since that's the one I have around now, but I will try to reproduce it in a debug build asap as well, will attach it later. This crash has happened in a 64-bit Fedora 22 machine with the latest code from WebKit's trunk compiled from sources, using the build-webkit script. Additional details: This issue was not happening at all in 2.6.2 and it's crashing from 2.10.0 as far as I can see, although in 2.8.x some graphics corruption is present already when reparenting already. Carlos explained to me on IRC that this is mostly to the move to using PlatformDisplay, which reuses GDK's connection to the X display and install a bunch of error handlers, which were not present before (thus the problem has probably been there for a while, and was simply ignored) Anyway, just for the sake of completeness I'm also linking an screencast showing the issue from inside a 32-bit Debian-like chroot: https://drive.google.com/file/d/0B6Gdj3EoWfFLSjZzdHFuVFJERGM/view?usp=sharing Next, I will attach two test cases I wrote which I can reproduce the bug reliably with
Created attachment 265296 [details] Test case in Javascript See attached a test case in JavaScript using GObject Introspection. Using this, I can reliably get the web process crashing inside my chroot (see the video linked in the bug's description), both by making WebKit load the simple HTML content included with the test or by loading another example from the web, such as the Poster Circle 3D CSS example (uncomment + comment the line in the code to try it out).
Created attachment 265297 [details] Test case in C See attached a test case in C that I used to get the backtraces from inside WebKitGTK's development jhbuild environment, using ToT. As with the other example, feel free to use either the sample HTML content included with the test or comment/uncomment the relevant lines to try it out with the Poster Circle (or anything else enabling AC, really) instead.
Created attachment 265302 [details] Backtraces for the UI and Web processes (Debug build) Seems like I end up getting the backtraces from a debug build (SVN r192098) sooner than what I expected, so I'm attaching them right now. I'm still not sure the web process backtrace is useful since it shows the moment the X error is noticed but not when it's generated, so I'll try to get one setting GDK_SYNCHRONIZE=1. In any case, it shows at least why the UI Process it's dying, which seems to be due to drawingArea being NULL in the following snippet: static void webkitWebViewBaseRealize(GtkWidget* widget) { WebKitWebViewBase* webView = WEBKIT_WEB_VIEW_BASE(widget); WebKitWebViewBasePrivate* priv = webView->priv; #if USE(REDIRECTED_XCOMPOSITE_WINDOW) [...] if (priv->redirectedWindow) { DrawingAreaProxyImpl* drawingArea = static_cast<DrawingAreaProxyImpl*>(priv->pageProxy->drawingArea()); drawingArea->setNativeSurfaceHandleForCompositing(priv->redirectedWindow->windowID()); } } #endif [...] } My theory is that when the X error comes bad things happen to the XRedirectedCompositeWindow and the drawing area dies too, causing the proxy here to die as well. But that's a theory, we need facts :)
I might be doing something wrong, because the backtrace I get by setting GDK_SYNCHRONIZE=1 is pretty much identical to the one I already posted... :/ Anyway, looking deeper with gdb, the problem seems to start with this: void DrawingAreaImpl::enterAcceleratedCompositingMode(GraphicsLayer* graphicsLayer) { m_exitCompositingTimer.stop(); m_wantsToExitAcceleratedCompositingMode = false; m_webPage.send(Messages::DrawingAreaProxy::WillEnterAcceleratedCompositingMode(m_backingStoreStateID)); ASSERT(!m_layerTreeHost); m_layerTreeHost = LayerTreeHost::create(&m_webPage); #if USE(TEXTURE_MAPPER) && PLATFORM(GTK) if (m_nativeSurfaceHandleForCompositing) m_layerTreeHost->setNativeSurfaceHandleForCompositing(m_nativeSurfaceHandleForCompositing); #endif [...] } It looks like the call m_layerTreeHost->setNativeSurfaceHandleForCompositing(m_nativeSurfaceHandleForCompositing) is happening over a handle that is not valid, hence the BadDrawable error later on when calling XGetWindowAttributes() from WebCore::GLContextGLX::createWindowContext(). Carlos, does this ring any bell? Could it be possible that there's a race condition between consecutive calls to DrawingAreaImpl::enterAcceleratedCompositingMode and DrawingAreaImpl::exitAcceleratedCompositingMode(), for instance, that might be causing that a rogue handler ID is kept there?
This is yet another regression introduced when the redirected x window creation was moved to realize. The problem is that when the web view is unrealized and realized again (for example when reparented like in your example), the redirected window is re-created. That happens quite fast by the unique_ptr, in the UI process, and the web process keeps using the old redirected window ID until a new SetNativeSurfaceHandleForCompositing message is sent. This makes the web process crash, and the drawing area is deleted, since we don't properly recover from the crash, the next realize/unrealize pair makes realize use the null drawing area causing also the ui process to crash. We need to delete the redirected window earlier, un unrealize, but notify the web process before to stop rendering layers.
Created attachment 265372 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Created attachment 265374 [details] Try to fix builds
Created attachment 265387 [details] Backtrace for the WebProcess with Carlos's patch As commented with Carlos on IRC, the patch definitely fixes the crash in the UI process (as the drawingAreaImpl variable is now null-checked before using) but I'm still seeing the crash in the WebProcess with the attached test cases, specially if running the Poster Circle example. You can see attached the full backtrace when that happens, although that's not a very useful one since it crashes on an ASSERT that it's probably wrong anyway. Now, removing that ASSERT and running it again, this is the backtrace I get on a Debug build: Breakpoint 1, gdk_x_error (xdisplay=0x46b8d0, error=0x7fffffffcb20) at gdkmain-x11.c:268 268 if (error->error_code) (gdb) bt #0 gdk_x_error (xdisplay=0x46b8d0, error=0x7fffffffcb20) at gdkmain-x11.c:268 #1 0x00007fffe7f3646d in _XError (dpy=dpy@entry=0x46b8d0, rep=rep@entry=0xacd270) at ../../src/XlibInt.c:1429 #2 0x00007fffe7f333a7 in handle_error (dpy=dpy@entry=0x46b8d0, err=0xacd270, in_XReply=in_XReply@entry=1) at ../../src/xcb_io.c:213 #3 0x00007fffe7f34525 in _XReply (dpy=dpy@entry=0x46b8d0, rep=rep@entry=0x7fffffffcce0, extra=extra@entry=0, discard=discard@entry=1) at ../../src/xcb_io.c:699 #4 0x00007fffe7f18cfe in XGetGeometry (dpy=0x46b8d0, d=27263103, root=0x7fffffffcd90, x=0x7fffffffcd9c, y=0x7fffffffcd98, width=0x7fffffffcd8c, height=0x7fffffffcd88, borderWidth=0x7fffffffcd84, depth=0x7fffffffcd80) at ../../src/GetGeom.c:47 #5 0x00007ffff39921d4 in WebCore::GLContextGLX::defaultFrameBufferSize (this=0x800140) at ../../Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:189 #6 0x00007ffff2453054 in WebKit::LayerTreeHostGtk::compositeLayersToContext (this=0x7fffdb2eec60, purpose=WebKit::LayerTreeHostGtk::NotForResize) at ../../Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:335 #7 0x00007ffff245321e in WebKit::LayerTreeHostGtk::flushAndRenderLayers (this=0x7fffdb2eec60) at ../../Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:368 #8 0x00007ffff2452e74 in WebKit::LayerTreeHostGtk::renderFrame (this=0x7fffdb2eec60) at ../../Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:307 #9 0x00007ffff2454dbf in std::_Mem_fn_base<bool (WebKit::LayerTreeHostGtk::*)(), true>::operator()<, void>(std::_Mem_fn_base<bool (WebKit::LayerTreeHostGtk::*)(), true>::_Class *) const (this=0x847370, __object=0x7fffdb2eec60) at /usr/include/c++/5/functional:600 #10 0x00007ffff2454d00 in std::_Bind<std::_Mem_fn<bool (WebKit::LayerTreeHostGtk::*)()>(WebKit::LayerTreeHostGtk*)>::__call<bool, 0ul>(<unknown type in /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37, CU 0x0, DIE 0x1540a0>, std::_Index_tuple<0ul>) (this=0x847370, __args=<unknown type in /home/mario/work/WebKit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37, CU 0x0, DIE 0x1540a0>) at /usr/include/c++/5/functional:1074 #11 0x00007ffff2454a92 in std::_Bind<std::_Mem_fn<bool (WebKit::LayerTreeHostGtk::*)()>(WebKit::LayerTreeHostGtk*)>::operator()<, bool>(void) (this=0x847370) at /usr/include/c++/5/functional:1133 #12 0x00007ffff2454635 in std::_Function_handler<bool(), std::_Bind<std::_Mem_fn<bool (WebKit::LayerTreeHostGtk::*)()>(WebKit::LayerTreeHostGtk*)> >::_M_invoke(const std::_Any_data &) (__functor=...) at /usr/include/c++/5/functional:1857 #13 0x00007ffff2453a8a in std::function<bool()>::operator()(void) const (this=0x7fffdb2eed50) at /usr/include/c++/5/functional:2271 #14 0x00007ffff2451d06 in WebKit::LayerTreeHostGtk::RenderFrameScheduler::renderFrame (this=0x7fffdb2eed50) at ../../Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:124 #15 0x00007ffff2454e90 in WTF::RunLoop::Timer<WebKit::LayerTreeHostGtk::RenderFrameScheduler>::fired (this=0x7fffdb2eed70) at ../../Source/WTF/wtf/RunLoop.h:131 #16 0x00007fffec3df2e7 in WTF::RunLoop::TimerBase::TimerBase(WTF::RunLoop&)::{lambda(void*)#1}::operator()(void*) const () at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:131 #17 0x00007fffec3df323 in WTF::RunLoop::TimerBase::TimerBase(WTF::RunLoop&)::{lambda(void*)#1}::_FUN(void*) () at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:135 #18 0x00007fffec3deb0c in WTF::<lambda(GSource*, GSourceFunc, gpointer)>::operator()(GSource *, GSourceFunc, gpointer) const (__closure=0x0, source=0x510e70, callback=0x7fffec3df306 <WTF::RunLoop::TimerBase::TimerBase(WTF::RunLoop&)::{lambda(void*)#1}::_FUN(void*)>, userData=0x7fffdb2eed70) at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:44 #19 0x00007fffec3deb3b in WTF::<lambda(GSource*, GSourceFunc, gpointer)>::_FUN(GSource *, GSourceFunc, gpointer) () at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:45 #20 0x00007fffe955bbea in g_main_dispatch (context=0x466640) at gmain.c:3122 #21 g_main_context_dispatch (context=context@entry=0x466640) at gmain.c:3737 #22 0x00007fffe955bf68 in g_main_context_iterate (context=0x466640, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3808 #23 0x00007fffe955c282 in g_main_loop_run (loop=0xaf5820) at gmain.c:4002 #24 0x00007fffec3df104 in WTF::RunLoop::run () at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:94 #25 0x00007ffff2450199 in WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain> (argc=2, argv=0x7fffffffd348) at ../../Source/WebKit2/Shared/unix/ChildProcessMain.h:61 #26 0x00007ffff2450000 in WebKit::WebProcessMainUnix (argc=2, argv=0x7fffffffd348) at ../../Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:77 #27 0x0000000000400c6a in main (argc=2, argv=0x7fffffffd348) at ../../Source/WebKit2/WebProcess/EntryPoint/unix/WebProcessMain.cpp:44
From the UI perspective, this what I get printed in the UI process after putting a few printf's around "m_layerTreeContext.contextID = handle" in LayerTreeHostGtk::setNativeSurfaceHandleForCompositing, and around the call to makeContextCurrent() (which always succeeds) in LayerTreeHostGtk::compositeLayersToContext: [../../Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:425] setNativeSurfaceHandleForCompositing :: old handle: 0 [../../Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:429] setNativeSurfaceHandleForCompositing :: new handle: 27263047 Reparenting the WebView... Old parent: 0xb9a660 [../../Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:425] setNativeSurfaceHandleForCompositing :: old handle: 27263047 [../../Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:429] setNativeSurfaceHandleForCompositing :: new handle: 0 New parent: 0xb9a7b0 Reparenting the WebView... Old parent: 0xb9a7b0 [../../Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:425] setNativeSurfaceHandleForCompositing :: old handle: 0 [../../Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:429] setNativeSurfaceHandleForCompositing :: new handle: 27263053 [../../Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:425] setNativeSurfaceHandleForCompositing :: old handle: 27263053 [../../Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:429] setNativeSurfaceHandleForCompositing :: new handle: 0 New parent: 0xb9a660 [../../Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:425] setNativeSurfaceHandleForCompositing :: old handle: 0 [../../Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:429] setNativeSurfaceHandleForCompositing :: new handle: 27263059 [../../Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:329] compositeLayersToContext :: context ID BEFORE makeContextCurrent: 27263059 [../../Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:334] compositeLayersToContext :: context ID AFTER makeContextCurrent (Success): 27263059 (WebKitWebProcess:29232): Gdk-ERROR **: The program 'WebKitWebProcess' received an X Window System error. This probably reflects a bug in the program. The error was 'BadDrawable (invalid Pixmap or Window parameter)'. (Details: serial 203 error_code 9 request_code 14 (core protocol) minor_code 0) (Note to programmers: normally, X errors are reported asynchronously; that is, you will receive the error a while after causing it. To debug your program, run it with the GDK_SYNCHRONIZE environment variable to change this behavior. You can then get a meaningful backtrace from your debugger if you break on the gdk_x_error() function.)
Finally, I think I found what was going on here: I think your last patch was almost perfect but I think there was one thing missing: As far as I can see the flow is between the UI and Web process when unrealizing a webview and then realizing a new one is something like this: 1. In the UIProcess, when REALIZING a new webview right before the first time we enter Accelerated Compositing mode: 1.1. webkitWebViewBaseRealize() creates a XRedirectedWindow, sends its ID (let's call it 'Id1') to the web process via the DrawingArea 1.2. The proxy for the DrawingArea sends a message to the WebProcess with the new XWindow's ID, Id1 2. In the WebProcess, as a response to (1.2): 2.1. DrawingAreaImpl::setNativeSurfaceHandleForCompositing(Id1) gets called in the WebProcess, which calls LayerTreeHostGtk::setNativeSurfaceHandleForCompositing(Id1) 2.2. LayerTreeHostGtk replaces the old handler (0, as AC was disabled) with the new one (Id1) and calls makeContextCurrent() 2.3. As m_layerTreeContext.contextID != 0 and m_context == nullptr when calling makeContextCurrent, a new GLContext for the handler ID is created 2.4. In my case, 1.4. ends up creating a GLXContext for the XRedirectedWindow created in the UI Process (let's call it GLXContext-Id1), and calling GLXContext::makeContextCurrent() 2.5. With a new GLContext created, LayerTreeHostGtk::setNativeSurfaceHandleForCompositing() finishes the work for this Realize process by creating a texture mapper for the layer and scheduling a layer flush. 3. In the UI Process now again, after removing the previous webview, we have an UNREALIZE event: 3.1. webkitWebViewBaseUnrealize is called which calls DrawingAreaProxyImpl::destroyNativeSurfaceHandleForCompositing() 3.2. A synchronous message is sent to the Web Process, to ensure LayoutTreeHostGtk clears its reference to the no longer valid XRedirectedWindow 4. In the Web process, as a response to (3.2): 4.1. DrawingAreaImpl::setNativeSurfaceHandleForCompositing(0) gets called in the WebProcess, which calls LayerTreeHostGtk::setNativeSurfaceHandleForCompositing(0) 4.2. LayerTreeHostGtk replaces the old handler ('Id1') with the new one (0) and calls makeCurrentContext() 4.3. Now THIS IS THE PROBLEM: makeContextCurrent() sees that m_layerTreeContext.contextID == 0 and early returns false. >> The problem is that the GLXContext created in 2.4, which is associated to the handler of the -now invalid- XRedirectedWindow, is kept referenced by m_context 4.4. As makeContextCurrent() returned false, LayerTreeHostGtk::setNativeSurfaceHandleForCompositing() returns early right after that call. 4.5. After this destruction process, the LayerTreeHostGtk is left with m_layerTreeContext.contextID set to ZERO, but with m_context set to the GLXContext created before (GLXContext-Id1) 5. In the UI Process again, a REALIZE event happens after re-adding the webview to a new parent: 5.1. webkitWebViewBaseRealize() creates a XRedirectedWindow, sends its ID (let's call it 'Id2') to the web process via the DrawingArea 5.2. The proxy for the DrawingArea sends a message to the WebProcess with the new XWindow's ID, Id2 6. In the WebProcess, as a response to (5.2): 6.1. DrawingAreaImpl::setNativeSurfaceHandleForCompositing(Id2) gets called in the WebProcess, which calls LayerTreeHostGtk::setNativeSurfaceHandleForCompositing(Id2) 6.2. LayerTreeHostGtk replaces the old handler (0, as per 4.2) with the new one (Id2) and calls makeContextCurrent() 6.3. As m_layerTreeContext.contextID != 0, makeContextCurrent() does not early return, but since m_context is NOT nullptr a new GLContext is NOT created now, and the obsolete GLXContext-Id1 is kept 6.4. Now GLXContext::makeContextCurrent() is called again, but since m_context points to the wrong context, that call is totally bogus, as m_window for that GLXContext will point to the wrong XWindow 6.5. Finally, LayerTreeHostGtk::setNativeSurfaceHandleForCompositing() finishes the work for this Realize process by creating a texture mapper for the layer and scheduling a layer flush. 7. Now that we are in a totally broken state, eventually the following will happen in the WebProcess: 7.1. LayerTreeHostGtk::flushAndRenderLayers() will get called, which will call LayerTreeHostGtk::compositeLayersToContext() 7.2. LayerTreeHostGtk::compositeLayersToContext() will call m_context->defaultFrameBufferSize() to get the size of the viewport 7.3. GLContextGLX::defaultFrameBufferSize() is called now, which calls XGetGeometry(display, m_window, &rootWindow, &x, &y, &width, &height, &borderWidth, &depth) 7.4. Since the reference stored in m_window points to the old 'Id1' of the no longer existing XRedirected, created in 1.1, XGetGeometry generates the BadDrawable error we are seeing 7.5. Boom! :) Now, as hidden as this issue was to my eyes, now that I know what's going on, I believe the solution is not that hard :). It looks to me like what we have to do is to complement Carlos's patch with yet one more addition. Just make sure that the m_context variable gets reset to nullptr when setting the contextID of LayerTreeHostGtk to zero, and we should be fine. And it looks to me like LayerTreeHostGtk::makeContextCurrent(), right before early returning, might be a good place for that: bool LayerTreeHostGtk::makeContextCurrent() { if (!m_layerTreeContext.contextID) { m_context = nullptr; return false; } if (!m_context) { m_context = GLContext::createContextForWindow(reinterpret_cast<GLNativeWindowType>(m_layerTreeContext.contextID), GLContext::sharingContext()); if (!m_context) return false; } return m_context->makeContextCurrent(); } Basically, it would be applying this patch on top of Carlos's last one: --- a/Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp +++ b/Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp @@ -145,8 +145,10 @@ LayerTreeHostGtk::LayerTreeHostGtk(WebPage* webPage) bool LayerTreeHostGtk::makeContextCurrent() { - if (!m_layerTreeContext.contextID) + if (!m_layerTreeContext.contextID) { + m_context = nullptr; return false; + } I've tested this locally and it fixes the issue for me reliably, both in Release and Debug builds. Carlos, what do you think?
(In reply to comment #11) > Basically, it would be applying this patch on top of Carlos's last one: > > --- a/Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp > +++ b/Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp > @@ -145,8 +145,10 @@ LayerTreeHostGtk::LayerTreeHostGtk(WebPage* webPage) > > bool LayerTreeHostGtk::makeContextCurrent() > { > - if (!m_layerTreeContext.contextID) > + if (!m_layerTreeContext.contextID) { > + m_context = nullptr; > return false; > + } > > > I've tested this locally and it fixes the issue for me reliably, both in > Release and Debug builds. > > Carlos, what do you think? I think you write too much :-) awesome analysis, and I agree with your solution, makeContextCurrent() is the one creating the context, so look like a good place to delete it also when the ID becomes 0. I'll submit a new patch. Thanks!
Created attachment 265572 [details] Updated patch Thank you Mario!
(In reply to comment #12) > [...] > I think you write too much :-) You lazy reader! > awesome analysis, and I agree with your solution, makeContextCurrent() is the one creating the context, so look like a good place to delete it also when the ID becomes 0. I'll submit a new patch. Thanks! Thanks for confirming the solution and submitting a new patch, I'll review it now
Comment on attachment 265572 [details] Updated patch Looks good to me. Also, I tested this locally and can confirm it fixes both the situation with the test case and with the original case that exposed the bug, without causing any regression I could notice so far.
Committed r192510: <http://trac.webkit.org/changeset/192510>