Bug 90085 – [GTK] Using a native window for the WebView breaks GtkOverlay

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

Which is the case of epiphany, the child widgets of the GtkOverlay are not shown when webkit is built with AC enabled. The problem happens with WebKit1 and WebKit2, and I've checked that it doesn't happen when building without AC (I used --disable-accelerated-compositing --disable-webgl --with-acceleration-backend=none configure options, it needs a patch to build with those flags, though). 
I think we should fix the build with AC disabled, and disable it by default until the problem is fixed beause it breaks important basic features in epiphany (loading progress bar, hovered link messages and fullscreen message box)

[Carlos Garcia Campos]

Btw, the OSD style class is relevant because it uses a transparent background for the GtkOverlay, so it's a problem with transparencies.

[Martin Robinson]

A quick update debugging this issue. It seems that the root of the problem is that GtkOverlay does not react well when the background widget has its own native window. This feels like a bug in GTK+, but I'm still digging for the cause.

What we know so far is that GtkOverlay creates child windows for all its overlaid widgets and calls gtk_widget_set_parent_window on them. I'm going to try to explore whether or not the overlaid widget windows are just hiding behind the WebView window for some reason.

[Carlos Garcia Campos]

I tried lowering and raising the child windows in the z-order and it didn't work. Note that it works even with AC enabled if the overlay uses an opaque background color. It seems the problem has to do with the way things are composed.

[Martin Robinson]

Another update: I haven't spent any time looking at the GTK+ bug, but I've successfully eliminated the need to have a native window for accelerated compositing support using XComposite to redirect a window a pixmap. This approach has quite a few other benefits as well such as allowing us to do pixel tests with accelerated compositing and 3D CSS transforms.

I have the patch cooking locally, but I hope to post it early next week. If there's a release coming up, I'm fine with disabling accelerated compositing for the moment. I wanted to delay turning it off in the case that the fix was simple.

Again I hope to post the patch within a few days though.

[Carlos Garcia Campos]

(In reply to comment #4)
> Another update: I haven't spent any time looking at the GTK+ bug, but I've successfully eliminated the need to have a native window for accelerated compositing support using XComposite to redirect a window a pixmap. This approach has quite a few other benefits as well such as allowing us to do pixel tests with accelerated compositing and 3D CSS transforms.

Great news.

> I have the patch cooking locally, but I hope to post it early next week. If there's a release coming up, I'm fine with disabling accelerated compositing for the moment. I wanted to delay turning it off in the case that the fix was simple.

Sure.

> Again I hope to post the patch within a few days though.

[Martin Robinson]

Created attachment 151106 [details]
Patch

[Martin Robinson]

Created attachment 152344 [details]
Add xcomposite pkg-config parsing to configuration, fix some assertions, and improve performance on the 2012 beercamp demo

[Martin Robinson]

Created attachment 157755 [details]
Patch

[Martin Robinson]

Created attachment 159158 [details]
Patch fixing issues with scrolling

[Carlos Garcia Campos]

Comment on attachment 159158 [details]
Patch fixing issues with scrolling

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

> Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:103
> +    cleanupPixmapAndPixmapSurface();

You should probably remove the source if m_pendingResizeSourceId > 0 to avoid the callback could be called on a destroyed object.

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:115
> +bool AcceleratedCompositingContext::renderLayersToWindow(cairo_t* cr, const IntRect& clipRect)

clipRect is passed but it doesn't seem to be used, is it really needed?

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:135
> +
> +

Extra line here?

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:228
> +    if (m_layerFlushTimerCallbackId) {
> +        g_source_remove(m_layerFlushTimerCallbackId);
> +        m_layerFlushTimerCallbackId = 0;
> +    }

This is done 3 times, maybe you could add a helper clearLayerFlushTimerSource() or seomthing similar.

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:321
> +    {
> +        Frame* frame = core(m_webView)->mainFrame();
> +        if (!frame || !frame->contentRenderer() || !frame->view())
> +            return;
> +        frame->view()->updateLayoutAndStyleIfNeededRecursive();
> +    }

why the braces?

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:1010
> +    IntRect webViewRect = getWebViewRect(m_webView);
> +    if (turningOnCompositing) {
> +        m_displayTimer.stop();
> +        m_webView->priv->backingStore = WebCore::WidgetBackingStore::create(GTK_WIDGET(m_webView), IntSize(1, 1));
> +    }
> +
> +    if (turningOffCompositing) {
> +        m_webView->priv->backingStore = WebCore::WidgetBackingStore::create(GTK_WIDGET(m_webView), webViewRect.size());
> +        RefPtr<cairo_t> cr = adoptRef(cairo_create(m_webView->priv->backingStore->cairoSurface()));
> +        clearEverywhereInBackingStore(m_webView, cr.get());
> +    }

webViewRect is only used when turning compositing off, so it could be moved here and maybe don't use a local variable:

m_webView->priv->backingStore = WebCore::WidgetBackingStore::create(GTK_WIDGET(m_webView), getWebViewRect().size());

[Martin Robinson]

(In reply to comment #10)

Thanks for the review!

> (From update of attachment 159158 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159158&action=review
> 
> > Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:103
> > +    cleanupPixmapAndPixmapSurface();
> 
> You should probably remove the source if m_pendingResizeSourceId > 0 to avoid the callback could be called on a destroyed object.

Okay.

> 
> > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:115
> > +bool AcceleratedCompositingContext::renderLayersToWindow(cairo_t* cr, const IntRect& clipRect)
> 
> clipRect is passed but it doesn't seem to be used, is it really needed?

It can't be removed, because it might be used by the other implementations. In fact, I'll use it here to speed up painting AC results after damage events.
> 
> > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:135
> > +
> > +
> 
> Extra line here?

Removed.


> > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:228
> > +    if (m_layerFlushTimerCallbackId) {
> > +        g_source_remove(m_layerFlushTimerCallbackId);
> > +        m_layerFlushTimerCallbackId = 0;
> > +    }
> 
> This is done 3 times, maybe you could add a helper clearLayerFlushTimerSource() or seomthing similar.

Okay.

> > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:321
> > +    {
> > +        Frame* frame = core(m_webView)->mainFrame();
> > +        if (!frame || !frame->contentRenderer() || !frame->view())
> > +            return;
> > +        frame->view()->updateLayoutAndStyleIfNeededRecursive();
> > +    }
> 
> why the braces?

There was a smart pointer in an older implementation. I've removed them.

> webViewRect is only used when turning compositing off, so it could be moved here and maybe don't use a local variable:

Sounds good.

[Martin Robinson]

Created attachment 159341 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 159341 [details]
Patch

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

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:-60
> -    if (m_syncTimerCallbackId)
> -        g_source_remove(m_syncTimerCallbackId);

This should be now stopAnyPendingLayerFlush();

[Martin Robinson]

Created attachment 159426 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 159426 [details]
Patch

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

> Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:108
> +    static int framebufferAttributes[] = {
> +        GLX_DRAWABLE_TYPE, GLX_WINDOW_BIT,
> +        GLX_DOUBLEBUFFER, bufferingType == DoubleBuffer ? True : False,
> +        GLX_RENDER_TYPE, GLX_RGBA_BIT,
> +        GLX_RED_SIZE, 1,
> +        GLX_GREEN_SIZE, 1,
> +        GLX_BLUE_SIZE, 1,
> +        GLX_STENCIL_SIZE, 1,
> +        None
> +    };

if this is static, GLX_DOUBLEBUFFER would only be initialized the first time with whatever bufferingType is, no?

> Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:114
> +    if (!numberReturned || !configs)
> +        return nullptr;

is configs leaked here if !numberReturned? or glXChooseFBConfig never sets numberReturned=0?

[Martin Robinson]

(In reply to comment #15)
> (From update of attachment 159426 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159426&action=review
> 
> > Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:108
> > +    static int framebufferAttributes[] = {
> > +        GLX_DRAWABLE_TYPE, GLX_WINDOW_BIT,
> > +        GLX_DOUBLEBUFFER, bufferingType == DoubleBuffer ? True : False,
> > +        GLX_RENDER_TYPE, GLX_RGBA_BIT,
> > +        GLX_RED_SIZE, 1,
> > +        GLX_GREEN_SIZE, 1,
> > +        GLX_BLUE_SIZE, 1,
> > +        GLX_STENCIL_SIZE, 1,
> > +        None
> > +    };
> 
> if this is static, GLX_DOUBLEBUFFER would only be initialized the first time with whatever bufferingType is, no?

Nice catch! This isn't a huge deal since there's no double-buffered GLContexts at the moment, but I've fixed this.

> > Source/WebCore/platform/graphics/glx/GLContextGLX.cpp:114
> > +    if (!numberReturned || !configs)
> > +        return nullptr;
> 
> is configs leaked here if !numberReturned? or glXChooseFBConfig never sets numberReturned=0?

I believe that when numberReturned == 0, configs will always be null.

[Martin Robinson]

Created attachment 159694 [details]
Patch

[Martin Robinson]

Created attachment 159728 [details]
Patch

[Alejandro G. Castro]

Comment on attachment 159728 [details]
Patch

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

Great patch, LGTM. This seems the way to go.

> Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:66
> +    m_parentWindow = XCreateWindow(display,
> +                                   RootWindow(display, DefaultScreen(display)),
> +                                    -100, -100, 1, 1,
> +                                    0,
> +                                    CopyFromParent,
> +                                    InputOutput,
> +                                    CopyFromParent,
> +                                    CWOverrideRedirect,
> +                                    &windowAttributes);

Indent typo.

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContext.h:-81
> -#if USE(TEXTURE_MAPPER_GL)
>      WebCore::GLContext* glContext();
>      OwnPtr<WebCore::GLContext> m_context;
>  #endif
> -#endif

Why do you remove this #if?

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:193
> +    // called. For non-animation situations we use this terrible hack until we can get to the

Trailing whitespace in horrible hack comment ;).

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:216
> +    // to update. This isn't a problem during animations, because swapBuffer is continuously 

Ditto.

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:348
> +    //    return;

I guess this is a leftover.

[Martin Robinson]

Committed r126182: <http://trac.webkit.org/changeset/126182>

[Martin Robinson]

(In reply to comment #19)

Thanks for the review!

> > Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp:66
> > +    m_parentWindow = XCreateWindow(display,
> > +                                   RootWindow(display, DefaultScreen(display)),
> > +                                    -100, -100, 1, 1,
> > +                                    0,
> > +                                    CopyFromParent,
> > +                                    InputOutput,
> > +                                    CopyFromParent,
> > +                                    CWOverrideRedirect,
> > +                                    &windowAttributes);
> 
> Indent typo.

Fixed.

> > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContext.h:-81
> > -#if USE(TEXTURE_MAPPER_GL)
> >      WebCore::GLContext* glContext();
> >      OwnPtr<WebCore::GLContext> m_context;
> >  #endif
> > -#endif
> 
> Why do you remove this #if?

I actually meant to remove these entirely. I've done that.

> > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:193
> > +    // called. For non-animation situations we use this terrible hack until we can get to the
> 
> Trailing whitespace in horrible hack comment ;).

Fixed.


> > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:348
> > +    //    return;
> 
> I guess this is a leftover.

I meant this to be

if (!flush....)
   return;

[WebKit Review Bot]

Re-opened since this is blocked by 94678

[Daniel Drake]

r126182 a regression in plugin display, filed as bug #98789.