Disable rendering when suspending on chrome os

New chrome os devices are going to make more frequent use of the dark
resume state - a hybrid power state where several hardware devices are
left off to save power.  One of these is the display.  Unfortunately
this creates issues with chrome, which believes that the display is on
and attempts to render things to the screen.  These calls fail in the
kernel and trigger a logical memory leak in the error handling code in
the GPU process.

In order to make this interaction more robust, have chrome stop sending
rendering requests at suspend time and restart them at resume time.
There are a few things to watch out for here.  If the user has requested
that the screen be locked at suspend time then rendering cannot be
stopped until the screen locker is visible.  Otherwise the contents of
the user's screen will be visible at resume time, which is a security
issue.  Additionally, the suspend must be delayed until any pending
requests have been handled by the GPU process to prevent a race where
the system suspends before all requests have been processed and they end
up being processed when the system later enters dark resume, triggering
the memory leak mentioned in the previous paragraph.

BUG=chrome-os-partner:35699

Review URL: https://codereview.chromium.org/910393002

Cr-Commit-Position: refs/heads/master@{#316946}

12 files changed, 225 insertions, 22 deletions

diff --git a/ash/shell.h b/ash/shell.h
index 155d339..b37f284 100644
--- a/ash/shell.h
+++ b/ash/shell.h
@@ -380,6 +380,9 @@ class ASH_EXPORT Shell : public SystemModalContainerEventFilterDelegate,
     return display_controller_.get();
   }
 #if defined(OS_CHROMEOS)
+  PowerEventObserver* power_event_observer() {
+    return power_event_observer_.get();
+  }
   TouchTransformerController* touch_transformer_controller() {
     return touch_transformer_controller_.get();
   }
diff --git a/ash/system/chromeos/power/power_event_observer.cc b/ash/system/chromeos/power/power_event_observer.cc
index 65567f4..1510ddd 100644
--- a/ash/system/chromeos/power/power_event_observer.cc
+++ b/ash/system/chromeos/power/power_event_observer.cc
@@ -10,13 +10,37 @@
 #include "ash/wm/power_button_controller.h"
 #include "base/prefs/pref_service.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
+#include "ui/aura/window.h"
+#include "ui/aura/window_tree_host.h"
 #include "ui/base/user_activity/user_activity_detector.h"
+#include "ui/compositor/compositor.h"
 #include "ui/display/chromeos/display_configurator.h"
 
 namespace ash {
 
+namespace {
+
+// Tells the compositor for each of the displays to finish all pending
+// rendering requests and block any new ones.
+void StopRenderingRequests() {
+  for (aura::Window* window : Shell::GetAllRootWindows()) {
+    ui::Compositor* compositor = window->GetHost()->compositor();
+    compositor->SetVisible(false);
+    compositor->FinishAllRendering();
+  }
+}
+
+// Tells the compositor for each of the displays to resume sending rendering
+// requests to the GPU.
+void ResumeRenderingRequests() {
+  for (aura::Window* window : Shell::GetAllRootWindows())
+    window->GetHost()->compositor()->SetVisible(true);
+}
+
+}  // namespace
+
 PowerEventObserver::PowerEventObserver()
-    : screen_locked_(false) {
+    : screen_locked_(false), waiting_for_lock_screen_animations_(false) {
   chromeos::DBusThreadManager::Get()->GetPowerManagerClient()->
       AddObserver(this);
   chromeos::DBusThreadManager::Get()->GetSessionManagerClient()->
@@ -30,6 +54,18 @@ PowerEventObserver::~PowerEventObserver() {
       RemoveObserver(this);
 }
 
+void PowerEventObserver::OnLockAnimationsComplete() {
+  VLOG(1) << "Screen locker animations have completed.";
+  waiting_for_lock_screen_animations_ = false;
+
+  if (!screen_lock_callback_.is_null()) {
+    StopRenderingRequests();
+
+    screen_lock_callback_.Run();
+    screen_lock_callback_.Reset();
+  }
+}
+
 void PowerEventObserver::BrightnessChanged(int level, bool user_initiated) {
   Shell::GetInstance()->power_button_controller()->OnScreenBrightnessChanged(
       static_cast<double>(level));
@@ -39,8 +75,19 @@ void PowerEventObserver::SuspendImminent() {
   Shell* shell = Shell::GetInstance();
   SessionStateDelegate* delegate = shell->session_state_delegate();
 
-  // If the lock-before-suspending pref is set, get a callback to block
-  // suspend and ask the session manager to lock the screen.
+  // This class is responsible for disabling all rendering requests at suspend
+  // time and then enabling them at resume time.  When the
+  // lock-before-suspending pref is not set this is easy to do since
+  // StopRenderingRequests() is just called directly from this function.  If the
+  // lock-before-suspending pref _is_ set, then the suspend needs to be delayed
+  // until the lock screen is fully visible.  While it is sufficient from a
+  // security perspective to block only until the lock screen is ready, which
+  // guarantees that the contents of the user's screen are no longer visible,
+  // this leads to poor UX on the first resume since neither the user pod nor
+  // the header bar will be visible for a few hundred milliseconds until the GPU
+  // process starts rendering again.  To deal with this, the suspend is delayed
+  // until all the lock screen animations have completed and the suspend request
+  // is unblocked from OnLockAnimationsComplete().
   if (!screen_locked_ && delegate->ShouldLockScreenBeforeSuspending() &&
       delegate->CanLockScreen()) {
     screen_lock_callback_ = chromeos::DBusThreadManager::Get()->
@@ -48,6 +95,21 @@ void PowerEventObserver::SuspendImminent() {
     VLOG(1) << "Requesting screen lock from PowerEventObserver";
     chromeos::DBusThreadManager::Get()->GetSessionManagerClient()->
         RequestLockScreen();
+  } else if (waiting_for_lock_screen_animations_) {
+    // The lock-before-suspending pref has been set and the lock screen is ready
+    // but the animations have not completed yet.  This can happen if a suspend
+    // request is canceled after the lock screen is ready but before the
+    // animations have completed and then another suspend request is immediately
+    // started.  In practice, it is highly unlikely that this will ever happen
+    // but it's better to be safe since the cost of not dealing with it properly
+    // is a memory leak in the GPU and weird artifacts on the screen.
+    screen_lock_callback_ = chromeos::DBusThreadManager::Get()
+                                ->GetPowerManagerClient()
+                                ->GetSuspendReadinessCallback();
+  } else {
+    // The lock-before-suspending pref is not set or the screen has already been
+    // locked and the animations have completed.  Rendering can be stopped now.
+    StopRenderingRequests();
   }
 
   ui::UserActivityDetector::Get()->OnDisplayPowerChanging();
@@ -57,19 +119,24 @@ void PowerEventObserver::SuspendImminent() {
 void PowerEventObserver::SuspendDone(const base::TimeDelta& sleep_duration) {
   Shell::GetInstance()->display_configurator()->ResumeDisplays();
   Shell::GetInstance()->system_tray_notifier()->NotifyRefreshClock();
+
+  // If the suspend request was being blocked while waiting for the lock
+  // animation to complete, clear the blocker since the suspend has already
+  // completed.  This prevents rendering requests from being blocked after a
+  // resume if the lock screen took too long to show.
+  screen_lock_callback_.Reset();
+
+  ResumeRenderingRequests();
 }
 
 void PowerEventObserver::ScreenIsLocked() {
   screen_locked_ = true;
+  waiting_for_lock_screen_animations_ = true;
 
-  // Stop blocking suspend after the screen is locked.
+  // The screen is now locked but the pending suspend, if any, will be blocked
+  // until all the animations have completed.
   if (!screen_lock_callback_.is_null()) {
     VLOG(1) << "Screen locked due to suspend";
-    // Run the callback asynchronously.  ScreenIsLocked() is currently
-    // called asynchronously after RequestLockScreen(), but this guards
-    // against it being made synchronous later.
-    base::MessageLoop::current()->PostTask(FROM_HERE, screen_lock_callback_);
-    screen_lock_callback_.Reset();
   } else {
     VLOG(1) << "Screen locked without suspend";
   }
diff --git a/ash/system/chromeos/power/power_event_observer.h b/ash/system/chromeos/power/power_event_observer.h
index 941e234..37fe5bb 100644
--- a/ash/system/chromeos/power/power_event_observer.h
+++ b/ash/system/chromeos/power/power_event_observer.h
@@ -23,6 +23,12 @@ class ASH_EXPORT PowerEventObserver
   PowerEventObserver();
   ~PowerEventObserver() override;
 
+  // Called by the WebUIScreenLocker when all the lock screen animations have
+  // completed.  This really should be implemented via an observer but since
+  // ash/ isn't allowed to depend on chrome/ we need to have the
+  // WebUIScreenLocker reach into ash::Shell to make this call.
+  void OnLockAnimationsComplete();
+
   // chromeos::PowerManagerClient::Observer overrides:
   void BrightnessChanged(int level, bool user_initiated) override;
   void SuspendImminent() override;
@@ -35,8 +41,11 @@ class ASH_EXPORT PowerEventObserver
   // Is the screen currently locked?
   bool screen_locked_;
 
-  // If set, called when the lock screen has been shown to confirm that the
-  // system is ready to be suspended.
+  // Have the lock screen animations completed?
+  bool waiting_for_lock_screen_animations_;
+
+  // If set, called when the lock screen animations have completed to confirm
+  // that the system is ready to be suspended.
   base::Closure screen_lock_callback_;
 
  private:
diff --git a/ash/system/chromeos/power/power_event_observer_unittest.cc b/ash/system/chromeos/power/power_event_observer_unittest.cc
index aaaf2c7..2fd1a1f 100644
--- a/ash/system/chromeos/power/power_event_observer_unittest.cc
+++ b/ash/system/chromeos/power/power_event_observer_unittest.cc
@@ -4,11 +4,15 @@
 
 #include "ash/system/chromeos/power/power_event_observer.h"
 
+#include "ash/shell.h"
 #include "ash/test/ash_test_base.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/time/time.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/power_manager_client.h"
+#include "ui/aura/window.h"
+#include "ui/aura/window_tree_host.h"
+#include "ui/compositor/compositor.h"
 
 namespace ash {
 
@@ -29,6 +33,16 @@ class PowerEventObserverTest : public test::AshTestBase {
   }
 
  protected:
+  int GetNumVisibleCompositors() {
+    int result = 0;
+    for (const auto& window : Shell::GetAllRootWindows()) {
+      if (window->GetHost()->compositor()->IsVisible())
+        ++result;
+    }
+
+    return result;
+  }
+
   scoped_ptr<PowerEventObserver> observer_;
 
  private:
@@ -47,15 +61,17 @@ TEST_F(PowerEventObserverTest, LockBeforeSuspend) {
   observer_->SuspendImminent();
   EXPECT_EQ(1, client->GetNumPendingSuspendReadinessCallbacks());
 
-  // It should run the callback when it hears that the screen is locked.
+  // It should run the callback when it hears that the screen is locked and the
+  // lock screen animations have completed.
   observer_->ScreenIsLocked();
-  RunAllPendingInMessageLoop();
+  observer_->OnLockAnimationsComplete();
   EXPECT_EQ(0, client->GetNumPendingSuspendReadinessCallbacks());
 
   // If the system is already locked, no callback should be requested.
   observer_->SuspendDone(base::TimeDelta());
   observer_->ScreenIsUnlocked();
   observer_->ScreenIsLocked();
+  observer_->OnLockAnimationsComplete();
   observer_->SuspendImminent();
   EXPECT_EQ(0, client->GetNumPendingSuspendReadinessCallbacks());
 
@@ -67,4 +83,79 @@ TEST_F(PowerEventObserverTest, LockBeforeSuspend) {
   EXPECT_EQ(0, client->GetNumPendingSuspendReadinessCallbacks());
 }
 
+TEST_F(PowerEventObserverTest, SetInvisibleBeforeSuspend) {
+  // Tests that all the Compositors are marked invisible before a suspend
+  // request when the screen is not supposed to be locked before a suspend.
+  EXPECT_EQ(1, GetNumVisibleCompositors());
+
+  observer_->SuspendImminent();
+  EXPECT_EQ(0, GetNumVisibleCompositors());
+  observer_->SuspendDone(base::TimeDelta());
+
+  // Tests that all the Compositors are marked invisible _after_ the screen lock
+  // animations have completed.
+  SetCanLockScreen(true);
+  SetShouldLockScreenBeforeSuspending(true);
+
+  observer_->SuspendImminent();
+  EXPECT_EQ(1, GetNumVisibleCompositors());
+
+  observer_->ScreenIsLocked();
+  EXPECT_EQ(1, GetNumVisibleCompositors());
+
+  observer_->OnLockAnimationsComplete();
+  EXPECT_EQ(0, GetNumVisibleCompositors());
+
+  observer_->SuspendDone(base::TimeDelta());
+  EXPECT_EQ(1, GetNumVisibleCompositors());
+}
+
+TEST_F(PowerEventObserverTest, CanceledSuspend) {
+  // Tests that the Compositors are not marked invisible if a suspend is
+  // canceled or the system resumes before the lock screen is ready.
+  SetCanLockScreen(true);
+  SetShouldLockScreenBeforeSuspending(true);
+  observer_->SuspendImminent();
+  EXPECT_EQ(1, GetNumVisibleCompositors());
+
+  observer_->SuspendDone(base::TimeDelta());
+  observer_->ScreenIsLocked();
+  observer_->OnLockAnimationsComplete();
+  EXPECT_EQ(1, GetNumVisibleCompositors());
+}
+
+TEST_F(PowerEventObserverTest, DelayResuspendForLockAnimations) {
+  // Tests that the following order of events is handled correctly:
+  //
+  // - A suspend request is started.
+  // - The screen is locked.
+  // - The suspend request is canceled.
+  // - Another suspend request is started.
+  // - The screen lock animations complete.
+  //
+  // In this case, the observer should block the second suspend request until
+  // the animations have completed.
+  SetCanLockScreen(true);
+  SetShouldLockScreenBeforeSuspending(true);
+
+  chromeos::PowerManagerClient* client =
+      chromeos::DBusThreadManager::Get()->GetPowerManagerClient();
+  observer_->SuspendImminent();
+  EXPECT_EQ(1, client->GetNumPendingSuspendReadinessCallbacks());
+
+  observer_->ScreenIsLocked();
+  observer_->SuspendDone(base::TimeDelta());
+  observer_->SuspendImminent();
+
+  // The expected number of suspend readiness callbacks is 2 because the
+  // observer has not run the callback that it got from the first suspend
+  // request.  The real PowerManagerClient would reset its internal counter in
+  // this situation but the stub client is not that smart.
+  EXPECT_EQ(2, client->GetNumPendingSuspendReadinessCallbacks());
+
+  observer_->OnLockAnimationsComplete();
+  EXPECT_EQ(1, client->GetNumPendingSuspendReadinessCallbacks());
+  EXPECT_EQ(0, GetNumVisibleCompositors());
+}
+
 }  // namespace ash
diff --git a/chrome/browser/chromeos/login/lock/screen_locker_delegate.h b/chrome/browser/chromeos/login/lock/screen_locker_delegate.h
index 3227a36..8ddd8bc 100644
--- a/chrome/browser/chromeos/login/lock/screen_locker_delegate.h
+++ b/chrome/browser/chromeos/login/lock/screen_locker_delegate.h
@@ -72,6 +72,9 @@ class ScreenLockerDelegate {
   // Called when webui lock screen wallpaper is loaded and displayed.
   virtual void OnLockBackgroundDisplayed() = 0;
 
+  // Called when the webui header bar becomes visible.
+  virtual void OnHeaderBarVisible() = 0;
+
   // Returns screen locker associated with delegate.
   ScreenLocker* screen_locker() { return screen_locker_; }
 
diff --git a/chrome/browser/chromeos/login/lock/webui_screen_locker.cc b/chrome/browser/chromeos/login/lock/webui_screen_locker.cc
index 0b558f7..08cdae9 100644
--- a/chrome/browser/chromeos/login/lock/webui_screen_locker.cc
+++ b/chrome/browser/chromeos/login/lock/webui_screen_locker.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/chromeos/login/lock/webui_screen_locker.h"
 
+#include "ash/shell.h"
+#include "ash/system/chromeos/power/power_event_observer.h"
 #include "ash/wm/lock_state_controller.h"
 #include "ash/wm/lock_state_observer.h"
 #include "base/command_line.h"
@@ -203,6 +205,12 @@ void WebUIScreenLocker::OnLockBackgroundDisplayed() {
                       base::TimeTicks::Now() - lock_time_);
 }
 
+void WebUIScreenLocker::OnHeaderBarVisible() {
+  DCHECK(ash::Shell::HasInstance());
+
+  ash::Shell::GetInstance()->power_event_observer()->OnLockAnimationsComplete();
+}
+
 OobeUI* WebUIScreenLocker::GetOobeUI() {
   return static_cast<OobeUI*>(GetWebUI()->GetController());
 }
diff --git a/chrome/browser/chromeos/login/lock/webui_screen_locker.h b/chrome/browser/chromeos/login/lock/webui_screen_locker.h
index 0be77ed..c3c1878 100644
--- a/chrome/browser/chromeos/login/lock/webui_screen_locker.h
+++ b/chrome/browser/chromeos/login/lock/webui_screen_locker.h
@@ -71,6 +71,7 @@ class WebUIScreenLocker : public WebUILoginView,
   content::WebUI* GetAssociatedWebUI() override;
   void OnLockWebUIReady() override;
   void OnLockBackgroundDisplayed() override;
+  void OnHeaderBarVisible() override;
 
   // LoginDisplay::Delegate:
   void CancelPasswordChangedFlow() override;
diff --git a/chrome/browser/resources/chromeos/login/header_bar.js b/chrome/browser/resources/chromeos/login/header_bar.js
index 3a3ba9b..d1d9781 100644
--- a/chrome/browser/resources/chromeos/login/header_bar.js
+++ b/chrome/browser/resources/chromeos/login/header_bar.js
@@ -316,11 +316,13 @@ cr.define('login', function() {
     },
 
     /**
-     * Animates Header bar to slowly appear on the screen.
+     * Animates Header bar to appear on the screen.
      *
+     * @param {boolean} fast Whether the animation should complete quickly or
+     *     slowly.
      * @param {function()} callback will be called once animation is finished.
      */
-    animateIn: function(callback) {
+    animateIn: function(fast, callback) {
       if (callback) {
         var launcher = this;
         launcher.addEventListener(
@@ -332,7 +334,7 @@ cr.define('login', function() {
         ensureTransitionEndEvent(launcher, 2250);
       }
 
-      if (Oobe.getInstance().displayType == DISPLAY_TYPE.OOBE) {
+      if (fast) {
         this.classList.remove('login-header-bar-animate-slow');
         this.classList.add('login-header-bar-animate-fast');
       } else {
@@ -354,8 +356,8 @@ cr.define('login', function() {
   /**
    * Convenience wrapper of animateIn.
    */
-  HeaderBar.animateIn = function(callback) {
-    $('login-header-bar').animateIn(callback);
+  HeaderBar.animateIn = function(fast, callback) {
+    $('login-header-bar').animateIn(fast, callback);
   };
 
   return {
diff --git a/chrome/browser/resources/chromeos/login/login_common.js b/chrome/browser/resources/chromeos/login/login_common.js
index 157c6c4..6f2bd4b 100644
--- a/chrome/browser/resources/chromeos/login/login_common.js
+++ b/chrome/browser/resources/chromeos/login/login_common.js
@@ -95,7 +95,7 @@ cr.define('cr.ui', function() {
 
       // Callback to animate the header bar in.
       var showHeaderBar = function() {
-        login.HeaderBar.animateIn(function() {
+        login.HeaderBar.animateIn(false, function() {
           chrome.send('headerBarVisible');
         });
       };
@@ -106,7 +106,7 @@ cr.define('cr.ui', function() {
       Oobe.getInstance().prepareForLoginDisplay_();
       // Ensure header bar is visible when switching to Login UI from oobe.
       if (Oobe.getInstance().displayType == DISPLAY_TYPE.OOBE)
-        login.HeaderBar.animateIn();
+        login.HeaderBar.animateIn(true);
     }
 
     Oobe.getInstance().headerHidden = false;
@@ -211,7 +211,9 @@ cr.define('cr.ui', function() {
    * Displays animations that have to happen once login UI is fully displayed.
    */
   Oobe.animateOnceFullyDisplayed = function() {
-    login.HeaderBar.animateIn();
+    login.HeaderBar.animateIn(true, function() {
+      chrome.send('headerBarVisible');
+    });
   };
 
   /**
diff --git a/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc b/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc
index 7944878..e55846d 100644
--- a/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc
+++ b/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc
@@ -11,6 +11,7 @@
 #include "chrome/browser/chromeos/accessibility/accessibility_manager.h"
 #include "chrome/browser/chromeos/accessibility/magnification_manager.h"
 #include "chrome/browser/chromeos/login/helper.h"
+#include "chrome/browser/chromeos/login/lock/screen_locker.h"
 #include "chrome/browser/chromeos/login/startup_utils.h"
 #include "chrome/browser/chromeos/login/ui/login_display_host_impl.h"
 #include "chrome/browser/chromeos/login/wizard_controller.h"
@@ -418,6 +419,8 @@ void CoreOobeHandler::HandleHeaderBarVisible() {
   LoginDisplayHost* login_display_host = LoginDisplayHostImpl::default_host();
   if (login_display_host)
     login_display_host->SetStatusAreaVisible(true);
+  if (ScreenLocker::default_screen_locker())
+    ScreenLocker::default_screen_locker()->delegate()->OnHeaderBarVisible();
 }
 
 void CoreOobeHandler::HandleSwitchToNewOobe() {
diff --git a/ui/compositor/compositor.cc b/ui/compositor/compositor.cc
index ea04b8c..5f2a43b 100644
--- a/ui/compositor/compositor.cc
+++ b/ui/compositor/compositor.cc
@@ -224,6 +224,10 @@ void Compositor::ScheduleRedrawRect(const gfx::Rect& damage_rect) {
   host_->SetNeedsCommit();
 }
 
+void Compositor::FinishAllRendering() {
+  host_->FinishAllRendering();
+}
+
 void Compositor::DisableSwapUntilResize() {
   host_->FinishAllRendering();
   context_factory_->ResizeDisplay(this, gfx::Size());
@@ -260,6 +264,10 @@ void Compositor::SetVisible(bool visible) {
   host_->SetVisible(visible);
 }
 
+bool Compositor::IsVisible() {
+  return host_->visible();
+}
+
 scoped_refptr<CompositorVSyncManager> Compositor::vsync_manager() const {
   return vsync_manager_;
 }
diff --git a/ui/compositor/compositor.h b/ui/compositor/compositor.h
index 1897cc3..3c99a16 100644
--- a/ui/compositor/compositor.h
+++ b/ui/compositor/compositor.h
@@ -182,6 +182,9 @@ class COMPOSITOR_EXPORT Compositor
   // from changes to layer properties.
   void ScheduleRedrawRect(const gfx::Rect& damage_rect);
 
+  // Finishes all outstanding rendering and disables swapping on this surface.
+  void FinishAllRendering();
+
   // Finishes all outstanding rendering and disables swapping on this surface
   // until it is resized.
   void DisableSwapUntilResize();
@@ -198,9 +201,12 @@ class COMPOSITOR_EXPORT Compositor
   // the |root_layer|.
   void SetBackgroundColor(SkColor color);
 
-  // Set the visibility of the underlying compositor.
+  // Sets the visibility of the underlying compositor.
   void SetVisible(bool visible);
 
+  // Gets the visibility of the underlying compositor.
+  bool IsVisible();
+
   // Returns the widget for this compositor.
   gfx::AcceleratedWidget widget() const { return widget_; }