[GTK] [Wayland] The MiniBrowser crashes inside Weston.

[Carlos Alberto Lopez Perez]

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.

[Carlos Alberto Lopez Perez]

Created attachment 254276 [details]
Patch

[Carlos Alberto Lopez Perez]

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)

[Carlos Garcia Campos]

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.

[Carlos Alberto Lopez Perez]

Created attachment 254351 [details]
Patch

[Carlos Alberto Lopez Perez]

Created attachment 254353 [details]
Patch

Only a trivial change to add a link on the changelog to the actual revision/line referenced.

[Martin Robinson]

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.

[Zan Dobersek]

(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.

[Zan Dobersek]

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.

[Zan Dobersek]

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.

[Carlos Alberto Lopez Perez]

(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

[Carlos Alberto Lopez Perez]

Created attachment 254358 [details]
Patch

Updated to not touch the else-if branch, but instead removed the check for the nested subcompositor in isInitialized()

[Zan Dobersek]

Comment on attachment 254358 [details]
Patch

OK, thanks.

[Carlos Alberto Lopez Perez]

Comment on attachment 254358 [details]
Patch

Clearing flags on attachment: 254358

Committed r185252: <http://trac.webkit.org/changeset/185252>

[Carlos Alberto Lopez Perez]

All reviewed patches have been landed.  Closing bug.