shill: Keep registered suspend delay after termination action completes

shill previously registered a suspend delay with power manager only when
there was a termination action to perform before suspend. Upon the
completion of the termination action, shill unregistered the suspend
delay before reporting the suspend readiness to power manager. However,
without the suspend delay, the corresponding SuspendDone callback
wouldn't be invoked after the completion of the suspend attempt. If a
cellular device was disabled by the termination action, it wouldn't be
re-enabled by the SuspendDone callback.

This CL addresses the issue by keeping the registered suspend delay
after the termination action completes and only unregisters the suspend
delay when the power manager is destructed.

BUG=chromium:371585
TEST=Tested the following:
1. Build and run unit tests.
2. Test on a daisy_spring device with an ALT3100 modem:
   a. Connect the modem to a cellular network. Make sure the cellular
      service is set to auto-connect.
   b. Run `powerd_dbus_suspend` under a root shell to suspend the device.
   c. Press a key to wake up the device.
   d. Verify that shill re-enables the modem and reconnects it to the
      network.
   e. Restart powerd and verify that shill reregisters the suspend
      delay.
   f. Repeat (a) to (d).

Change-Id: I6651523f77bbfd918f40a6cc7a3702a4f3e6d2ee
Reviewed-on: https://chromium-review.googlesource.com/199810
Reviewed-by: Daniel Erat <derat@chromium.org>
Commit-Queue: Ben Chan <benchan@chromium.org>
Tested-by: Ben Chan <benchan@chromium.org>

manager.cc

diff --git a/manager.cc b/manager.cc
index cd1e072..022d3d5 100644
--- a/manager.cc
+++ b/manager.cc
@@ -1063,6 +1063,11 @@
                                    const base::Closure &start) {
   if (termination_actions_.IsEmpty() && power_manager_.get() &&
       !suspend_delay_registered_) {
+    // TODO(benchan): We can probably combine PowerManager::Start() and
+    // PowerManager::AddSuspendDelay() after simplifying PowerManager to support
+    // only one suspend delay. As PowerManager observes the presence of powerd,
+    // it's also more robust to retry the suspend delay registration via
+    // PowerManager::OnPowerManagerAppeared (crbug.com/373348).
     suspend_delay_registered_ = power_manager_->AddSuspendDelay(
         kPowerManagerKey,
         kSuspendDelayDescription,
@@ -1081,17 +1086,7 @@
 
 void Manager::RemoveTerminationAction(const string &name) {
   SLOG(Manager, 2) << __func__;
-  if (termination_actions_.IsEmpty()) {
-    return;
-  }
   termination_actions_.Remove(name);
-  if (termination_actions_.IsEmpty() && power_manager_.get() &&
-      suspend_delay_registered_) {
-    SLOG(Manager, 2) << "Unregistering suspend delay.";
-    if (power_manager_->RemoveSuspendDelay(kPowerManagerKey)) {
-      suspend_delay_registered_ = false;
-    }
-  }
 }
 
 void Manager::RunTerminationActions(

manager_unittest.cc

diff --git a/manager_unittest.cc b/manager_unittest.cc
index 9c44842..b961956 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -3062,8 +3062,7 @@
   MockPowerManager &power_manager = *power_manager_;
   SetPowerManager();
 
-  // Removing an action when the hook table is empty should not result in any
-  // calls to the power manager.
+  // Removing an action when the hook table is empty.
   EXPECT_CALL(power_manager, RemoveSuspendDelay(_)).Times(0);
   EXPECT_TRUE(GetTerminationActions()->IsEmpty());
   manager()->RemoveTerminationAction("unknown");
@@ -3076,17 +3075,14 @@
   manager()->AddTerminationAction(kKey2, base::Closure());
   Mock::VerifyAndClearExpectations(&power_manager);
 
-  // Removing an action that ends up with a non-empty hook table should not
-  // result in any calls to the power manager.
+  // Removing an action that ends up with a non-empty hook table.
   EXPECT_CALL(power_manager, RemoveSuspendDelay(_)).Times(0);
   manager()->RemoveTerminationAction(kKey1);
   EXPECT_FALSE(GetTerminationActions()->IsEmpty());
   Mock::VerifyAndClearExpectations(&power_manager);
 
-  // Removing the last action should trigger unregistering from the power
-  // manager.
-  EXPECT_CALL(power_manager, RemoveSuspendDelay(_))
-      .WillOnce(Return(true));
+  // Removing the last action.
+  EXPECT_CALL(power_manager, RemoveSuspendDelay(_)).Times(0);
   manager()->RemoveTerminationAction(kKey2);
   EXPECT_TRUE(GetTerminationActions()->IsEmpty());
 }

power_manager.cc

diff --git a/power_manager.cc b/power_manager.cc
index 6b55dc2..87e4916 100644
--- a/power_manager.cc
+++ b/power_manager.cc
@@ -34,6 +34,10 @@
       suspending_(false) {}
 
 PowerManager::~PowerManager() {
+  for (const auto &delay_entry : suspend_delays_) {
+    ignore_result(power_manager_proxy_->UnregisterSuspendDelay(
+        delay_entry.second.delay_id));
+  }
 }
 
 void PowerManager::Start(DBusManager *dbus_manager) {

power_manager.h

diff --git a/power_manager.h b/power_manager.h
index cae4c19..e991c9b 100644
--- a/power_manager.h
+++ b/power_manager.h
@@ -25,6 +25,9 @@
 class EventDispatcher;
 class ProxyFactory;
 
+// TODO(benchan): The current implementation supports multiple suspend delays,
+// which seems unnecessary as shill only registers one. We should simplify the
+// implementation (crbug.com/373348).
 class PowerManager : public PowerManagerProxyDelegate {
  public:
   // This callback is called prior to a suspend attempt.  When it is OK for the
@@ -43,8 +46,7 @@
   // |proxy_factory| creates the PowerManagerProxy.  Usually this is
   // ProxyFactory::GetInstance().  Use a fake for testing.
   // Note: |Start| should be called to initialize this object before using it.
-  PowerManager(EventDispatcher *dispatcher,
-               ProxyFactory *proxy_factory);
+  PowerManager(EventDispatcher *dispatcher, ProxyFactory *proxy_factory);
   virtual ~PowerManager();
 
   bool suspending() const { return suspending_; }