1398539 - WakeLockListener should disable DPMS if enabled on Linux

Status: RESOLVED FIXED

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

Description

[Lee Bousfield]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170909100226

Steps to reproduce:

Watch a video on Linux in Firefox with no desktop environment (I'm using i3), then wait.


Actual results:

My monitor fell asleep. dbus-monitor shows Firefox tries to prevent it, but fails since I don't have a screensaver setup.


Expected results:

Firefox should have disabled DPMS, preventing the monitor from falling asleep.

Comment 1

[Lee Bousfield]

Alternatively, Firefox could use xdg-screensaver from xdg-utils, which implements a number of methods for disabling screen sleeping: https://github.com/mtorromeo/xdg-utils/blob/master/scripts/xdg-screensaver.in

Comment 2

[Lee Bousfield]

It seems most implementations, rather than fully disable DPMS, simply continually refresh the timer (by disabling then re-enabling DPMS), or suspend the timer with XScreenSaverSuspend. A very nice benefit of the latter is "If a client that has suspended the screensaver becomes disconnected from the X server, the screensaver timer will automatically be  restarted" (from the man page). It also takes into account multiple clients suspending the screensaver. I wrote an initial patch to fully disable DPMS, but after seeing the flaws of that approach (it had shutdown issues), I'll look into implementing XScreenSaverSuspend instead.

Comment 3

[Karl Tomlinson (back June 29 :karlt)]

Please see the patch in bug 1168090 and see whether finishing that would also address this situation.

Comment 4

[Lee Bousfield]

Yes, it would. Ideally we'd have a more extensive builtin solution, but still fallback on xdg-screensaver. I'll probably finish my current XScreenSaverSuspend code, then rebase the other patch (and fix it up, maybe just start from the ground up).

Comment 5

[Lee Bousfield]

Attachment: Bug 1398539: Inhibit screensaver with XScreenSaverSuspend

Review commit: https://reviewboard.mozilla.org/r/179552/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179552/

Comment 6

[Lee Bousfield]

The problem with the xdg-screensaver fallback after this patch is that I can't find a good way to check if XScreenSaver or DPMS are being used (so I don't know when to fall back).

Comment 7

[Lee Bousfield]

To clarify, this patch is fine. In the future, if/when we use xdg-screensaver, we'll just have to prioritize it over XScreenSaver.

Comment 8

[Karl Tomlinson (back June 29 :karlt)]

Comment on attachment 8907875 [details]
Bug 1398539: Inhibit screensaver with XScreenSaverSuspend

https://reviewboard.mozilla.org/r/179552/#review187820

This approach looks good thanks.  The main thing though is doing this without adding a hard runtime dependency on libXss.so.1.

::: widget/gtk/WakeLockListener.cpp:42
(Diff revision 1)
>  
>  
>  enum DesktopEnvironment {
>    FreeDesktop,
>    GNOME,
> +#if defined(HAVE_LIBXSS) && defined(MOZ_X11) && defined(MOZ_WIDGET_GTK)

No need for MOZ_WIDGET_GTK, as all files in this directory are compiled only in the GTK port.

I don't mind whether or not MOZ_X11 is tested.
I assume it will be a long time before that platform is dropped.

HAVE_LIBXSS also won't be required.  See below.

::: widget/gtk/WakeLockListener.cpp:165
(Diff revision 1)
> +  Display* display = GDK_DISPLAY_XDISPLAY(gdk_display_get_default());
> +  if (!display) return false;

A Wayland port is being developed, and it is considered a programming error to
call gdk_x11_display_get_xdisplay() on a GdkDisplay that is not an X11
display, and so please first test with GDK_IS_X11_DISPLAY().  That makes the
null check on the Display* unnecessary.

::: widget/gtk/WakeLockListener.cpp:172
(Diff revision 1)
> +  if (major < 1) return false;
> +  if (major == 1 && minor < 1) return false;

Should be an equality check on the major version.  i.e. return false if major != 1.

"This API is considered as experimental.  The Xss library major revision may be incremented whenever incompatible changes are done to the API without notice."

::: widget/gtk/WakeLockListener.cpp:309
(Diff revision 1)
>    } else {
> -    NS_ASSERTION(mDesktopEnvironment == GNOME, "Unknown desktop environment");
>      mDesktopEnvironment = Unsupported;
> +  }
> +  if (mDesktopEnvironment == Unsupported) {
>      mShouldInhibit = false;
>    }

Please leave "mShouldInhibit = false;" in the else block, to make it clear that is the only path for reaching that line.

::: widget/gtk/moz.build:153
(Diff revision 1)
>      CXXFLAGS += CONFIG['MOZ_DBUS_GLIB_CFLAGS']
>  
>  CXXFLAGS += ['-Wno-error=shadow']
> +
> +if CONFIG['MOZ_X11'] and CONFIG['XSS_LIBS'] and CONFIG['MOZ_WIDGET_TOOLKIT'].startswith('gtk'):
> +    OS_LIBS += CONFIG['XSS_LIBS']

Adding this dependency may cause Firefox to fail to start on some systems.
I don't see much on my system that depends on libXss.so.1, for example.
nsIdleServiceGTK.cpp checks for libXss.so.1 dynamically, and I think this code
should do the same.

Comment 9

[Lee Bousfield]

I'm currently working on wrapping the actual XScreenSaver code into nsIdleServiceGTK, because they share some XSS functions. In addition, the XSS library can't be unloaded because of some technicalities (see comments in nsIdleServiceGTK.cpp), so it's best if we load it as little as possible.

Is this a good decision, or should the code be somewhere else?

Comment 10

[Karl Tomlinson (back June 29 :karlt)]

Loading an already loaded library again won't add any further resources; it will
just add one to the reference count.

If XScreenSaverQueryExtension is the only function shared, then I wouldn't
bother trying to share a single copy of the function pointer.
Even if there is more than one function, I'm not sure its worthwhile, but it's
not unreasonable to share the code in nsIdleServiceGTK, if the redirection is
not too complicated.

Comment 11

[Lee Bousfield]

Comment on attachment 8907875 [details]
Bug 1398539: Inhibit screensaver with XScreenSaverSuspend

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179552/diff/1-2/

Comment 12

[Lee Bousfield]

I've decided to keep the code in WakeLockListener (not nsIdleServiceGTK).

Comment 13

[Karl Tomlinson (back June 29 :karlt)]

Comment on attachment 8907875 [details]
Bug 1398539: Inhibit screensaver with XScreenSaverSuspend

https://reviewboard.mozilla.org/r/179552/#review199732

Looks good, thanks.  Please remove the claim that a race condition is fine, and there are some simplifications because these methods are used only on the main thread and so there are no races.

::: widget/gtk/WakeLockListener.cpp:182
(Diff revisions 1 - 2)
> -  Display* display = GDK_DISPLAY_XDISPLAY(gdk_display_get_default());
> -  if (!display) return false;
> +  if (!sXssLib) {
> +    PRLibrary* xssLib = PR_LoadLibrary("libXss.so.1");
> +    if (!xssLib) {
> +      return false;
> +    }
> +    // A race condition here is fine.

The comment about the race condition here makes me uncomfortable.  Races without thread-safe primitives are rarely fine.  They may be likely benign, but compilers are free to make assumptions that make races have unexpected consequences.
https://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong

But this file is already assuming that the methods are only called on the main thread, and so just assume that there will be no races and assign the result of PR_LoadLibrary directly to sXssLib.

::: widget/gtk/WakeLockListener.cpp:188
(Diff revisions 1 - 2)
> +  if (!_XSSQueryExtension) {
> +    _XSSQueryExtension = (_XScreenSaverQueryExtension_fn)
> +        PR_FindFunctionSymbol(sXssLib, "XScreenSaverQueryExtension");
> +  }
> +  if (!_XSSQueryVersion) {
> +    _XSSQueryVersion = (_XScreenSaverQueryVersion_fn)
> +        PR_FindFunctionSymbol(sXssLib, "XScreenSaverQueryVersion");
> +  }
> +  if (!_XSSSuspend) {
> +    _XSSSuspend = (_XScreenSaverSuspend_fn)
> +        PR_FindFunctionSymbol(sXssLib, "XScreenSaverSuspend");
> +  }

Perhaps these checks for each function pointer were also to avoid races which won't exist.

Once the library is loaded, and PR_FindFunctionSymbol() returns null for a symbol, another call for the same symbol will not return a different result.

That and the fact that this is used only on the main thread mean that the null checks on each function pointer are unnecessary if the PR_FindFunctionSymbol() calls are moved into the !sXssLib block.

Comment 14

[Lee Bousfield]

Comment on attachment 8907875 [details]
Bug 1398539: Inhibit screensaver with XScreenSaverSuspend

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179552/diff/2-3/

Comment 15

[Lee Bousfield]

Comment on attachment 8907875 [details]
Bug 1398539: Inhibit screensaver with XScreenSaverSuspend

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179552/diff/3-4/

Comment 16

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 15048ce8e0ec -d fa0f66eaa81f: rebasing 432202:15048ce8e0ec "Bug 1398539: Inhibit screensaver with XScreenSaverSuspend r=karlt" (tip)
merging widget/gtk/WakeLockListener.cpp
warning: conflicts while merging widget/gtk/WakeLockListener.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 17

[Lee Bousfield]

Comment on attachment 8907875 [details]
Bug 1398539: Inhibit screensaver with XScreenSaverSuspend

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179552/diff/4-5/

Comment 18

[Lee Bousfield]

Merge conflicts should be resolved.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/baff3e812914
Inhibit screensaver with XScreenSaverSuspend r=karlt

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/baff3e812914