path: root/x11-ozone-fix-X11-screensaver-suspension.patch

From 8c1ebea5f601b0b5247535dcdfd01755f3e6e1a6 Mon Sep 17 00:00:00 2001
From: Andrew Wolfers <aswolfers@chromium.org>
Date: Tue, 19 Jul 2022 15:01:25 +0000
Subject: [PATCH] [x11][ozone] Fix X11 screensaver suspension.

X11 screensaver suspension was broken by https://crrev.com/c/3507472,
in which usage patterns were migrated to a non-stacking paradigm.

"Non-stacking" screensaver suspension describes an overriding behavior,
such that the last suspending or un-suspending call defines the current
state. Conversely, a "stacking" screensaver suspension paradigm allows
for parallel suspension, such that suspending calls are expected to be
matched by an equal number of un-suspending calls.

Documentation for `PlatformScreen::SetScreenSaverSuspended` (inherited
by `X11ScreenOzone`) explains that it should be used in a non-stacking
manner [1], which contradicts the child class's underlying
implementation [2].

> If XScreenSaverSuspend is called multiple times with suspend set to
> 'True', it must be called an equal number of times with suspend set
> to 'False' in order for the screensaver timer to be restarted.

This change updates the documentation/API of the `PlatformScreen`
parent class to correctly describe the stacking behavior of child class
`X11ScreenOzone`. This change also updates the implementation of
`WaylandScreen` to a stacking version. Lastly, this change updates the
call sites of `PlatformScreen` according to the API change.

[1] https://crsrc.org/c/ui/ozone/public/platform_screen.h;l=96
[2] https://linux.die.net/man/3/xscreensaverunsetattributes

Bug: b:193670013
Bug: b:196213351
Bug: 1329573
Bug: 1339361
Change-Id: I60975c8da9f86a0f26f3c32cf49c4a7eeeea6a12
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3759067
Commit-Queue: Andrew Wolfers <aswolfers@chromium.org>
Reviewed-by: Thomas Anderson <thomasanderson@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1025717}

(cherry picked from commit e61f08f8dbf1ec7cead427f3c497934e9d0db35f)
---
 ui/aura/screen_ozone.cc                       | 14 ++++++--
 ui/aura/screen_ozone.h                        | 29 ++++++++++++----
 ui/base/x/x11_util.h                          |  4 ++-
 ui/display/screen.cc                          | 21 ++----------
 ui/display/screen.h                           | 34 ++++++-------------
 .../platform/wayland/host/wayland_screen.cc   | 31 +++++++++++++++++
 .../platform/wayland/host/wayland_screen.h    | 30 +++++++++++++++-
 ui/ozone/platform/x11/x11_screen_ozone.cc     | 27 +++++++++++++--
 ui/ozone/platform/x11/x11_screen_ozone.h      | 19 ++++++++++-
 ui/ozone/public/platform_screen.cc            |  8 +++--
 ui/ozone/public/platform_screen.h             | 26 +++++++++++---
 11 files changed, 182 insertions(+), 61 deletions(-)

diff --git a/ui/aura/screen_ozone.cc b/ui/aura/screen_ozone.cc
index a78a6a48f1..09f62dc982 100644
--- a/ui/aura/screen_ozone.cc
+++ b/ui/aura/screen_ozone.cc
@@ -4,6 +4,8 @@
 
 #include "ui/aura/screen_ozone.h"
 
+#include <memory>
+
 #include "ui/aura/client/screen_position_client.h"
 #include "ui/aura/window.h"
 #include "ui/aura/window_tree_host.h"
@@ -108,8 +110,16 @@ display::Display ScreenOzone::GetPrimaryDisplay() const {
 }
 
 #if BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
-bool ScreenOzone::SetScreenSaverSuspended(bool suspend) {
-  return platform_screen_->SetScreenSaverSuspended(suspend);
+ScreenOzone::ScreenSaverSuspenderOzone::ScreenSaverSuspenderOzone(
+    std::unique_ptr<ui::PlatformScreen::PlatformScreenSaverSuspender> suspender)
+    : suspender_(std::move(suspender)) {}
+
+ScreenOzone::ScreenSaverSuspenderOzone::~ScreenSaverSuspenderOzone() = default;
+
+std::unique_ptr<display::Screen::ScreenSaverSuspender>
+ScreenOzone::SuspendScreenSaver() {
+  return std::make_unique<ScreenSaverSuspenderOzone>(
+      platform_screen_->SuspendScreenSaver());
 }
 #endif  // BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
 
diff --git a/ui/aura/screen_ozone.h b/ui/aura/screen_ozone.h
index 2970a0e0e7..d033abf366 100644
--- a/ui/aura/screen_ozone.h
+++ b/ui/aura/screen_ozone.h
@@ -11,10 +11,7 @@
 #include "build/chromeos_buildflags.h"
 #include "ui/aura/aura_export.h"
 #include "ui/display/screen.h"
-
-namespace ui {
-class PlatformScreen;
-}
+#include "ui/ozone/public/platform_screen.h"
 
 namespace aura {
 
@@ -48,6 +45,10 @@ class AURA_EXPORT ScreenOzone : public display::Screen {
   display::Display GetDisplayMatching(
       const gfx::Rect& match_rect) const override;
   display::Display GetPrimaryDisplay() const override;
+#if BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
+  std::unique_ptr<display::Screen::ScreenSaverSuspender> SuspendScreenSaver()
+      override;
+#endif  // BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
   bool IsScreenSaverActive() const override;
   base::TimeDelta CalculateIdleTime() const override;
   void AddObserver(display::DisplayObserver* observer) override;
@@ -65,11 +66,27 @@ class AURA_EXPORT ScreenOzone : public display::Screen {
  protected:
   ui::PlatformScreen* platform_screen() { return platform_screen_.get(); }
 
+ private:
 #if BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
-  bool SetScreenSaverSuspended(bool suspend) override;
+  class ScreenSaverSuspenderOzone
+      : public display::Screen::ScreenSaverSuspender {
+   public:
+    explicit ScreenSaverSuspenderOzone(
+        std::unique_ptr<ui::PlatformScreen::PlatformScreenSaverSuspender>
+            suspender);
+
+    ScreenSaverSuspenderOzone(const ScreenSaverSuspenderOzone&) = delete;
+    ScreenSaverSuspenderOzone& operator=(const ScreenSaverSuspenderOzone&) =
+        delete;
+
+    ~ScreenSaverSuspenderOzone() override;
+
+   private:
+    std::unique_ptr<ui::PlatformScreen::PlatformScreenSaverSuspender>
+        suspender_;
+  };
 #endif  // BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
 
- private:
   gfx::AcceleratedWidget GetAcceleratedWidgetForWindow(
       aura::Window* window) const;
 
diff --git a/ui/base/x/x11_util.h b/ui/base/x/x11_util.h
index bf36efe170..0692571582 100644
--- a/ui/base/x/x11_util.h
+++ b/ui/base/x/x11_util.h
@@ -337,7 +337,9 @@ COMPONENT_EXPORT(UI_BASE_X) bool IsCompositingManagerPresent();
 COMPONENT_EXPORT(UI_BASE_X) bool IsX11WindowFullScreen(x11::Window window);
 
 // Suspends or resumes the X screen saver, and returns whether the operation was
-// successful.  Must be called on the UI thread.
+// successful.  Must be called on the UI thread. If called multiple times with
+// |suspend| set to true, the screen saver is not un-suspended until this method
+// is called an equal number of times with |suspend| set to false.
 COMPONENT_EXPORT(UI_BASE_X) bool SuspendX11ScreenSaver(bool suspend);
 
 // Returns true if the window manager supports the given hint.
diff --git a/ui/display/screen.cc b/ui/display/screen.cc
index b9723889ce..70dc0a9f5c 100644
--- a/ui/display/screen.cc
+++ b/ui/display/screen.cc
@@ -85,26 +85,11 @@ void Screen::SetDisplayForNewWindows(int64_t display_id) {
 }
 
 #if BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
-std::unique_ptr<Screen::ScreenSaverSuspender> Screen::SuspendScreenSaver() {
-  SetScreenSaverSuspended(true);
-  screen_saver_suspension_count_++;
-  return base::WrapUnique(new Screen::ScreenSaverSuspender(this));
-}
-
-Screen::ScreenSaverSuspender::~ScreenSaverSuspender() {
-  // Check that this suspender still refers to the active screen. Particularly
-  // in tests, the screen might be destructed before the suspender.
-  if (screen_ == GetScreen()) {
-    screen_->screen_saver_suspension_count_--;
-    if (screen_->screen_saver_suspension_count_ == 0) {
-      screen_->SetScreenSaverSuspended(false);
-    }
-  }
-}
+Screen::ScreenSaverSuspender::~ScreenSaverSuspender() = default;
 
-bool Screen::SetScreenSaverSuspended(bool suspend) {
+std::unique_ptr<Screen::ScreenSaverSuspender> Screen::SuspendScreenSaver() {
   NOTIMPLEMENTED_LOG_ONCE();
-  return false;
+  return nullptr;
 }
 #endif  // BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
 
diff --git a/ui/display/screen.h b/ui/display/screen.h
index a86c5b63cc..d04534006f 100644
--- a/ui/display/screen.h
+++ b/ui/display/screen.h
@@ -133,28 +133,22 @@ class DISPLAY_EXPORT Screen {
   // its existence.
   class ScreenSaverSuspender {
    public:
-    ScreenSaverSuspender(const Screen&) = delete;
-    ScreenSaverSuspender& operator=(const Screen&) = delete;
+    ScreenSaverSuspender() = default;
 
-    // Notifies |screen_| that this instance is being destructed, and causes its
-    // platform-specific screensaver to be un-suspended if this is the last such
-    // remaining instance.
-    ~ScreenSaverSuspender();
+    ScreenSaverSuspender(const ScreenSaverSuspender&) = delete;
+    ScreenSaverSuspender& operator=(const ScreenSaverSuspender&) = delete;
 
-   private:
-    friend class Screen;
-
-    explicit ScreenSaverSuspender(Screen* screen) : screen_(screen) {}
-
-    Screen* screen_;
+    // Causes the platform-specific screensaver to be un-suspended iff this is
+    // the last remaining instance.
+    virtual ~ScreenSaverSuspender() = 0;
   };
 
   // Suspends the platform-specific screensaver until the returned
-  // |ScreenSaverSuspender| is destructed. This method allows stacking multiple
-  // overlapping calls, such that the platform-specific screensaver will not be
-  // un-suspended until all returned |SreenSaverSuspender| instances have been
-  // destructed.
-  std::unique_ptr<ScreenSaverSuspender> SuspendScreenSaver();
+  // |ScreenSaverSuspender| is destructed, or returns nullptr if suspension
+  // failed. This method allows stacking multiple overlapping calls, such that
+  // the platform-specific screensaver will not be un-suspended until all
+  // returned |ScreenSaverSuspender| instances have been destructed.
+  virtual std::unique_ptr<ScreenSaverSuspender> SuspendScreenSaver();
 #endif  // BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
 
   // Returns whether the screensaver is currently running.
@@ -200,12 +194,6 @@ class DISPLAY_EXPORT Screen {
       const gfx::GpuExtraInfo& gpu_extra_info);
 
  protected:
-#if BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
-  // Suspends or un-suspends the platform-specific screensaver, and returns
-  // whether the operation was successful.
-  virtual bool SetScreenSaverSuspended(bool suspend);
-#endif  // BUILDFLAG(IS_CHROMEOS_LACROS) || BUILDFLAG(IS_LINUX)
-
   void set_shutdown(bool shutdown) { shutdown_ = shutdown; }
 
  private:
diff --git a/ui/ozone/platform/wayland/host/wayland_screen.cc b/ui/ozone/platform/wayland/host/wayland_screen.cc
index 0c7dc5c02b..18cd81b472 100644
--- a/ui/ozone/platform/wayland/host/wayland_screen.cc
+++ b/ui/ozone/platform/wayland/host/wayland_screen.cc
@@ -327,6 +327,37 @@ display::Display WaylandScreen::GetDisplayMatching(
   return display_matching ? *display_matching : GetPrimaryDisplay();
 }
 
+std::unique_ptr<WaylandScreen::WaylandScreenSaverSuspender>
+WaylandScreen::WaylandScreenSaverSuspender::Create(WaylandScreen& screen) {
+  auto suspender = base::WrapUnique(new WaylandScreenSaverSuspender(screen));
+  if (suspender->is_suspending_) {
+    screen.screen_saver_suspension_count_++;
+    return suspender;
+  }
+
+  return nullptr;
+}
+
+WaylandScreen::WaylandScreenSaverSuspender::WaylandScreenSaverSuspender(
+    WaylandScreen& screen)
+    : screen_(screen.GetWeakPtr()) {
+  is_suspending_ = screen.SetScreenSaverSuspended(true);
+}
+
+WaylandScreen::WaylandScreenSaverSuspender::~WaylandScreenSaverSuspender() {
+  if (screen_ && is_suspending_) {
+    screen_->screen_saver_suspension_count_--;
+    if (screen_->screen_saver_suspension_count_ == 0) {
+      screen_->SetScreenSaverSuspended(false);
+    }
+  }
+}
+
+std::unique_ptr<PlatformScreen::PlatformScreenSaverSuspender>
+WaylandScreen::SuspendScreenSaver() {
+  return WaylandScreenSaverSuspender::Create(*this);
+}
+
 bool WaylandScreen::SetScreenSaverSuspended(bool suspend) {
   if (!connection_->zwp_idle_inhibit_manager())
     return false;
diff --git a/ui/ozone/platform/wayland/host/wayland_screen.h b/ui/ozone/platform/wayland/host/wayland_screen.h
index 87358f4f06..8e5515104a 100644
--- a/ui/ozone/platform/wayland/host/wayland_screen.h
+++ b/ui/ozone/platform/wayland/host/wayland_screen.h
@@ -68,7 +68,8 @@ class WaylandScreen : public PlatformScreen {
       const gfx::Point& point) const override;
   display::Display GetDisplayMatching(
       const gfx::Rect& match_rect) const override;
-  bool SetScreenSaverSuspended(bool suspend) override;
+  std::unique_ptr<PlatformScreen::PlatformScreenSaverSuspender>
+  SuspendScreenSaver() override;
   bool IsScreenSaverActive() const override;
   base::TimeDelta CalculateIdleTime() const override;
   void AddObserver(display::DisplayObserver* observer) override;
@@ -76,7 +77,33 @@ class WaylandScreen : public PlatformScreen {
   std::vector<base::Value> GetGpuExtraInfo(
       const gfx::GpuExtraInfo& gpu_extra_info) override;
 
+ protected:
+  // Suspends or un-suspends the platform-specific screensaver, and returns
+  // whether the operation was successful. Can be called more than once with the
+  // same value for |suspend|, but those states should not stack: the first
+  // alternating value should toggle the state of the suspend.
+  bool SetScreenSaverSuspended(bool suspend);
+
  private:
+  class WaylandScreenSaverSuspender
+      : public PlatformScreen::PlatformScreenSaverSuspender {
+   public:
+    WaylandScreenSaverSuspender(const WaylandScreenSaverSuspender&) = delete;
+    WaylandScreenSaverSuspender& operator=(const WaylandScreenSaverSuspender&) =
+        delete;
+
+    ~WaylandScreenSaverSuspender() override;
+
+    static std::unique_ptr<WaylandScreenSaverSuspender> Create(
+        WaylandScreen& screen);
+
+   private:
+    explicit WaylandScreenSaverSuspender(WaylandScreen& screen);
+
+    base::WeakPtr<WaylandScreen> screen_;
+    bool is_suspending_ = false;
+  };
+
   // All parameters are in DIP screen coordinates/units except |physical_size|,
   // which is in physical pixels.
   void AddOrUpdateDisplay(uint32_t output_id,
@@ -103,6 +130,7 @@ class WaylandScreen : public PlatformScreen {
 #endif
 
   wl::Object<zwp_idle_inhibitor_v1> idle_inhibitor_;
+  uint32_t screen_saver_suspension_count_ = 0;
 
   base::WeakPtrFactory<WaylandScreen> weak_factory_;
 };
diff --git a/ui/ozone/platform/x11/x11_screen_ozone.cc b/ui/ozone/platform/x11/x11_screen_ozone.cc
index 53265ab58a..b450df9c83 100644
--- a/ui/ozone/platform/x11/x11_screen_ozone.cc
+++ b/ui/ozone/platform/x11/x11_screen_ozone.cc
@@ -4,6 +4,8 @@
 
 #include "ui/ozone/platform/x11/x11_screen_ozone.h"
 
+#include <memory>
+
 #include "base/containers/flat_set.h"
 #include "ui/base/linux/linux_desktop.h"
 #include "ui/base/x/x11_idle_query.h"
@@ -131,8 +133,29 @@ display::Display X11ScreenOzone::GetDisplayMatching(
   return matching_display ? *matching_display : GetPrimaryDisplay();
 }
 
-bool X11ScreenOzone::SetScreenSaverSuspended(bool suspend) {
-  return SuspendX11ScreenSaver(suspend);
+X11ScreenOzone::X11ScreenSaverSuspender::X11ScreenSaverSuspender() {
+  is_suspending_ = SuspendX11ScreenSaver(true);
+}
+
+std::unique_ptr<X11ScreenOzone::X11ScreenSaverSuspender>
+X11ScreenOzone::X11ScreenSaverSuspender::Create() {
+  auto suspender = base::WrapUnique(new X11ScreenSaverSuspender());
+  if (suspender->is_suspending_) {
+    return suspender;
+  }
+
+  return nullptr;
+}
+
+X11ScreenOzone::X11ScreenSaverSuspender::~X11ScreenSaverSuspender() {
+  if (is_suspending_) {
+    SuspendX11ScreenSaver(false);
+  }
+}
+
+std::unique_ptr<PlatformScreen::PlatformScreenSaverSuspender>
+X11ScreenOzone::SuspendScreenSaver() {
+  return X11ScreenSaverSuspender::Create();
 }
 
 bool X11ScreenOzone::IsScreenSaverActive() const {
diff --git a/ui/ozone/platform/x11/x11_screen_ozone.h b/ui/ozone/platform/x11/x11_screen_ozone.h
index d86acae9aa..81e0fd13d8 100644
--- a/ui/ozone/platform/x11/x11_screen_ozone.h
+++ b/ui/ozone/platform/x11/x11_screen_ozone.h
@@ -50,7 +50,8 @@ class X11ScreenOzone : public PlatformScreen,
       const gfx::Point& point) const override;
   display::Display GetDisplayMatching(
       const gfx::Rect& match_rect) const override;
-  bool SetScreenSaverSuspended(bool suspend) override;
+  std::unique_ptr<PlatformScreen::PlatformScreenSaverSuspender>
+  SuspendScreenSaver() override;
   bool IsScreenSaverActive() const override;
   base::TimeDelta CalculateIdleTime() const override;
   void AddObserver(display::DisplayObserver* observer) override;
@@ -66,6 +67,22 @@ class X11ScreenOzone : public PlatformScreen,
  private:
   friend class X11ScreenOzoneTest;
 
+  class X11ScreenSaverSuspender
+      : public PlatformScreen::PlatformScreenSaverSuspender {
+   public:
+    X11ScreenSaverSuspender(const X11ScreenSaverSuspender&) = delete;
+    X11ScreenSaverSuspender& operator=(const X11ScreenSaverSuspender&) = delete;
+
+    ~X11ScreenSaverSuspender() override;
+
+    static std::unique_ptr<X11ScreenSaverSuspender> Create();
+
+   private:
+    X11ScreenSaverSuspender();
+
+    bool is_suspending_ = false;
+  };
+
   // Overridden from ui::XDisplayManager::Delegate:
   void OnXDisplayListUpdated() override;
   float GetXDisplayScaleFactor() const override;
diff --git a/ui/ozone/public/platform_screen.cc b/ui/ozone/public/platform_screen.cc
index 98f599aa41..2353208396 100644
--- a/ui/ozone/public/platform_screen.cc
+++ b/ui/ozone/public/platform_screen.cc
@@ -30,9 +30,13 @@ std::string PlatformScreen::GetCurrentWorkspace() {
   return {};
 }
 
-bool PlatformScreen::SetScreenSaverSuspended(bool suspend) {
+PlatformScreen::PlatformScreenSaverSuspender::~PlatformScreenSaverSuspender() =
+    default;
+
+std::unique_ptr<PlatformScreen::PlatformScreenSaverSuspender>
+PlatformScreen::SuspendScreenSaver() {
   NOTIMPLEMENTED_LOG_ONCE();
-  return false;
+  return nullptr;
 }
 
 bool PlatformScreen::IsScreenSaverActive() const {
diff --git a/ui/ozone/public/platform_screen.h b/ui/ozone/public/platform_screen.h
index 091220a99f..e4adfafce3 100644
--- a/ui/ozone/public/platform_screen.h
+++ b/ui/ozone/public/platform_screen.h
@@ -89,11 +89,27 @@ class COMPONENT_EXPORT(OZONE_BASE) PlatformScreen {
   virtual display::Display GetDisplayMatching(
       const gfx::Rect& match_rect) const = 0;
 
-  // Suspends or un-suspends the platform-specific screensaver, and returns
-  // whether the operation was successful. Can be called more than once with the
-  // same value for |suspend|, but those states should not stack: the first
-  // alternating value should toggle the state of the suspend.
-  virtual bool SetScreenSaverSuspended(bool suspend);
+  // Object which suspends the platform-specific screensaver for the duration of
+  // its existence.
+  class PlatformScreenSaverSuspender {
+   public:
+    PlatformScreenSaverSuspender() = default;
+
+    PlatformScreenSaverSuspender(const PlatformScreenSaverSuspender&) = delete;
+    PlatformScreenSaverSuspender& operator=(
+        const PlatformScreenSaverSuspender&) = delete;
+
+    // Causes the platform-specific screensaver to be un-suspended iff this is
+    // the last remaining instance.
+    virtual ~PlatformScreenSaverSuspender() = 0;
+  };
+
+  // Suspends the platform-specific screensaver until the returned
+  // |PlatformScreenSaverSuspender| is destructed, or returns nullptr if
+  // suspension failed. This method allows stacking multiple overlapping calls,
+  // such that the platform-specific screensaver will not be un-suspended until
+  // all returned |PlatformScreenSaverSuspender| instances have been destructed.
+  virtual std::unique_ptr<PlatformScreenSaverSuspender> SuspendScreenSaver();
 
   // Returns whether the screensaver is currently running.
   virtual bool IsScreenSaverActive() const;