Bug 156972 – [GTK] Crashes if DISPLAY is unset

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Alberto Garcia]

This happens when the gtk-doc scanner is linked against webkit2gtk:

gtkdoc-scangobj  $scanobj_options --module=libyelp

(process:14998): Gtk-CRITICAL **: gtk_icon_theme_get_for_screen: assertion 'GDK_IS_SCREEN (screen)' failed

** (process:14998): WARNING **: Unable to connect to dbus: Cannot autolaunch D-Bus without X11 $DISPLAY
Segmentation fault

The segfault comes from webkitgtk:

(gdb) bt
#0  XCloseDisplay (dpy=0x0) at ../../src/ClDisplay.c:51
#1  0x00007ffff4eab831 in WebCore::PlatformDisplayX11::~PlatformDisplayX11 (this=0x7fffe2dfc028, __in_chrg=<optimized out>)
    at /build/webkit2gtk-ea7lQt/webkit2gtk-2.12.1/Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:53

The problem seems to be that m_display is initialized in the constructor of PlatformDisplayX11, but it's assumed not to be NULL.

[Alberto Garcia]

Created attachment 277239 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 277239 [details]
Patch

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

> Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:52
> -    if (m_ownedDisplay)
> +    if (m_ownedDisplay && m_display)

what about initializing m_ownedDisplay to false if display is NULL? we can't own a display that doesn't exist.

[Alberto Garcia]

Created attachment 277247 [details]
Patch

Ok, I chose to initialize m_ownedDisplay in the body of the constructor to make sure that m_display is set first. Otherwise we'd have to rely on the order in which m_display and m_ownedDisplay are declared in the class definition.

[Carlos Garcia Campos]

(In reply to comment #3)
> Created attachment 277247 [details]
> Patch
> 
> Ok, I chose to initialize m_ownedDisplay in the body of the constructor to
> make sure that m_display is set first. Otherwise we'd have to rely on the
> order in which m_display and m_ownedDisplay are declared in the class
> definition.

Isn't the order ensured by the compiler? why do we have a warning when the order in the list doesn't match the one in the declaration, then?

[Carlos Garcia Campos]

Comment on attachment 277247 [details]
Patch

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

> Source/WebCore/platform/graphics/x11/PlatformDisplayX11.cpp:41
> +    m_ownedDisplay = m_display != nullptr;

We don't explicitly check nullptr, m_ownedDisplay = m_display; should just work, I think.

[Michael Catanzaro]

(In reply to comment #4) 
> Isn't the order ensured by the compiler? why do we have a warning when the
> order in the list doesn't match the one in the declaration, then?

Member variables are always initialized in the order they are declared in the class, NOT the order they are listed in the initializer list. Putting them out of order in the initializer list is thus very confusing, and worth warning about. We should never do that in WebKit.

Since relying on the order of declarations is risky -- someone could easily swap those in a "cleanup" and not notice the warning -- I agree with Berto that it's a good idea to do the assignment in the body of the constructor.

[Alberto Garcia]

Committed r200046: <http://trac.webkit.org/changeset/200046>