Introduce two-phase shutdown of PowerMonitorSource

PowerMonitorBroadcastSource suffers from raciness in that it can
procress incoming Mojo messages on one thread while both it and the
process-wide PowerMonitor instance are being destroyed on another
thread. A previous CL introduced a mitigation for this problem by
shutting down the processing of incoming Mojo messages in the
PowerMonitorBroadcastSource destructor before proceeding with
destruction (https://chromium-review.googlesource.com/1041215). However,
that turns out not to be sufficient to eliminate the race:
the PowerMonitorBroadcastSource instance can end up waiting for the
lock in its destructor while on another thread it is holding the lock
to process an incoming Mojo message. That second thread can then call
back into the process-wide PowerMonitor instance while it is being
torn down, resulting in crashes due to undefined state (specifically,
the PowerMonitor instance has already set its PowerMonitorSource
instance to nullptr, resulting in a crash when that instance is being
accessed -- ironically, the very instance that is currently blocked
in its destructor waiting for the processing of the incoming Mojo
message to finish :).

This CL removes the possibility of that race on shutdown by introducing
a two-phase shutdown process for PowerMonitorSource. In PowerMonitor's
destructor, it first invokes PowerMonitorSource::Shutdown() before
proceeding. The contract of PowerMonitorSource::Shutdown() is that
subclasses must take any necessary action to ensure that on return
from that method, they will no longer call into PowerMonitor.
PowerMonitorBroadcastSource fulfills this contract by stopping the
process of incoming Mojo messages in
PowerMonitorBroadcastSource::Shutdown(). In the above scenario,
the broadcast source will end up waiting for the processing of the
incoming Mojo message to finish *inside*
PowerMonitorBroadcastSource::Shutdown(), which is safe by design as it
is before PowerMonitor has done any teardown.

Note that it is possible that there are still other kinds of
raciness between PowerMonitorBroadcastSource and PowerMonitor; as I
expressed on
https://bugs.chromium.org/p/chromium/issues/detail?id=834312#c33, I am
not confident about the overall threading model here.

TBR=xjz@chromium.org, mmenke@chromium.org

Bug: 809031
Change-Id: I9d0f291995b8185e16dff1f88564b6e5344439b2
Reviewed-on: https://chromium-review.googlesource.com/1098932Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567302}

base/power_monitor/power_monitor.cc

@@ -20,6 +20,7 @@ PowerMonitor::PowerMonitor(std::unique_ptr<PowerMonitorSource> source)
 }
 
 PowerMonitor::~PowerMonitor() {
+  source_->Shutdown();
   DCHECK_EQ(this, g_power_monitor);
   g_power_monitor = nullptr;
 }

base/power_monitor/power_monitor_device_source.cc

@@ -25,4 +25,9 @@ PowerMonitorDeviceSource::~PowerMonitorDeviceSource() {
 #endif
 }
 
+// PowerMonitorDeviceSource does not need to take any special action to ensure
+// that it doesn't callback into PowerMonitor after this phase of shutdown has
+// completed.
+void PowerMonitorDeviceSource::Shutdown() {}
+
 }  // namespace base

base/power_monitor/power_monitor_device_source.h

@@ -28,6 +28,8 @@ class BASE_EXPORT PowerMonitorDeviceSource : public PowerMonitorSource {
   PowerMonitorDeviceSource();
   ~PowerMonitorDeviceSource() override;
 
+  void Shutdown() override;
+
 #if defined(OS_MACOSX)
   // Allocate system resources needed by the PowerMonitor class.
   //

base/power_monitor/power_monitor_source.h

@@ -31,6 +31,14 @@ class BASE_EXPORT PowerMonitorSource {
   // Is the computer currently on battery power. Can be called on any thread.
   bool IsOnBatteryPower();
 
+  // Called by PowerMonitor just before PowerMonitor destroys both itself and
+  // this instance). After return from this call it is no longer safe for
+  // subclasses to call into PowerMonitor (e.g., via PowerMonitor::Get(). Hence,
+  // subclasses should take any necessary actions here to ensure that after
+  // return from this invocation they will no longer make any calls on
+  // PowerMonitor.
+  virtual void Shutdown() = 0;
+
  protected:
   friend class PowerMonitorTest;
 

base/test/power_monitor_test_base.cc

@@ -20,6 +20,8 @@ PowerMonitorTestSource::PowerMonitorTestSource()
 
 PowerMonitorTestSource::~PowerMonitorTestSource() = default;
 
+void PowerMonitorTestSource::Shutdown() {}
+
 void PowerMonitorTestSource::GeneratePowerStateEvent(bool on_battery_power) {
   test_on_battery_power_ = on_battery_power;
   ProcessPowerEvent(POWER_STATE_EVENT);

base/test/power_monitor_test_base.h

@@ -14,6 +14,7 @@ class PowerMonitorTestSource : public PowerMonitorSource {
  public:
   PowerMonitorTestSource();
   ~PowerMonitorTestSource() override;
+  void Shutdown() override;
 
   void GeneratePowerStateEvent(bool on_battery_power);
   void GenerateSuspendEvent();

media/cast/sender/h264_vt_encoder_unittest.cc

@@ -191,6 +191,8 @@ void CreateFrameAndMemsetPlane(VideoFrameFactory* const video_frame_factory) {
 
 class TestPowerSource : public base::PowerMonitorSource {
  public:
+  void Shutdown() override {}
+
   void GenerateSuspendEvent() {
     ProcessPowerEvent(SUSPEND_EVENT);
     base::RunLoop().RunUntilIdle();

net/url_request/url_request_unittest.cc

@@ -328,6 +328,8 @@ class TestPowerMonitorSource : public base::PowerMonitorSource {
   TestPowerMonitorSource() = default;
   ~TestPowerMonitorSource() override = default;
 
+  void Shutdown() override {}
+
   void Suspend() { ProcessPowerEvent(SUSPEND_EVENT); }
 
   void Resume() { ProcessPowerEvent(RESUME_EVENT); }

services/device/public/cpp/power_monitor/power_monitor_broadcast_source.cc

@@ -25,12 +25,6 @@ PowerMonitorBroadcastSource::PowerMonitorBroadcastSource(
     : client_(std::move(client)), task_runner_(task_runner) {}
 
 PowerMonitorBroadcastSource::~PowerMonitorBroadcastSource() {
-  // When power monitor and source are destroyed, the IO thread could still be
-  // receiving mojo messages that access the monitor and source in
-  // |ProcessPowerEvent|, thus causing a data race. Calling Shutdown() on the
-  // client tells it to ignore future mojo messages, and thus prevents the data
-  // race.
-  client_->Shutdown();
   task_runner_->DeleteSoon(FROM_HERE, client_.release());
 }
 
@@ -43,6 +37,16 @@ void PowerMonitorBroadcastSource::Init(service_manager::Connector* connector) {
   }
 }
 
+void PowerMonitorBroadcastSource::Shutdown() {
+  // When power monitor and source are destroyed, the IO thread could still be
+  // receiving mojo messages that access the monitor and source in
+  // |ProcessPowerEvent|, thus causing a data race. Calling Shutdown() on the
+  // client tells it to ignore future mojo messages; by making this call
+  // *before* the power monitor and source destruction proceed, we  prevent the
+  // data race.
+  client_->Shutdown();
+}
+
 bool PowerMonitorBroadcastSource::IsOnBatteryPowerImpl() {
   return client_->last_reported_on_battery_power_state();
 }

services/device/public/cpp/power_monitor/power_monitor_broadcast_source.h

@@ -37,6 +37,8 @@ class PowerMonitorBroadcastSource : public base::PowerMonitorSource {
   // the Mojo connection is set up.
   void Init(service_manager::Connector* connector);
 
+  void Shutdown() override;
+
  private:
   friend class PowerMonitorBroadcastSourceTest;
   friend class MockClient;