[Re-land] Add network throttling as enterprise policy

[Re-land] Add network throttling as enterprise policy

This is a re-land of codereview.chromium.org/2364703002/
which was reverted in https://codereview.chromium.org/2461383002/
due to a memory leak in network_throttling_observer_unittest causing
the long-running linux-chromeos ASAN bot to fail.
All files stay the same except for the policy enum advancing to 351 from 350 (350 was taken while this CL was reverted) and memory leak fixes to network_throttling_observer_unittest.cc

Add support for an enterprise policy that lets the admin
turn on network throtttling and specify the upload/download
rates in kbps to throttle to.

BUG=642811
TBR=atwilson@chromium.org,brettw@chromium.org,stevenjb@chromium.org
TEST=(1) unit-tests (2) Added network policy to YAPS
server, enabled throttling with various values, disabled
throttling and checked that the values propagate to client

Committed: https://crrev.com/857eeac9ace6b79bd8b99fceac315455ee82f617
Cr-Commit-Position: refs/heads/master@{#430082}

[kirtika1, 2016-10-31 19:58:59]

The CQ bit was checked by kirtika@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 19:59:29]

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-10-31 22:39:14]

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

[commit-bot: I haz the power, 2016-10-31 22:39:15]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-11-03 19:38:38]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2016-11-03 19:38:39]

https://codereview.chromium.org/2466693002/diff/1/chrome/browser/chromeos/net...
File chrome/browser/chromeos/net/network_throttling_observer_unittest.cc
(right):

https://codereview.chromium.org/2466693002/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_throttling_observer_unittest.cc:1: //
Copyright (c) 2016 The Chromium Authors. All rights reserved.
nit: no (c)

https://codereview.chromium.org/2466693002/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_throttling_observer_unittest.cc:31:
DBusThreadManager::GetSetterForTesting();
Previously, you had:

DBusThreadManager::Initialize();
DBusThreadManager::GetSetterForTesting();

That's 2 DBusThreadManager initializations / 0 shutdowns.
Now you have 2 DBusThreadManager initializations / 1 shutdowns -> still leaking.

Drop the DBusThreadManager::Initialize(), you don't need it.

https://codereview.chromium.org/2466693002/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_throttling_observer_unittest.cc:36:
NetworkStateHandler::InitializeForTest();
You need to take the returned pointer and free it later. Look at every existing
caller and see what they do. Note that you have to explicitly free it before
DBusThreadManager::Shutdown() happens.

https://codereview.chromium.org/2466693002/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_throttling_observer_unittest.cc:38:
local_state_ = new TestingPrefServiceSimple();
Who frees this? Did you try freeing it before? Did that fail and then you
decided it was "better" to not free it?

You should turn |local_state_| into an unique_ptr. However, the tricky bit here
is that |observer_| depends on |local_state_|, so you have to declare
|local_state_| before |observer_|. That way, when the class gets destroyed,
|observer_| gets destroyed first. The other way around does not work and results
in an use-after-free.

https://codereview.chromium.org/2466693002/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_throttling_observer_unittest.cc:49:
std::unique_ptr<base::MessageLoop> message_loop_;
Just drop the std::unique_ptr.

[kirtika1, 2016-11-04 00:27:07]

The CQ bit was checked by kirtika@google.com to run a CQ dry run

[kirtika1, 2016-11-04 00:27:33]

https://codereview.chromium.org/2466693002/diff/1/chrome/browser/chromeos/net...
File chrome/browser/chromeos/net/network_throttling_observer_unittest.cc
(right):

https://codereview.chromium.org/2466693002/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_throttling_observer_unittest.cc:1: //
Copyright (c) 2016 The Chromium Authors. All rights reserved.
On 2016/11/03 19:38:39, Lei Zhang wrote:
> nit: no (c) 

Done.

https://codereview.chromium.org/2466693002/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_throttling_observer_unittest.cc:31:
DBusThreadManager::GetSetterForTesting();
On 2016/11/03 19:38:39, Lei Zhang wrote:
> Previously, you had:
> 
> DBusThreadManager::Initialize();
> DBusThreadManager::GetSetterForTesting();
> 
> That's 2 DBusThreadManager initializations / 0 shutdowns.
> Now you have 2 DBusThreadManager initializations / 1 shutdowns -> still
leaking.
> 
> Drop the DBusThreadManager::Initialize(), you don't need it.

Done.

https://codereview.chromium.org/2466693002/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_throttling_observer_unittest.cc:36:
NetworkStateHandler::InitializeForTest();
On 2016/11/03 19:38:39, Lei Zhang wrote:
> You need to take the returned pointer and free it later. Look at every
existing
> caller and see what they do. Note that you have to explicitly free it before
> DBusThreadManager::Shutdown() happens.

Done.

https://codereview.chromium.org/2466693002/diff/1/chrome/browser/chromeos/net...
chrome/browser/chromeos/net/network_throttling_observer_unittest.cc:49:
std::unique_ptr<base::MessageLoop> message_loop_;
On 2016/11/03 19:38:39, Lei Zhang wrote:
> Just drop the std::unique_ptr.

Done.

[commit-bot: I haz the power, 2016-11-04 00:28:38]

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

[Lei Zhang, 2016-11-04 00:44:19]

https://codereview.chromium.org/2466693002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/net/network_throttling_observer_unittest.cc
(right):

https://codereview.chromium.org/2466693002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_throttling_observer_unittest.cc:37:
local_state_.reset(new TestingPrefServiceSimple());
Prefer: local_state_ = base::MakeUnique<TestingPrefServiceSimple>();

https://codereview.chromium.org/2466693002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_throttling_observer_unittest.cc:38:
local_state_.get()->registry()->RegisterDictionaryPref(
You don't need the ".get()" here. std::unique_ptr has an operator->().

https://codereview.chromium.org/2466693002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_throttling_observer_unittest.cc:44:
observer_.reset();
These 2 are probably not necessary, but probably can't hurt.

https://codereview.chromium.org/2466693002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/net/network_throttling_observer_unittest.cc:52:
base::MessageLoop* message_loop_;
This is not what I mean, just declare this as: base::MessageLoop message_loop_;

[Lei Zhang, 2016-11-04 00:46:36]

I think patch set 2 should not leak. Please remember to fix up the CL subject
line / description when you are ready, and send it to your original reviewers
for another review. Please refer to https://codereview.chromium.org/2465743003/
in the CL description, so readers can understand what happened previously.

[commit-bot: I haz the power, 2016-11-04 02:26:43]

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

[commit-bot: I haz the power, 2016-11-04 02:26:44]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[kirtika1, 2016-11-04 03:19:44]

Description was changed from

==========
[Draft only] Investigating memory leak issue in NetworkThrottlingObserverTest
==========

to

==========
This is a re-land of codereview.chromium.org/2364703002/
which was reverted in https://codereview.chromium.org/2461383002/ 
due to a memory leak in network_throttling_observer_unittest causing 
the long-running linux-chromeos ASAN bot to fail. 
All files stay the same except for the policy enum advancing to 351 from 350
(350 was taken while this CL was reverted) and memory leak fixes to
network_throttling_observer_unittest.cc

Add support for an enterprise policy that lets the admin
turn on network throtttling and specify the upload/download
rates in kbps to throttle to.

BUG=642811
TEST=(1) unit-tests (2) Added network policy to YAPS
server, enabled throttling with various values, disabled
throttling and checked that the values propagate to client
==========

[kirtika1, 2016-11-04 03:20:06]

The CQ bit was checked by kirtika@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 03:20:42]

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

[kirtika1, 2016-11-04 03:20:54]

On 2016/11/04 00:46:36, Lei Zhang wrote:
> I think patch set 2 should not leak. Please remember to fix up the CL subject
> line / description when you are ready, and send it to your original reviewers
> for another review. Please refer to
https://codereview.chromium.org/2465743003/
> in the CL description, so readers can understand what happened previously.

Added PS#3 with your corrections, and updated description.

[kirtika1, 2016-11-04 03:21:46]

kirtika@google.com changed reviewers:
+ atwilson@chromium.org, brettw@chromium.org, gab@chromium.org,
isherman@chromium.org, mnissler@chromium.org, stevenjb@chromium.org

[kirtika1, 2016-11-04 03:28:06]

On 2016/11/04 03:20:54, kirtika1 wrote:
> On 2016/11/04 00:46:36, Lei Zhang wrote:
> > I think patch set 2 should not leak. Please remember to fix up the CL
subject
> > line / description when you are ready, and send it to your original
reviewers
> > for another review. Please refer to
> https://codereview.chromium.org/2465743003/
> > in the CL description, so readers can understand what happened previously.
> 
> Added PS#3 with your corrections, and updated description.

Adding reviewers from https://codereview.chromium.org/2465743003/
for a re-LGTM. All files stay the same, except
network_throttling_observer_unittest.cc

Seeking LGTMs from - 

atwilson@: 
  chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc
  chrome/browser/chromeos/policy/proto/chrome_device_policy.proto
  components/policy/resources/policy_templates.json

stevenjb@:
  chrome/browser/chromeos/chrome_browser_main_chromeos.cc
  chrome/browser/chromeos/chrome_browser_main_chromeos.h
  chrome/browser/chromeos/net/network_throttling_observer.cc
  chrome/browser/chromeos/net/network_throttling_observer.h
  chrome/browser/chromeos/net/network_throttling_observer_unittest.cc
  chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc
  chrome/browser/chromeos/policy/proto/chrome_device_policy.proto
  chromeos/dbus/fake_shill_manager_client.cc
  chromeos/dbus/fake_shill_manager_client.h
  chromeos/dbus/mock_shill_manager_client.h
  chromeos/dbus/shill_manager_client.cc
  chromeos/dbus/shill_manager_client.h
  chromeos/dbus/shill_manager_client_unittest.cc
  chromeos/network/network_state_handler.cc
  chromeos/network/network_state_handler.h
  chromeos/network/shill_property_handler.cc
  chromeos/network/shill_property_handler.h

brettw@: 
  chrome/browser/policy/configuration_policy_handler_list_factory.cc

isherman@:
  tools/metrics/histograms/histograms.xml

gab@:
  chrome/browser/prefs/browser_prefs.cc

[commit-bot: I haz the power, 2016-11-04 05:29:11]

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

[commit-bot: I haz the power, 2016-11-04 05:29:12]

Dry run: This issue passed the CQ dry run.

[gab, 2016-11-04 14:35:20]

c/b/prefs lgtm % comment

https://codereview.chromium.org/2466693002/diff/40001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/2466693002/diff/40001/chrome/common/pref_name...
chrome/common/pref_names.h:496: extern const char kNetworkThrottlingEnabled[];
This should also be in an OS_CHROMEOS ifdef block.

[Ilya Sherman, 2016-11-04 18:37:40]

histograms.xml lgtm (and it's generally fine to TBR owners for relands, when the
code they reviewed hasn't changed)

[kirtika1, 2016-11-04 21:16:59]

Description was changed from

==========
This is a re-land of codereview.chromium.org/2364703002/
which was reverted in https://codereview.chromium.org/2461383002/ 
due to a memory leak in network_throttling_observer_unittest causing 
the long-running linux-chromeos ASAN bot to fail. 
All files stay the same except for the policy enum advancing to 351 from 350
(350 was taken while this CL was reverted) and memory leak fixes to
network_throttling_observer_unittest.cc

Add support for an enterprise policy that lets the admin
turn on network throtttling and specify the upload/download
rates in kbps to throttle to.

BUG=642811
TEST=(1) unit-tests (2) Added network policy to YAPS
server, enabled throttling with various values, disabled
throttling and checked that the values propagate to client
==========

to

==========
[Re-land] Add network throttling as enterprise policy

This is a re-land of codereview.chromium.org/2364703002/
which was reverted in https://codereview.chromium.org/2461383002/ 
due to a memory leak in network_throttling_observer_unittest causing 
the long-running linux-chromeos ASAN bot to fail. 
All files stay the same except for the policy enum advancing to 351 from 350
(350 was taken while this CL was reverted) and memory leak fixes to
network_throttling_observer_unittest.cc

Add support for an enterprise policy that lets the admin
turn on network throtttling and specify the upload/download
rates in kbps to throttle to.

BUG=642811
TBR=atwilson@chromium.org,brettw@chromium.org,stevenjb@chromium.org
TEST=(1) unit-tests (2) Added network policy to YAPS
server, enabled throttling with various values, disabled
throttling and checked that the values propagate to client
==========

[Lei Zhang, 2016-11-04 21:17:20]

lgtm

[kirtika1, 2016-11-04 21:18:40]

The CQ bit was checked by kirtika@google.com

[kirtika1, 2016-11-04 21:18:41]

The patchset sent to the CQ was uploaded after l-g-t-m from kirtika@google.com,
gab@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2466693002/#ps60001
(title: "[Re-land] Add network throttling as enterprise policy")

[commit-bot: I haz the power, 2016-11-04 21:19:18]

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-11-04 22:06:01]

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

[commit-bot: I haz the power, 2016-11-04 22:06:03]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Lei Zhang, 2016-11-04 22:11:57]

https://codereview.chromium.org/2466693002/diff/60001/chrome/browser/policy/c...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/2466693002/diff/60001/chrome/browser/policy/c...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:607: {
key::kNetworkThrottlingEnabled,
Also needs to be ifdef...

[kirtika1, 2016-11-04 22:21:04]

The CQ bit was checked by kirtika@google.com

[kirtika1, 2016-11-04 22:21:04]

The patchset sent to the CQ was uploaded after l-g-t-m from kirtika@google.com,
thestig@chromium.org, gab@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2466693002/#ps80001
(title: "[Re-land] Add network throttling as enterprise policy")

[commit-bot: I haz the power, 2016-11-04 22:21:48]

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

[stevenjb, 2016-11-04 22:36:45]

rs lgtm

[commit-bot: I haz the power, 2016-11-05 00:24:57]

Description was changed from

==========
[Re-land] Add network throttling as enterprise policy

This is a re-land of codereview.chromium.org/2364703002/
which was reverted in https://codereview.chromium.org/2461383002/ 
due to a memory leak in network_throttling_observer_unittest causing 
the long-running linux-chromeos ASAN bot to fail. 
All files stay the same except for the policy enum advancing to 351 from 350
(350 was taken while this CL was reverted) and memory leak fixes to
network_throttling_observer_unittest.cc

Add support for an enterprise policy that lets the admin
turn on network throtttling and specify the upload/download
rates in kbps to throttle to.

BUG=642811
TBR=atwilson@chromium.org,brettw@chromium.org,stevenjb@chromium.org
TEST=(1) unit-tests (2) Added network policy to YAPS
server, enabled throttling with various values, disabled
throttling and checked that the values propagate to client
==========

to

==========
[Re-land] Add network throttling as enterprise policy

This is a re-land of codereview.chromium.org/2364703002/
which was reverted in https://codereview.chromium.org/2461383002/ 
due to a memory leak in network_throttling_observer_unittest causing 
the long-running linux-chromeos ASAN bot to fail. 
All files stay the same except for the policy enum advancing to 351 from 350
(350 was taken while this CL was reverted) and memory leak fixes to
network_throttling_observer_unittest.cc

Add support for an enterprise policy that lets the admin
turn on network throtttling and specify the upload/download
rates in kbps to throttle to.

BUG=642811
TBR=atwilson@chromium.org,brettw@chromium.org,stevenjb@chromium.org
TEST=(1) unit-tests (2) Added network policy to YAPS
server, enabled throttling with various values, disabled
throttling and checked that the values propagate to client
==========

[commit-bot: I haz the power, 2016-11-05 00:24:59]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-05 00:27:54]

Description was changed from

==========
[Re-land] Add network throttling as enterprise policy

This is a re-land of codereview.chromium.org/2364703002/
which was reverted in https://codereview.chromium.org/2461383002/ 
due to a memory leak in network_throttling_observer_unittest causing 
the long-running linux-chromeos ASAN bot to fail. 
All files stay the same except for the policy enum advancing to 351 from 350
(350 was taken while this CL was reverted) and memory leak fixes to
network_throttling_observer_unittest.cc

Add support for an enterprise policy that lets the admin
turn on network throtttling and specify the upload/download
rates in kbps to throttle to.

BUG=642811
TBR=atwilson@chromium.org,brettw@chromium.org,stevenjb@chromium.org
TEST=(1) unit-tests (2) Added network policy to YAPS
server, enabled throttling with various values, disabled
throttling and checked that the values propagate to client
==========

to

==========
[Re-land] Add network throttling as enterprise policy

This is a re-land of codereview.chromium.org/2364703002/
which was reverted in https://codereview.chromium.org/2461383002/
due to a memory leak in network_throttling_observer_unittest causing
the long-running linux-chromeos ASAN bot to fail.
All files stay the same except for the policy enum advancing to 351 from 350
(350 was taken while this CL was reverted) and memory leak fixes to
network_throttling_observer_unittest.cc

Add support for an enterprise policy that lets the admin
turn on network throtttling and specify the upload/download
rates in kbps to throttle to.

BUG=642811
TBR=atwilson@chromium.org,brettw@chromium.org,stevenjb@chromium.org
TEST=(1) unit-tests (2) Added network policy to YAPS
server, enabled throttling with various values, disabled
throttling and checked that the values propagate to client

Committed: https://crrev.com/857eeac9ace6b79bd8b99fceac315455ee82f617
Cr-Commit-Position: refs/heads/master@{#430082}
==========

[commit-bot: I haz the power, 2016-11-05 00:27:56]

Patchset 5 (id:??) landed as
https://crrev.com/857eeac9ace6b79bd8b99fceac315455ee82f617
Cr-Commit-Position: refs/heads/master@{#430082}

[gab, 2016-11-05 01:19:40]

https://codereview.chromium.org/2466693002/diff/80001/chrome/common/pref_name...
File chrome/common/pref_names.cc (left):

https://codereview.chromium.org/2466693002/diff/80001/chrome/common/pref_name...
chrome/common/pref_names.cc:1851: const char kReportArcStatusEnabled[] =
"arc.status_reporting_enabled";
It should be here, to match header file re-use the existing ifdef.

[gab, 2016-11-07 22:36:16]

On 2016/11/05 01:19:40, gab wrote:
>
https://codereview.chromium.org/2466693002/diff/80001/chrome/common/pref_name...
> File chrome/common/pref_names.cc (left):
> 
>
https://codereview.chromium.org/2466693002/diff/80001/chrome/common/pref_name...
> chrome/common/pref_names.cc:1851: const char kReportArcStatusEnabled[] =
> "arc.status_reporting_enabled";
> It should be here, to match header file re-use the existing ifdef.

ping

[Lei Zhang, 2016-11-09 21:39:44]

On 2016/11/07 22:36:16, gab wrote:
> On 2016/11/05 01:19:40, gab wrote:
> >
>
https://codereview.chromium.org/2466693002/diff/80001/chrome/common/pref_name...
> > File chrome/common/pref_names.cc (left):
> > 
> >
>
https://codereview.chromium.org/2466693002/diff/80001/chrome/common/pref_name...
> > chrome/common/pref_names.cc:1851: const char kReportArcStatusEnabled[] =
> > "arc.status_reporting_enabled";
> > It should be here, to match header file re-use the existing ifdef.
> 
> ping

Did y'all resolve this in a separate CL?

[kirtika1, 2016-11-09 21:47:00]

> 
> Did y'all resolve this in a separate CL?

I've been OOO until yesterday, will send out a new CL for this.

[kirtika1, 2016-11-10 21:39:06]

On 2016/11/07 22:36:16, gab wrote:
> On 2016/11/05 01:19:40, gab wrote:
> >
>
https://codereview.chromium.org/2466693002/diff/80001/chrome/common/pref_name...
> > File chrome/common/pref_names.cc (left):
> > 
> >
>
https://codereview.chromium.org/2466693002/diff/80001/chrome/common/pref_name...
> > chrome/common/pref_names.cc:1851: const char kReportArcStatusEnabled[] =
> > "arc.status_reporting_enabled";
> > It should be here, to match header file re-use the existing ifdef.
> 
> ping

Added to
https://codereview.chromium.org/2492043002/