Bug 145645

Summary: [GTK] [Wayland] The MiniBrowser crashes inside Weston.
Product: WebKit Reporter: Carlos Alberto Lopez Perez <clopez>
Component: WebKitGTKAssignee: Carlos Alberto Lopez Perez <clopez>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, itoral, mrobinson, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 81456    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Carlos Alberto Lopez Perez 2015-06-04 10:29:27 PDT
The MiniBrowser inside Weston crashes if you build current trunk of WebKitGTK+ with the Wayland target.

The issue seems to be that we are not properly initializing the Wayland display.

Patch is coming.
Comment 1 Carlos Alberto Lopez Perez 2015-06-04 11:00:11 PDT
Created attachment 254276 [details]
Patch
Comment 2 Carlos Alberto Lopez Perez 2015-06-04 11:07:39 PDT
BTW, if you run weston inside X, you have to unset the DISPLAY environment value before executing the MiniBrowser inside the weston terminal. Otherwise another crash happens (I still didn't checked that one, but I think is unrelated with the one on this bug)
Comment 3 Carlos Garcia Campos 2015-06-05 00:18:03 PDT
Comment on attachment 254276 [details]
Patch

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

Commenting only about minor stuff, I leave the actual review for Zan

> Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:60
> +    struct wl_display* wlDisplay = wl_display_connect(0);

0 -> nullptr

> Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:62
> +        g_warning("PlatformDisplayWayland initialization: failed to connect to the Wayland server socket. Check your WAYLAND_DISPLAY or WAYLAND_SOCKET environment variables.");

Use WTFLogAlways instead of g_warning

> Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:68
> +        g_warning("PlatformDisplayWayland initialization: failed to complete the initialization of the display.");

Ditto.
Comment 4 Carlos Alberto Lopez Perez 2015-06-05 05:34:51 PDT
Created attachment 254351 [details]
Patch
Comment 5 Carlos Alberto Lopez Perez 2015-06-05 06:24:54 PDT
Created attachment 254353 [details]
Patch

Only a trivial change to add a link on the changelog to the actual revision/line referenced.
Comment 6 Martin Robinson 2015-06-05 07:08:39 PDT
Comment on attachment 254353 [details]
Patch

Perhaps Zan can confirm, but I wonder if this is simply because the UIProcess-embedded Wayland subcompositer is not implemented yet. If so, we can always change the strings back when it's in place.
Comment 7 Zan Dobersek 2015-06-05 08:59:05 PDT
(In reply to comment #6)
> Comment on attachment 254353 [details]
> Patch
> 
> Perhaps Zan can confirm, but I wonder if this is simply because the
> UIProcess-embedded Wayland subcompositer is not implemented yet. If so, we
> can always change the strings back when it's in place.

That's pretty much the reason why a specific string is used in the wl_display_connect() call, and why that leads to a failure when invoked from the UIProcess.

I think there should be possible to connect the WebProcess and the nested compositor in the UIProcess some other way, so the socket name can be removed.
Comment 8 Zan Dobersek 2015-06-05 09:00:47 PDT
Comment on attachment 254353 [details]
Patch

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

> Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:50
> -    else if (!std::strcmp(interface, "wl_webkitgtk"))
> +    else if (!std::strcmp(interface, "wl_shell"))
>          display->m_webkitgtk = static_cast<struct wl_webkitgtk*>(wl_registry_bind(registry, name, &wl_webkitgtk_interface, 1));

This else-if branch should be removed completely at this point.
Comment 9 Zan Dobersek 2015-06-05 09:03:10 PDT
Comment on attachment 254353 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        (WebCore::PlatformDisplayWayland::globalCallback): We should check for the
> +        string "wl_shell" in order to get the notification that the shell surface is
> +        available because this is the name of this object in the Wayland protocol.

wl_webkitgtk and wl_shell interfaces are not interchangeable, so this is not correct.

wl_webkitgtk interface is specific to the nested compositor implementation, used by the WebProcess which behaves as a client to that compositor.
wl_shell is a more generic interface with greater capabilities, exposed by the parent compositor.
Comment 10 Carlos Alberto Lopez Perez 2015-06-05 09:38:10 PDT
(In reply to comment #8)
> Comment on attachment 254353 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254353&action=review
> 
> > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:50
> > -    else if (!std::strcmp(interface, "wl_webkitgtk"))
> > +    else if (!std::strcmp(interface, "wl_shell"))
> >          display->m_webkitgtk = static_cast<struct wl_webkitgtk*>(wl_registry_bind(registry, name, &wl_webkitgtk_interface, 1));
> 
> This else-if branch should be removed completely at this point.

If I remove that branch then I have to also to apply this patch:

--- a/Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h
+++ b/Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h
@@ -58,7 +58,7 @@ private:
     static void globalRemoveCallback(void* data, struct wl_registry*, uint32_t name);
 
     PlatformDisplayWayland(struct wl_display*);
-    bool isInitialized() { return m_compositor && m_webkitgtk && m_eglDisplay != EGL_NO_DISPLAY && m_eglConfigChosen; }
+    bool isInitialized() { return m_compositor && m_eglDisplay != EGL_NO_DISPLAY && m_eglConfigChosen; }
 
     Type type() const override { return PlatformDisplay::Type::Wayland; }


In order to isInitialized return true.


So, I can let that else-if branch without touching it because we not longer check m_webkitgtk in isInitialized
Comment 11 Carlos Alberto Lopez Perez 2015-06-05 09:55:56 PDT
Created attachment 254358 [details]
Patch

Updated to not touch the else-if branch, but instead removed the check for the nested subcompositor in isInitialized()
Comment 12 Zan Dobersek 2015-06-05 10:16:34 PDT
Comment on attachment 254358 [details]
Patch

OK, thanks.
Comment 13 Carlos Alberto Lopez Perez 2015-06-05 10:20:22 PDT
Comment on attachment 254358 [details]
Patch

Clearing flags on attachment: 254358

Committed r185252: <http://trac.webkit.org/changeset/185252>
Comment 14 Carlos Alberto Lopez Perez 2015-06-05 10:20:35 PDT
All reviewed patches have been landed.  Closing bug.