Bug 231670

Created attachment 441062 [details] Patch

Created attachment 441063 [details] Patch

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

(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

(In reply to Carlos Garcia Campos from comment #4) > Why? If I'm not wrong, using the portal normally requires more DBus messages > than using the original DBus service directly. It doesn't matter. I normally prefer to use newer things and fall back to older things only if the newer thing is not present. If the newer API works in all cases, that keeps the code simpler. But in this case, the newer API does not work in all cases, so you really need to choose, which is why your changes to SleepDisablerGLib.cpp look good. > > So checking whether we're sandboxed > > first isn't *generally* recommended or necessary. > > I took this approach from GTK4 for what is worth. Right, but GTK does not do screen sharing or camera access, so our needs are different. > What's wrong with Sandbox.h? The naming is OK as your patch is now, but it won't make as much sense aftre my suggested changes.

(In reply to Michael Catanzaro from comment #5) > (In reply to Carlos Garcia Campos from comment #4) > > Why? If I'm not wrong, using the portal normally requires more DBus messages > > than using the original DBus service directly. > > It doesn't matter. I normally prefer to use newer things and fall back to > older things only if the newer thing is not present. If the newer API works > in all cases, that keeps the code simpler. But in this case, the newer API > does not work in all cases, so you really need to choose, which is why your > changes to SleepDisablerGLib.cpp look good. I don't think it's a matter of new vs old APIs, AFAIK portals where created for sandboxed applications, no? > > > So checking whether we're sandboxed > > > first isn't *generally* recommended or necessary. > > > > I took this approach from GTK4 for what is worth. > > Right, but GTK does not do screen sharing or camera access, so our needs are > different. If we prefer to use portals for screen sharing and camera access we can simply avoid the shouldUsePortal check in those particular cases, since we already know we should use portals. > > What's wrong with Sandbox.h? > > The naming is OK as your patch is now, but it won't make as much sense aftre > my suggested changes. Why? Your suggestions are still related to sandboxing, no?

(In reply to Carlos Garcia Campos from comment #6) > If we prefer to use portals for screen sharing and camera access we can > simply avoid the shouldUsePortal check in those particular cases, since we > already know we should use portals. OK, fine. Note: something is wrong with the API tests EWS. (In reply to Carlos Garcia Campos from comment #6) > Why? Your suggestions are still related to sandboxing, no? I suppose. Testing for containerization frameworks vs. testing for actual sandboxing is a little different, though.

Comment on attachment 441063 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441063&action=review >>> 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. > > Anyway, you've convinced me on everything else, but still need to fix isRunningInSandbox. At minimum, test for snap as well. But probably better to delete it and just directly call isInsideFlatpak/isInsideSnap from shouldUsePortal, and not have isRunningInSandbox at all. Definitely do not implement isInsideFlatpak using isRunningInSandbox.

(In reply to Michael Catanzaro from comment #8) > Comment on attachment 441063 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=441063&action=review > > >>> 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. > > > > > > Anyway, you've convinced me on everything else, but still need to fix > isRunningInSandbox. At minimum, test for snap as well. But probably better > to delete it and just directly call isInsideFlatpak/isInsideSnap from > shouldUsePortal, and not have isRunningInSandbox at all. > > Definitely do not implement isInsideFlatpak using isRunningInSandbox. isInsideFlatpak is not exactly the same in this case, that caches the result and it's used by the process launcher to see if we are already under flatpak. If we are not under flatpak false will be cached, but then the bubblewrap launcher will create the flatpak-info, but isInsideFlatpak will return false. That's why isInsideSandbox doesn't cache the result.

(In reply to Carlos Garcia Campos from comment #9) > isInsideFlatpak is not exactly the same in this case, that caches the result > and it's used by the process launcher to see if we are already under > flatpak. > If we are not under flatpak false will be cached, but then the > bubblewrap launcher will create the flatpak-info, but isInsideFlatpak will > return false. That's why isInsideSandbox doesn't cache the result. The .flatpak-info is created in the filesystem namespace of the sandboxed subprocess that is going to be launched. The UI process won't see it. Both isInsideSandbox and isInsideFlatpak would behave the same: you're just doing extra work in the case of isInsideSandbox. You can safely change isInsideSandbox to cache its result.

Created attachment 441571 [details] Patch

Comment on attachment 441571 [details] Patch I left some comments about the helper functions, but as the code was just moved around it's fine—but if you feel like changing them before landing, that would be neat :) View in context: https://bugs.webkit.org/attachment.cgi?id=441571&action=review > Source/WTF/wtf/glib/Sandbox.cpp:39 > + return *ret; It should not be needed to use std::optional here, you can just: bool isInsideFlatpak() { static bool ret = g_file_test("/.flatpak-info", G_FILE_TEST_EXISTS); return ret; } The C++11 specification guarantees that a static local is initialized only once when program control flow goes through it, and that it is thread-safe (amusingly, manually checking the optional is not thread-safe :D) The same applies to the other checks below. > Source/WTF/wtf/glib/Sandbox.cpp:70 > + return usePortal[0] != '0'; This one is a bit trickier, but one can use a lambda that gets invoked immediately to initialize the static value: bool shouldUsePortal() { static bool ret = ([]() -> bool { // do checks, return true/false })(); return ret; }

Committed r284370 (243155@main): <https://commits.webkit.org/243155@main>

<rdar://problem/84367775>

(In reply to Adrian Perez from comment #12) > The C++11 specification guarantees that a static local is initialized only > once when program control flow goes through it, and that it is thread-safe > (amusingly, manually checking the optional is not thread-safe :D) Remember Apple ports use some compiler flag to disable the threadsafety guarantee (presumably it has a noticeable perf impact?), so you can only rely on this in Linux-specific code, never in cross-platform code. (In this case, it's fine only because these functions are GLib-specific.)