Revert of Attempting to resolve a race condition with PowerMonitor (h…

…ttps://codereview.chromium.org/179923006/)

Reason for revert:
Failing interactive_ui_tests dbg:

http://build.chromium.org/p/chromium.win/builders/Interactive%20Tests%20(dbg)/builds/47083/steps/interactive_ui_tests/logs/stdio

Original issue's description:
> Attempting to resolve a race condition with PowerMonitor.
> 
> ThreadSanitizer caught multiple instances where PowerMonitor::Get or PowerMonitor::Add/RemoveObserver were being called concurrently with the PowerMonitor constructor in the main thread. These functions access a process-global PowerMontior instance (g_power_monitor), which was not thread safe.
> 
> This change adds locks around PowerMonitor creation and deletion, and forces Add/RemoveObserver to be called in a threadsafe manner. It also removes the need to call PowerMonitor::Get.
> 
> BUG=268924
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262018

TBR=mattm@chromium.org,jyasskin@chromium.org,kbr@chromium.org,bradnelson@chromium.org,brettw@chromium.org,bradchen@chromium.org,willchan@chromium.org,jam@chromium.org,jochen@chromium.org,timurrrr@chromium.org,glider@chromium.org,acolwell@chromium.org,scherkus@chromium.org,bajones@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=268924

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

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

base/power_monitor/power_monitor.cc

@@ -3,108 +3,59 @@
 // found in the LICENSE file.
 
 #include "base/power_monitor/power_monitor.h"
-
-#include "base/lazy_instance.h"
 #include "base/power_monitor/power_monitor_source.h"
-#include "base/synchronization/lock.h"
 
 namespace base {
 
-class PowerMonitorImpl;
-
-LazyInstance<Lock>::Leaky g_power_monitor_lock = LAZY_INSTANCE_INITIALIZER;
-static PowerMonitorImpl* g_power_monitor = NULL;
-
-// A class used to monitor the power state change and notify the observers about
-// the change event.
-class BASE_EXPORT PowerMonitorImpl {
- public:
-  explicit PowerMonitorImpl(scoped_ptr<PowerMonitorSource> source)
-      : observers_(new ObserverListThreadSafe<PowerObserver>()),
-      source_(source.Pass()) { }
-
-  ~PowerMonitorImpl()  { }
-
-  scoped_refptr<ObserverListThreadSafe<PowerObserver> > observers_;
-  scoped_ptr<PowerMonitorSource> source_;
-
-  DISALLOW_COPY_AND_ASSIGN(PowerMonitorImpl);
-};
+static PowerMonitor* g_power_monitor = NULL;
 
-void PowerMonitor::Initialize(scoped_ptr<PowerMonitorSource> source) {
-  AutoLock(g_power_monitor_lock.Get());
+PowerMonitor::PowerMonitor(scoped_ptr<PowerMonitorSource> source)
+    : observers_(new ObserverListThreadSafe<PowerObserver>()),
+      source_(source.Pass()) {
   DCHECK(!g_power_monitor);
-  g_power_monitor = new PowerMonitorImpl(source.Pass());
+  g_power_monitor = this;
 }
 
-void PowerMonitor::ShutdownForTesting() {
-  AutoLock(g_power_monitor_lock.Get());
-  if (g_power_monitor) {
-    delete g_power_monitor;
-    g_power_monitor = NULL;
-  }
+PowerMonitor::~PowerMonitor() {
+  DCHECK_EQ(this, g_power_monitor);
+  g_power_monitor = NULL;
 }
 
-bool PowerMonitor::IsInitialized() {
-  AutoLock(g_power_monitor_lock.Get());
-  return g_power_monitor != NULL;
+// static
+PowerMonitor* PowerMonitor::Get() {
+  return g_power_monitor;
 }
 
-bool PowerMonitor::AddObserver(PowerObserver* observer) {
-  AutoLock(g_power_monitor_lock.Get());
-  if (!g_power_monitor)
-    return false;
-  g_power_monitor->observers_->AddObserver(observer);
-  return true;
+void PowerMonitor::AddObserver(PowerObserver* obs) {
+  observers_->AddObserver(obs);
 }
 
-bool PowerMonitor::RemoveObserver(PowerObserver* observer) {
-  AutoLock(g_power_monitor_lock.Get());
-  if (!g_power_monitor)
-    return false;
-  g_power_monitor->observers_->RemoveObserver(observer);
-  return true;
+void PowerMonitor::RemoveObserver(PowerObserver* obs) {
+  observers_->RemoveObserver(obs);
 }
 
-bool PowerMonitor::IsOnBatteryPower() {
-  g_power_monitor_lock.Get().AssertAcquired();
-  if (!g_power_monitor)
-    return false;
-  return g_power_monitor->source_->IsOnBatteryPower();
-}
-
-Lock* PowerMonitor::GetLock() {
-  return &g_power_monitor_lock.Get();
+PowerMonitorSource* PowerMonitor::Source() {
+  return source_.get();
 }
 
-bool PowerMonitor::IsInitializedLocked() {
-  g_power_monitor_lock.Get().AssertAcquired();
-  return g_power_monitor != NULL;
-}
-
-PowerMonitorSource* PowerMonitor::GetSource() {
-  g_power_monitor_lock.Get().AssertAcquired();
-  return g_power_monitor->source_.get();
+bool PowerMonitor::IsOnBatteryPower() {
+  return source_->IsOnBatteryPower();
 }
 
 void PowerMonitor::NotifyPowerStateChange(bool battery_in_use) {
   DVLOG(1) << "PowerStateChange: " << (battery_in_use ? "On" : "Off")
            << " battery";
-  g_power_monitor_lock.Get().AssertAcquired();
-  g_power_monitor->observers_->Notify(&PowerObserver::OnPowerStateChange,
-      battery_in_use);
+  observers_->Notify(&PowerObserver::OnPowerStateChange, battery_in_use);
 }
 
 void PowerMonitor::NotifySuspend() {
   DVLOG(1) << "Power Suspending";
-  g_power_monitor_lock.Get().AssertAcquired();
-  g_power_monitor->observers_->Notify(&PowerObserver::OnSuspend);
+  observers_->Notify(&PowerObserver::OnSuspend);
 }
 
 void PowerMonitor::NotifyResume() {
   DVLOG(1) << "Power Resuming";
-  g_power_monitor_lock.Get().AssertAcquired();
-  g_power_monitor->observers_->Notify(&PowerObserver::OnResume);
+  observers_->Notify(&PowerObserver::OnResume);
 }
 
 }  // namespace base

base/power_monitor/power_monitor.h

@@ -10,7 +10,6 @@
 #include "base/memory/ref_counted.h"
 #include "base/observer_list_threadsafe.h"
 #include "base/power_monitor/power_observer.h"
-#include "base/synchronization/lock.h"
 
 namespace base {
 
@@ -21,31 +20,32 @@ class PowerMonitorSource;
 class BASE_EXPORT PowerMonitor {
  public:
   // Takes ownership of |source|.
-  static void Initialize(scoped_ptr<PowerMonitorSource> source);
-  static void ShutdownForTesting();
+  explicit PowerMonitor(scoped_ptr<PowerMonitorSource> source);
+  ~PowerMonitor();
 
-  // Has the PowerMonitor already been initialized?
-  static bool IsInitialized();
+  // Get the process-wide PowerMonitor (if not present, returns NULL).
+  static PowerMonitor* Get();
 
   // Add and remove an observer.
   // Can be called from any thread.
   // Must not be called from within a notification callback.
-  static bool AddObserver(PowerObserver* observer);
-  static bool RemoveObserver(PowerObserver* observer);
+  void AddObserver(PowerObserver* observer);
+  void RemoveObserver(PowerObserver* observer);
 
   // Is the computer currently on battery power.
-  static bool IsOnBatteryPower();
+  bool IsOnBatteryPower();
 
  private:
   friend class PowerMonitorSource;
 
-  static Lock* GetLock();
-  // Must manually acquire lock before calling any of the following functions.
-  static bool IsInitializedLocked();
-  static PowerMonitorSource* GetSource();
-  static void NotifyPowerStateChange(bool battery_in_use);
-  static void NotifySuspend();
-  static void NotifyResume();
+  PowerMonitorSource* Source();
+
+  void NotifyPowerStateChange(bool battery_in_use);
+  void NotifySuspend();
+  void NotifyResume();
+
+  scoped_refptr<ObserverListThreadSafe<PowerObserver> > observers_;
+  scoped_ptr<PowerMonitorSource> source_;
 
   DISALLOW_COPY_AND_ASSIGN(PowerMonitor);
 };

base/power_monitor/power_monitor_source.cc

@@ -22,11 +22,11 @@ bool PowerMonitorSource::IsOnBatteryPower() {
 }
 
 void PowerMonitorSource::ProcessPowerEvent(PowerEvent event_id) {
-  AutoLock lock(*PowerMonitor::GetLock());
-  if (!PowerMonitor::IsInitializedLocked())
+  PowerMonitor* monitor = PowerMonitor::Get();
+  if (!monitor)
     return;
 
-  PowerMonitorSource* source = PowerMonitor::GetSource();
+  PowerMonitorSource* source = monitor->Source();
 
   // Suppress duplicate notifications.  Some platforms may
   // send multiple notifications of the same event.
@@ -45,19 +45,19 @@ void PowerMonitorSource::ProcessPowerEvent(PowerEvent event_id) {
         }
 
         if (changed)
-          PowerMonitor::NotifyPowerStateChange(new_on_battery_power);
+          monitor->NotifyPowerStateChange(new_on_battery_power);
       }
       break;
     case RESUME_EVENT:
       if (source->suspended_) {
         source->suspended_ = false;
-        PowerMonitor::NotifyResume();
+        monitor->NotifyResume();
       }
       break;
     case SUSPEND_EVENT:
       if (!source->suspended_) {
         source->suspended_ = true;
-        PowerMonitor::NotifySuspend();
+        monitor->NotifySuspend();
       }
       break;
   }

base/power_monitor/power_monitor_source.h

@@ -40,6 +40,8 @@ class BASE_EXPORT PowerMonitorSource {
   // Get the process-wide PowerMonitorSource (if not present, returns NULL).
   static PowerMonitorSource* Get();
 
+  // ProcessPowerEvent should only be called from a single thread, most likely
+  // the UI thread or, in child processes, the IO thread.
   static void ProcessPowerEvent(PowerEvent event_id);
 
   // Platform-specific method to check whether the system is currently

base/power_monitor/power_monitor_unittest.cc

@@ -12,17 +12,17 @@ class PowerMonitorTest : public testing::Test {
  protected:
   PowerMonitorTest() {
     power_monitor_source_ = new PowerMonitorTestSource();
-    PowerMonitor::Initialize(
-        scoped_ptr<PowerMonitorSource>(power_monitor_source_));
+    power_monitor_.reset(new PowerMonitor(
+        scoped_ptr<PowerMonitorSource>(power_monitor_source_)));
   }
-  virtual ~PowerMonitorTest() {
-    PowerMonitor::ShutdownForTesting();
-  };
+  virtual ~PowerMonitorTest() {};
 
   PowerMonitorTestSource* source() { return power_monitor_source_; }
+  PowerMonitor* monitor() { return power_monitor_.get(); }
 
  private:
   PowerMonitorTestSource* power_monitor_source_;
+  scoped_ptr<PowerMonitor> power_monitor_;
 
   DISALLOW_COPY_AND_ASSIGN(PowerMonitorTest);
 };
@@ -34,7 +34,7 @@ TEST_F(PowerMonitorTest, PowerNotifications) {
 
   PowerMonitorTestObserver observers[kObservers];
   for (int index = 0; index < kObservers; ++index)
-    PowerMonitor::AddObserver(&observers[index]);
+    monitor()->AddObserver(&observers[index]);
 
   // Sending resume when not suspended should have no effect.
   source()->GenerateResumeEvent();

base/timer/hi_res_timer_manager_unittest.cc

@@ -19,7 +19,8 @@ TEST(HiResTimerManagerTest, DISABLED_ToggleOnOff) {
   base::MessageLoop loop;
   scoped_ptr<base::PowerMonitorSource> power_monitor_source(
       new base::PowerMonitorDeviceSource());
-  base::PowerMonitor::Initialize(power_monitor_source.Pass());
+  scoped_ptr<base::PowerMonitor> power_monitor(
+      new base::PowerMonitor(power_monitor_source.Pass()));
   HighResolutionTimerManager manager;
 
   // At this point, we don't know if the high resolution timers are on or off,
@@ -54,8 +55,6 @@ TEST(HiResTimerManagerTest, DISABLED_ToggleOnOff) {
     // De-activate the high resolution timer.
     base::Time::ActivateHighResolutionTimer(false);
   }
-
-  base::PowerMonitor::ShutdownForTesting();
 }
 #endif  // defined(OS_WIN)
 

base/timer/hi_res_timer_manager_win.cc

@@ -11,12 +11,14 @@ namespace base {
 
 HighResolutionTimerManager::HighResolutionTimerManager()
     : hi_res_clock_available_(false) {
-  base::PowerMonitor::AddObserver(this);
-  UseHiResClock(!base::PowerMonitor::IsOnBatteryPower());
+  base::PowerMonitor* power_monitor = base::PowerMonitor::Get();
+  DCHECK(power_monitor != NULL);
+  power_monitor->AddObserver(this);
+  UseHiResClock(!power_monitor->IsOnBatteryPower());
 }
 
 HighResolutionTimerManager::~HighResolutionTimerManager() {
-  base::PowerMonitor::RemoveObserver(this);
+  base::PowerMonitor::Get()->RemoveObserver(this);
   UseHiResClock(false);
 }
 

chrome/browser/extensions/event_router_forwarder_unittest.cc

@@ -97,11 +97,7 @@ class EventRouterForwarderTest : public testing::Test {
 #endif
     scoped_ptr<base::PowerMonitorSource> power_monitor_source(
       new base::PowerMonitorDeviceSource());
-    base::PowerMonitor::Initialize(power_monitor_source.Pass());
-  }
-
-  virtual ~EventRouterForwarderTest() {
-    base::PowerMonitor::ShutdownForTesting();
+    dummy.reset(new base::PowerMonitor(power_monitor_source.Pass()));
   }
 
   virtual void SetUp() {
@@ -118,6 +114,7 @@ class EventRouterForwarderTest : public testing::Test {
   content::TestBrowserThread ui_thread_;
   content::TestBrowserThread io_thread_;
   TestingProfileManager profile_manager_;
+  scoped_ptr<base::PowerMonitor> dummy;
   // Profiles are weak pointers, owned by ProfileManager in |browser_process_|.
   TestingProfile* profile1_;
   TestingProfile* profile2_;

components/nacl/loader/nacl_helper_win_64.cc

@@ -32,7 +32,7 @@ int NaClBrokerMain(const content::MainFunctionParams& parameters) {
 
   scoped_ptr<base::PowerMonitorSource> power_monitor_source(
       new base::PowerMonitorDeviceSource());
-  base::PowerMonitor::Initialize(power_monitor_source.Pass());
+  base::PowerMonitor power_monitor(power_monitor_source.Pass());
   base::HighResolutionTimerManager hi_res_timer_manager;
 
   NaClBrokerListener listener;

components/nacl/loader/nacl_main.cc

@@ -24,7 +24,7 @@ int NaClMain(const content::MainFunctionParams& parameters) {
 
   scoped_ptr<base::PowerMonitorSource> power_monitor_source(
       new base::PowerMonitorDeviceSource());
-  base::PowerMonitor::Initialize(power_monitor_source.Pass());
+  base::PowerMonitor power_monitor(power_monitor_source.Pass());
   base::HighResolutionTimerManager hi_res_timer_manager;
 
 #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX)
@@ -49,6 +49,5 @@ int NaClMain(const content::MainFunctionParams& parameters) {
 #else
   NOTIMPLEMENTED() << " not implemented startup, plugin startup dialog etc.";
 #endif
-
   return 0;
 }

content/browser/browser_main_loop.cc

@@ -448,8 +448,8 @@ void BrowserMainLoop::MainMessageLoopStart() {
   {
     TRACE_EVENT0("startup", "BrowserMainLoop::Subsystem:PowerMonitor")
     scoped_ptr<base::PowerMonitorSource> power_monitor_source(
-        new base::PowerMonitorDeviceSource());
-    base::PowerMonitor::Initialize(power_monitor_source.Pass());
+      new base::PowerMonitorDeviceSource());
+    power_monitor_.reset(new base::PowerMonitor(power_monitor_source.Pass()));
   }
   {
     TRACE_EVENT0("startup", "BrowserMainLoop::Subsystem:HighResTimerManager")

content/browser/browser_main_loop.h

@@ -16,6 +16,7 @@ class CommandLine;
 class FilePath;
 class HighResolutionTimerManager;
 class MessageLoop;
+class PowerMonitor;
 class SystemMonitor;
 namespace debug {
 class TraceMemoryController;
@@ -138,6 +139,7 @@ class CONTENT_EXPORT BrowserMainLoop {
   // Members initialized in |MainMessageLoopStart()| ---------------------------
   scoped_ptr<base::MessageLoop> main_message_loop_;
   scoped_ptr<base::SystemMonitor> system_monitor_;
+  scoped_ptr<base::PowerMonitor> power_monitor_;
   scoped_ptr<base::HighResolutionTimerManager> hi_res_timer_manager_;
   scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_;
   // user_input_monitor_ has to outlive audio_manager_, so declared first.

content/browser/power_monitor_message_broadcaster.cc

@@ -13,11 +13,15 @@ namespace content {
 PowerMonitorMessageBroadcaster::PowerMonitorMessageBroadcaster(
     IPC::Sender* sender)
     : sender_(sender) {
-  base::PowerMonitor::AddObserver(this);
+  base::PowerMonitor* power_monitor = base::PowerMonitor::Get();
+  if (power_monitor)
+    power_monitor->AddObserver(this);
 }
 
 PowerMonitorMessageBroadcaster::~PowerMonitorMessageBroadcaster() {
-  base::PowerMonitor::RemoveObserver(this);
+  base::PowerMonitor* power_monitor = base::PowerMonitor::Get();
+  if (power_monitor)
+    power_monitor->RemoveObserver(this);
 }
 
 void PowerMonitorMessageBroadcaster::OnPowerStateChange(bool on_battery_power) {