Bug 199402 – [GTK] Crash in webkitWebViewBaseRenderHostFileDescriptor

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

From this backtrace:

                Stack trace of thread 764:
                #0  0x00007fbbc42b16df _Z41webkitWebViewBaseRenderHostFileDescriptorP18_WebKitWebViewBase (libwebkit2gtk-4.0.so.37)
                #1  0x00007fbbc42a1c6a _ZN6WebKit14PageClientImpl18hostFileDescriptorEv (libwebkit2gtk-4.0.so.37)
                #2  0x00007fbbc41d33d0 _ZN6WebKit12WebPageProxy18creationParametersERNS_15WebProcessProxyERNS_16DrawingAreaProxyE (libwebkit2gtk-4.0.so.37)
                #3  0x00007fbbc41d38f0 _ZN6WebKit12WebPageProxy17initializeWebPageEv (libwebkit2gtk-4.0.so.37)
                #4  0x00007fbbc41d3aed _ZN6WebKit12WebPageProxy27finishAttachingToWebProcessENS0_13IsProcessSwapE (libwebkit2gtk-4.0.so.37)
                #5  0x00007fbbc41d3e8e _ZN6WebKit12WebPageProxy11loadRequestEON7WebCore15ResourceRequestENS1_28ShouldOpenExternalURLsPolicyEPN3API6ObjectE (libwebkit2gtk-4.0.so.37)
                #6  0x00007fbbc4292f6a webkit_web_view_load_uri (libwebkit2gtk-4.0.so.37)
                #7  0x00007fbbc7e2bee2 ephy_web_view_load_url (libephymain.so)
                #8  0x00007fbbc7e2f96a ephy_web_view_load_homepage (libephymain.so)
                #9  0x00007fbbc7e0c2f6 ephy_window_open_link (libephymain.so)
                #10 0x00007fbbc1a36bb8 ffi_call_unix64 (libffi.so.6)
                #11 0x00007fbbc1a363b4 ffi_call (libffi.so.6)
                #12 0x00007fbbc73f024d g_cclosure_marshal_generic (libgobject-2.0.so.0)
                #13 0x00007fbbc73ef742 g_closure_invoke (libgobject-2.0.so.0)
                #14 0x00007fbbc7402cf6 signal_emit_unlocked_R (libgobject-2.0.so.0)
                #15 0x00007fbbc740b94e g_signal_emit_valist (libgobject-2.0.so.0)
                #16 0x00007fbbc740c963 g_signal_emit (libgobject-2.0.so.0)
                #17 0x00007fbbc7df6fff ephy_link_open (libephymain.so)
                #18 0x00007fbbc7e00f8a ephy_session_resume (libephymain.so)
                #19 0x00007fbbc73ef996 _g_closure_invoke_va (libgobject-2.0.so.0)
                #20 0x00007fbbc740c31c g_signal_emit_valist (libgobject-2.0.so.0)
                #21 0x00007fbbc740c963 g_signal_emit (libgobject-2.0.so.0)
                #22 0x00007fbbc751c5f8 g_application_real_local_command_line (libgio-2.0.so.0)
                #23 0x00007fbbc751c7b2 g_application_run (libgio-2.0.so.0)
                #24 0x00005594709be096 main (epiphany)
                #25 0x00007fbbc6f45f13 __libc_start_main (libc.so.6)
                #26 0x00005594709be31e _start (epiphany)

We can see a crash here, probably because acceleratedBackingStore is null:

#if USE(WPE_RENDERER)
int webkitWebViewBaseRenderHostFileDescriptor(WebKitWebViewBase* webkitWebViewBase)
{
    return webkitWebViewBase->priv->acceleratedBackingStore->renderHostFileDescriptor();
}
#endif

It can happen if AcceleratedBackingStore::create returns null. AcceleratedBackingStore::create has a release assert to make sure that never happens directly, but AcceleratedBackingStoreWayland::create and AcceleratedBackingStoreX11::create do not. Both can return null for various reasons. The crash occurs on a VM image that's probably either got broken OpenGL or a missing GL extension, but it's impossible to tell which from this backtrace because the crash occurs too late.

I suggest we either make WebKitWebViewBase expect and handle the case where acceleratedBackingStore is null, or else crash earlier with a RELEASE_ASSERT in AcceleratedBackingStoreWayland::create and AcceleratedBackingStoreX11::create when it's not. Ideally line number of the crash would indicate why creation failed, e.g.:

RELEASE_ASSERT(display.supportsXComposite());
RELEASE_ASSERT(display.supportsXDamage(s_damageEventBase, s_damageErrorBase));

Missing information:

 * I don't know whether X11 or Wayland is being used here, but it's probably not relevant to this bug because both can return null
 * I don't have an actual proper backtrace from gdb, but I think we see what's going wrong without it

[Michael Catanzaro]

BTW USE(WPE_RENDERER) is obviously enabled and very recent libwpe and wpebackend-fdo is in use.

[Carlos Garcia Campos]

Yes, this is because OpenGL is not available, or any of the extensions not present. In some cases we don't null check because when hw acceleration is not available HardwareAccelerationManager disables AC mode, so we know those methods will never be called. However, in the case of WPE renderer we are not doing all the required checks in HardwareAccelerationManager and some places like webkitWebViewBaseRenderHostFileDescriptor() are always called and null-check is always required. So, we have a combinations of issues here.

[Carlos Garcia Campos]

Created attachment 374372 [details]
Patch

[EWS Watchlist]

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

[Michael Catanzaro]

Comment on attachment 374372 [details]
Patch

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

> Source/WebKit/UIProcess/gtk/AcceleratedBackingStoreWayland.cpp:76
> -std::unique_ptr<AcceleratedBackingStoreWayland> AcceleratedBackingStoreWayland::create(WebPageProxy& webPage)
> +bool AcceleratedBackingStoreWayland::checkRequirements()
>  {
>  #if USE(WPE_RENDERER)
>      if (!glImageTargetTexture2D) {
>          if (!wpe_fdo_initialize_for_egl_display(PlatformDisplay::sharedDisplay().eglDisplay()))
> -            return nullptr;
> +            return false;
>  
>          std::unique_ptr<WebCore::GLContext> eglContext = GLContext::createOffscreenContext();
>          if (!eglContext)
> -            return nullptr;
> +            return false;

Hmm, you sure this is a good idea? This does a lot of serious work under the guise of a checkRequirements function. I don't think a reasonable programmer calling AcceleratedBackingStore::checkRequirements would expect a function with this name to be setting up graphics resources like this. I think it's only a naming problem; you could call it AcceleratedBackingStore::setup or something like that, for example. (Although that too is an odd name for a static method....)

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #5)
> Comment on attachment 374372 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374372&action=review
> 
> > Source/WebKit/UIProcess/gtk/AcceleratedBackingStoreWayland.cpp:76
> > -std::unique_ptr<AcceleratedBackingStoreWayland> AcceleratedBackingStoreWayland::create(WebPageProxy& webPage)
> > +bool AcceleratedBackingStoreWayland::checkRequirements()
> >  {
> >  #if USE(WPE_RENDERER)
> >      if (!glImageTargetTexture2D) {
> >          if (!wpe_fdo_initialize_for_egl_display(PlatformDisplay::sharedDisplay().eglDisplay()))
> > -            return nullptr;
> > +            return false;
> >  
> >          std::unique_ptr<WebCore::GLContext> eglContext = GLContext::createOffscreenContext();
> >          if (!eglContext)
> > -            return nullptr;
> > +            return false;
> 
> Hmm, you sure this is a good idea? This does a lot of serious work under the
> guise of a checkRequirements function. I don't think a reasonable programmer
> calling AcceleratedBackingStore::checkRequirements would expect a function
> with this name to be setting up graphics resources like this. I think it's
> only a naming problem; you could call it AcceleratedBackingStore::setup or
> something like that, for example. (Although that too is an odd name for a
> static method....)

It's just caching global stuff.

[Carlos Garcia Campos]

Committed r247563: <https://trac.webkit.org/changeset/247563>

[Yuri Konotopov]

This change breaks Liferea compilation in headless (without X or Wayland) environment, because it calls webkit_settings_new() while compilling GIR XML.

As I see it's not possible to call webkit_settings_new in headless environment now because it fails in WebKit::AcceleratedBackingStore::checkRequirements ().
Is it desired behavior?

Backtrace follows.
#0  0x00007f520aba0ae0 in raise () from /lib64/libc.so.6
[Current thread is 1 (Thread 0x7f52060b3a00 (LWP 53958))]
(gdb) bt full
#0  0x00007f520aba0ae0 in raise () from /lib64/libc.so.6
No symbol table info available.
#1  0x00007f520ab8c2c4 in abort () from /lib64/libc.so.6
No symbol table info available.
#2  0x00007f520d7f2f77 in CRASH_WITH_INFO(...) ()
    at DerivedSources/ForwardingHeaders/wtf/Assertions.h:658
No locals.
#3  WebKit::AcceleratedBackingStore::checkRequirements ()
    at  webkit-gtk-2.26.2/work/webkitgtk-2.26.2/Source/WebKit/UIProcess/gtk/AcceleratedBackingStore.cpp:56
No locals.
#4  0x00007f520dc3b27b in WebKit::HardwareAccelerationManager::HardwareAccelerationManager (
    this=0x7f5210209530 <WebKit::HardwareAccelerationManager::singleton()::manager>)
    at  webkit-gtk-2.26.2/work/webkitgtk-2.26.2/Source/WebKit/UIProcess/gtk/HardwareAccelerationManager.cpp:55
        disableCompositing = <optimized out>
        forceCompositing = <optimized out>
        disableCompositing = <optimized out>
        forceCompositing = <optimized out>
#5  0x00007f520dc3b2fc in WTF::NeverDestroyed<WebKit::HardwareAccelerationManager>::NeverDestroyed<>() (this=0x7f5210209530 <WebKit::HardwareAccelerationManager::singleton()::manager>)
    at /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/new:174
No locals.
#6  WebKit::HardwareAccelerationManager::singleton ()
    at  webkit-gtk-2.26.2/work/webkitgtk-2.26.2/Source/WebKit/UIProcess/gtk/HardwareAccelerationManager.cpp:36
        manager = {m_storage = {__data = "\001", __align = {<No data fields>}}}
#7  0x00007f520dc441ad in WebKit::WebPreferences::platformInitializeStore (this=0x7f52046fe000)
    at  webkit-gtk-2.26.2/work/webkitgtk-2.26.2/Source/WebKit/UIProcess/gtk/WebPreferencesGtk.cpp:37
No locals.
#8  0x00007f520daf3e72 in WebKit::WebPreferences::create (identifier=..., keyPrefix=..., globalDebugKeyPrefix=...) at DerivedSources/ForwardingHeaders/wtf/ThreadSafeRefCounted.h:37
No locals.
#9  0x00007f520db96d04 in _WebKitSettingsPrivate::_WebKitSettingsPrivate (this=0x55acc9cd2b80) at DerivedSources/ForwardingHeaders/wtf/RefPtr.h:56
No locals.
#10 0x00007f520af2b129 in g_type_create_instance () from /usr/lib64/libgobject-2.0.so.0
No symbol table info available.
#11 0x00007f520af0d1ed in ?? () from /usr/lib64/libgobject-2.0.so.0
No symbol table info available.
#12 0x00007f520af0ed5d in g_object_new_with_properties () from /usr/lib64/libgobject-2.0.so.0
No symbol table info available.
#13 0x00007f520af0f779 in g_object_new () from /usr/lib64/libgobject-2.0.so.0
No symbol table info available.
#14 0x00007f520db914ba in webkit_settings_new () at  webkit-gtk-2.26.2/work/webkitgtk-2.26.2/Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1535
No locals.
#15 0x000055acc93b249f in liferea_webkit_impl_init (self=0x7f51f800dd00) at webkit.c:380
        disable_javascript = 21932
        enable_plugins = 48
        font = <optimized out>
        fontSize = <optimized out>
        security_manager = <optimized out>

[Michael Catanzaro]

I think you can just remove the RELEASE_ASSERT() from AcceleratedBackingStore::checkRequirements. There's no reason that should crash in headless mode. A crash is only warranted if AcceleratedBackingStore::create gets called.