1168090 - Screensaver inhibition fails

Status: RESOLVED DUPLICATE of bug 1398539

(Core :: Widget: Gtk, defect, P3)

Description

[Martin Stránský [:stransky] (ni? me)]

Follow up from Bug 811261

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Can you please install some dbus inspection utility (d-feet for instance) and check that your desktop provides dbus interfaces "org.freedesktop.ScreenSaver" or "org.gnome.SessionManager"? Because only those two are reflected.

For instance MATE provides only org.mate.ScreenSaver which is not covered by Firefox.

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Note - look at "SessionBus" interface.

Comment 3

[electrovalent]

In latest debian testing with xfce i can only see org.xfce.SessionManager dbus interface instead.

Comment 4

[Martin Stránský [:stransky] (ni? me)]

Well, that's the problem then. Xfce should provide the universal org.freedesktop.ScreenSaver interface (see http://people.freedesktop.org/~hadess/idle-inhibition-spec/re01.html).

Comment 5

[Martin Stránský [:stransky] (ni? me)]

FYI you can get the list by this command:

dbus-send --session --dest=org.freedesktop.DBus --type=method_call --print-reply /org/freedesktop/DBus org.freedesktop.DBus.ListNames | grep ScreenSaver

would be interested to see which interfaces are provided by other WM like KDE, Cinamon and so on.

Comment 6

[Martin Stránský [:stransky] (ni? me)]

I see on Fedora (21,22) those interfaces:

Gtk3/gnome-shell: org.freedesktop.ScreenSaver, org.gnome.ScreenSaver
KDE: org.freedesktop.ScreenSaver
MATE: org.mate.ScreenSaver

Comment 7

[electrovalent]

Freedesktop.org provides xdg-utils to handle screensaver. Both xfce and VLC without being the only one, are following this standard. It is the reason why VLC is able to disable both screensaver and power management when enters fullscreen. Is there a reason why Firefox doesn't follow this paradigm? If there is a solution why searching for a new one?

Comment 8

[Mike Hommey [:glandium]]

Firefox could use xdg-screensaver as a fallback when the dbus method is not supported.

Comment 9

[Petr Jašíček]

Attachment: 001_screen_saver.patch

--- Work in progress ---

In this patch I added support for screensaver inhibition for Mate and as fallback I added xdg-screensaver which is invoked periodically every 60 seconds using nsTimer.

I could not think of a way to determine whether xdg-screensaver utility is available on system before creating the timer in XDG SendInhibit() method - maybe check it using system("xdg-screensaver") but that does not feel like a proper solution to me. So right now the xdg-screensaver utility presence is checked in the timer callback.

What do you think, Mike ?

Comment 10

[Mike Hommey [:glandium]]

How likely is xdg-screensaver to be on the user system but not in /usr/bin? I'd just hardcode /usr/bin/xdg-screensaver.

Note you should use nsIProcess instead of execlp.

Comment 11

[Petr Jašíček]

Attachment: 002_screen_saver.patch

Thanks for the feedback, Mike.

I updated the patch. Before creating a timer it now searches for /usr/bin/xdg-screensaver, if it does not exist, it returns and does not create the timer.

The timer callback now uses nsIProcess run asynchronously instead of execlp.

What do you think ?

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8669606 [details] [diff] [review]
002_screen_saver.patch

Review of attachment 8669606 [details] [diff] [review]:
-----------------------------------------------------------------

At quick glance, that looks reasonable, but that should be reviewed by karlt. Note you have plenty of spurious whitespaces in this patch.

Comment 13

[Petr Jašíček]

Attachment: 003_screen_saver.patch

Removed excessive white spaces

Try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e8f59dda765

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 8671215 [details] [diff] [review]
003_screen_saver.patch

Using the "reset" command of xdg-screensaver works around the worst of the
xdg-screensaver shortcomings [1], and hopefully testing for the freedesktop
interface first avoids others such as shared state files.

No new usage of nsIProcess should be added at least until bug 678369 is fixed
(see also [2] [3] [4]), but it is not necessary to use nsIProcess
here.  g_spawn_async() is a nice helper that catches errors in launching the
child process.  It is not "COM"taminated [5], it doesn't suffer from [4]
AFAICS, and it even handles ECHILD from waitpid() if NSPR steals the child.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=811261#c83
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=227246
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=933680#c44
[4] https://bugzilla.mozilla.org/show_bug.cgi?id=705543#c13
[5] https://wiki.mozilla.org/Gecko:De_COMtamination

>+    , mTimer(nullptr)

No need for this as nsCOMPtr<> has a default constructor.

>   bool mWaitingForReply;
>+  
>+  nsCOMPtr<nsITimer> mTimer;

Move mTimer declaration to alongside other pointers (mConnection) for packing.

>+WakeLockTopic::SendMateInhibitMessage()

This looks like the same code as SendGNOMEInhibitMessage(), presumably because
it is the same interface, but with different names.

If so, please use the existing SendGNOMEInhibitMessage() function, and create
the method call with different names according to mDesktopEnvironment.

>+  if (self->GetShouldInhibit()) {

Is this method required?

If the timer is cancelling correctly, then shouldn't it always return true
here?

>+    const char* args[] = {"reset"};

This command parameter seems to do what is necessary, but it is not clear from
the documentation:

       reset
           Turns the screensaver off immediately. If the screen was locked the
           user may be asked to authenticate first.

Please describe the effect this code is depending on here.

>+bool WakeLockTopic::SendXDGInhibitMessage()
>+{
>+  // Check if xdg-screensaver utility is available
>+  nsresult rv;
>+  bool exists;
>+  nsCOMPtr<nsIFile> file = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID);
>+  file->InitWithNativePath(NS_LITERAL_CSTRING(XDG_SCREENSAVER_PATH));
>+  rv = file->Exists(&exists);
>+  if (!exists || NS_FAILED(rv)) {
>+    return false;
>+  }

With g_spawn_async(), there will be no need to test for file existence as the
function will return an error if it does not exist.

>+  if (mTimer) {
>+    return true;
>+  }
>+
>+  if (mTimer) {
>+    mTimer->InitWithFuncCallback(XDGScreenSaverReset,
>+                                 this,
>+                                 XDG_SCREEN_SAVER_RESET_INTERVAL,
>+                                 nsITimer::TYPE_REPEATING_SLACK);
>+  }
>+  else {
>+    return false;
>+  }
>+}

Missing return value on success, but returning true sets mWaitingForReply,
which means UninhibitScreensaver() always returns early.

Perhaps this method doesn't need a return value.

If SendXDGInhibitMessage merely starts a timer, then it could be better named
to indicate that.  It could alternatively spawn the first xdg command and set
the timer on success.

>+    if (mTimer) {
>+      mTimer->Cancel();
>+      mTimer = nullptr;
>+      mInhibitRequest = 0;

Is there a need to reset mInhibitRequest here?
InhibitSucceeded() has not been called to set it and it won't be used anyway.

>+      return true;
>+    }
>   }
>+  
> 
>   if (!message) {
>     return false;
>   }

Return true early even when !mTimer for consistency as SendUninhibit() may be
called more than once.  (The return value is not used, but if these methods
have return values then they should be sane.)

>   case Unsupported:
>     return false;

"Unsupported" can't be reached now.

Comment 15

[Petr Jašíček]

Attachment: 004_screen_saver.patch

Thank you for your feedback, Karl.

I updated the patch, what do you think ?

Comment 16

[Karl Tomlinson (:karlt)]

Comment on attachment 8683645 [details] [diff] [review]
004_screen_saver.patch

>+#define XDG_SCREENSAVER_RESET             "/usr/bin/xdg-screensaver reset"

Now that g_spawn_async() reports errors when exec fails and the file existence
test has been removed, the full path need not be provided.
Use only the executable name without the full path and G_SPAWN_SEARCH_PATH, to allow a user to provide their own version of the command, if required.

>+  bool SendMateInhibitMessage();

No longer defined/required.

>+  }
>+  else {

Gecko uses "} else {" (except in some old code).

>+WakeLockTopic::XDGScreenSaverReset(nsITimer* aTimer, void* aWakeLockTopic)

Please rename aWakeLockTopic to indicate that it is not used.
I don't know for sure, but I expect this will compile:
 WakeLockTopic::XDGScreenSaverReset(nsITimer* aTimer, void*)

>+{
>+  gchar** command;
>+  g_shell_parse_argv(XDG_SCREENSAVER_RESET, nullptr, &command, nullptr);
>+  g_spawn_async(nullptr,
>+                command,
>+                nullptr,
>+                G_SPAWN_DEFAULT,
>+                nullptr,
>+                nullptr,
>+                nullptr,
>+                nullptr);

Either free command appropriately or construct command on the stack, so that
the heap allocations are not required and there is no need to free.  See, for
example,
https://dxr.mozilla.org/mozilla-central/rev/e790bba372f14241addda469a4bdb7ab00786ab3/dom/ipc/ContentChild.cpp#629

Please place this in a helper function, so that it can be shared with
CreateXDGResetTimer.

>+    if (mTimer) {
>+      mTimer->InitWithFuncCallback(XDGScreenSaverReset,
>+                                   NULL,
>+                                   XDG_SCREEN_SAVER_RESET_INTERVAL,
>+                                   nsITimer::TYPE_REPEATING_SLACK);      
>+      return true;
>+    }
>+    else {
>+      return false;
>+    }
>+  }
>+  else {
>+    return false;
>+  }
>+}

No need for the two else and return paths.
Use a single "return false" at the end of the function with no else.

>+  case Mate:
>     sendOk = SendGNOMEInhibitMessage();
>     break;

>+  case Other:
>+    sendOk = CreateXDGResetTimer();
>+    break;
>   }
> 
>   if (sendOk) {
>     mWaitingForReply = true;
>   }

CreateXDGResetTimer returning true sets mWaitingForReply,
which means UninhibitScreensaver() always returns early.

Instead always return early in the Other case.

>+  } else if (mDesktopEnvironment == Other) {
>+    // xdg-screensaver does not use dbus
>+    if (mTimer) {
>+      mTimer->Cancel();
>+      mTimer = nullptr;
>+      return true;
>+    }
>   }
>+  
> 

Return early (with true) when (mDesktopEnvironment == Other) even when !mTimer
for consistency as SendUninhibit() may be called more than once.  (The return
value is not used, but if these methods have return values then they should be
sane.)

>   if (!message) {
>-    return false;
>+    return true;
>   }
> 
>   dbus_message_append_args(message,

Leave this returning false, to indicate an error.

Comment 17

[Steve Dodier]

Martin,

Xfce developer here. Just a couple of notes and an idea to fix screensaver inhibition on Xfce and possibly elsewhere.

Firstly we can't support the screensaver DBus interface because our users use multiple lockers / screensavers alongside Xfce which may or may not implement it themselves. This is already a delicate situation for us as we have to do dirty things to figure out which locker is in use, and it would be generally a poor idea to enter a competition with these lockers for an interface we don't even use.

What we do support however is the FreeDesktop Power Management interface (org.freedesktop.powermanagement). I'm not sure if it has been standardised or if someone just forgot to upload an API documentation, but you can see the API here:

http://git.xfce.org/xfce/xfce4-power-manager/tree/src/org.freedesktop.PowerManagement.xml

and

http://git.xfce.org/xfce/xfce4-power-manager/tree/src/org.freedesktop.PowerManagement.Inhibit.xml

Now, using this API also triggers the presentation mode in Xfce, which prevents the screen dimming or going black. We think it's a nicer approach than simply managing lockers, and this is why in Xfce it's the power manager's job to call the locker when the session is inactive for long enough (because most users would have their screen dim before or at the same time as the session locks, rather than after).

Messaging this interface alongside the existing ScreenSaver interface would solve the screensaver inhibition problem as the Xfce power manager would both keep the screen on and prevent the locker from kicking in.

As far as I know this interface is also used by KDE so it could also inhibit screen dimming on KDE.

Would that be an option for Firefox?

Comment 18

[Victor Engmark]

Very basic setup which exhibits this behaviour:

- Up-to-date Arch Linux with Firefox 48, slock-1.3, xss-lock 0.3.0.
- X profile like in <https://github.com/l0b0/tilde/blob/c0e34f86b3893e0815fea4afe3af7f545228d4ee/.xprofile#L59>.
- `xset q` reports "prefer blanking: yes allow exposures: yes" and "timeout: 600 cycle: 600"

Using this setup VLC is able to inhibit the screensaver but Firefox is not.

Comment 19

[jamesmcampbell]

When implementing a fix for this, please use a single generic solution which will work for any and all desktop environments which use the X server.

Example: the mpv video player (https://mpv.io/) runs on every linux desktop environment AFAIK, and has a single method to disable/enable the screensaver.  They don't have to use case statements to check if you're on Gnome or KDE or XFCE or MATE or ...

mpv uses XScreenSaverSuspend (X11/extensions/scrnsaver.h) and DPMSEnable/DPMSDisable (X11/extensions/dpms.h).

Here are details on XScreenSaverSuspend and DPMSEnable/DPMSDisable:
    https://www.x.org/archive/current/doc/man/man3/Xss.3.xhtml
    https://www.x.org/archive/current/doc/man/man3/DPMSEnable.3.xhtml
    https://www.x.org/archive/current/doc/man/man3/DPMSDisable.3.xhtml

Here is a link to the code where mpv implements their screensaver:
    https://github.com/mpv-player/mpv/blob/master/video/out/x11_common.c

Here is a code snippet from mpv's x11_common.c:

.....

static int xss_suspend(Display *mDisplay, Bool suspend)
{
#if !HAVE_XSS
    return 0;
#else
    int event, error, major, minor;
    if (XScreenSaverQueryExtension(mDisplay, &event, &error) != True ||
        XScreenSaverQueryVersion(mDisplay, &major, &minor) != True)
        return 0;
    if (major < 1 || (major == 1 && minor < 1))
        return 0;
    XScreenSaverSuspend(mDisplay, suspend);
    return 1;
#endif
}

static void set_screensaver(struct vo_x11_state *x11, bool enabled)
{
    Display *mDisplay = x11->display;
    if (!mDisplay || x11->screensaver_enabled == enabled)
        return;
    MP_VERBOSE(x11, "%s screensaver.\n", enabled ? "Enabling" : "Disabling");
    x11->screensaver_enabled = enabled;
    if (xss_suspend(mDisplay, !enabled))
        return;
#if HAVE_XEXT
    int nothing;
    if (DPMSQueryExtension(mDisplay, &nothing, &nothing)) {
        BOOL onoff = 0;
        CARD16 state;
        DPMSInfo(mDisplay, &state, &onoff);
        if (!x11->dpms_touched && enabled)
            return; // enable DPMS only we we disabled it before
        if (enabled != !!onoff) {
            MP_VERBOSE(x11, "Setting DMPS: %s.\n", enabled ? "on" : "off");
            if (enabled) {
                DPMSEnable(mDisplay);
            } else {
                DPMSDisable(mDisplay);
                x11->dpms_touched = true;
            }
            DPMSInfo(mDisplay, &state, &onoff);
            if (enabled != !!onoff)
                MP_WARN(x11, "DPMS state could not be set.\n");
        }
    }
#endif
}

.....

Comment 20

[Vladimir Panteleev]

Hello,

In WakeLockListener.cpp, there is the line:

#define FREEDESKTOP_SCREENSAVER_OBJECT    "/ScreenSaver"

Is this line correct?

The specification in https://people.freedesktop.org/~hadess/idle-inhibition-spec/index.html does not specify an object path. However, in all dbus examples dealing with org.freedesktop.ScreenSaver, the path I saw was "/ScreenSaver", not "/ScreenSaver".

Comment 21

[Vladimir Panteleev]

> the path I saw was "/ScreenSaver", not "/ScreenSaver".

... the path I saw was "/org/freedesktop/ScreenSaver", not "/ScreenSaver".

Comment 22

[Martin Stránský [:stransky] (ni? me)]

Thanks, I'll look at it.

Comment 24

[Martin Stránský [:stransky] (ni? me)]

Jan, can you look at it please?

Comment 25

[Marcos Mello]

Here is Chromium implementation:

https://chromium.googlesource.com/chromium/src/+/master/services/device/wake_lock/power_save_blocker/power_save_blocker_x11.cc

It works fine on XFCE. The API from Comment 17 is used in addition to GNOME's one.

Comment 26

[johannes.loher]

The chromium implementation also works with my setup (Arch Linux + i3) while the solution that firefox uses does not work. I'd love to switch back from chromium to firefox but this is a blocker for me, so I'd be really happy if this issue saw some attention.

Comment 27

[Lee Bousfield]

johannes.loher, could you try Firefox nightly? I recently added XScreenSaver support which may fix the issue with your setup.

Comment 28

[Alessandro Pezzoni]

Lee, I have a similar setup as johannes.loher's, with xss-lock running i3locker on Arch Linux. The issue seems to be fixed in Firefox nightly for me. I tested this by playing a YouTube video both fullscreen and not. Do you need any other tests?

Comment 29

[Lee Bousfield]

Those tests are enough. It fixed my setup too. Is anyone else still experiencing this issue, or should it be marked as resolved?

Comment 30

[johannes.loher]

Lee, I tested it with Firefox nightly and the issue indeed seems to be fixed on my system. Thank you very much for working on this!

Comment 31

[Stanislav Brabec]

I am using Mozilla Firefox 52.5.3 in openSUSE Leap 42.3 and GNOME 3.20.

The problem is not fully fixed. GNOME still needs to disable power management, otherwise the system may go to sleep or hibernate.

Here is a work around for GNOME:

gsettings set org.gnome.settings-daemon.plugins.power sleep-inactive-ac-type nothing
gsettings set org.gnome.settings-daemon.plugins.power sleep-inactive-battery-type nothing

Maybe this problem could be considered as a GNOME problem not correctly implementing Idle Inhibition Service Draft.

Comment 32

[Martin Stránský [:stransky] (ni? me)]

(In reply to Stanislav Brabec from comment #31)
> I am using Mozilla Firefox 52.5.3 in openSUSE Leap 42.3 and GNOME 3.20.
> 
> The problem is not fully fixed. GNOME still needs to disable power
> management, otherwise the system may go to sleep or hibernate.
> 
> Here is a work around for GNOME:
> 
> gsettings set org.gnome.settings-daemon.plugins.power sleep-inactive-ac-type
> nothing
> gsettings set org.gnome.settings-daemon.plugins.power
> sleep-inactive-battery-type nothing
> 
> Maybe this problem could be considered as a GNOME problem not correctly
> implementing Idle Inhibition Service Draft.

That's because Bug 1398539 was fixed for Firefox 58.