Bug 209106 – [GTK] Crash in WebKit::LayerTreeHost::LayerTreeHost with bubblewrap sandbox enabled

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Дилян Палаузов]

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 .

[Michael Catanzaro]

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

[Michael Catanzaro]

Problem is simply that nobody has ever tested -DUSE_WPE_RENDERER=OFF since bubblewrap sandbox was enabled.

[Michael Catanzaro]

Created attachment 394377 [details]
Patch

[Michael Catanzaro]

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

[Michael Catanzaro]

Created attachment 394379 [details]
Patch

[EWS]

Committed r258923: <https://trac.webkit.org/changeset/258923>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394379 [details].

[Carlos Garcia Campos]

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

[Michael Catanzaro]

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.

[Michael Catanzaro]

Note the whole function is already guarded by #if PLATFORM(WAYLAND) && USE(EGL).

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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

[Michael Catanzaro]

Created attachment 394497 [details]
Patch

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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.

[Michael Catanzaro]

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

[Michael Catanzaro]

Created attachment 394604 [details]
Patch

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

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)

[Carlos Garcia Campos]

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

[Michael Catanzaro]

Created attachment 394607 [details]
Patch for landing

[Michael Catanzaro]

PLATFORM(WAYLAND) and USE(EGL) are not redundant, though? Is it really possible to use Wayland without EGL?

[EWS]

Committed r259044: <https://trac.webkit.org/changeset/259044>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394607 [details].

[Michael Catanzaro]

*** Bug 209377 has been marked as a duplicate of this bug. ***