Fix cyclic dependency between ProfilePolicyConnector and PrefService.

Fix cyclic dependency between ProfilePolicyConnector and PrefService.

PolicyCertVerifier lives (at least partially) on IO to provide certificate trust anchors to the net subsystem.
The list of trust anchors is provided by the policy subsystem, which lives on UI.
On each usage of one of the trust anchors, the profile must be tainted by setting a specific Pref value, which must happen on UI.

There were several problems and bugs, all of which are solved with this CL:
- NetworkConfigurationUpdater keeps a PolicyCertVerifier* until destruction, although PolicyCertVerifier is invalidated earlier and destructed in parallel on IO.
- Instead of explicitly managing lifetime/dependencies, PolicyCertVerifier accesses ProfilePolicyConnector through a WeakPtr.
- Cyclic static dependency between ProfilePolicyConnector and PrefService.
- Each, the original profile and the OffTheRecordProfile (OTRProfile), have a separate PolicyCertVerifier instance. The ProfilePolicyConnector/UserNetworkConfigurationUpdater are shared however. This wasn't considered during the implementation of the latter. They only have a SetPolicyCertVerifier instead of a AddPolicyCertVerifier method.
- ProfilePolicyConnector stores a Profile* instead of a PrefService* .

This CL moves the certificate related parts out of ProfilePolicyConnector and puts them into a separate keyed service NetworkPolicyService (living on UI, taking care of syncing with IO) which is tightly coupled with the PolicyCertVerifier (purely living on IO).

The new dependencies are:
NetworkConfigurationUpdater --(Observer::OnTrustAnchorsChanged)-> NetworkPolicyService --(post to IO)-> PolicyCertVerifier --(run callback, post to UI)-> NetworkPolicyService

For a summary of the dependencies see the accompanying bug.

Depends on:
https://codereview.chromium.org/53923004/

BUG=312660, 77155
TBR=ben@chromium.org,jcivelli@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234806

[pneubeck (no reviews), 2013-09-19 13:55:38]

I still have to see if all tests are passing and maybe add/update some comments.

[Mattias Nissler (ping if slow), 2013-09-20 12:35:23]

https://chromiumcodereview-hr.appspot.com/24153012/diff/49001/chrome/browser/...
File chrome/browser/chromeos/policy/network_policy_service.cc (right):

https://chromiumcodereview-hr.appspot.com/24153012/diff/49001/chrome/browser/...
chrome/browser/chromeos/policy/network_policy_service.cc:40: // Allow trusted
certs from policy only for managed regular accounts.
While at it, can you clarify the comment to say "only for accounts with managed
user affiliation, i.e users that are managed by the same domain as the device."

https://chromiumcodereview-hr.appspot.com/24153012/diff/49001/chrome/browser/...
chrome/browser/chromeos/policy/network_policy_service.cc:47: // the Updater is
destructed during Shutdown.
Why? This comment doesn't add any information, just points out the obvious.

https://chromiumcodereview-hr.appspot.com/24153012/diff/49001/chrome/browser/...
File chrome/browser/chromeos/policy/network_policy_service.h (right):

https://chromiumcodereview-hr.appspot.com/24153012/diff/49001/chrome/browser/...
chrome/browser/chromeos/policy/network_policy_service.h:32:
NetworkPolicyService(Profile* profile, PolicyService* policy_service);
You actually don't need a full profile, please pass a PrefService here.

For background, passing a Profile* makes it easy for people to create cyclic
references between BCKS, which is why I'm advising everybody to pass
dependencies explicitly to the ctor.

https://chromiumcodereview-hr.appspot.com/24153012/diff/49001/chrome/browser/...
chrome/browser/chromeos/policy/network_policy_service.h:43: void
SetUsedPolicyCertificatesOnce();
Why is this public?

https://chromiumcodereview-hr.appspot.com/24153012/diff/49001/chrome/browser/...
chrome/browser/chromeos/policy/network_policy_service.h:47: // the UI thread)
even after this Connector is destructed.
I'm still feeling a bit uneasy about the weak ptr here, since it's just a way to
work around a lifetime mismatch problem that's caused elsewhere (i.e. Profile
initialization / shutdown). I think a better way would be for
NetworkPolicyService to generate notifications that NetworkPolicyService can
listen for. That's now very easy with base/callback_registry.h, so you might
want to give that a try.

https://chromiumcodereview-hr.appspot.com/24153012/diff/49001/chrome/browser/...
File chrome/browser/profiles/profile_io_data.cc (right):

https://chromiumcodereview-hr.appspot.com/24153012/diff/49001/chrome/browser/...
chrome/browser/profiles/profile_io_data.cc:231: DCHECK(service);
Remove, you're dereferencing the pointer anyways.

[pneubeck (no reviews), 2013-09-20 12:39:36]

https://codereview.chromium.org/24153012/diff/49001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_policy_service.h (right):

https://codereview.chromium.org/24153012/diff/49001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service.h:47: // the UI thread)
even after this Connector is destructed.
On 2013/09/20 12:35:23, Mattias Nissler wrote:
> I'm still feeling a bit uneasy about the weak ptr here, since it's just a way
to
> work around a lifetime mismatch problem that's caused elsewhere (i.e. Profile
> initialization / shutdown). I think a better way would be for
> NetworkPolicyService to generate notifications that NetworkPolicyService can
> listen for. That's now very easy with base/callback_registry.h, so you might
> want to give that a try.

Who should generate and who should listen? (your comment seems to contain a
typo)

[Joao da Silva, 2013-09-20 13:00:01]

This is quite a nice cleanup. First round of comments!

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_policy_service.cc (right):

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service.cc:25: :
user_prefs_(profile->GetPrefs()), weak_ptr_factory_(this) {
Pass the Prefs in the ctor directly

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service.cc:27: return;
How about doing this check at the factory, and not creating the service at all
for the signin profile?

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service.cc:35: return;
Move this to the factory as well?

That would mean that this service can sometimes be NULL, which I think is a good
thing; if a Profile can't have injected certs then this shouldn't exist.

The callback for the PolicyCertVerifier of Profiles that don't have this should
just CHECK, since they should never use trust anchors.

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service.cc:86:
network_configuration_updater_.reset();
weak_ptr_factory_.InvalidateWeakPtrs()?

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_policy_service.h (right):

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service.h:19: class
CertTrustAnchorProvider;
Not used

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service.h:30: class
NetworkPolicyService : public BrowserContextKeyedService {
Document

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service.h:43: void
SetUsedPolicyCertificatesOnce();
Can't this be private?

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_policy_service_factory.cc (right):

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service_factory.cc:8: #include
"base/prefs/pref_service.h"
Not used

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service_factory.cc:14: #include
"chrome/common/pref_names.h"
Not used

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_policy_service_factory.h (right):

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service_factory.h:19: // Profiles.
Update this comment :-)

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/policy/pro...
File chrome/browser/policy/profile_policy_connector.cc (right):

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/policy/pro...
chrome/browser/policy/profile_policy_connector.cc:35: {
{}

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/policy/pro...
chrome/browser/policy/profile_policy_connector.cc:55: if
(chromeos::ProfileHelper::IsSigninProfile(profile)) {
The |profile| is passed in just for this call. I suggest passing in a boolean
"is_signin_profile" instead, and making this test at the
ProfilePolicyConnectorFactory.

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/policy/pro...
File chrome/browser/policy/profile_policy_connector_factory.cc (right):

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/policy/pro...
chrome/browser/policy/profile_policy_connector_factory.cc:41:
CloudPolicyManager* user_cloud_policy_manager,
I don't think this should come in  in this call.

This factory declares that it depends on
UserCloudPolicyManagerFactory[ChromeOS], so I think it's more idiomatic to get
the manager using the UCPMFactory from here.

Passing the manager to the ctor of ProfilePolicyConnector is a good idea.

[pneubeck (no reviews), 2013-10-15 13:23:10]

https://codereview.chromium.org/24153012/diff/49001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_policy_service.cc (right):

https://codereview.chromium.org/24153012/diff/49001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service.cc:40: // Allow trusted
certs from policy only for managed regular accounts.
On 2013/09/20 12:35:23, Mattias Nissler (OOO till Nov) wrote:
> While at it, can you clarify the comment to say "only for accounts with
managed
> user affiliation, i.e users that are managed by the same domain as the
device."

Done.

https://codereview.chromium.org/24153012/diff/49001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service.cc:47: // the Updater is
destructed during Shutdown.
On 2013/09/20 12:35:23, Mattias Nissler (OOO till Nov) wrote:
> Why? This comment doesn't add any information, just points out the obvious.

It's not obvious whether the returned Updater stores a pointer to |user| or
whether it uses it only during initialization.

I was asked to add this comment in a previous CL. However, I think it's
sufficient to have such documentation in CreateForUserPolicy's  function
comment.

https://codereview.chromium.org/24153012/diff/49001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_policy_service.h (right):

https://codereview.chromium.org/24153012/diff/49001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service.h:32:
NetworkPolicyService(Profile* profile, PolicyService* policy_service);
On 2013/09/20 12:35:23, Mattias Nissler (OOO till Nov) wrote:
> You actually don't need a full profile, please pass a PrefService here.
> 
> For background, passing a Profile* makes it easy for people to create cyclic
> references between BCKS, which is why I'm advising everybody to pass
> dependencies explicitly to the ctor.

Done.

https://codereview.chromium.org/24153012/diff/49001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service.h:43: void
SetUsedPolicyCertificatesOnce();
On 2013/09/20 12:35:23, Mattias Nissler (OOO till Nov) wrote:
> Why is this public?

Done.

https://codereview.chromium.org/24153012/diff/49001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/24153012/diff/49001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.cc:231: DCHECK(service);
On 2013/09/20 12:35:23, Mattias Nissler (OOO till Nov) wrote:
> Remove, you're dereferencing the pointer anyways.

Done.

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_policy_service.cc (right):

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service.cc:25: :
user_prefs_(profile->GetPrefs()), weak_ptr_factory_(this) {
On 2013/09/20 13:00:01, Joao da Silva wrote:
> Pass the Prefs in the ctor directly

Done.

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service.cc:27: return;
On 2013/09/20 13:00:01, Joao da Silva wrote:
> How about doing this check at the factory, and not creating the service at all
> for the signin profile?

Done.

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service.cc:35: return;
On 2013/09/20 13:00:01, Joao da Silva wrote:
> Move this to the factory as well?
> 
> That would mean that this service can sometimes be NULL, which I think is a
good
> thing; if a Profile can't have injected certs then this shouldn't exist.
> 
> The callback for the PolicyCertVerifier of Profiles that don't have this
should
> just CHECK, since they should never use trust anchors.

Done. But instead, I stopped creating PolicyCertVerifier in cases where it's
unused. In that case the callback is never passed...

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service.cc:86:
network_configuration_updater_.reset();
On 2013/09/20 13:00:01, Joao da Silva wrote:
> weak_ptr_factory_.InvalidateWeakPtrs()?

Done.

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_policy_service.h (right):

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service.h:19: class
CertTrustAnchorProvider;
On 2013/09/20 13:00:01, Joao da Silva wrote:
> Not used

Done.

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service.h:43: void
SetUsedPolicyCertificatesOnce();
On 2013/09/20 13:00:01, Joao da Silva wrote:
> Can't this be private?

Done.

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_policy_service_factory.cc (right):

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service_factory.cc:8: #include
"base/prefs/pref_service.h"
On 2013/09/20 13:00:01, Joao da Silva wrote:
> Not used

Done.

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service_factory.cc:14: #include
"chrome/common/pref_names.h"
On 2013/09/20 13:00:01, Joao da Silva wrote:
> Not used

Done.

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_policy_service_factory.h (right):

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_policy_service_factory.h:19: // Profiles.
On 2013/09/20 13:00:01, Joao da Silva wrote:
> Update this comment :-)

Done.

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/policy/pro...
File chrome/browser/policy/profile_policy_connector.cc (right):

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/policy/pro...
chrome/browser/policy/profile_policy_connector.cc:35: {
On 2013/09/20 13:00:01, Joao da Silva wrote:
> {}

Done.

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/policy/pro...
chrome/browser/policy/profile_policy_connector.cc:55: if
(chromeos::ProfileHelper::IsSigninProfile(profile)) {
On 2013/09/20 13:00:01, Joao da Silva wrote:
> The |profile| is passed in just for this call. I suggest passing in a boolean
> "is_signin_profile" instead, and making this test at the
> ProfilePolicyConnectorFactory.

Done. However, the argument is used only for ChromeOS. Not nice. However, I will
not fix this in this CL, maybe a follow up.

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/policy/pro...
File chrome/browser/policy/profile_policy_connector_factory.cc (right):

https://codereview.chromium.org/24153012/diff/55001/chrome/browser/policy/pro...
chrome/browser/policy/profile_policy_connector_factory.cc:41:
CloudPolicyManager* user_cloud_policy_manager,
On 2013/09/20 13:00:01, Joao da Silva wrote:
> I don't think this should come in  in this call.
> 
> This factory declares that it depends on
> UserCloudPolicyManagerFactory[ChromeOS], so I think it's more idiomatic to get
> the manager using the UCPMFactory from here.
> 
> Passing the manager to the ctor of ProfilePolicyConnector is a good idea.

Done.

[Joao da Silva, 2013-10-16 12:44:58]

Almost there. This is a quite a nice cleanup.

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/network_policy_service.cc (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/network_policy_service.cc:29: chromeos::User*
user = user_manager->GetActiveUser();
Pass |user| in the ctor

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/network_policy_service.cc:39: user->GetType() ==
chromeos::User::USER_TYPE_REGULAR;
pass in ctor

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/network_policy_service.h (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/network_policy_service.h:28: class
NetworkPolicyService : public BrowserContextKeyedService {
Document this class

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/network_policy_service.h:44: base::Closure
GetPolicyCertTrustedCallback();
Remove this

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/network_policy_service_factory.cc (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/network_policy_service_factory.cc:54: return
NULL;
Check instead UserManager::GetUserByProfile(profile) !=
UserManager::GetPrimaryUser()

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/network_policy_service_factory.h (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/network_policy_service_factory.h:36:
content::BrowserContext* context) const OVERRIDE;
#include "base/compiler_specific.h"

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/network_policy_service_factory.h:40:
DISALLOW_COPY_AND_ASSIGN(NetworkPolicyServiceFactory);
#include "base/basictypes.h"

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/policy_cert_verifier.cc (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/policy_cert_verifier.cc:77:
weak_ptr_factory_.GetWeakPtr());
can't do, this is on IO. Use |weak_ptr_| here

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/policy_cert_verifier.cc:80:
MaybeSignalAnchorUse(error, *verify_result, weak_ptr_factory_.GetWeakPtr());
Same here, use |weak_ptr_|

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/policy_cert_verifier.cc:94: void
PolicyCertVerifier::RunCallback() {
rename to NotifyOnUI

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:60: //
Destroy |cert_verifier_| before destroying the ThreadBundle, otherwise
indent

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:98:
cert_verifier_subscription_;
Can't this come below after |thread_bundle_|, since it's reset() on TearDown()
anyway?

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
File chrome/browser/policy/profile_policy_connector.cc (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
chrome/browser/policy/profile_policy_connector.cc:13: #include
"chrome/browser/policy/policy_service_impl.h"
include policy_service.h instead

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
chrome/browser/policy/profile_policy_connector.cc:21: #include
"chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.h"
not used

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
chrome/browser/policy/profile_policy_connector.cc:22: #include
"chrome/browser/policy/policy_service.h"
already included above, after the 1st comment change

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
chrome/browser/policy/profile_policy_connector.cc:25: #include
"chrome/browser/policy/cloud/user_cloud_policy_manager_factory.h"
not used

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
chrome/browser/policy/profile_policy_connector.cc:56: // For Signin profile
terminate sentence with a .

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
chrome/browser/policy/profile_policy_connector.cc:64:
chromeos::UserManager::Get()->GetLoggedInUsers().size() == 1;
is_primary_user_ = user == UserManager::Get()->GetPrimaryUser();

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
File chrome/browser/policy/profile_policy_connector.h (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
chrome/browser/policy/profile_policy_connector.h:14: class Profile;
not used

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
File chrome/browser/policy/profile_policy_connector_factory.cc (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
chrome/browser/policy/profile_policy_connector_factory.cc:139:
user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
This registration should move to network_policy_service_factory

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
File chrome/browser/policy/profile_policy_connector_factory.h (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
chrome/browser/policy/profile_policy_connector_factory.h:27: class
CloudPolicyManager;
not used

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
File chrome/browser/policy/profile_policy_connector_stub.cc (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
chrome/browser/policy/profile_policy_connector_stub.cc:17: Profile* profile,
This should be CloudPolicyManager*.

Submit to the linux_redux bot to see if this file is right

[pneubeck (no reviews), 2013-10-22 18:47:41]

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/network_policy_service.cc (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/network_policy_service.cc:29: chromeos::User*
user = user_manager->GetActiveUser();
On 2013/10/16 12:44:58, Joao da Silva wrote:
> Pass |user| in the ctor

Done.

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/network_policy_service.cc:39: user->GetType() ==
chromeos::User::USER_TYPE_REGULAR;
On 2013/10/16 12:44:58, Joao da Silva wrote:
> pass in ctor

do you mean, that I should forward user to 
UserNetworkConfigurationUpdater::CreateForUserPolicy and determine
|allow_trusted_certs_from_policy| there?

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/network_policy_service.h (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/network_policy_service.h:28: class
NetworkPolicyService : public BrowserContextKeyedService {
On 2013/10/16 12:44:58, Joao da Silva wrote:
> Document this class

Done.

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/network_policy_service.h:44: base::Closure
GetPolicyCertTrustedCallback();
On 2013/10/16 12:44:58, Joao da Silva wrote:
> Remove this

Done.

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/network_policy_service_factory.cc (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/network_policy_service_factory.cc:54: return
NULL;
On 2013/10/16 12:44:58, Joao da Silva wrote:
> Check instead UserManager::GetUserByProfile(profile) !=
> UserManager::GetPrimaryUser()

Done.

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/network_policy_service_factory.h (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/network_policy_service_factory.h:36:
content::BrowserContext* context) const OVERRIDE;
On 2013/10/16 12:44:58, Joao da Silva wrote:
> #include "base/compiler_specific.h"

Done.

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/network_policy_service_factory.h:40:
DISALLOW_COPY_AND_ASSIGN(NetworkPolicyServiceFactory);
On 2013/10/16 12:44:58, Joao da Silva wrote:
> #include "base/basictypes.h"

Done.

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/policy_cert_verifier.cc (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/policy_cert_verifier.cc:77:
weak_ptr_factory_.GetWeakPtr());
On 2013/10/16 12:44:58, Joao da Silva wrote:
> can't do, this is on IO. Use |weak_ptr_| here

doh. Of course. That was the original intention when I added weak_ptr_,
otherwise weak_ptr_ is even unused... doh.
Thanks for checking that!

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/policy_cert_verifier.cc:80:
MaybeSignalAnchorUse(error, *verify_result, weak_ptr_factory_.GetWeakPtr());
On 2013/10/16 12:44:58, Joao da Silva wrote:
> Same here, use |weak_ptr_|

Done.

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/policy_cert_verifier.cc:94: void
PolicyCertVerifier::RunCallback() {
On 2013/10/16 12:44:58, Joao da Silva wrote:
> rename to NotifyOnUI

Done.

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:60: //
Destroy |cert_verifier_| before destroying the ThreadBundle, otherwise
On 2013/10/16 12:44:58, Joao da Silva wrote:
> indent

Done.

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:98:
cert_verifier_subscription_;
On 2013/10/16 12:44:58, Joao da Silva wrote:
> Can't this come below after |thread_bundle_|, since it's reset() on TearDown()
> anyway?

Yes. I think the reason why I moved it here vanished during some change...
Done.

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
File chrome/browser/policy/profile_policy_connector.cc (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
chrome/browser/policy/profile_policy_connector.cc:13: #include
"chrome/browser/policy/policy_service_impl.h"
On 2013/10/16 12:44:58, Joao da Silva wrote:
> include policy_service.h instead

Done.

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
chrome/browser/policy/profile_policy_connector.cc:21: #include
"chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.h"
On 2013/10/16 12:44:58, Joao da Silva wrote:
> not used

Done.

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
chrome/browser/policy/profile_policy_connector.cc:22: #include
"chrome/browser/policy/policy_service.h"
On 2013/10/16 12:44:58, Joao da Silva wrote:
> already included above, after the 1st comment change

Done.

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
chrome/browser/policy/profile_policy_connector.cc:25: #include
"chrome/browser/policy/cloud/user_cloud_policy_manager_factory.h"
On 2013/10/16 12:44:58, Joao da Silva wrote:
> not used

Done.

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
chrome/browser/policy/profile_policy_connector.cc:56: // For Signin profile
On 2013/10/16 12:44:58, Joao da Silva wrote:
> terminate sentence with a .

it was not a sentence though.

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
chrome/browser/policy/profile_policy_connector.cc:64:
chromeos::UserManager::Get()->GetLoggedInUsers().size() == 1;
On 2013/10/16 12:44:58, Joao da Silva wrote:
> is_primary_user_ = user == UserManager::Get()->GetPrimaryUser();

Done.

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
File chrome/browser/policy/profile_policy_connector.h (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
chrome/browser/policy/profile_policy_connector.h:14: class Profile;
On 2013/10/16 12:44:58, Joao da Silva wrote:
> not used

Done.

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
File chrome/browser/policy/profile_policy_connector_factory.cc (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
chrome/browser/policy/profile_policy_connector_factory.cc:139:
user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
On 2013/10/16 12:44:58, Joao da Silva wrote:
> This registration should move to network_policy_service_factory

Done.

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
File chrome/browser/policy/profile_policy_connector_factory.h (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/policy/pr...
chrome/browser/policy/profile_policy_connector_factory.h:27: class
CloudPolicyManager;
On 2013/10/16 12:44:58, Joao da Silva wrote:
> not used

Done.

[Joao da Silva, 2013-10-23 07:45:54]

lgtm

This change is quite substantial, it's a good idea to manually verify everything
still works as intended before landing :-)

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/network_policy_service.cc (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/network_policy_service.cc:39: user->GetType() ==
chromeos::User::USER_TYPE_REGULAR;
On 2013/10/22 18:47:41, pneubeck wrote:
> On 2013/10/16 12:44:58, Joao da Silva wrote:
> > pass in ctor
> 
> do you mean, that I should forward user to 
> UserNetworkConfigurationUpdater::CreateForUserPolicy and determine
> |allow_trusted_certs_from_policy| there?

I was suggesting to pass |allow_trusted_certs_from_policy| as a boolean to
NetworkPolicyService::NetworkPolicyService() (this ctor), and determine its
value from network_policy_service_factory.cc.

[pneubeck (no reviews), 2013-10-23 11:22:09]

Ok. The problem is now:

OTRProfile reuses the same NetworkPolicyService as the regular profile.
Both profile's create their own CertVerifier.

Thus, we have two event sources (the Verifier) and one sink (the Service). And
the one source (from OTRProfile) is deleted before the sink is destroyed.

Somehow this doesn't fit callback_list, which follows the pattern that the
source outlives the sink!

Any ideas?

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/network_policy_service.cc (right):

https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/network_policy_service.cc:39: user->GetType() ==
chromeos::User::USER_TYPE_REGULAR;
On 2013/10/23 07:45:54, Joao da Silva wrote:
> On 2013/10/22 18:47:41, pneubeck wrote:
> > On 2013/10/16 12:44:58, Joao da Silva wrote:
> > > pass in ctor
> > 
> > do you mean, that I should forward user to 
> > UserNetworkConfigurationUpdater::CreateForUserPolicy and determine
> > |allow_trusted_certs_from_policy| there?
> 
> I was suggesting to pass |allow_trusted_certs_from_policy| as a boolean to
> NetworkPolicyService::NetworkPolicyService() (this ctor), and determine its
> value from network_policy_service_factory.cc.

Done.

[pneubeck (no reviews), 2013-10-23 11:25:04]

On 2013/10/23 11:22:09, pneubeck wrote:
> Ok. The problem is now:
> 
> OTRProfile reuses the same NetworkPolicyService as the regular profile.
> Both profile's create their own CertVerifier.
> 
> Thus, we have two event sources (the Verifier) and one sink (the Service). And
> the one source (from OTRProfile) is deleted before the sink is destroyed.
> 
> Somehow this doesn't fit callback_list, which follows the pattern that the
> source outlives the sink!
> 
> Any ideas?

One solution is:

To support registering several Verifier at the
NetworkPolicyService/NetworkConfigUpdater and also allow deregistering.
Deregister in ~ProfileImpl before the KeyedServices are shutdown.

> 
>
https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
> File chrome/browser/chromeos/policy/network_policy_service.cc (right):
> 
>
https://codereview.chromium.org/24153012/diff/151001/chrome/browser/chromeos/...
> chrome/browser/chromeos/policy/network_policy_service.cc:39: user->GetType()
==
> chromeos::User::USER_TYPE_REGULAR;
> On 2013/10/23 07:45:54, Joao da Silva wrote:
> > On 2013/10/22 18:47:41, pneubeck wrote:
> > > On 2013/10/16 12:44:58, Joao da Silva wrote:
> > > > pass in ctor
> > > 
> > > do you mean, that I should forward user to 
> > > UserNetworkConfigurationUpdater::CreateForUserPolicy and determine
> > > |allow_trusted_certs_from_policy| there?
> > 
> > I was suggesting to pass |allow_trusted_certs_from_policy| as a boolean to
> > NetworkPolicyService::NetworkPolicyService() (this ctor), and determine its
> > value from network_policy_service_factory.cc.
> 
> Done.

[pneubeck (no reviews), 2013-10-24 20:45:03]

apart from some debug logs that i have to remove, this should be good now.

[pneubeck (no reviews), 2013-10-24 20:52:17]

@William, could you please take a look at the profile* changes?
The additional ReleaseFromBrowserContextServices methods are necessary to
unregister callbacks before the pointers (to the BCKS) are getting invalid.

(Alternatively, if we don't add the additional
ReleaseFromBrowserContextServices, then the pointers will be invalid temporarily
although still _not_ accessed because of the current particular shutdown order.)

[pneubeck (no reviews), 2013-10-25 08:46:05]

@Joao, ptal.

[Joao da Silva, 2013-10-25 11:57:10]

lgtm

https://codereview.chromium.org/24153012/diff/1198001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/policy_cert_verifier.h (right):

https://codereview.chromium.org/24153012/diff/1198001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/policy_cert_verifier.h:35: // anchors (set with
SetTrustAnchors) is used. This notifications are stopped
*These

https://codereview.chromium.org/24153012/diff/1198001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/user_network_configuration_updater.cc
(right):

https://codereview.chromium.org/24153012/diff/1198001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_network_configuration_updater.cc:108:
web_trust_certs_));
Leave a comment here explaining why it's safe to post an unretained
PolicyCertVerifier from UI to IO.

https://codereview.chromium.org/24153012/diff/1198001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/24153012/diff/1198001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:293: << initialized_;
Remove this

https://codereview.chromium.org/24153012/diff/1198001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/24153012/diff/1198001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:305: cert_verifier_ =
CreatePolicyCertVerifier(profile);
Move this below, next to the initialization of the URLBlacklistManager (which
has similar requirements)

[pneubeck (no reviews), 2013-10-25 12:16:59]

https://codereview.chromium.org/24153012/diff/1198001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/policy_cert_verifier.h (right):

https://codereview.chromium.org/24153012/diff/1198001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/policy_cert_verifier.h:35: // anchors (set with
SetTrustAnchors) is used. This notifications are stopped
On 2013/10/25 11:57:10, Joao da Silva wrote:
> *These

Done.

https://codereview.chromium.org/24153012/diff/1198001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/user_network_configuration_updater.cc
(right):

https://codereview.chromium.org/24153012/diff/1198001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_network_configuration_updater.cc:108:
web_trust_certs_));
On 2013/10/25 11:57:10, Joao da Silva wrote:
> Leave a comment here explaining why it's safe to post an unretained
> PolicyCertVerifier from UI to IO.

Done.

https://codereview.chromium.org/24153012/diff/1198001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/24153012/diff/1198001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:293: << initialized_;
On 2013/10/25 11:57:10, Joao da Silva wrote:
> Remove this

Done.

https://codereview.chromium.org/24153012/diff/1198001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/24153012/diff/1198001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:305: cert_verifier_ =
CreatePolicyCertVerifier(profile);
On 2013/10/25 11:57:10, Joao da Silva wrote:
> Move this below, next to the initialization of the URLBlacklistManager (which
> has similar requirements)

Done.

[willchan no longer on Chromium, 2013-10-29 01:21:09]

I'm trying to digest the dependency graph and am having some trouble. Can you
explain what the relevant classes are, the original dependency graph, and the
new one? My gut instinct is the
ProfileIOData::ReleaseFromBrowserContextServices() should not be necessary. But
I need to examine the dependencies to understand why. And it'd be good for the
CL description to explain the dependency changes in detail rather than just
saying it fixes it.

https://codereview.chromium.org/24153012/diff/1498001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/24153012/diff/1498001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:392: is_login_profile_ =
chromeos::ProfileHelper::IsSigninProfile(this);
Which code needs |is_login_profile_| to be set earlier? Is it possible to make
this dependency explicit by passing |is_login_profile_| directly to the
function? I worry about code reordering subtly breaking this invariant.

[pneubeck (no reviews), 2013-10-29 09:55:47]

On 2013/10/29 01:21:09, willchan wrote:
> I'm trying to digest the dependency graph and am having some trouble. Can you
> explain what the relevant classes are, the original dependency graph, and the
> new one? My gut instinct is the
> ProfileIOData::ReleaseFromBrowserContextServices() should not be necessary.
But
> I need to examine the dependencies to understand why. And it'd be good for the
> CL description to explain the dependency changes in detail rather than just
> saying it fixes it.

I made the description more detailed. However, it got a bit lengthy to describe
all the dependencies.
I added ReleaseFromBrowserContextService in order to unset a pointer from
PolicyCertVerifier to the BCKS NetworkPolicyService, before the latter is
destructed, i.e. as long as the pointer is still valid.

> 
>
https://codereview.chromium.org/24153012/diff/1498001/chrome/browser/profiles...
> File chrome/browser/profiles/profile_impl.cc (right):
> 
>
https://codereview.chromium.org/24153012/diff/1498001/chrome/browser/profiles...
> chrome/browser/profiles/profile_impl.cc:392: is_login_profile_ =
> chromeos::ProfileHelper::IsSigninProfile(this);
> Which code needs |is_login_profile_| to be set earlier? Is it possible to make
> this dependency explicit by passing |is_login_profile_| directly to the
> function? I worry about code reordering subtly breaking this invariant.

[pneubeck (no reviews), 2013-10-29 09:56:21]

https://codereview.chromium.org/24153012/diff/1498001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/24153012/diff/1498001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:392: is_login_profile_ =
chromeos::ProfileHelper::IsSigninProfile(this);
On 2013/10/29 01:21:09, willchan wrote:
> Which code needs |is_login_profile_| to be set earlier? Is it possible to make
> this dependency explicit by passing |is_login_profile_| directly to the
> function? I worry about code reordering subtly breaking this invariant.

ProfilePolicyConnectorFactory::CreateForProfile(Internal) uses
Profile::IsLoginProfile.
I'll pass it explicitly.

[willchan no longer on Chromium, 2013-10-30 01:12:30]

Thanks for the explanation, this makes sense to me now. I have one dependency
ordering suggestion. I prefer my suggestion because it removes the need for an
extra ReleaseFromBrowserContextServices() function, and just relies on the
existing Handle::~Handle().

https://codereview.chromium.org/24153012/diff/1498001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/24153012/diff/1498001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:673:
io_data_.ReleaseFromBrowserContextServices(this);
Would it work to let the ProfileImplIOData::Handle::~Handle() call
ProfileIOData::ShutdownOnUIThread() and move whatever invalidation logic needs
to happen into ProfileIOData::ShtudownOnUIThread()? That way we can consolidate
all that logic for the two step destruction of ProfileIOData (step 1 on the UI
thread, step two on the IO thread).

This changes the order slightly, but AIUI, the BCKSes all get destroyed before
ProfileImplIOData::Handle::~Handle(). Therefore, whenever
PolicyCertVerifier::SetTrustAnchor() gets called, it'll post a task to a
PolicyCertVerifier on the IO thread that is still alive, since the ordering
requirement is that the task gets posted before Handle::~Handle() executes.

[pneubeck (no reviews), 2013-10-30 08:32:02]

https://codereview.chromium.org/24153012/diff/1498001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/24153012/diff/1498001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:392: is_login_profile_ =
chromeos::ProfileHelper::IsSigninProfile(this);
On 2013/10/29 09:56:22, pneubeck wrote:
> On 2013/10/29 01:21:09, willchan wrote:
> > Which code needs |is_login_profile_| to be set earlier? Is it possible to
make
> > this dependency explicit by passing |is_login_profile_| directly to the
> > function? I worry about code reordering subtly breaking this invariant.
> 
> ProfilePolicyConnectorFactory::CreateForProfile(Internal) uses
> Profile::IsLoginProfile.
> I'll pass it explicitly.

Actually, my original plan was to inline ProfileHelper::IsSigninProfile into the
initialization of is_login_profile_, i.e. 

ProfileImpl::ProfileImpl(...) 
  : is_login_profile_(path.BaseName().value() == chrome::kInitialProfile)

However, I'd like to do that in a follow-up CL, in which I can also remove
ProfileHelper::IsSigninProfile completely and replace it by
Profile::IsLoginProfile.

https://codereview.chromium.org/24153012/diff/1498001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:673:
io_data_.ReleaseFromBrowserContextServices(this);
On 2013/10/30 01:12:31, willchan wrote:
> Would it work to let the ProfileImplIOData::Handle::~Handle() call
> ProfileIOData::ShutdownOnUIThread() and move whatever invalidation logic needs
> to happen into ProfileIOData::ShtudownOnUIThread()? That way we can
consolidate
> all that logic for the two step destruction of ProfileIOData (step 1 on the UI
> thread, step two on the IO thread).
> 
> This changes the order slightly, but AIUI, the BCKSes all get destroyed before
> ProfileImplIOData::Handle::~Handle(). Therefore, whenever
> PolicyCertVerifier::SetTrustAnchor() gets called, it'll post a task to a
> PolicyCertVerifier on the IO thread that is still alive, since the ordering
> requirement is that the task gets posted before Handle::~Handle() executes.

Yes, that will work. But again only, because everything is destructed in one
pass.
The subtle glitch that I see with your proposal: PolicyCertVerifier will have a
callback with a pointer to NetworkPolicyService (the BCKS) that is invalid
between the destruction of the BCKSes and the ShutdownOnUI().
With my current solution, the pointers from PolicyCertVerifier to
NetworkPolicyService and the other way round are always valid.

[willchan no longer on Chromium, 2013-10-30 17:20:48]

https://codereview.chromium.org/24153012/diff/1498001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/24153012/diff/1498001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:392: is_login_profile_ =
chromeos::ProfileHelper::IsSigninProfile(this);
On 2013/10/30 08:32:03, pneubeck wrote:
> On 2013/10/29 09:56:22, pneubeck wrote:
> > On 2013/10/29 01:21:09, willchan wrote:
> > > Which code needs |is_login_profile_| to be set earlier? Is it possible to
> make
> > > this dependency explicit by passing |is_login_profile_| directly to the
> > > function? I worry about code reordering subtly breaking this invariant.
> > 
> > ProfilePolicyConnectorFactory::CreateForProfile(Internal) uses
> > Profile::IsLoginProfile.
> > I'll pass it explicitly.
> 
> Actually, my original plan was to inline ProfileHelper::IsSigninProfile into
the
> initialization of is_login_profile_, i.e. 
> 
> ProfileImpl::ProfileImpl(...) 
>   : is_login_profile_(path.BaseName().value() == chrome::kInitialProfile)
> 
> However, I'd like to do that in a follow-up CL, in which I can also remove
> ProfileHelper::IsSigninProfile completely and replace it by
> Profile::IsLoginProfile.

If that's a short CL, can you do that first and rebase this one on that one? Or
is it dependent?

https://codereview.chromium.org/24153012/diff/1498001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:673:
io_data_.ReleaseFromBrowserContextServices(this);
On 2013/10/30 08:32:03, pneubeck wrote:
> On 2013/10/30 01:12:31, willchan wrote:
> > Would it work to let the ProfileImplIOData::Handle::~Handle() call
> > ProfileIOData::ShutdownOnUIThread() and move whatever invalidation logic
needs
> > to happen into ProfileIOData::ShtudownOnUIThread()? That way we can
> consolidate
> > all that logic for the two step destruction of ProfileIOData (step 1 on the
UI
> > thread, step two on the IO thread).
> > 
> > This changes the order slightly, but AIUI, the BCKSes all get destroyed
before
> > ProfileImplIOData::Handle::~Handle(). Therefore, whenever
> > PolicyCertVerifier::SetTrustAnchor() gets called, it'll post a task to a
> > PolicyCertVerifier on the IO thread that is still alive, since the ordering
> > requirement is that the task gets posted before Handle::~Handle() executes.
> 
> Yes, that will work. But again only, because everything is destructed in one
> pass.
> The subtle glitch that I see with your proposal: PolicyCertVerifier will have
a
> callback with a pointer to NetworkPolicyService (the BCKS) that is invalid
> between the destruction of the BCKSes and the ShutdownOnUI().
> With my current solution, the pointers from PolicyCertVerifier to
> NetworkPolicyService and the other way round are always valid.

Great point! I personally mentally dismissed that consideration, because in my
mind, PolicyCertVerifier is an IO thread object that is created on the UI thread
but immediately gets passed over to the IO thread and lives on that thread
thereafter. The one awkwardness here is the trampoline function to bounce calls
back through PolicyCertVerifier to the UI thread so it can call
NetworkPolicyService. You're right that this is the hole that can lead to the
"glitch".

My two initial instincts were to either:
(1) ignore the glitch, as I did in my proposal. It's bad to have more UI
thread-isms in a mostly single threaded IO thread object. But there's nothing
enforcing this and someone could accidentally violate this invariant with
nothing except comments to protect against it
(2) Use a WeakPtr, which is what you're moving away from in favor of explicit
dependency management. I'm generally in favor of explicit dependency management,
so I like that you've gotten rid of the WeakPtr. But I dislike that you've added
more fragile ordering code in both ProfileImpl and ProfileIOData.

I now have another proposal. What if we split PolicyCertVerifier into its UI and
IO thread components. Most of the logic and state just needs to live in the IO
thread side. But the trampolining can live in the UI thread. And between those
two objects, it seems reasonable to use WeakPtrs. Splitting this two objects up
will allow us to move the UI thread object into being a BCKS, so the dependency
management there is explicit. And the UI thread object can create the IO thread
object in CreatePolicyCertVerifier() and hand a UI

Naming gets tricky, but what I would do is sort of like what I did for
ProfileIOData. Have a PolicyCertVerifier::Handle class that is the UI thread
object and is the BCKS and does the trampolining. It will be a friend of the
PolicyCertVerifier class, since nested classes are always friends of the
containing class. So now we can have a private constructor for
PolicyCertVerifier that takes a base::WeakPtr<Handle>. This removes the need for
the explicit ReleaseFromBrowserContextServices(), since the Handle will have a
raw pointer to the relevant BCKS it depends on, and since it is a BCKS, the
dependency ordering should reflect the correct construction/destruction order.

WDYT? This is definitely going to be more boilerplate, but I think it solves the
dependency management issue and removes the gap for invalid pointers, at the
cost of extra boilerplate, which I consider acceptable in this case.

[pneubeck (no reviews), 2013-10-31 08:41:39]

https://codereview.chromium.org/24153012/diff/1498001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/24153012/diff/1498001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:392: is_login_profile_ =
chromeos::ProfileHelper::IsSigninProfile(this);
On 2013/10/30 17:20:48, willchan wrote:
> On 2013/10/30 08:32:03, pneubeck wrote:
> > On 2013/10/29 09:56:22, pneubeck wrote:
> > > On 2013/10/29 01:21:09, willchan wrote:
> > > > Which code needs |is_login_profile_| to be set earlier? Is it possible
to
> > make
> > > > this dependency explicit by passing |is_login_profile_| directly to the
> > > > function? I worry about code reordering subtly breaking this invariant.
> > > 
> > > ProfilePolicyConnectorFactory::CreateForProfile(Internal) uses
> > > Profile::IsLoginProfile.
> > > I'll pass it explicitly.
> > 
> > Actually, my original plan was to inline ProfileHelper::IsSigninProfile into
> the
> > initialization of is_login_profile_, i.e. 
> > 
> > ProfileImpl::ProfileImpl(...) 
> >   : is_login_profile_(path.BaseName().value() == chrome::kInitialProfile)
> > 
> > However, I'd like to do that in a follow-up CL, in which I can also remove
> > ProfileHelper::IsSigninProfile completely and replace it by
> > Profile::IsLoginProfile.
> 
> If that's a short CL, can you do that first and rebase this one on that one?
Or
> is it dependent?

CL is in review: https://codereview.chromium.org/53383002/

[pneubeck (no reviews), 2013-11-05 15:37:33]

Thanks William for thorough review. I think, I could address all your points and
the result is IMO much better.

Please take another look at Patch Set 13 or later.

[willchan no longer on Chromium, 2013-11-05 17:01:36]

Apologies, I'm in Vancouver IETF now. I hope to have time today to get to
this.


On Tue, Nov 5, 2013 at 7:37 AM, <pneubeck@chromium.org> wrote:

> Thanks William for thorough review. I think, I could address all your
> points and
> the result is IMO much better.
>
> Please take another look at Patch Set 13 or later.
>
>
> https://codereview.chromium.org/24153012/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[willchan no longer on Chromium, 2013-11-06 18:04:01]

LGTM, thanks for incorporating my feedback.

[pneubeck (no reviews), 2013-11-07 15:30:21]

@Joao, please take another look.

[Joao da Silva, 2013-11-11 12:38:18]

IIUC this isn't working in incognito, but I might be wrong. Did you test it?

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/network_policy_service.h (right):

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/network_policy_service.h:20: class User;
not used

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/network_policy_service.h:25: typedef
std::vector<scoped_refptr<X509Certificate> > CertificateList;
#include ref_counted.h

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/network_policy_service.h:31: class PolicyService;
not used

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/network_policy_service_factory.cc (right):

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/network_policy_service_factory.cc:62:
profile->GetOriginalProfile()->GetPrefs());
This factory must override GetBrowserContextToUse() to return "own instance" in
incognito; the default returns NULL and means that this service is not created
in incognito, and so incognito does not get a PolicyCertVerifier.

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/network_policy_service_factory.h (right):

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/network_policy_service_factory.h:31: //
|profile|, i.e. if NetworkConfigurationUpdater doesn't exists.
*exist

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model_unittest.cc (right):

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:189: #endif
Isn't it cleaner to override ServiceIsNULLWhileTesting() at the factory?

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_unittest.cc (right):

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_unittest.cc:92: #endif
Same

[pneubeck (no reviews), 2013-11-12 10:07:53]

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/network_policy_service.h (right):

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/network_policy_service.h:20: class User;
On 2013/11/11 12:38:19, Joao da Silva wrote:
> not used

Done.

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/network_policy_service.h:25: typedef
std::vector<scoped_refptr<X509Certificate> > CertificateList;
On 2013/11/11 12:38:19, Joao da Silva wrote:
> #include ref_counted.h

Done.

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/network_policy_service.h:31: class PolicyService;
On 2013/11/11 12:38:19, Joao da Silva wrote:
> not used

Done.

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/network_policy_service_factory.cc (right):

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/network_policy_service_factory.cc:62:
profile->GetOriginalProfile()->GetPrefs());
On 2013/11/11 12:38:19, Joao da Silva wrote:
> This factory must override GetBrowserContextToUse() to return "own instance"
in
> incognito; the default returns NULL and means that this service is not created
> in incognito, and so incognito does not get a PolicyCertVerifier.

ops.
Done.

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/network_policy_service_factory.h (right):

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/network_policy_service_factory.h:31: //
|profile|, i.e. if NetworkConfigurationUpdater doesn't exists.
On 2013/11/11 12:38:19, Joao da Silva wrote:
> *exist

Done.

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model_unittest.cc (right):

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:189: #endif
On 2013/11/11 12:38:19, Joao da Silva wrote:
> Isn't it cleaner to override ServiceIsNULLWhileTesting() at the factory?

Done.

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/website_settings_unittest.cc (right):

https://codereview.chromium.org/24153012/diff/2418001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/website_settings_unittest.cc:92: #endif
On 2013/11/11 12:38:19, Joao da Silva wrote:
> Same

Done.

[pneubeck (no reviews), 2013-11-12 10:09:15]

@Ben: chrome/browser/ui/**
@Jay: chrome/test/base/testing_profile.cc

[Joao da Silva, 2013-11-12 14:29:37]

lgtm, thanks!

[commit-bot: I haz the power, 2013-11-12 15:03:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/24153012/2878001

[commit-bot: I haz the power, 2013-11-12 15:17:43]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-11-12 15:41:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/24153012/2878001

[commit-bot: I haz the power, 2013-11-12 16:14:16]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[pneubeck (no reviews), 2013-11-13 09:38:59]

@Joao, I had to rebase onto your SchemaRegistryService addition to
ProfilePolicyConnector. As with the other dependencies, I pulled it out into the
factory.

[commit-bot: I haz the power, 2013-11-13 09:39:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/24153012/3338001

[Joao da Silva, 2013-11-13 09:45:57]

https://codereview.chromium.org/24153012/diff/3338001/chrome/browser/policy/p...
File chrome/browser/policy/profile_policy_connector_factory.cc (right):

https://codereview.chromium.org/24153012/diff/3338001/chrome/browser/policy/p...
chrome/browser/policy/profile_policy_connector_factory.cc:96: schema_registry =
SchemaRegistryServiceFactory::GetForContext(profile);
The schema_registry is also needed for non-signin profiles.

[commit-bot: I haz the power, 2013-11-13 10:09:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/24153012/3468001

[commit-bot: I haz the power, 2013-11-13 12:33:41]

Change committed as 234806