Make the SystemMonitor observer list thread safe.

This is done by using the new observer_list_threadsafe.

Review URL: http://codereview.chromium.org/4288

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@4452 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 119 insertions, 64 deletions

diff --git a/base/observer_list_threadsafe.h b/base/observer_list_threadsafe.h
index fcfa0ed..f76f063 100644
--- a/base/observer_list_threadsafe.h
+++ b/base/observer_list_threadsafe.h
@@ -81,11 +81,15 @@ class ObserverListThreadSafe :
   void RemoveObserver(ObserverType* obs) {
     ObserverList<ObserverType>* list = NULL;
     MessageLoop* loop = MessageLoop::current();
+    if (!loop)
+      return;  // On shutdown, it is possible that current() is already null.
     {
       AutoLock lock(list_lock_);
-      DCHECK(observer_lists_.find(loop) != observer_lists_.end()) <<
-          "RemoveObserver called on for unknown thread";
       list = observer_lists_[loop];
+      if (!list) {
+        NOTREACHED() << "RemoveObserver called on for unknown thread";
+        return;
+      }
 
       // If we're about to remove the last observer from the list,
       // then we can remove this observer_list entirely.
diff --git a/base/system_monitor.cc b/base/system_monitor.cc
index 37dc48a..1d44e3c 100644
--- a/base/system_monitor.cc
+++ b/base/system_monitor.cc
@@ -3,19 +3,26 @@
 // found in the LICENSE file.

 

 #include "base/system_monitor.h"

+#include "base/logging.h"

+#include "base/message_loop.h"

 

 namespace base {

 

+// The amount of time (in ms) to wait before running the initial

+// battery check.

+static int kDelayedBatteryCheckMs = 10 * 1000;

+

 SystemMonitor::SystemMonitor()

-    : battery_in_use_(IsBatteryPower()),

+    : battery_in_use_(false),

       suspended_(false) {

+  observer_list_ = new ObserverListThreadSafe<PowerObserver>();

 }

 

 void SystemMonitor::ProcessPowerMessage(PowerEvent event_id) {

   // Suppress duplicate notifications.  Some platforms may 

   // send multiple notifications of the same event.

   switch (event_id) {

-    case PowerStateEvent:

+    case POWER_STATE_EVENT:

       {

         bool on_battery = IsBatteryPower();

         if (on_battery != battery_in_use_) {

@@ -24,13 +31,13 @@ void SystemMonitor::ProcessPowerMessage(PowerEvent event_id) {
         }

       }

       break;

-    case ResumeEvent:

+    case RESUME_EVENT:

       if (suspended_) {

         suspended_ = false;

         NotifyResume();

       }

       break;

-    case SuspendEvent:

+    case SUSPEND_EVENT:

       if (!suspended_) {

         suspended_ = true;

         NotifySuspend();

@@ -39,4 +46,40 @@ void SystemMonitor::ProcessPowerMessage(PowerEvent event_id) {
   }

 }

 

+void SystemMonitor::AddObserver(PowerObserver* obs) {

+  observer_list_->AddObserver(obs);

+}

+

+void SystemMonitor::RemoveObserver(PowerObserver* obs) {

+  observer_list_->RemoveObserver(obs);

+}

+

+void SystemMonitor::NotifyPowerStateChange() {

+  LOG(INFO) << L"PowerStateChange: " 

+           << (BatteryPower() ? L"On" : L"Off") << L" battery";

+  observer_list_->Notify(&PowerObserver::OnPowerStateChange, this);

+}

+

+void SystemMonitor::NotifySuspend() {

+  LOG(INFO) << L"Power Suspending";

+  observer_list_->Notify(&PowerObserver::OnSuspend, this);

+}

+

+void SystemMonitor::NotifyResume() {

+  LOG(INFO) << L"Power Resuming";

+  observer_list_->Notify(&PowerObserver::OnResume, this);

+}

+

+void SystemMonitor::Start() {

+  DCHECK(MessageLoop::current());  // Can't call start too early.

+  SystemMonitor* monitor = Get();

+  monitor->delayed_battery_check_.Start(

+      TimeDelta::FromMilliseconds(kDelayedBatteryCheckMs), monitor,

+      &SystemMonitor::BatteryCheck);

+}

+

+void SystemMonitor::BatteryCheck() {

+  ProcessPowerMessage(SystemMonitor::POWER_STATE_EVENT);

+}

+

 } // namespace base

diff --git a/base/system_monitor.h b/base/system_monitor.h
index ec4c593..7a2d4d5 100644
--- a/base/system_monitor.h
+++ b/base/system_monitor.h
@@ -5,13 +5,12 @@
 #ifndef BASE_SYSTEM_MONITOR_H_

 #define BASE_SYSTEM_MONITOR_H_

 

-#include "base/logging.h"

-#include "base/observer_list.h"

+#include "base/observer_list_threadsafe.h"

 #include "base/singleton.h"

 

 namespace base {

 

-// Singleton class for monitoring various system-related subsystems

+// Class for monitoring various system-related subsystems

 // such as power management, network status, etc.

 // TODO(mbelshe):  Add support beyond just power management.

 class SystemMonitor {

@@ -21,25 +20,33 @@ class SystemMonitor {
     return Singleton<SystemMonitor>::get();

   }

 

+  // To start the System Monitor within an application

+  // use this call.

+  static void Start();

+

   //

   // Power-related APIs

   //

 

   // Is the computer currently on battery power.

-  bool BatteryPower() { return battery_in_use_; }

+  // Can be called on any thread.

+  bool BatteryPower() { 

+    // Using a lock here is not necessary for just a bool.

+    return battery_in_use_;

+  }

 

   // Normalized list of power events.

   enum PowerEvent {

-    // The Power status of the system has changed.

-    PowerStateEvent,

-

-    // The system is being suspended.

-    SuspendEvent,

-

-    // The system is being resumed.

-    ResumeEvent

+    POWER_STATE_EVENT,  // The Power status of the system has changed.

+    SUSPEND_EVENT,      // The system is being suspended.

+    RESUME_EVENT        // The system is being resumed.

   };

 

+  // Callbacks will be called on the thread which creates the SystemMonitor.

+  // During the callback, Add/RemoveObserver will block until the callbacks

+  // are finished. Observers should implement quick callback functions; if

+  // lengthy operations are needed, the observer should take care to invoke

+  // the operation on an appropriate thread.

   class PowerObserver {

   public:

     // Notification of a change in power status of the computer, such

@@ -53,32 +60,15 @@ class SystemMonitor {
     virtual void OnResume(SystemMonitor*) = 0;

   };

 

-  void AddObserver(PowerObserver* obs) {

-    observer_list_.AddObserver(obs);

-  }

-

-  void RemoveObserver(PowerObserver* obs) {

-    observer_list_.RemoveObserver(obs);

-  }

-

-  void NotifyPowerStateChange() {

-    LOG(INFO) << L"PowerStateChange: " 

-             << (BatteryPower() ? L"On" : L"Off") << L" battery";

-    FOR_EACH_OBSERVER(PowerObserver, observer_list_,

-      OnPowerStateChange(this));

-  }

+  // Add a new observer.

+  // Can be called from any thread.

+  // Must not be called from within a notification callback.

+  void AddObserver(PowerObserver* obs);

 

-  void NotifySuspend() {

-    FOR_EACH_OBSERVER(PowerObserver, observer_list_, OnSuspend(this));

-  }

-

-  void NotifyResume() {

-    FOR_EACH_OBSERVER(PowerObserver, observer_list_, OnResume(this));

-  }

-

-  // Constructor.  

-  // Don't use this; access SystemMonitor via the Singleton.

-  SystemMonitor();

+  // Remove an existing observer.

+  // Can be called from any thread.

+  // Must not be called from within a notification callback.

+  void RemoveObserver(PowerObserver* obs);

 

 #if defined(OS_WIN)

   // Windows-specific handling of a WM_POWERBROADCAST message.

@@ -88,19 +78,33 @@ class SystemMonitor {
 #endif

 

   // Cross-platform handling of a power event.

-  // This is only exposed for testing.

   void ProcessPowerMessage(PowerEvent event_id);

 

+  // Constructor.  

+  // Don't use this; access SystemMonitor via the Singleton.

+  SystemMonitor();

+

  private:

   // Platform-specific method to check whether the system is currently

   // running on battery power.  Returns true if running on batteries,

   // false otherwise.

   bool IsBatteryPower();

 

-  ObserverList<PowerObserver> observer_list_;

+  // Checks the battery status and notifies observers if the battery

+  // status has changed.

+  void BatteryCheck();

+

+  // Functions to trigger notifications.

+  void NotifyPowerStateChange();

+  void NotifySuspend();

+  void NotifyResume();

+

+  scoped_refptr<ObserverListThreadSafe<PowerObserver> > observer_list_;

   bool battery_in_use_;

   bool suspended_;

 

+  base::OneShotTimer<SystemMonitor> delayed_battery_check_;

+

   DISALLOW_COPY_AND_ASSIGN(SystemMonitor);

 };

 

diff --git a/base/system_monitor_unittest.cc b/base/system_monitor_unittest.cc
index aced8fe..b3deccc 100644
--- a/base/system_monitor_unittest.cc
+++ b/base/system_monitor_unittest.cc
@@ -32,35 +32,37 @@ class PowerTest : public base::SystemMonitor::PowerObserver {
 };

 

 TEST(SystemMonitor, PowerNotifications) {

+  const int kObservers = 5;

   base::SystemMonitor* monitor = base::SystemMonitor::Get();

-  PowerTest test;

-  monitor->AddObserver(&test);

+  PowerTest test[kObservers];

+  for (int index = 0; index < kObservers; ++index)

+    monitor->AddObserver(&test[index]);

 

   // Send a bunch of power changes.  Since the battery power hasn't

   // actually changed, we shouldn't get notifications.

   for (int index = 0; index < 5; index++) {

-    monitor->ProcessPowerMessage(base::SystemMonitor::PowerStateEvent);

-    EXPECT_EQ(test.power_state_changes(), 0);

+    monitor->ProcessPowerMessage(base::SystemMonitor::POWER_STATE_EVENT);

+    EXPECT_EQ(test[0].power_state_changes(), 0);

   }

 

   // Sending resume when not suspended should have no effect.

-  monitor->ProcessPowerMessage(base::SystemMonitor::ResumeEvent);

-  EXPECT_EQ(test.resumes(), 0);

+  monitor->ProcessPowerMessage(base::SystemMonitor::RESUME_EVENT);

+  EXPECT_EQ(test[0].resumes(), 0);

 

   // Pretend we suspended.

-  monitor->ProcessPowerMessage(base::SystemMonitor::SuspendEvent);

-  EXPECT_EQ(test.suspends(), 1);

+  monitor->ProcessPowerMessage(base::SystemMonitor::SUSPEND_EVENT);

+  EXPECT_EQ(test[0].suspends(), 1);

 

   // Send a second suspend notification.  This should be suppressed.

-  monitor->ProcessPowerMessage(base::SystemMonitor::SuspendEvent);

-  EXPECT_EQ(test.suspends(), 1);

+  monitor->ProcessPowerMessage(base::SystemMonitor::SUSPEND_EVENT);

+  EXPECT_EQ(test[0].suspends(), 1);

 

   // Pretend we were awakened.

-  monitor->ProcessPowerMessage(base::SystemMonitor::ResumeEvent);

-  EXPECT_EQ(test.resumes(), 1);

+  monitor->ProcessPowerMessage(base::SystemMonitor::RESUME_EVENT);

+  EXPECT_EQ(test[0].resumes(), 1);

 

   // Send a duplicate resume notification.  This should be suppressed.

-  monitor->ProcessPowerMessage(base::SystemMonitor::ResumeEvent);

-  EXPECT_EQ(test.resumes(), 1);

+  monitor->ProcessPowerMessage(base::SystemMonitor::RESUME_EVENT);

+  EXPECT_EQ(test[0].resumes(), 1);

 }

 

diff --git a/base/system_monitor_win.cc b/base/system_monitor_win.cc
index 0603797..2a42e90 100644
--- a/base/system_monitor_win.cc
+++ b/base/system_monitor_win.cc
@@ -10,13 +10,13 @@ void SystemMonitor::ProcessWmPowerBroadcastMessage(int event_id) {
   PowerEvent power_event;

   switch (event_id) {

     case PBT_APMPOWERSTATUSCHANGE:

-      power_event = PowerStateEvent;

+      power_event = POWER_STATE_EVENT;

       break;

     case PBT_APMRESUMEAUTOMATIC:

-      power_event = ResumeEvent;

+      power_event = RESUME_EVENT;

       break;

     case PBT_APMSUSPEND:

-      power_event = SuspendEvent;

+      power_event = SUSPEND_EVENT;

       break;

     default:

       DCHECK(false);

@@ -37,4 +37,4 @@ bool SystemMonitor::IsBatteryPower() {
 

 

 

-} // namespace base
\ No newline at end of file
+} // namespace base

diff --git a/base/time_win.cc b/base/time_win.cc
index bc9d0c7..06bf1d3 100644
--- a/base/time_win.cc
+++ b/base/time_win.cc
@@ -233,7 +233,9 @@ class NowSingleton : public base::SystemMonitor::PowerObserver {
 
   ~NowSingleton() {
     UseHiResClock(false);
-    base::SystemMonitor::Get()->RemoveObserver(this);
+    base::SystemMonitor* monitor = base::SystemMonitor::Get();
+    if (monitor)
+      monitor->RemoveObserver(this);
   }
 
   TimeDelta Now() {