Created attachment 393583 [details] Backtrace As of https://gitlab.gnome.org/GNOME/epiphany/issues/1128 WebKit 2.28, embedded within Epiphany (Gnome Web) crashes when opening certain pages, like nodejs.org, but not for other pages like developer.mozilla.com. The MiniBrowser works fine. 2.26 worked without problem. Note, that 2.26 I have compiled without BubbleWrap, and 2.28 is with bubblewrap, otherwise the build options shall be identical. The backtrace on crash is attached.
I have compiled 2.26 without GLES2. For compiling 2.28 I enabled GLES2, as WebKitGTK has not complied otherwise, per https://bugs.webkit.org/show_bug.cgi?id=208907 .
(In reply to Дилян Палаузов from comment #0) > Note, that 2.26 I > have compiled without BubbleWrap, and 2.28 is with bubblewrap, otherwise the > build options shall be identical. First step would be to disable that and confirm bubblewrap sandbox is to blame (very likely). Then we can try to figure out what we need to whitelist for the sandbox to make the crash go away. And, finally, we need to crash in a way better manner rather than inside WebKit::LayerTreeHost::LayerTreeHost when something is wrong with accelerated compositing mode.
I disabled ENABLE_BUBBLEWRAP_SANDBOX and Epiphany stopped crashing. For that bubble I have not installed any configuration files, just compiled the source code and installed it.
Problem is simply that nobody has ever tested -DUSE_WPE_RENDERER=OFF since bubblewrap sandbox was enabled.
Created attachment 394377 [details] Patch
Comment on attachment 394377 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394377&action=review > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:23 > +#include "WaylandCompositor.h" OK, it's not in the include path for WPE....
Created attachment 394379 [details] Patch
Committed r258923: <https://trac.webkit.org/changeset/258923> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394379 [details].
Comment on attachment 394379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394379&action=review > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:342 > +#if PLATFORM(GTK) && !USE(WPE_RENDERER) > + String displayName = WaylandCompositor::singleton().displayName(); > + waylandRuntimeFile.reset(g_build_filename(runtimeDir, displayName.utf8().data(), nullptr)); > + bindIfExists(args, waylandRuntimeFile.get(), BindFlags::ReadWrite); > +#endif This should be done only under wayland, you should check: #if PLATFORM(WAYLAND) && !USE(WPE_RENDERER) if (WebCore::PlatformDisplay::sharedDisplay().type() == WebCore::PlatformDisplay::Type::Wayland && WaylandCompositor::singleton().isRunning()) { waylandRuntimeFile.reset(g_build_filename(runtimeDir, WaylandCompositor::singleton().displayName().utf8().data(), nullptr)); bindIfExists(args, waylandRuntimeFile.get(), BindFlags::ReadWrite); } #endif
Right. It's safe to bind extra files, but probably using WaylandCompositor when we shouldn't is not good. Note: WaylandCompositor::singleton() will create and start running the WaylandCompositor if it doesn't already exist.
Note the whole function is already guarded by #if PLATFORM(WAYLAND) && USE(EGL).
(In reply to Michael Catanzaro from comment #10) > Right. > > It's safe to bind extra files, but probably using WaylandCompositor when we > shouldn't is not good. > > Note: WaylandCompositor::singleton() will create and start running the > WaylandCompositor if it doesn't already exist. But it can fail to initialize, that's why there's isRunning()
(In reply to Michael Catanzaro from comment #11) > Note the whole function is already guarded by #if PLATFORM(WAYLAND) && > USE(EGL). Then GTK ifdef is not needed.
In the top of the file: 33 #if PLATFORM(GTK) 34 #include "WaylandCompositor.h" 35 #endif 36 37 #if PLATFORM(GTK) 38 #define BASE_DIRECTORY "webkitgtk" 39 #elif PLATFORM(WPE) the include from line 34 can be moved on line 38 and lines 33, 35 and 36 can be deleted.
Created attachment 394497 [details] Patch
(In reply to Carlos Garcia Campos from comment #13) > Then GTK ifdef is not needed. WPE doesn't use EGL...? (In reply to Дилян Палаузов from comment #14) > the include from line 34 can be moved on line 38 and lines 33, 35 and 36 can > be deleted. Let's leave the second chain for BASE_DIRECTORY.
(In reply to Michael Catanzaro from comment #16) > WPE doesn't use EGL...? If so, then the sandbox is probably busted on WPE? Or does WPE only use the Wayland socket from the UI process? (Without this code, the web process has no access to the Wayland socket.)
(In reply to Michael Catanzaro from comment #16) > (In reply to Carlos Garcia Campos from comment #13) > > Then GTK ifdef is not needed. > > WPE doesn't use EGL...? Yes, but PLATFORM(WAYLAND) is only defined by GTK port. > (In reply to Дилян Палаузов from comment #14) > > the include from line 34 can be moved on line 38 and lines 33, 35 and 36 can > > be deleted. > > Let's leave the second chain for BASE_DIRECTORY.
Comment on attachment 394497 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394497&action=review > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:339 > + if (PlatformDisplay::sharedDisplay().type() == PlatformDisplay::Type::Wayland && WaylandCompositor::singleton().isRunning()) { Maybe we should move this check at the beginning of bindWayland and simply return early. We are currently doing the wayland display bind in X11 too.
(In reply to Carlos Garcia Campos from comment #19) > Maybe we should move this check at the beginning of bindWayland and simply > return early. We are currently doing the wayland display bind in X11 too. It's fine either way. There's no disadvantage to binding a file that doesn't exist or won't be used. I figure it's more important to avoid instantiating the WaylandCompositor when it's not going to be used. But yeah, returning early is fine too, so I'll update the patch as requested.
Created attachment 394604 [details] Patch
I'm not sure I understand what WPE needs here. This patch preserves the current behavior, which is that WPE's web process has no access to the Wayland socket. It moves the PLATFORM(GTK) guard outside the function, for clarity. This is OK if WPE only uses Wayland from the UI process. If WPE needs the Wayland socket from the web process, and doesn't define PLATFORM(WAYLAND), then it was already busted and must not have ever been tested with the sandbox enabled.
Comment on attachment 394604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394604&action=review > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:327 > +#if PLATFORM(GTK) && PLATFORM(WAYLAND) && USE(EGL) Again, PLATFORM(GTK) && PLATFORM(WAYLAND) is redundant, because only GTK port uses PLATFORM(WAYLAND)
(In reply to Michael Catanzaro from comment #22) > I'm not sure I understand what WPE needs here. This patch preserves the > current behavior, which is that WPE's web process has no access to the > Wayland socket. It moves the PLATFORM(GTK) guard outside the function, for > clarity. This is OK if WPE only uses Wayland from the UI process. If WPE > needs the Wayland socket from the web process, and doesn't define > PLATFORM(WAYLAND), then it was already busted and must not have ever been > tested with the sandbox enabled. WPE doesn't connect to the wayland display from the web process, only to the nested compositor and when fdo backend is used.
Created attachment 394607 [details] Patch for landing
PLATFORM(WAYLAND) and USE(EGL) are not redundant, though? Is it really possible to use Wayland without EGL?
Committed r259044: <https://trac.webkit.org/changeset/259044> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394607 [details].
*** Bug 209377 has been marked as a duplicate of this bug. ***