Initialize data reduction proxy bypass stats on IO thread

Initialize data reduction proxy bypass stats on IO thread

Initialize data reduction proxy (DRP) bypass stats on IO thread, and
register as a network change notifier on the IO thread. This ensures
that network change notifier does not notify DRP bypass stats on IO
thread after DRP bypass stats has been destroyed. This prevents the
use after null crash.

Also, add thread checker to some of the other DRP classes.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=669929
Committed: https://crrev.com/7877436e368495c0ce10eb32a65748079936be92
Cr-Commit-Position: refs/heads/master@{#436101}

[tbansal1, 2016-12-02 07:24:34]

Description was changed from

==========
Init bypass stats on IO thread

BUG=
==========

to

==========
Init bypass stats on IO thread


CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=
==========

[tbansal1, 2016-12-02 07:46:12]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 07:46:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 08:33:37]

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

[commit-bot: I haz the power, 2016-12-02 08:33:38]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-12-02 17:41:38]

Patchset #1 (id:1) has been deleted

[tbansal1, 2016-12-02 17:41:45]

Patchset #1 (id:20001) has been deleted

[tbansal1, 2016-12-02 17:46:37]

Description was changed from

==========
Init bypass stats on IO thread


CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=
==========

to

==========
Initialize data reduction proxy bypass stats on IO thread

Initialize data reduction proxy (DRP) bypass stats on IO thread, and register as
a network change notifier on the IO thread. This ensures 
that network change notifier does not notify DRP bypass stats on IO 
thread after DRP bypass stats has been destroyed. This prevents the
use after null crash.

Also, add thread checker to some of the other DRP classes.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=
==========

[tbansal1, 2016-12-02 17:47:14]

Description was changed from

==========
Initialize data reduction proxy bypass stats on IO thread

Initialize data reduction proxy (DRP) bypass stats on IO thread, and register as
a network change notifier on the IO thread. This ensures 
that network change notifier does not notify DRP bypass stats on IO 
thread after DRP bypass stats has been destroyed. This prevents the
use after null crash.

Also, add thread checker to some of the other DRP classes.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=
==========

to

==========
Initialize data reduction proxy bypass stats on IO thread

Initialize data reduction proxy (DRP) bypass stats on IO thread, and 
register as a network change notifier on the IO thread. This ensures 
that network change notifier does not notify DRP bypass stats on IO 
thread after DRP bypass stats has been destroyed. This prevents the
use after null crash.

Also, add thread checker to some of the other DRP classes.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=669929
==========

[tbansal1, 2016-12-02 17:47:31]

tbansal@chromium.org changed reviewers:
+ ryansturm@chromium.org

[tbansal1, 2016-12-02 17:47:31]

ryansturm: ptal. Thanks.

[RyanSturm, 2016-12-02 21:24:18]

On 2016/12/02 17:47:31, tbansal1 wrote:
> ryansturm: ptal. Thanks.

I have a copy of this, but feel free to leave this up when you leave. I'll close
it eventually.

[RyanSturm, 2016-12-02 21:24:49]

On 2016/12/02 21:24:18, Ryan Sturm wrote:
> On 2016/12/02 17:47:31, tbansal1 wrote:
> > ryansturm: ptal. Thanks.
> 
> I have a copy of this, but feel free to leave this up when you leave. I'll
close
> it eventually.

[RyanSturm, 2016-12-02 21:25:12]

On 2016/12/02 21:24:49, Ryan Sturm wrote:
> On 2016/12/02 21:24:18, Ryan Sturm wrote:
> > On 2016/12/02 17:47:31, tbansal1 wrote:
> > > ryansturm: ptal. Thanks.
> > 
> > I have a copy of this, but feel free to leave this up when you leave. I'll
> close
> > it eventually.

Oops. That was meant for a different CL.

[RyanSturm, 2016-12-02 21:43:09]

lgtm % comment

https://codereview.chromium.org/2546023002/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.cc
(right):

https://codereview.chromium.org/2546023002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.cc:142:
NetworkChangeNotifier::RemoveNetworkChangeObserver(this);
So if the IO thread exists, this will be removed on the IO thread, which matches
the thread it is created on.

If this is removed on the UI thread, there won't be a UAF as the IO thread won't
exist anymore, but at the same time, there will be an outstanding pointer in the
IO thread observer list.

I suppose it's not terrible if there is a list of IO observer pointers during
shutdown.

It's not strictly necessary to call RemoveNetworkChangeObserver when this is on
the UI thread, as it doesn't actually remove it, but there is no harm there.

Maybe add a comment about some of this.

[tbansal1, 2016-12-02 23:50:42]

https://codereview.chromium.org/2546023002/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.cc
(right):

https://codereview.chromium.org/2546023002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.cc:142:
NetworkChangeNotifier::RemoveNetworkChangeObserver(this);
On 2016/12/02 21:43:09, Ryan Sturm wrote:
> So if the IO thread exists, this will be removed on the IO thread, which
matches
> the thread it is created on.
> 
> If this is removed on the UI thread, there won't be a UAF as the IO thread
won't
> exist anymore, but at the same time, there will be an outstanding pointer in
the
> IO thread observer list.
> 
> I suppose it's not terrible if there is a list of IO observer pointers during
> shutdown.
> 
> It's not strictly necessary to call RemoveNetworkChangeObserver when this is
on
> the UI thread, as it doesn't actually remove it, but there is no harm there.
> 
> Maybe add a comment about some of this.

I think this is now same as other DRP classes that have Init method, that move
from UI to IO thread. So we do not really need to say more. I did add a comment
to DRP IO Data based on what we learned recently.

[tbansal1, 2016-12-02 23:58:40]

The CQ bit was checked by tbansal@chromium.org

[commit-bot: I haz the power, 2016-12-02 23:59:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-03 00:39:14]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480723120831490,
"parent_rev": "201c70684f446566ab4dc09d1a7e9cb1fe5bd3a7", "commit_rev":
"c5f37a9a456e3d67369a725a14e6623914103972"}

[commit-bot: I haz the power, 2016-12-03 00:39:48]

Description was changed from

==========
Initialize data reduction proxy bypass stats on IO thread

Initialize data reduction proxy (DRP) bypass stats on IO thread, and 
register as a network change notifier on the IO thread. This ensures 
that network change notifier does not notify DRP bypass stats on IO 
thread after DRP bypass stats has been destroyed. This prevents the
use after null crash.

Also, add thread checker to some of the other DRP classes.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=669929
==========

to

==========
Initialize data reduction proxy bypass stats on IO thread

Initialize data reduction proxy (DRP) bypass stats on IO thread, and 
register as a network change notifier on the IO thread. This ensures 
that network change notifier does not notify DRP bypass stats on IO 
thread after DRP bypass stats has been destroyed. This prevents the
use after null crash.

Also, add thread checker to some of the other DRP classes.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=669929
==========

[commit-bot: I haz the power, 2016-12-03 00:39:48]

Committed patchset #1 (id:40001)

[commit-bot: I haz the power, 2016-12-03 00:43:06]

Description was changed from

==========
Initialize data reduction proxy bypass stats on IO thread

Initialize data reduction proxy (DRP) bypass stats on IO thread, and 
register as a network change notifier on the IO thread. This ensures 
that network change notifier does not notify DRP bypass stats on IO 
thread after DRP bypass stats has been destroyed. This prevents the
use after null crash.

Also, add thread checker to some of the other DRP classes.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=669929
==========

to

==========
Initialize data reduction proxy bypass stats on IO thread

Initialize data reduction proxy (DRP) bypass stats on IO thread, and
register as a network change notifier on the IO thread. This ensures
that network change notifier does not notify DRP bypass stats on IO
thread after DRP bypass stats has been destroyed. This prevents the
use after null crash.

Also, add thread checker to some of the other DRP classes.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=669929

Committed: https://crrev.com/7877436e368495c0ce10eb32a65748079936be92
Cr-Commit-Position: refs/heads/master@{#436101}
==========

[commit-bot: I haz the power, 2016-12-03 00:43:07]

Patchset 1 (id:??) landed as
https://crrev.com/7877436e368495c0ce10eb32a65748079936be92
Cr-Commit-Position: refs/heads/master@{#436101}