Closed Bug 1398539 Opened 2 years ago Closed 2 years ago

WakeLockListener should disable DPMS if enabled on Linux

Categories

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

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: ljbousfield, Assigned: ljbousfield)

References

Details

Attachments

(1 file)

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.
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
Component: Untriaged → Widget: Gtk
Product: Firefox → Core
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.
Please see the patch in bug 1168090 and see whether finishing that would also address this situation.
See Also: → 1168090
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).
Attachment #8907875 - Flags: review?(karlt)
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).
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 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.
Attachment #8907875 - Flags: review?(karlt)
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?
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.
Priority: -- → P3
See Also: → 1406064
I've decided to keep the code in WakeLockListener (not nsIdleServiceGTK).
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.
Attachment #8907875 - Flags: review?(karlt) → review+
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)
Merge conflicts should be resolved.
Pushed by ktomlinson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/baff3e812914
Inhibit screensaver with XScreenSaverSuspend r=karlt
https://hg.mozilla.org/mozilla-central/rev/baff3e812914
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Duplicate of this bug: 1168090
Assignee: nobody → ljbousfield
You need to log in before you can comment on or make changes to this bug.