Bug 178485 – [WPE][GTK] Implement PAL::SleepDisabler

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

[Michael Catanzaro]

We should implement PAL::SleepDisabler and PAL::SystemSleepListener for GTK and WPE using the logind D-Bus API:

https://www.freedesktop.org/wiki/Software/systemd/inhibit/

If logind is not available, then of course it should gracefully fallback to doing nothing.

We should also figure out how we currently block sleep when playing video. I see HTMLMediaElement uses SleepDisabler, but we have not implemented it. Yet I'm also positive that we do somewhere inhibit screen blank when video is playing.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #0)
> We should implement PAL::SleepDisabler and PAL::SystemSleepListener for GTK
> and WPE using the logind D-Bus API:
> 
> https://www.freedesktop.org/wiki/Software/systemd/inhibit/
> 
> If logind is not available, then of course it should gracefully fallback to
> doing nothing.
> 
> We should also figure out how we currently block sleep when playing video. I
> see HTMLMediaElement uses SleepDisabler, but we have not implemented it. Yet
> I'm also positive that we do somewhere inhibit screen blank when video is
> playing.

I haven't checked the current code, but I am confident that it uses the
X11 Screensaver extension. It could be that GNOME Shell under Wayland
also honors it thanks to XWayland, but this one bit is speculation on
my side :-)

[Carlos Garcia Campos]

We do that from the UI process when entering fullscreen video.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #2)
> We do that from the UI process when entering fullscreen video.

Yes, I noticed this the other day. We fail to inhibit suspend when the video is not fullscreen. So we need a proper SleepDisabler. :)

[Michael Catanzaro]

Let's simplify this. We really need PAL::SleepDisabler. The need for PAL::SystemSleepListener is less clear. It's used only for PlatformMediaSessionManager::systemWillSleep and PlatformMediaSessionManager::systemDidWake, which are only used by WebAudio, and it's not clear to me how important those are. Implementing this would also require depending on yet another D-Bus API, which will be problematic for flatpak (bug #180039).

But we do need PAL::SleepDisabler.

[Michael Catanzaro]

Well, I've wasted three days trying to figure out how to run a modified WebKit inside Flatpak. It's too hard and not worth the effort. So I don't know if my code works, and because of that I'm not willing to commit it.

But the non-Flatpak codepath works fine, and it's a nice cleanup. So let's land Flatpak support separately.

[Michael Catanzaro]

Created attachment 327908 [details]
Patch

[Michael Catanzaro]

Flatpak support committed in https://git.gnome.org/browse/gnome-sdk-images/commit/?id=9e50b39d397af2a15dedb99f6dc68825bae4d3c6. It can live there until it is possible to test it. (Might be a long time, because I was not successful in adding perl-File-Copy-Recursive to the SDK.)

[Carlos Garcia Campos]

Comment on attachment 327908 [details]
Patch

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

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:54
> +    g_dbus_proxy_new_for_bus(G_BUS_TYPE_SESSION, static_cast<GDBusProxyFlags>(G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES | G_DBUS_PROXY_FLAGS_DO_NOT_CONNECT_SIGNALS),
> +        nullptr, "org.freedesktop.ScreenSaver", "/ScreenSaver", "org.freedesktop.ScreenSaver", m_cancellable.get(),

One good thing of the current code is that we connect to the screensaver on demand and only once. Here it happens on demand, because SleepDisabler is only created when needed and then destroyed, but we are connecting to the bus every time it's created. Maybe we can create a private singleton for the proxy.

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:57
> +            auto* self = static_cast<SleepDisablerGtk*>(userData);
> +            self->m_screenSaverProxy = adoptGRef(g_dbus_proxy_new_for_bus_finish(result, nullptr));

self might be destroyed here at this point. You need to finish the operation and properly handle the case of cancellation. In case of cancellation just return silently, in case of other error, show a warnings and otherwise then get self and initialize the proxy.

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:61
> +                GUniquePtr<char> nameOwner(g_dbus_proxy_get_name_owner(self->m_screenSaverProxy.get()));
> +                if (nameOwner)
> +                    self->acquireInhibitor();

How can happen that you get a valid proxy for ScreenSaver and name_owner is nullptr?

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:87
> +            auto* self = static_cast<SleepDisablerGtk*>(userData);
> +            GUniqueOutPtr<GError> error;
> +            GRefPtr<GVariant> returnValue = adoptGRef(g_dbus_proxy_call_finish(self->m_screenSaverProxy.get(), result, &error.outPtr()));

Same here about self and cancellation.

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.h:45
> +    GRefPtr<GDBusProxy> m_screenSaverProxy { nullptr };

GRefPtr already initializes the pointer to nullptr.

> Source/WebCore/html/HTMLMediaElement.cpp:6614
> +    // FIXME: This #if should be removed, because GTK now supports SleepDisabler, and because this
> +    // code is harmless on ports where SleepDisabler does nothing. However, after brief testing on
> +    // YouTube, I noticed that sleep is not disabled during normal video playback, but it *is*
> +    // disabled when a page playing video enters the page cache, until the associated web view is
> +    // destroyed. Keep it Cocoa-only for now, so that cached pages don't block sleep indefinitely.

Why don't you open a bug report to fix whatever is wrong instead of adding a comment in the code?

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:194
> +    std::unique_ptr<PAL::SleepDisabler> sleepDisabler { nullptr };

unique_ptr already initializes the pointer to nullptr.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1246
> -    webkitWebViewBaseInhibitScreenSaver(webkitWebViewBase);
> +    priv->sleepDisabler = PAL::SleepDisabler::create(_("Website running in fullscreen mode"), PAL::SleepDisabler::Type::Display);

So, do we need to inhibit the screensaver from both the UI and Web process?

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #8)
> One good thing of the current code is that we connect to the screensaver on
> demand and only once. Here it happens on demand, because SleepDisabler is
> only created when needed and then destroyed, but we are connecting to the
> bus every time it's created. Maybe we can create a private singleton for the
> proxy.

OK.
 
> > Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:57
> > +            auto* self = static_cast<SleepDisablerGtk*>(userData);
> > +            self->m_screenSaverProxy = adoptGRef(g_dbus_proxy_new_for_bus_finish(result, nullptr));
> 
> self might be destroyed here at this point. You need to finish the operation
> and properly handle the case of cancellation. In case of cancellation just
> return silently, in case of other error, show a warnings and otherwise then
> get self and initialize the proxy.

Yes. I should know this by now, but clearly I haven't learned....

> > Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:61
> > +                GUniquePtr<char> nameOwner(g_dbus_proxy_get_name_owner(self->m_screenSaverProxy.get()));
> > +                if (nameOwner)
> > +                    self->acquireInhibitor();
> 
> How can happen that you get a valid proxy for ScreenSaver and name_owner is
> nullptr?

I copied it from GtkApplication, but I think it's correct. Proxy creation does not fail due to the absence of a name owner; it will succeed and you'll have an inert proxy where the method calls all fail. If the name is unowned, then we want to skip over it and try the fallback (flatpak portal) codepath (which I removed from this patch, but which I intend to restore eventually).

I found a nice test to demonstrate. Using glib/gio/tests/gdbus-example-watch-proxy.c as test.c:

$ gcc `pkg-config --cflags --libs gio-2.0` test.c
$ ./a.out -n org.gnome.Contacts -o /org/gnome/Contacts -i org.gtk.Application --no-auto-start

Notice that the proxy is created successfully, though there is no name owner because you're not running Contacts. Then launch Contacts and watch it own the name. GDBusProxy hides the messiness of changing name owners and ensures all methods/properties/signals go to/from the current name owner.

All that said... I don't pretend to understand D-Bus.

> > Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:87
> > +            auto* self = static_cast<SleepDisablerGtk*>(userData);
> > +            GUniqueOutPtr<GError> error;
> > +            GRefPtr<GVariant> returnValue = adoptGRef(g_dbus_proxy_call_finish(self->m_screenSaverProxy.get(), result, &error.outPtr()));
> 
> Same here about self and cancellation.

Yes.

> > Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.h:45
> > +    GRefPtr<GDBusProxy> m_screenSaverProxy { nullptr };
> 
> GRefPtr already initializes the pointer to nullptr.

OK.

> > Source/WebCore/html/HTMLMediaElement.cpp:6614
> > +    // FIXME: This #if should be removed, because GTK now supports SleepDisabler, and because this
> > +    // code is harmless on ports where SleepDisabler does nothing. However, after brief testing on
> > +    // YouTube, I noticed that sleep is not disabled during normal video playback, but it *is*
> > +    // disabled when a page playing video enters the page cache, until the associated web view is
> > +    // destroyed. Keep it Cocoa-only for now, so that cached pages don't block sleep indefinitely.
> 
> Why don't you open a bug report to fix whatever is wrong instead of adding a
> comment in the code?

I'll report a bug.

> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:194
> > +    std::unique_ptr<PAL::SleepDisabler> sleepDisabler { nullptr };
> 
> unique_ptr already initializes the pointer to nullptr.

OK.

> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1246
> > -    webkitWebViewBaseInhibitScreenSaver(webkitWebViewBase);
> > +    priv->sleepDisabler = PAL::SleepDisabler::create(_("Website running in fullscreen mode"), PAL::SleepDisabler::Type::Display);
> 
> So, do we need to inhibit the screensaver from both the UI and Web process?

Currently, PAL::SleepDisabler is only used from the UI process. But it should be used from the web process as well, by HTMLMediaElement.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #8)
> One good thing of the current code is that we connect to the screensaver on
> demand and only once. Here it happens on demand, because SleepDisabler is
> only created when needed and then destroyed, but we are connecting to the
> bus every time it's created. Maybe we can create a private singleton for the
> proxy.

There's a disadvantage to using a private singleton: the proxy will stay alive forever, even when no SleepDisabler exists, which seems unnecessary. That is, unless we create a global list of SleepDisablers, and then destroy the global proxy when no more SleepDisablers exist, and recreate it when there is a new SleepDisabler, which seems excessive. So my preference would be to leave this unchanged; SleepDisablers should be created rarely, and the general case will be to have zero. But, after you've considered that, I'll implement what you prefer.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #10)
> (In reply to Carlos Garcia Campos from comment #8)
> > One good thing of the current code is that we connect to the screensaver on
> > demand and only once. Here it happens on demand, because SleepDisabler is
> > only created when needed and then destroyed, but we are connecting to the
> > bus every time it's created. Maybe we can create a private singleton for the
> > proxy.
> 
> There's a disadvantage to using a private singleton: the proxy will stay
> alive forever, even when no SleepDisabler exists, which seems unnecessary.
> That is, unless we create a global list of SleepDisablers, and then destroy
> the global proxy when no more SleepDisablers exist, and recreate it when
> there is a new SleepDisabler, which seems excessive. So my preference would
> be to leave this unchanged; SleepDisablers should be created rarely, and the
> general case will be to have zero. But, after you've considered that, I'll
> implement what you prefer.

Ok, it's fine as it is.

[Michael Catanzaro]

Created attachment 327980 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 327980 [details]
Patch

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

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:38
> +SleepDisablerGtk::SleepDisablerGtk(const char* reason, Type type)

I'm sorry I didn't notice this before, but this is not specific to GTK at all, I would call this GLib instead or even Fdo since this is the Freedesktop.org implementation.

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:58
> +            if (error && g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED))

g_error_matches() handles null errors

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:62
> +            auto* self = static_cast<SleepDisablerGtk*>(userData);
> +            if (!error) {

You could early return here in case of error, just setting m_cancellable to nullptr, and saving an indentation level for the rest of the code. Actually, since it's expected that proxy is nullptr in case of error, I would remove the assret adn check proxy instead of error to return early.

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:65
> +                GUniquePtr<char> nameOwner(g_dbus_proxy_get_name_owner(proxy.get()));
> +                if (nameOwner) {

if (GUniquePtr<char> nameOwner(g_dbus_proxy_get_name_owner(proxy.get())))

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:66
> +                    self->m_screenSaverProxy = proxy;

WTFMove(proxy)

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:92
> +            if (error && g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED))

g_error_matches() handles null errors

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:116
> +            GUniqueOutPtr<GError> error;
> +            GRefPtr<GVariant> returnValue = adoptGRef(g_dbus_proxy_call_finish(G_DBUS_PROXY(proxy), result, &error.outPtr()));
> +            if (error)
> +                g_warning("Calling %s.UnInhibit failed: %s", g_dbus_proxy_get_interface_name(G_DBUS_PROXY(proxy)), error->message);

Is this really useful? I would call g_dbus_proxy_call without a callback.

> Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.h:30
> +#include <gio/gio.h>

I don't think this is needed, but if you need it, try to forward declare GDBusProxy and GCancellable and move the header inclusion to the cpp

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:-503
> -    g_cancellable_cancel(webView->priv->screenSaverInhibitCancellable.get());

sleepDisabler = nullptr to cancel it earlier.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1246
> +    priv->sleepDisabler = PAL::SleepDisabler::create(_("Website running in fullscreen mode"), PAL::SleepDisabler::Type::Display);

Does the reason really need to be translated? where is the user going to see this message?

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #13)
> I'm sorry I didn't notice this before, but this is not specific to GTK at
> all, I would call this GLib instead or even Fdo since this is the
> Freedesktop.org implementation.

I'll call it GLib. fdo would work for now, but we don't use that suffix elsewhere in WebKit, and who knows: we might change the D-Bus interfaces that we use in the future.

I had considered GLib, but at the time I had been trying to use org.gnome.SessionManager, so the Gtk suffix seemed more appropriate. And half the code had been Flatpak support at the time.

> > Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:58
> > +            if (error && g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED))
> 
> g_error_matches() handles null errors

OK.

> > Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:62
> > +            auto* self = static_cast<SleepDisablerGtk*>(userData);
> > +            if (!error) {
> 
> You could early return here in case of error, just setting m_cancellable to
> nullptr, and saving an indentation level for the rest of the code. Actually,
> since it's expected that proxy is nullptr in case of error, I would remove
> the assret adn check proxy instead of error to return early.

OK.

> > Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:65
> > +                GUniquePtr<char> nameOwner(g_dbus_proxy_get_name_owner(proxy.get()));
> > +                if (nameOwner) {
> 
> if (GUniquePtr<char> nameOwner(g_dbus_proxy_get_name_owner(proxy.get())))

OK.

> > Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:66
> > +                    self->m_screenSaverProxy = proxy;
> 
> WTFMove(proxy)

OK.

> > Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:92
> > +            if (error && g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED))
> 
> g_error_matches() handles null errors

OK.

> > Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:116
> > +            GUniqueOutPtr<GError> error;
> > +            GRefPtr<GVariant> returnValue = adoptGRef(g_dbus_proxy_call_finish(G_DBUS_PROXY(proxy), result, &error.outPtr()));
> > +            if (error)
> > +                g_warning("Calling %s.UnInhibit failed: %s", g_dbus_proxy_get_interface_name(G_DBUS_PROXY(proxy)), error->message);
> 
> Is this really useful? I would call g_dbus_proxy_call without a callback.

I think it's useful, because otherwise there will be no error message if uninhibiting fails, and your screen will never lock. It's unlikely that this call would ever fail, but it seems good to have a warning in this case, because it indicates something has gone wrong.

In contrast, the initial inhibit calls are expected to fail and should not cause warnings.

> > Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.h:30
> > +#include <gio/gio.h>
> 
> I don't think this is needed, but if you need it, try to forward declare
> GDBusProxy and GCancellable and move the header inclusion to the cpp

OK.

> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:-503
> > -    g_cancellable_cancel(webView->priv->screenSaverInhibitCancellable.get());
> 
> sleepDisabler = nullptr to cancel it earlier.
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1246
> > +    priv->sleepDisabler = PAL::SleepDisabler::create(_("Website running in fullscreen mode"), PAL::SleepDisabler::Type::Display);
> 
> Does the reason really need to be translated? where is the user going to see
> this message?

The reason should be displayed on the inhibitor review dialog that may appear if you try to log out while an inhibitor is in place. There have been GNOME Session bugs in the past, so I'm not sure if that functionality is currently working or not in GNOME. But yes, it's supposed to be translated. This is actually in conflict with Apple's use of this parameter to include a reverse-DNS programmer-visible string. A follow-up patch might split reason into two parameters, one a translated human-readable string for use by SleepDisablerGtk/GLib, and the other the untranslated reverse-DNS string for use by SleepDisablerCocoa.

[Michael Catanzaro]

Comment on attachment 327980 [details]
Patch

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

>> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:-503
>> -    g_cancellable_cancel(webView->priv->screenSaverInhibitCancellable.get());
> 
> sleepDisabler = nullptr to cancel it earlier.

Oops, yes.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #13)
> > Source/WebCore/PAL/pal/system/gtk/SleepDisablerGtk.cpp:62
> > +            auto* self = static_cast<SleepDisablerGtk*>(userData);
> > +            if (!error) {
> 
> You could early return here in case of error, just setting m_cancellable to
> nullptr, and saving an indentation level for the rest of the code. 

It didn't work out as well as I'd hoped; I'd rather have an extra level of indentation than duplicate the self->m_cancellable = nullptr assignment, which is two lines of code as I'd like to keep the comment above it. And it was fated to be reverted when adding Flatpak portal support anyway.

> Actually,
> since it's expected that proxy is nullptr in case of error, I would remove
> the assret adn check proxy instead of error to return early.

Done.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #13)
> if (GUniquePtr<char> nameOwner(g_dbus_proxy_get_name_owner(proxy.get())))

It doesn't build. I think it has to be an assignment.

[Michael Catanzaro]

Created attachment 328141 [details]
Patch

[Michael Catanzaro]

Note: I used cq? here since I failed to implement a significant number of your suggestions. Will set cq+ tomorrow.

[Carlos Garcia Campos]

Comment on attachment 328141 [details]
Patch

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

> Source/WebCore/PAL/pal/system/glib/SleepDisablerGLib.cpp:66
> +                    self->m_screenSaverProxy = proxy;

Why couldn't you use WTFMove here?

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #20)
> Comment on attachment 328141 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328141&action=review
> 
> > Source/WebCore/PAL/pal/system/glib/SleepDisablerGLib.cpp:66
> > +                    self->m_screenSaverProxy = proxy;
> 
> Why couldn't you use WTFMove here?

I think I added it, then accidentally Ctrl+Zed it away and failed to notice.

[Michael Catanzaro]

Committed r225477: <https://trac.webkit.org/changeset/225477>

[Michael Catanzaro]

The commit message got messed up... I think it's because I forgot to rename one of the changelog entries.