Attempting to resolve a race condition with PowerMonitor

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

[bajones, 2014-02-27 23:55:43]

PTAL at any files you own. Most are small changes.

[mattm, 2014-02-28 00:11:27]

On 2014/02/27 23:55:43, bajones wrote:
> PTAL at any files you own. Most are small changes.

Who do you want to look at which files?

[willchan no longer on Chromium, 2014-02-28 00:14:10]

Can you update this CL description? What's the race and what are you doing to
fix it? I see a whole bunch of threads in the crbug, and I don't really want to
have to grok that first. It'd be nice if the CL description explained that.

[bajones, 2014-02-28 00:51:43]

On 2014/02/28 00:14:10, willchan wrote:
> Can you update this CL description? What's the race and what are you doing to
> fix it? I see a whole bunch of threads in the crbug, and I don't really want
to
> have to grok that first. It'd be nice if the CL description explained that.

My apologies, the description has been updated.

[Jeffrey Yasskin, 2014-02-28 00:54:53]

event_router_forwarder_unittest.cc lgtm, but I haven't looked at any
thread-safety aspects.

[Ken Russell (switch to Gerrit), 2014-02-28 02:02:42]

https://codereview.chromium.org/179923006/diff/20001/base/power_monitor/power...
File base/power_monitor/power_monitor.h (right):

https://codereview.chromium.org/179923006/diff/20001/base/power_monitor/power...
base/power_monitor/power_monitor.h:22: static PowerMonitor*
Create(scoped_ptr<PowerMonitorSource> source);
I find the pattern here confusing where PowerMonitor is implicitly a singleton,
but there's still a factory method for creating one. This was a problem in the
earlier code too.

Perhaps a better paradigm would be to have static Initialize and Shutdown
methods, replacing Create and the destructor. Then the constructor and
destructor can both be private.

The class could expose just static AddObserverThreadSafe,
RemoveObserverThreadSave, and IsOnBatteryPower methods, and do all locking and
fetching of the global instance internally. They could call private instance
methods.

Would that be feasible?

[jam, 2014-02-28 17:05:18]

https://codereview.chromium.org/179923006/diff/20001/base/power_monitor/power...
File base/power_monitor/power_monitor.h (right):

https://codereview.chromium.org/179923006/diff/20001/base/power_monitor/power...
base/power_monitor/power_monitor.h:22: static PowerMonitor*
Create(scoped_ptr<PowerMonitorSource> source);
On 2014/02/28 02:02:42, Ken Russell wrote:
> I find the pattern here confusing where PowerMonitor is implicitly a
singleton,
> but there's still a factory method for creating one. This was a problem in the
> earlier code too.
> 
> Perhaps a better paradigm would be to have static Initialize and Shutdown
> methods, replacing Create and the destructor. Then the constructor and
> destructor can both be private.
> 
> The class could expose just static AddObserverThreadSafe,
> RemoveObserverThreadSave, and IsOnBatteryPower methods, and do all locking and
> fetching of the global instance internally. They could call private instance
> methods.
> 
> Would that be feasible?

+1

[bajones, 2014-02-28 20:08:11]

Thanks Ken, good suggestion! Refactored.

[Ken Russell (switch to Gerrit), 2014-02-28 21:30:09]

Thanks, that looks much better. LGTM with one suggestion.

https://codereview.chromium.org/179923006/diff/40001/base/power_monitor/power...
File base/power_monitor/power_monitor.cc (right):

https://codereview.chromium.org/179923006/diff/40001/base/power_monitor/power...
base/power_monitor/power_monitor.cc:38: return false;
Maybe this, RemoveObserver and IsOnBatteryPower should DCHECK in debug builds if
the g_power_monitor is NULL to avoid surprises.

[jam, 2014-02-28 21:56:37]

sorry I didn't see this earlier: but now that this is a singleton, we normally
don't shut singletons down on process shutdown. it's not harmful, but it just
adds extra code that's not necessary.

can you rename Shutdown() to be ShutdownForTesting(), and then call it in the
unittests? then remove the other shutdown calls.

lgtm with that

[willchan no longer on Chromium, 2014-02-28 22:15:21]

I started skimming this. Is this a problem with how the unit test is
constructed, or is this a problem with actual production code?

From reading the bug, it appears the problem is a race with adding observers
while the PowerMonitor is getting constructed. In production code, do we lazily
construct the PowerMonitor? I'm not sufficiently familiar with ChildThread to
know.

In many of our singletons, we have this same kind of race, but we initialize the
singleton in a single threaded context, and functions like AddObserver can only
get called after it's already been initialized. Is that not true in this case,
and is that why we want to add a lock to make sure that's safe here?

[bajones, 2014-03-04 18:55:28]

On 2014/02/28 22:15:21, willchan wrote:
> I started skimming this. Is this a problem with how the unit test is
> constructed, or is this a problem with actual production code?
> 
> From reading the bug, it appears the problem is a race with adding observers
> while the PowerMonitor is getting constructed. In production code, do we
lazily
> construct the PowerMonitor? I'm not sufficiently familiar with ChildThread to
> know.

We do not lazily construct it. The PowerMonitor is constructed during the thread
startup in ChildThread and BrowserMainLoop.

> In many of our singletons, we have this same kind of race, but we initialize
the
> singleton in a single threaded context, and functions like AddObserver can
only
> get called after it's already been initialized. Is that not true in this case,
> and is that why we want to add a lock to make sure that's safe here?

As far as I can tell the problem appears to only be when running in single
process mode. It seems that some ChildThreads are starting up before
BrowserMainLoop::MainMessageLoopStart is run? I have to admit that I don't know
Chrome's startup well enough to reliably predict the order things happen in,
especially in single process mode. This patch does ensure that accidental races
don't cause problems, though.

[Ken Russell (switch to Gerrit), 2014-03-27 19:02:10]

Ping - is this planned to be landed? On the whole it makes the PowerMonitor
instantiation cleaner, and I think it's worth landing.

[bajones, 2014-03-27 20:06:59]

On 2014/03/27 19:02:10, Ken Russell wrote:
> Ping - is this planned to be landed? On the whole it makes the PowerMonitor
> instantiation cleaner, and I think it's worth landing.

Thanks for the reminder. This fell off my radar for a bit. I still need LG on:

base/power_monitor (brettw@, willchan@)
components/nacl (bradchen@, bradnelson@)
net/url_request (mattm@)

before I can land it. I'd appreciate owner reviews/feedback!

[bradnelson, 2014-03-27 20:08:33]

LGTM for nacl

[mattm, 2014-03-28 19:32:31]

net/url_request lgtm

[bajones, 2014-03-28 22:13:11]

Missed that I also need a LG for media/audio. Could I get a review acolwell@ or
scherkus@, perhaps? (Or a recommendation for a more appropriate reviewer if
there is one?)

[acolwell GONE FROM CHROMIUM, 2014-03-28 23:45:38]

lgtm media/

[willchan no longer on Chromium, 2014-03-31 19:15:05]

I think there are improvements to be made in the interface, but I am not sure I
like all the things that have to be done simply due to the testing problems with
single process mode. Here are some thoughts and issues:

* If we're going to use statics, we should go all the way down with the class
and move all the member functions/variables out into something defined within
the .cc file. There's no need to clutter the header file with all that stuff.

* There are two advantages that I see of keeping the public member based
approach rather than public statics:
  (1) Future flexibility, which is definitely not needed yet (and hopefully we
can limit the need for libbase to become a fully fledged library which should
support needs like this). It's somewhat unfortunate to force within a class
definition the singleton property, since the singleton property could always be
done at the embedder level. On the flip side, baking the singleton property in
lower makes it easier to understand, which is probably currently more important
  (2) Testability - this public static method of shutting down for testing is
one of the ugly things about singletons that I hate. The other problem within
this is it requires adding the singleton Lock you provided, which we now have to
use everywhere. My instinct is that we should fix the test instead :(

I'm fine with moving to using statics as there's a simplicity win there in the
current Chromium embedders we support. I'm disappointed by the addition of this
lock to solve the data race you encountered which appears to only be necessary
for the test. But I guess it makes the code more future proof too and shouldn't
have much performance cost since I can't imagine us using this code in a tight
loop anywhere.

So I'd be fine with you proceeding with moving all the private members out into
the .cc file. You can encapsulate them with a file local class (PowerMonitorImpl
or something).

[bajones, 2014-04-02 22:24:20]

On 2014/03/31 19:15:05, willchan wrote:
> So I'd be fine with you proceeding with moving all the private members out
into
> the .cc file. You can encapsulate them with a file local class
(PowerMonitorImpl
> or something).

This is done now. PTAL.

[willchan no longer on Chromium, 2014-04-03 23:43:33]

I'm not very happy with the friendship relationship of PowerMonitor and
PowerMonitorSource, but this changelist doesn't make things worse, and does fix
the race condition. LGTM.

https://codereview.chromium.org/179923006/diff/100001/base/power_monitor/powe...
File base/power_monitor/power_monitor_source.cc (right):

https://codereview.chromium.org/179923006/diff/100001/base/power_monitor/powe...
base/power_monitor/power_monitor_source.cc:24: void
PowerMonitorSource::ProcessPowerEvent(PowerEvent event_id) {
Do we need to update the comments in power_monitor_source.h about which thread
this can be called on?

[bajones, 2014-04-04 17:25:42]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-04-04 17:26:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/179923006/120001

[commit-bot: I haz the power, 2014-04-04 18:10:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-04 18:10:10]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[bajones, 2014-04-04 19:43:51]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-04-04 19:44:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/179923006/140001

[commit-bot: I haz the power, 2014-04-04 20:27:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-04 20:27:31]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[bajones, 2014-04-04 21:33:33]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-04-04 21:38:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/179923006/160001

[commit-bot: I haz the power, 2014-04-04 22:14:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-04 22:14:19]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[bajones, 2014-04-04 22:49:23]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-04-04 22:49:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/179923006/160001

[commit-bot: I haz the power, 2014-04-05 01:47:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-05 01:47:57]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[bajones, 2014-04-05 16:54:18]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-04-05 16:54:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/179923006/160001

[commit-bot: I haz the power, 2014-04-05 17:28:42]

Change committed as 262018

[scottmg, 2014-04-05 18:55:25]

A revert of this CL has been created in
https://codereview.chromium.org/226263008/ by scottmg@chromium.org.

The reason for reverting is: Failing interactive_ui_tests dbg:

http://build.chromium.org/p/chromium.win/builders/Interactive%20Tests%20(dbg)....