Bug 156972 - [GTK] Crashes if DISPLAY is unset
Summary: [GTK] Crashes if DISPLAY is unset
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-25 04:49 PDT by Alberto Garcia
Modified: 2016-04-25 13:47 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.26 KB, patch)
2016-04-25 04:53 PDT, Alberto Garcia
no flags Details | Formatted Diff | Diff
Patch (1.30 KB, patch)
2016-04-25 07:37 PDT, Alberto Garcia
cgarcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alberto Garcia 2016-04-25 04:49:42 PDT
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.
Comment 1 Alberto Garcia 2016-04-25 04:53:58 PDT
Created attachment 277239 [details]
Patch
Comment 2 Carlos Garcia Campos 2016-04-25 05:31:11 PDT
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.
Comment 3 Alberto Garcia 2016-04-25 07:37:46 PDT
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.
Comment 4 Carlos Garcia Campos 2016-04-25 08:08:49 PDT
(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?
Comment 5 Carlos Garcia Campos 2016-04-25 08:10:11 PDT
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.
Comment 6 Michael Catanzaro 2016-04-25 10:26:08 PDT
(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.
Comment 7 Alberto Garcia 2016-04-25 13:47:47 PDT
Committed r200046: <http://trac.webkit.org/changeset/200046>