Bug 204732 – [GTK][WPE] Re-add flatpak sandbox support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Patrick Griffis]

[GTK][WPE] Re-add flatpak sandbox support

[Patrick Griffis]

Created attachment 384581 [details]
Patch

[Patrick Griffis]

Before being committed there are some remaining upstream flatpak issues that are being worked on: https://github.com/flatpak/flatpak-xdg-utils/pull/24

[Patrick Griffis]

Created attachment 388009 [details]
Patch

[Michael Catanzaro]

Comment on attachment 388009 [details]
Patch

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

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:54
> +static bool isFlatpakSpawnUsable(const ProcessLauncher::LaunchOptions& launchOptions)

Don't we need to check if running under flatpak before trying to use flatpak-spawn?

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:66
> +            ret = false;
> +            return *ret;

Please print a g_warning() in this case. We shouldn't silently disable the sandbox.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:203
>  
> +

Should be only one line of whitespace.

[Patrick Griffis]

(In reply to Michael Catanzaro from comment #4)
> Comment on attachment 388009 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388009&action=review
> 
> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:54
> > +static bool isFlatpakSpawnUsable(const ProcessLauncher::LaunchOptions& launchOptions)
> 
> Don't we need to check if running under flatpak before trying to use
> flatpak-spawn?

It will just safely fail on the host.

[Patrick Griffis]

Created attachment 389961 [details]
Patch

[Patrick Griffis]

Created attachment 389964 [details]
Patch

[Michael Catanzaro]

(In reply to Patrick Griffis from comment #5)
> It will just safely fail on the host.

There is the new warning message though, which we don't want printing when not running in flatpak. Makes sense to add another check, yes?

[Michael Catanzaro]

Maybe we don't need the check for /tmp after all. We don't support any stable flatpaks of Epiphany, only Epiphany Technology Preview. So there's nothing to keep working. Right?

[Patrick Griffis]

Created attachment 389971 [details]
Patch

[Patrick Griffis]

(In reply to Michael Catanzaro from comment #9)
> Maybe we don't need the check for /tmp after all. We don't support any
> stable flatpaks of Epiphany, only Epiphany Technology Preview. So there's
> nothing to keep working. Right?

Yeah that is fair. Removed.

[WebKit Commit Bot]

Comment on attachment 389971 [details]
Patch

Clearing flags on attachment: 389971

Committed r255969: <https://trac.webkit.org/changeset/255969>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.