[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

Michael Catanzaro <mcatanzaro at gnome.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mcatanzaro at gnome.org

--- Comment #3 from Michael Catanzaro <mcatanzaro at gnome.org> ---
Comment on attachment 441063
  --> https://bugs.webkit.org/attachment.cgi?id=441063
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. So checking whether we're sandboxed first isn't *generally* recommended or necessary. 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?

> 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/20211013/48ff958d/attachment.htm>