Bug 142879

This specific error should be fixed by bug #136831. Other relevant bugs are bugs #142875, #142876 and #142877. #142877 disables accelerated compositing for now, but there's still one build failure due to improper GLContext creation in LayerTreeHostGtk.

Created attachment 254169 [details] Patch

Comment on attachment 254169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254169&action=review > Source/WebCore/ChangeLog:11 > + * PlatformGTK.cmake: When the Wayland target is enabled, its > + sources should be included in WebCore list. This is not what you are doing, you are moving the wayland sources from WebCorePlatformGTK to WebCore_SOURCES, which is probably correct because the wayland sources don't use GTK+, but why does it fail to build when wayland sources are in WebCorePlatformGTK? > Source/WebCore/ChangeLog:19 > + (WebCore::PlatformDisplayWayland::create): Use the default value > + for the Wayland socket. Why? > Source/WebCore/ChangeLog:24 > + * platform/graphics/wayland/PlatformDisplayWayland.h: Remove dead code. Is dead code causing build failure? > Source/WebCore/platform/graphics/PlatformDisplay.cpp:128 > + if (m_eglDisplayInitialized) > + return; This si not needed, this is only called from PlatformDisplay::eglDisplay() that already does that check. > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:58 > - struct wl_display* wlDisplay = wl_display_connect("webkitgtk-wayland-compositor-socket"); > + struct wl_display* wlDisplay = wl_display_connect(0); Why was this wrong? > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h:59 > - Type type() const override { return PlatformDisplay::Type::Wayland; } > + virtual Type type() const override { return PlatformDisplay::Type::Wayland; } I don't think this is needed, does this cause a build failure? > Source/cmake/OptionsGTK.cmake:-85 > -# FIXME: Should be possible to build with support for both X11 and Wayland. > -WEBKIT_OPTION_CONFLICT(ENABLE_WAYLAND_TARGET ENABLE_X11_TARGET) > - I seems to me that this goes beyond the build fix > ChangeLog:13 > + * Source/cmake/OptionsGTK.cmake: Remove cmake conflict for building > + at the same time the X11 and Wayland targets that r183491 added. > + Building both was previously supported and continues to be. > + When running it, the Wayland target will be used if we are running > + inside a Wayland compositor (Weston for example) and our environment > + don't has set the DISPLAY variable. I think this should be a separate bug, since this is about fixing the build.

(In reply to comment #3) > Comment on attachment 254169 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254169&action=review > > > Source/WebCore/ChangeLog:11 > > + * PlatformGTK.cmake: When the Wayland target is enabled, its > > + sources should be included in WebCore list. > > This is not what you are doing, you are moving the wayland sources from > WebCorePlatformGTK to WebCore_SOURCES, which is probably correct because the > wayland sources don't use GTK+, but why does it fail to build when wayland > sources are in WebCorePlatformGTK? > It gave me build errors at some point, but it seems not longer fails if I revert that change. Do you think I should revert it? > > Source/WebCore/ChangeLog:19 > > + (WebCore::PlatformDisplayWayland::create): Use the default value > > + for the Wayland socket. > > Why? > > > > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:58 > > - struct wl_display* wlDisplay = wl_display_connect("webkitgtk-wayland-compositor-socket"); > > + struct wl_display* wlDisplay = wl_display_connect(0); > > Why was this wrong? > Without that, the MiniBrowser will crash on my Weston test environment. This is the backtrace: Program terminated with signal SIGSEGV, Segmentation fault. #0 0x00007f7a8099fb3e in webkitWebViewBaseCreateWebPage(_WebKitWebViewBase*, WebKit::WebProcessPool*, WebKit::WebPreferences*, WebKit::WebPageGroup*, WebKit::WebUserContentControllerProxy*, WebKit::WebPageProxy*) () from /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 [...] Thread 1 (Thread 0x7f7a8237c9c0 (LWP 3916)): #0 0x00007f7a8099fb3e in webkitWebViewBaseCreateWebPage(_WebKitWebViewBase*, WebKit::WebProcessPool*, WebKit::WebPreferences*, WebKit::WebPageGroup*, WebKit::WebUserContentControllerProxy*, WebKit::WebPageProxy*) () from /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #1 0x00007f7a80990772 in webkitWebContextCreatePageForWebView(_WebKitWebContext*, _WebKitWebView*, _WebKitUserContentManager*, _WebKitWebView*) () from /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #2 0x00007f7a8099d105 in webkitWebViewConstructed(_GObject*) () from /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #3 0x00007f7a7d9a29d4 in g_object_new_internal (class=0x7f7a821e1218 <WebCore::PlatformDisplay::sharedDisplay()::onceFlag>, class@entry=0xddcc00, params=0x81, params@entry=0x7fff1103ca60, n_params=1) at /tmp/buildd/glib2.0-2.42.1/./gobject/gobject.c:1814 #4 0x00007f7a7d9a4675 in g_object_new_valist (object_type=object_type@entry=14843504, first_property_name=first_property_name@entry=0x7f7a819c62c6 <.L.str5> "web-context", var_args=var_args@entry=0x7fff1103cbb0) at /tmp/buildd/glib2.0-2.42.1/./gobject/gobject.c:2034 #5 0x00007f7a7d9a49b1 in g_object_new (object_type=14843504, first_property_name=0x7f7a819c62c6 <.L.str5> "web-context") at /tmp/buildd/glib2.0-2.42.1/./gobject/gobject.c:1617 #6 0x00007f7a8099775a in webkit_web_view_new_with_context () from /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #7 0x0000000000410754 in createBrowserWindow () #8 0x0000000000410608 in main () Not sure why this change is needed or not, but without it the Minibrowser crashes for me. > > Source/WebCore/ChangeLog:24 > > + * platform/graphics/wayland/PlatformDisplayWayland.h: Remove dead code. > > Is dead code causing build failure? > Yes, because it gets compiled even if is not used. [9/51] Linking CXX shared library lib/libwebkit2gtk-4.0.so.37.8.0 FAILED: : && /usr/lib/ccache/clang++ -fPIC -std=c++11 -fcolor-diagnostics -Qunused-arguments -O3 -DNDEBUG -fno-exceptions -fno-strict-aliasing -fno-rtti -Wl,--no-undefined -fuse-ld=gold -Wl,--disable-new-dtags -fuse-ld=gold -Wl,--disable-new-dtags -shared -Wl,-soname,libwebkit2gtk-4.0.so.37 -o lib/libwebkit2gtk-4.0.so.37.8.0 @CMakeFiles/WebKit2.rsp && : lib/libWebCorePlatformGTK.a(lib/../Source/WebCore/CMakeFiles/WebCorePlatformGTK.dir/platform/graphics/wayland/PlatformDisplayWayland.cpp.o):/home/clopez/webkit/.ccache/tmp/PlatformDi.tmp.trinity.32422.ii:function WebCore::PlatformDisplayWayland::createSharingGLContext(): error: undefined reference to 'WebCore::GLContextEGL::createWindowContext(wl_egl_window*, WebCore::GLContext*)' clang: error: linker command failed with exit code 1 (use -v to see invocation) > > Source/WebCore/platform/graphics/PlatformDisplay.cpp:128 > > + if (m_eglDisplayInitialized) > > + return; > > This si not needed, this is only called from PlatformDisplay::eglDisplay() > that already does that check. I was suspecting something like that, but I wasn't sure and maybe somebody calls this function from another site on the future so I thought that adding that check was worth. Do you think I should remove it? > > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h:59 > > - Type type() const override { return PlatformDisplay::Type::Wayland; } > > + virtual Type type() const override { return PlatformDisplay::Type::Wayland; } > > I don't think this is needed, does this cause a build failure? > No it don't causes it, but the other methods are set as Virtual. Check: $ grep -r ' Type type() const override' Source/WebCore/platform/graphics Do you think is not right the change? > > Source/cmake/OptionsGTK.cmake:-85 > > -# FIXME: Should be possible to build with support for both X11 and Wayland. > > -WEBKIT_OPTION_CONFLICT(ENABLE_WAYLAND_TARGET ENABLE_X11_TARGET) > > - > > I seems to me that this goes beyond the build fix > > > ChangeLog:13 > > + * Source/cmake/OptionsGTK.cmake: Remove cmake conflict for building > > + at the same time the X11 and Wayland targets that r183491 added. > > + Building both was previously supported and continues to be. > > + When running it, the Wayland target will be used if we are running > > + inside a Wayland compositor (Weston for example) and our environment > > + don't has set the DISPLAY variable. > > I think this should be a separate bug, since this is about fixing the build. I'm fixing the build both for ENABLE_WAYLAND_TARGET alone and for ENABLE_X11_TARGET and ENABLE_WAYLAND_TARGET at the same time. I can only fix for ENABLE_WAYLAND_TARGET alone and after that open another bug to fix it for both at the same time if you think is worth. Do you think is worth to do this in a new bug?

(In reply to comment #4) > (In reply to comment #3) > > Comment on attachment 254169 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=254169&action=review > > > > > Source/WebCore/ChangeLog:11 > > > + * PlatformGTK.cmake: When the Wayland target is enabled, its > > > + sources should be included in WebCore list. > > > > This is not what you are doing, you are moving the wayland sources from > > WebCorePlatformGTK to WebCore_SOURCES, which is probably correct because the > > wayland sources don't use GTK+, but why does it fail to build when wayland > > sources are in WebCorePlatformGTK? > > > > It gave me build errors at some point, but it seems not longer fails if > I revert that change. > Do you think I should revert it? > If those sources don't use GTK+ at all, they should be moved to WebCore_SOURCES, but in a different bug/patch. If you don't want to open a new bug land it with rs=me. > > > > > > Source/WebCore/ChangeLog:19 > > > + (WebCore::PlatformDisplayWayland::create): Use the default value > > > + for the Wayland socket. > > > > Why? > > > > > > > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.cpp:58 > > > - struct wl_display* wlDisplay = wl_display_connect("webkitgtk-wayland-compositor-socket"); > > > + struct wl_display* wlDisplay = wl_display_connect(0); > > > > Why was this wrong? > > > Without that, the MiniBrowser will crash on my Weston test environment. > This is the backtrace: > > Program terminated with signal SIGSEGV, Segmentation fault. > #0 0x00007f7a8099fb3e in > webkitWebViewBaseCreateWebPage(_WebKitWebViewBase*, WebKit::WebProcessPool*, > WebKit::WebPreferences*, WebKit::WebPageGroup*, > WebKit::WebUserContentControllerProxy*, WebKit::WebPageProxy*) () from > /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > [...] > Thread 1 (Thread 0x7f7a8237c9c0 (LWP 3916)): > #0 0x00007f7a8099fb3e in > webkitWebViewBaseCreateWebPage(_WebKitWebViewBase*, WebKit::WebProcessPool*, > WebKit::WebPreferences*, WebKit::WebPageGroup*, > WebKit::WebUserContentControllerProxy*, WebKit::WebPageProxy*) () from > /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #1 0x00007f7a80990772 in > webkitWebContextCreatePageForWebView(_WebKitWebContext*, _WebKitWebView*, > _WebKitUserContentManager*, _WebKitWebView*) () from > /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #2 0x00007f7a8099d105 in webkitWebViewConstructed(_GObject*) () from > /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #3 0x00007f7a7d9a29d4 in g_object_new_internal (class=0x7f7a821e1218 > <WebCore::PlatformDisplay::sharedDisplay()::onceFlag>, class@entry=0xddcc00, > params=0x81, params@entry=0x7fff1103ca60, n_params=1) at > /tmp/buildd/glib2.0-2.42.1/./gobject/gobject.c:1814 > #4 0x00007f7a7d9a4675 in g_object_new_valist > (object_type=object_type@entry=14843504, > first_property_name=first_property_name@entry=0x7f7a819c62c6 <.L.str5> > "web-context", var_args=var_args@entry=0x7fff1103cbb0) at > /tmp/buildd/glib2.0-2.42.1/./gobject/gobject.c:2034 > #5 0x00007f7a7d9a49b1 in g_object_new (object_type=14843504, > first_property_name=0x7f7a819c62c6 <.L.str5> "web-context") at > /tmp/buildd/glib2.0-2.42.1/./gobject/gobject.c:1617 > #6 0x00007f7a8099775a in webkit_web_view_new_with_context () from > /home/clopez/webkit/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 > #7 0x0000000000410754 in createBrowserWindow () > #8 0x0000000000410608 in main () > > > Not sure why this change is needed or not, but without it the Minibrowser > crashes for me. > We need to explain in the ChangeLog why we do the changes, this is a bug about fixing the build. This should be moved to a new bug about crashes when using wayland, so that wayland experts can discuss this crash, and the best wat to solve it. > > > > > > Source/WebCore/ChangeLog:24 > > > + * platform/graphics/wayland/PlatformDisplayWayland.h: Remove dead code. > > > > Is dead code causing build failure? > > > > Yes, because it gets compiled even if is not used. > > [9/51] Linking CXX shared library lib/libwebkit2gtk-4.0.so.37.8.0 > FAILED: : && /usr/lib/ccache/clang++ -fPIC -std=c++11 -fcolor-diagnostics > -Qunused-arguments -O3 -DNDEBUG -fno-exceptions -fno-strict-aliasing > -fno-rtti -Wl,--no-undefined -fuse-ld=gold -Wl,--disable-new-dtags > -fuse-ld=gold -Wl,--disable-new-dtags -shared > -Wl,-soname,libwebkit2gtk-4.0.so.37 -o lib/libwebkit2gtk-4.0.so.37.8.0 > @CMakeFiles/WebKit2.rsp && : > lib/libWebCorePlatformGTK.a(lib/../Source/WebCore/CMakeFiles/ > WebCorePlatformGTK.dir/platform/graphics/wayland/PlatformDisplayWayland.cpp. > o):/home/clopez/webkit/.ccache/tmp/PlatformDi.tmp.trinity.32422.ii:function > WebCore::PlatformDisplayWayland::createSharingGLContext(): error: undefined > reference to 'WebCore::GLContextEGL::createWindowContext(wl_egl_window*, > WebCore::GLContext*)' > clang: error: linker command failed with exit code 1 (use -v to see > invocation) > > > Ok, let's remove it for now, if it's eventually needed, it will be added again and fixed to build. > > > > > > > Source/WebCore/platform/graphics/PlatformDisplay.cpp:128 > > > + if (m_eglDisplayInitialized) > > > + return; > > > > This si not needed, this is only called from PlatformDisplay::eglDisplay() > > that already does that check. > > I was suspecting something like that, but I wasn't sure and maybe somebody > calls this function from another site on the future so I thought that adding > that check was worth. That method is protected so only derived classes can call it. The public users should use eglDisplay(), that's why the check is there. > Do you think I should remove it? > Yes, please, also it doesn't fix the build as the bug title claims. > > > > > > > Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h:59 > > > - Type type() const override { return PlatformDisplay::Type::Wayland; } > > > + virtual Type type() const override { return PlatformDisplay::Type::Wayland; } > > > > I don't think this is needed, does this cause a build failure? > > > No it don't causes it, but the other methods are set as Virtual. Check: > > $ grep -r ' Type type() const override' Source/WebCore/platform/graphics > > Do you think is not right the change? Again, even if the change is correct, it's unrelated to this bug. > > > > > Source/cmake/OptionsGTK.cmake:-85 > > > -# FIXME: Should be possible to build with support for both X11 and Wayland. > > > -WEBKIT_OPTION_CONFLICT(ENABLE_WAYLAND_TARGET ENABLE_X11_TARGET) > > > - > > > > I seems to me that this goes beyond the build fix > > > > > ChangeLog:13 > > > + * Source/cmake/OptionsGTK.cmake: Remove cmake conflict for building > > > + at the same time the X11 and Wayland targets that r183491 added. > > > + Building both was previously supported and continues to be. > > > + When running it, the Wayland target will be used if we are running > > > + inside a Wayland compositor (Weston for example) and our environment > > > + don't has set the DISPLAY variable. > > > > I think this should be a separate bug, since this is about fixing the build. > > I'm fixing the build both for ENABLE_WAYLAND_TARGET alone and for > ENABLE_X11_TARGET and ENABLE_WAYLAND_TARGET at the same time. > > I can only fix for ENABLE_WAYLAND_TARGET alone and after that open another > bug to fix it for both at the same time if you think is worth. > > Do you think is worth to do this in a new bug? Indeed.

Created attachment 254185 [details] Patch Minimal patch for fixing the build with Wayland target enabled. Compared to the previous patch, this one dont removes the previous dead code that was failing before to build. Instead it makes it build by defining GLNativeWindowType to the one that Wayland expects (change picked from patch on bug 136831). Not sure if this approach is better. Let me know

Attachment 254185 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/GLContext.h:31: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #6) > Created attachment 254185 [details] > Patch > > Minimal patch for fixing the build with Wayland target enabled. Compared to > the previous patch, this one dont removes the previous dead code that was > failing before to build. Instead it makes it build by defining > GLNativeWindowType to the one that Wayland expects (change picked from patch > on bug 136831). Not sure if this approach is better. Let me know Yes, this approach is better and makes my life easier when backporting patches to stable branches, for example. If I see a commit that claims to fix the build but ends up doing more things, I would need to check the contents and decide what to merge individually. If the commit actually only fixes the build I can just cherry-pick it. But please, use new bugs for fixing the other issues.

Comment on attachment 254185 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254185&action=review > Source/WebCore/platform/graphics/PlatformDisplay.cpp:48 > +#if PLATFORM(WAYLAND) && ! defined(GTK_API_VERSION_2) Extra space after ! > Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:87 > - m_context = GLContext::createContextForWindow(m_layerTreeContext.contextID, GLContext::sharingContext()); > + m_context = GLContext::createContextForWindow((GLNativeWindowType) m_layerTreeContext.contextID, GLContext::sharingContext()); Use a C++ cast here, please.

Committed r185198: <http://trac.webkit.org/changeset/185198>