Check to ensure PowerObservers are added and removed on the same thread

Check to ensure PowerObservers are added and removed on the same thread

This behavior is required by ObserverListThreadSafe and may leads to crashes if not followed

BUG=404767
Committed: https://crrev.com/012b3711f9b51af158598adc2f65e8b667f975a3
Cr-Commit-Position: refs/heads/master@{#293230}

[bajones, 2014-08-25 21:19:32]

bajones@chromium.org changed reviewers:
+ brettw@chromium.org, scottmg@chromium.org, thakis@chromium.org

[bajones, 2014-08-25 21:19:32]

Check will enable us to prevent removal race conditions like the one causing
https://code.google.com/p/chromium/issues/detail?id=404767

[scottmg, 2014-08-25 21:43:17]

Is ThreadChecker insufficient? (i.e. do you think it's important that this is on
in Release?)

https://codereview.chromium.org/502003003/diff/1/base/power_monitor/power_mon...
File base/power_monitor/power_monitor.cc (right):

https://codereview.chromium.org/502003003/diff/1/base/power_monitor/power_mon...
base/power_monitor/power_monitor.cc:31: CHECK (obs->power_monitor_thread_ == 0);
nit; no space before (

https://codereview.chromium.org/502003003/diff/1/base/power_monitor/power_mon...
base/power_monitor/power_monitor.cc:38: CHECK (base::PlatformThread::CurrentId()
== obs->power_monitor_thread_);
no space, and CHECK_EQ instead

[bajones, 2014-08-25 22:22:38]

On 2014/08/25 at 21:43:17, scottmg wrote:
> Is ThreadChecker insufficient? (i.e. do you think it's important that this is
on in Release?)

Didn't even know we had that! Thanks! Updated the CL.

[scottmg, 2014-08-25 22:44:54]

Normally it'd be an inline member and then .CalledOnValidThread() in Add/Remove,
but is it the case that the creation is on one thread, but the Add/Remove are on
another? If so, non-owners lgtm.

[bajones, 2014-08-25 22:48:52]

On 2014/08/25 at 22:44:54, scottmg wrote:
> Normally it'd be an inline member and then .CalledOnValidThread() in
Add/Remove, but is it the case that the creation is on one thread, but the
Add/Remove are on another? If so, non-owners lgtm.

I think that the construction and add will normally be on a single thread, but
the only thing that we need to enforce for correctness is that the add and
remove and on the same thread. As a result I felt that creating the
ThreadChecker in the add was more appropriate.

[scottmg, 2014-08-25 23:05:28]

On 2014/08/25 22:48:52, bajones wrote:
> On 2014/08/25 at 22:44:54, scottmg wrote:
> > Normally it'd be an inline member and then .CalledOnValidThread() in
> Add/Remove, but is it the case that the creation is on one thread, but the
> Add/Remove are on another? If so, non-owners lgtm.
> 
> I think that the construction and add will normally be on a single thread, but
> the only thing that we need to enforce for correctness is that the add and
> remove and on the same thread. As a result I felt that creating the
> ThreadChecker in the add was more appropriate.

sgtm, paging brettw.

[brettw, 2014-08-26 18:03:34]

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

https://codereview.chromium.org/502003003/diff/20001/base/power_monitor/power...
base/power_monitor/power_observer.h:32: scoped_ptr<base::ThreadChecker>
thread_checker_;
A lot of stuff derives from this class. Having this object be both protected and
have a general name like this seems potentially confusing to derived classes.

Can this be private and have a name that implies it's for checking observer
adds/removes (a comment is probably also a good idea).

[bajones, 2014-08-26 20:08:39]

On 2014/08/26 at 18:03:34, brettw wrote:
>
https://codereview.chromium.org/502003003/diff/20001/base/power_monitor/power...
> File base/power_monitor/power_observer.h (right):
> 
>
https://codereview.chromium.org/502003003/diff/20001/base/power_monitor/power...
> base/power_monitor/power_observer.h:32: scoped_ptr<base::ThreadChecker>
thread_checker_;
> A lot of stuff derives from this class. Having this object be both protected
and have a general name like this seems potentially confusing to derived
classes.
> 
> Can this be private and have a name that implies it's for checking observer
adds/removes (a comment is probably also a good idea).

Great feedback, thanks! Fixed.

[brettw, 2014-08-26 20:25:39]

lgtm

https://codereview.chromium.org/502003003/diff/60001/base/power_monitor/power...
File base/power_monitor/power_observer.h (right):

https://codereview.chromium.org/502003003/diff/60001/base/power_monitor/power...
base/power_monitor/power_observer.h:33: 
Nit: no blank here.

[bajones, 2014-08-26 20:29:34]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-08-26 20:30:37]

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

[bajones, 2014-08-26 21:02:56]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-08-26 21:04:30]

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

[commit-bot: I haz the power, 2014-08-26 22:30:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-26 22:37:05]

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

[commit-bot: I haz the power, 2014-08-26 22:37:06]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bajones, 2014-09-03 18:22:32]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-09-03 18:23:37]

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

[commit-bot: I haz the power, 2014-09-03 19:39:30]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[commit-bot: I haz the power, 2014-09-03 20:17:18]

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

[commit-bot: I haz the power, 2014-09-03 20:17:19]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bajones, 2014-09-03 23:12:53]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-09-03 23:23:14]

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

[commit-bot: I haz the power, 2014-09-04 00:12:04]

Committed patchset #7 (id:120001) as 518c46fbba70c91169bfc0c64b542f06e56b3915

[dcheng, 2014-09-05 05:04:48]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/544913003/ by dcheng@chromium.org.

The reason for reverting is: Causing failures on Mac try jobs:
ExtensionServiceTest.ExternalExtensionAutoAcknowledgement (run #1):
[ RUN      ] ExtensionServiceTest.ExternalExtensionAutoAcknowledgement
[8427:1799:0904/211500:4724222818698:FATAL:power_monitor.cc(38)] Check failed:
obs->power_monitor_thread_checker_..

[commit-bot: I haz the power, 2014-09-10 03:28:51]

Patchset 7 (id:??) landed as
https://crrev.com/012b3711f9b51af158598adc2f65e8b667f975a3
Cr-Commit-Position: refs/heads/master@{#293230}