fix: power observer dbus crash (electron#15030)

* fix: check dbus response for nullptr before using * chore: use BindOnce for ProxyObject::CallMethod cb * chore: comment out name of unused argument * fix: re-enable and fix linux power monitor tests * fix: change tyop from code comments * refactor: don't keep unnecessary dbus pointer * chore: remove the 'TODO: fix this' spec comment
daverodgman · Oct 9, 2018 · 05f4860 · 05f4860
1 parent 7df51ee
commit 05f4860
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 39 deletions.
diff --git a/atom/browser/lib/power_observer_linux.cc b/atom/browser/lib/power_observer_linux.cc
@@ -33,21 +33,28 @@ namespace atom {
 
 PowerObserverLinux::PowerObserverLinux()
     : lock_owner_name_(get_executable_basename()), weak_ptr_factory_(this) {
-  auto* dbus_thread_manager = bluez::DBusThreadManagerLinux::Get();
-  if (dbus_thread_manager) {
-    bus_ = dbus_thread_manager->GetSystemBus();
-    if (bus_) {
-      logind_ = bus_->GetObjectProxy(kLogindServiceName,
-                                     dbus::ObjectPath(kLogindObjectPath));
-      logind_->WaitForServiceToBeAvailable(
-          base::Bind(&PowerObserverLinux::OnLoginServiceAvailable,
-                     weak_ptr_factory_.GetWeakPtr()));
-    } else {
-      LOG(WARNING) << "Failed to get system bus connection";
-    }
-  } else {
-    LOG(WARNING) << "DBusThreadManagerLinux instance isn't available";
+  auto* bus = bluez::DBusThreadManagerLinux::Get()->GetSystemBus();
+  if (!bus) {
+    LOG(WARNING) << "Failed to get system bus connection";
+    return;
   }
+
+  // set up the logind proxy
+
+  const auto weakThis = weak_ptr_factory_.GetWeakPtr();
+
+  logind_ = bus->GetObjectProxy(kLogindServiceName,
+                                dbus::ObjectPath(kLogindObjectPath));
+  logind_->ConnectToSignal(
+      kLogindManagerInterface, "PrepareForShutdown",
+      base::BindRepeating(&PowerObserverLinux::OnPrepareForShutdown, weakThis),
+      base::BindRepeating(&PowerObserverLinux::OnSignalConnected, weakThis));
+  logind_->ConnectToSignal(
+      kLogindManagerInterface, "PrepareForSleep",
+      base::BindRepeating(&PowerObserverLinux::OnPrepareForSleep, weakThis),
+      base::BindRepeating(&PowerObserverLinux::OnSignalConnected, weakThis));
+  logind_->WaitForServiceToBeAvailable(base::BindRepeating(
+      &PowerObserverLinux::OnLoginServiceAvailable, weakThis));
 }
 
 PowerObserverLinux::~PowerObserverLinux() = default;
@@ -57,17 +64,6 @@ void PowerObserverLinux::OnLoginServiceAvailable(bool service_available) {
     LOG(WARNING) << kLogindServiceName << " not available";
     return;
   }
-  // Connect to PrepareForShutdown/PrepareForSleep signals
-  logind_->ConnectToSignal(kLogindManagerInterface, "PrepareForShutdown",
-                           base::Bind(&PowerObserverLinux::OnPrepareForShutdown,
-                                      weak_ptr_factory_.GetWeakPtr()),
-                           base::Bind(&PowerObserverLinux::OnSignalConnected,
-                                      weak_ptr_factory_.GetWeakPtr()));
-  logind_->ConnectToSignal(kLogindManagerInterface, "PrepareForSleep",
-                           base::Bind(&PowerObserverLinux::OnPrepareForSleep,
-                                      weak_ptr_factory_.GetWeakPtr()),
-                           base::Bind(&PowerObserverLinux::OnSignalConnected,
-                                      weak_ptr_factory_.GetWeakPtr()));
   // Take sleep inhibit lock
   BlockSleep();
 }
@@ -81,10 +77,10 @@ void PowerObserverLinux::BlockSleep() {
   inhibit_writer.AppendString(lock_owner_name_);                      // who
   inhibit_writer.AppendString("Application cleanup before suspend");  // why
   inhibit_writer.AppendString("delay");                               // mode
-  logind_->CallMethod(&sleep_inhibit_call,
-                      dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-                      base::Bind(&PowerObserverLinux::OnInhibitResponse,
-                                 weak_ptr_factory_.GetWeakPtr(), &sleep_lock_));
+  logind_->CallMethod(
+      &sleep_inhibit_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+      base::BindOnce(&PowerObserverLinux::OnInhibitResponse,
+                     weak_ptr_factory_.GetWeakPtr(), &sleep_lock_));
 }
 
 void PowerObserverLinux::UnblockSleep() {
@@ -104,8 +100,8 @@ void PowerObserverLinux::BlockShutdown() {
   inhibit_writer.AppendString("delay");                    // mode
   logind_->CallMethod(
       &shutdown_inhibit_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-      base::Bind(&PowerObserverLinux::OnInhibitResponse,
-                 weak_ptr_factory_.GetWeakPtr(), &shutdown_lock_));
+      base::BindOnce(&PowerObserverLinux::OnInhibitResponse,
+                     weak_ptr_factory_.GetWeakPtr(), &shutdown_lock_));
 }
 
 void PowerObserverLinux::UnblockShutdown() {
@@ -123,8 +119,10 @@ void PowerObserverLinux::SetShutdownHandler(base::Callback<bool()> handler) {
 
 void PowerObserverLinux::OnInhibitResponse(base::ScopedFD* scoped_fd,
                                            dbus::Response* response) {
-  dbus::MessageReader reader(response);
-  reader.PopFileDescriptor(scoped_fd);
+  if (response != nullptr) {
+    dbus::MessageReader reader(response);
+    reader.PopFileDescriptor(scoped_fd);
+  }
 }
 
 void PowerObserverLinux::OnPrepareForSleep(dbus::Signal* signal) {
@@ -159,7 +157,7 @@ void PowerObserverLinux::OnPrepareForShutdown(dbus::Signal* signal) {
   }
 }
 
-void PowerObserverLinux::OnSignalConnected(const std::string& interface,
+void PowerObserverLinux::OnSignalConnected(const std::string& /*interface*/,
                                            const std::string& signal,
                                            bool success) {
   LOG_IF(WARNING, !success) << "Failed to connect to " << signal;

diff --git a/atom/browser/lib/power_observer_linux.h b/atom/browser/lib/power_observer_linux.h
@@ -40,7 +40,6 @@ class PowerObserverLinux : public base::PowerObserver {
 
   base::Callback<bool()> should_shutdown_;
 
-  scoped_refptr<dbus::Bus> bus_;
   scoped_refptr<dbus::ObjectProxy> logind_;
   std::string lock_owner_name_;
   base::ScopedFD sleep_lock_;

diff --git a/spec/api-power-monitor-spec.js b/spec/api-power-monitor-spec.js
@@ -16,8 +16,7 @@ chai.use(dirtyChai)
 
 const skip = process.platform !== 'linux' || !process.env.DBUS_SYSTEM_BUS_ADDRESS
 
-// TODO(alexeykuzmin): [Ch66] Crashes on Linux ia32. Fix it and enable back.
-xdescribe('powerMonitor', () => {
+describe('powerMonitor', () => {
   let logindMock, dbusMockPowerMonitor, getCalls, emitSignal, reset
 
   if (!skip) {
@@ -31,7 +30,9 @@ xdescribe('powerMonitor', () => {
       reset = Promise.promisify(logindMock.Reset, { context: logindMock })
     })
 
-    after(reset)
+    after(async () => {
+      await reset()
+    })
   }
 
   (skip ? describe.skip : describe)('when powerMonitor module is loaded with dbus mock', () => {
@@ -129,8 +130,12 @@ xdescribe('powerMonitor', () => {
 
     describe('powerMonitor.querySystemIdleState', () => {
       it('notify current system idle state', done => {
+        // this function is not mocked out, so we can test the result's
+        // form and type but not its value.
         powerMonitor.querySystemIdleState(1, idleState => {
-          expect(idleState).to.be.true()
+          expect(idleState).to.be.a('string')
+          const validIdleStates = [ 'active', 'idle', 'locked', 'unknown' ]
+          expect(validIdleStates).to.include(idleState)
           done()
         })
       })