Bug 156972

Summary: [GTK] Crashes if DISPLAY is unset
Product: WebKit Reporter: Alberto Garcia <berto>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, cgarcia, kapouer, mcatanzaro
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: http://bugs.debian.org/803104
https://bugzilla.gnome.org/show_bug.cgi?id=757061
Attachments:
Description Flags
Patch
none
Patch cgarcia: review+

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>