Fix a potential multi-thread issue when manipulating removable storage map

base/system_monitor/system_monitor.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/system_monitor/system_monitor.h"
 
 #include <utility>
 
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/stl_util.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
 
 namespace base {
 
 static SystemMonitor* g_system_monitor = NULL;

[Philippe, 2012/12/14 13:53:03]

Could we use a leaky lazy instance here? I'm concerned about uses after free.
The Get() method below could make the client think that he can safely access the
instance at any time from any thread.

[vandebo (ex-Chrome), 2013/01/04 19:46:54]

g_system_monitor will go away during the refactor.

 
 #if defined(ENABLE_BATTERY_MONITORING)
 // The amount of time (in ms) to wait before running the initial
 // battery check.
 static int kDelayedBatteryCheckMs = 10 * 1000;
 #endif  // defined(ENABLE_BATTERY_MONITORING)
 
 SystemMonitor::RemovableStorageInfo::RemovableStorageInfo() {
 }
 
@@ skipping 32 matching lines @@
 #endif
   DCHECK_EQ(this, g_system_monitor);
   g_system_monitor = NULL;
 }
 
 // static
 SystemMonitor* SystemMonitor::Get() {
   return g_system_monitor;
 }
 
 void SystemMonitor::ProcessPowerMessage(PowerEvent event_id) {

[Philippe, 2012/12/14 13:53:03]

I believe this function is supposed to be callable from any thread, right?
It's accessing some state without any kind of synchronization.

[Hongbo Min, 2012/12/15 14:03:28]

There is a bug for refactoring SystemMonitor to remove Process* series methods.
See http://crbug.com/149059

[vandebo (ex-Chrome), 2013/01/04 19:46:54]

This function will go away in the refactor, but it should only be called from a
single thread.

   // Suppress duplicate notifications.  Some platforms may
   // send multiple notifications of the same event.
   switch (event_id) {
     case POWER_STATE_EVENT:
       {
         bool on_battery = IsBatteryPower();
         if (on_battery != battery_in_use_) {
           battery_in_use_ = on_battery;
           NotifyPowerStateChange();
         }
@@ skipping 15 matching lines @@
 }
 
 void SystemMonitor::ProcessDevicesChanged(DeviceType device_type) {
   NotifyDevicesChanged(device_type);
 }
 
 void SystemMonitor::ProcessRemovableStorageAttached(
     const std::string& id,
     const string16& name,
     const FilePath::StringType& location) {
-  if (ContainsKey(removable_storage_map_, id)) {
-    // This can happen if our unique id scheme fails. Ignore the incoming
-    // non-unique attachment.
-    return;
+  {
+    base::AutoLock lock(removable_storage_lock_);
+    if (ContainsKey(removable_storage_map_, id)) {
+      // This can happen if our unique id scheme fails. Ignore the incoming
+      // non-unique attachment.
+      return;
+    }
+    RemovableStorageInfo info(id, name, location);
+    removable_storage_map_.insert(std::make_pair(id, info));
   }
-  RemovableStorageInfo info(id, name, location);
-  removable_storage_map_.insert(std::make_pair(id, info));
   NotifyRemovableStorageAttached(id, name, location);
 }
 
 void SystemMonitor::ProcessRemovableStorageDetached(const std::string& id) {
-  RemovableStorageMap::iterator it = removable_storage_map_.find(id);
-  if (it == removable_storage_map_.end())
-    return;
-  removable_storage_map_.erase(it);
+  {
+    base::AutoLock lock(removable_storage_lock_);
+    RemovableStorageMap::iterator it = removable_storage_map_.find(id);
+    if (it == removable_storage_map_.end())
+      return;
+    removable_storage_map_.erase(it);
+  }
   NotifyRemovableStorageDetached(id);
 }
 
 std::vector<SystemMonitor::RemovableStorageInfo>
 SystemMonitor::GetAttachedRemovableStorage() const {
   std::vector<RemovableStorageInfo> results;
+
+  base::AutoLock lock(removable_storage_lock_);
   for (RemovableStorageMap::const_iterator it = removable_storage_map_.begin();
        it != removable_storage_map_.end();
        ++it) {
     results.push_back(it->second);
   }
   return results;
 }
 
 void SystemMonitor::AddPowerObserver(PowerObserver* obs) {
   power_observer_list_->AddObserver(obs);
@@ skipping 48 matching lines @@
 void SystemMonitor::NotifyResume() {
   DVLOG(1) << "Power Resuming";
   power_observer_list_->Notify(&PowerObserver::OnResume);
 }
 
 void SystemMonitor::BatteryCheck() {
   ProcessPowerMessage(SystemMonitor::POWER_STATE_EVENT);
 }
 
 }  // namespace base