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

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

TBR=willchan@chromium.org
BUG=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156486

[Hongbo Min, 2012-09-12 14:04:20]

vandebo,

Since the removable_storage_map_ may be manipulated from more than one thread,
we need to use lock to synchronize its access  in SystemMonitor.

[vandebo (ex-Chrome), 2012-09-12 16:56:46]

LGTM

Will, this is what we talked about so TBRing you.

[commit-bot: I haz the power, 2012-09-12 16:57:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10905237/1

[willchan no longer on Chromium, 2012-09-12 17:00:45]

Minor nit, which doesn't really matter since I think everyone always has access
to a non-const SystemMonitor.

https://chromiumcodereview.appspot.com/10905237/diff/1/base/system_monitor/sy...
File base/system_monitor/system_monitor.h (right):

https://chromiumcodereview.appspot.com/10905237/diff/1/base/system_monitor/sy...
base/system_monitor/system_monitor.h:98: std::vector<RemovableStorageInfo>
GetAttachedRemovableStorage();
This should probably stay const. If you only need the const for the Lock, then
make the Lock mutable.

[vandebo (ex-Chrome), 2012-09-12 17:12:57]

http://codereview.chromium.org/10905237/diff/1/base/system_monitor/system_mon...
File base/system_monitor/system_monitor.h (right):

http://codereview.chromium.org/10905237/diff/1/base/system_monitor/system_mon...
base/system_monitor/system_monitor.h:11: 
Should include the lock header.

http://codereview.chromium.org/10905237/diff/1/base/system_monitor/system_mon...
base/system_monitor/system_monitor.h:98: std::vector<RemovableStorageInfo>
GetAttachedRemovableStorage();
On 2012/09/12 17:00:45, willchan wrote:
> This should probably stay const. If you only need the const for the Lock, then
> make the Lock mutable.

Hmm, I haven't seen a lot of mutable annotations in Chrome.  codesearch shows
that it's mostly used for locks.  Thanks.

Unchecked the commit box since I noticed a couple more minor things too.

http://codereview.chromium.org/10905237/diff/1/base/system_monitor/system_mon...
base/system_monitor/system_monitor.h:222: // The lock for manipluating
removable_storage_map_ structure.
nit: Remove the: "Lock for..."

[Hongbo Min, 2012-09-13 00:02:12]

vandebo, does it LGTM again now? If yes, please help check the commit box.
Thanks.

[vandebo (ex-Chrome), 2012-09-13 00:43:30]

LGTM

[commit-bot: I haz the power, 2012-09-13 00:46:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10905237/2003

[commit-bot: I haz the power, 2012-09-13 01:04:55]

Try job failure for 10905237-2003 (retry) on win for step "runhooks" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-09-13 01:07:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10905237/2003

[commit-bot: I haz the power, 2012-09-13 03:14:20]

Change committed as 156486

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

Hi guys,

The threading guarantees of this class are not really clear.

I would like to have your opinion before I send a CL fixing the issues I
mentioned.

https://chromiumcodereview.appspot.com/10905237/diff/2003/base/system_monitor...
File base/system_monitor/system_monitor.cc (right):

https://chromiumcodereview.appspot.com/10905237/diff/2003/base/system_monitor...
base/system_monitor/system_monitor.cc:17: static SystemMonitor* g_system_monitor
= NULL;
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.

https://chromiumcodereview.appspot.com/10905237/diff/2003/base/system_monitor...
base/system_monitor/system_monitor.cc:70: void
SystemMonitor::ProcessPowerMessage(PowerEvent event_id) {
I believe this function is supposed to be callable from any thread, right?
It's accessing some state without any kind of synchronization.

https://chromiumcodereview.appspot.com/10905237/diff/2003/base/system_monitor...
File base/system_monitor/system_monitor.h (right):

https://chromiumcodereview.appspot.com/10905237/diff/2003/base/system_monitor...
base/system_monitor/system_monitor.h:108: // Using a lock here is not necessary
for just a bool.
This is wrong on ARM multi cores.

[Hongbo Min, 2012-12-15 14:03:27]

Hi, Philippe,

I still have no much idea about the design rationale of SystemMonitor, and I
have same concerns about SystemMonitor implementation. I ever uploaded a patch
to refactor it but no everyone agreed to refactor it. See
https://codereview.chromium.org/10959020/.

willchan@, could you point the right person to answer the question about
SystemMonitor.

https://chromiumcodereview.appspot.com/10905237/diff/2003/base/system_monitor...
File base/system_monitor/system_monitor.cc (right):

https://chromiumcodereview.appspot.com/10905237/diff/2003/base/system_monitor...
base/system_monitor/system_monitor.cc:70: void
SystemMonitor::ProcessPowerMessage(PowerEvent event_id) {
On 2012/12/14 13:53:03, Philippe wrote:
> I believe this function is supposed to be callable from any thread, right?
> It's accessing some state without any kind of synchronization.

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]

https://chromiumcodereview.appspot.com/10905237/diff/2003/base/system_monitor...
File base/system_monitor/system_monitor.cc (right):

https://chromiumcodereview.appspot.com/10905237/diff/2003/base/system_monitor...
base/system_monitor/system_monitor.cc:17: static SystemMonitor* g_system_monitor
= NULL;
On 2012/12/14 13:53:03, Philippe wrote:
> 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.

g_system_monitor will go away during the refactor.

https://chromiumcodereview.appspot.com/10905237/diff/2003/base/system_monitor...
base/system_monitor/system_monitor.cc:70: void
SystemMonitor::ProcessPowerMessage(PowerEvent event_id) {
On 2012/12/14 13:53:03, Philippe wrote:
> I believe this function is supposed to be callable from any thread, right?
> It's accessing some state without any kind of synchronization.

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

https://chromiumcodereview.appspot.com/10905237/diff/2003/base/system_monitor...
File base/system_monitor/system_monitor.h (right):

https://chromiumcodereview.appspot.com/10905237/diff/2003/base/system_monitor...
base/system_monitor/system_monitor.h:108: // Using a lock here is not necessary
for just a bool.
On 2012/12/14 13:53:03, Philippe wrote:
> This is wrong on ARM multi cores.

Feel free to fix.