[Webkit-unassigned] [Bug 151139] [GTK] Web Process crashes on reparenting a WebView with AC mode on

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=151139

--- Comment #11 from Mario Sanchez Prada <mario at webkit.org> ---
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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151113/a8099350/attachment.html>