Fix cyclic dependency between ProfilePolicyConnector and PrefService.

chrome/browser/profiles/profile_impl.cc

Index: chrome/browser/profiles/profile_impl.cc
diff --git a/chrome/browser/profiles/profile_impl.cc b/chrome/browser/profiles/profile_impl.cc
index 9c9c1de7255042138862ab90b30df1d8917e3cd2..998f26fbd6fca5e46b70102352a576a618648794 100644
--- a/chrome/browser/profiles/profile_impl.cc
+++ b/chrome/browser/profiles/profile_impl.cc
@@ -388,6 +388,10 @@ ProfileImpl::ProfileImpl(
   DCHECK(!path.empty()) << "Using an empty path will attempt to write " <<
                             "profile files to the root directory!";
 
+#if defined(OS_CHROMEOS)
+  is_login_profile_ = chromeos::ProfileHelper::IsSigninProfile(this);

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

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:22]

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

[pneubeck (no reviews), 2013/10/30 08:32:03]

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.

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

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

[pneubeck (no reviews), 2013/10/31 08:41:40]

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

+#endif
+
 #if defined(ENABLE_SESSION_SERVICE)
   create_session_service_timer_.Start(FROM_HERE,
       TimeDelta::FromMilliseconds(kCreateSessionServiceDelayMS), this,
@@ -424,7 +428,6 @@ ProfileImpl::ProfileImpl(
   bool async_prefs = create_mode == CREATE_MODE_ASYNCHRONOUS;
 
 #if defined(OS_CHROMEOS)
-  is_login_profile_ = chromeos::ProfileHelper::IsSigninProfile(this);
   if (is_login_profile_)
     chrome::RegisterLoginProfilePrefs(pref_registry_.get());
   else
@@ -667,6 +670,8 @@ ProfileImpl::~ProfileImpl() {
       io_data_.GetResourceContextNoInit());
 #endif
 
+  io_data_.ReleaseFromBrowserContextServices(this);

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

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:03]

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]

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.

+
   // Destroy OTR profile and its profile services first.
   if (off_the_record_profile_) {
     ProfileDestroyer::DestroyOffTheRecordProfileNow(