[Webkit-unassigned] [Bug 231670] [GLIB] Simplify SleepDisabler by checking if we are under sandbox

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=231670

--- Comment #4 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to Michael Catanzaro from comment #3)
> Comment on attachment 441063 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441063&action=review
> 
> > Source/WTF/ChangeLog:10
> > +        whether portal sandbox should be used. WEBKIT_USE_PORTAL environment variable can be used to force the use of
> > +        portals when not running under the sandbox.
> 
> Hmmmm. I do like the changes to SleepDisabler.cpp. Picking one API or the
> other to use does seem nicer. Thing is, many of the portals are good to use
> always, including when not sandboxed.

Why? If I'm not wrong, using the portal normally requires more DBus messages than using the original DBus service directly.

> So checking whether we're sandboxed
> first isn't *generally* recommended or necessary.

I took this approach from GTK4 for what is worth.

> I would normally actually
> try the portal API first, since that is the more modern API, and use the
> original API only as a fallback if the portal API is not available. But I'm
> pretty sure the inhibit portal is different and just does not work at all if
> the application is not sandboxed: it will fail if it can't read the
> non-falsifiable app ID from ./flatpak-info or wherever snap stores it. So
> while some portals are OK to use always, this one is not. Then there are
> some other portals that you *really* want to use always. E.g. for WebRTC
> stuff, we would always prefer to use the portal for desktop sharing or
> webcam access, because the portals are the preferred method to talk to
> Pipewire. I could imagine Pipewire might decide to claim v4l2 for itself in
> the future, to block other apps from using it and ensure it's always
> available for Pipewire. Another example: some apps that relied on
> gnome-shell's screen recorder D-Bus API instead of using the portal are just
> now discovering that they're blocked in GNOME 41. So we have:
> 
>  * Some portals that we always want to use, where using the original
> non-portal APIs is undesirable even when not sandboxed
>  * Some portals that are OK to use when not sandboxed, but also OK to ignore
> and use the original host APIs
>  * Some portals that must only be used when sandboxed and will fail when not
> sandboxed (pretty sure this is true for the Inhibit portal)
> 
> It seems like an xdg-desktop-portal design failure to have three different
> cases here, but whatever. The implication for this patch is that having
> WTF::shouldUsePortal probably doesn't make sense, since the right behavior
> depends on the particular portal in question. I thought about suggesting
> that you move it to SleepDisabler.cpp instead, and have that as a policy
> decision specific to the SleepDisabler code rather than assuming it will
> make sense for every use of any desktop portal anywhere in WebKit. But maybe
> it would be simpler to just rename it to WTF::mustUsePortal (or
> WTF::mustUseDesktopPortal) to indicate only the case where you really always
> have to use portals. Then code that talks to portals can decide for itself
> whether or not to use WTF::mustUsePortal/mustUseDesktopPortal, since this
> naming implies it is OK to use the portal anyway even if it returns false,
> whereas the name WTF::shouldUsePortal implies the code should use the portal
> only if it returns true.
> 
> > Source/WTF/wtf/glib/Sandbox.cpp:27
> > +#include <wtf/glib/Sandbox.h>
> 
> Alternative names:
> 
> wtf/glib/Containers.h?
> wtf/glib/IsInside.h?
> wtf/glib/MustUseDesktopPortal.h?

What's wrong with Sandbox.h?

> > Source/WTF/wtf/glib/Sandbox.cpp:35
> > +    return g_file_test("/.flatpak-info", G_FILE_TEST_EXISTS);
> 
> Hm, it sould test for snap too, since we should also generally use portals
> when running under snap. But not under docker, since portals do not expect
> to be used from docker, even though docker is also a sandbox.
> 
> I think I would move isInsideDocker, isInsideFlatpak, and isInsideSnap to
> here from ProcessLauncherGLib.cpp, then rename shouldUsePortal to
> mustUseDesktopPortal, and have it check isInsideFlatpak || isInsideSnap, but
> not isInsideDocker.
>
> > Source/WebCore/PAL/ChangeLog:9
> > +        one. It's simpler to check if we are under sandbox and it avoids a connection that we know it's always going to
> 
> is always going

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211014/63b3a99e/attachment-0001.htm>