Explicitly wait for user policy before completing login.

Explicitly wait for user policy before completing login.

This enables a further simplification of the underlying policy PrefStore, and
makes the waiting path more clear.

BUG=chromium-os:17698, 108999
TEST=All policy tests pass, in particular browser_tests:LoginUtils*

[Joao da Silva, 2012-02-15 16:29:07]

Please review.

@mnissler: main review
@nkostylev: owner review for the changes to login_utils.cc

[Mattias Nissler (ping if slow), 2012-02-16 10:24:11]

http://codereview.chromium.org/9404011/diff/1/chrome/browser/chromeos/login/l...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/9404011/diff/1/chrome/browser/chromeos/login/l...
chrome/browser/chromeos/login/login_utils.cc:679: // profile creation and user
policy readiness notifications come
s/come/are reported/

http://codereview.chromium.org/9404011/diff/1/chrome/browser/chromeos/login/l...
chrome/browser/chromeos/login/login_utils.cc:789:
base::Bind(&LoginUtilsImpl::OnProfileCreated, AsWeakPtr()));
Since user policy is not initialized at this point, I'm worrying that running
ProfileImpl::OnPrefsLoaded will actually start up lots of pref-consuming code
without policy in place. I think we need to split profile initialization to not
automatically call ProfileImpl::OnPrefsLoaded, s.t. we can create the profile
and its PrefService and wait for user policy. Once all that has finished we
should kick off what's now happening in OnPrefsLoaded.

http://codereview.chromium.org/9404011/diff/1/chrome/browser/policy/browser_p...
File chrome/browser/policy/browser_policy_connector.h (right):

http://codereview.chromium.org/9404011/diff/1/chrome/browser/policy/browser_p...
chrome/browser/policy/browser_policy_connector.h:116: // |ready_callback|, if
not null, is invoked when the user policy is ready.
Can you give some more detail on what the actual conditions for being "ready"
are? It's a bit unclear that "fully initialized" and "ready" are the same thing.

http://codereview.chromium.org/9404011/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_controller_unittest.cc (right):

http://codereview.chromium.org/9404011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_controller_unittest.cc:120: void
ReadyCallback() {
Suggestion: You can actually declare this as a MOCK_METHOD, remove the
ready_callback_was_invoked_ flag, and use EXPECT_CALL below.

http://codereview.chromium.org/9404011/diff/1/chrome/browser/policy/device_to...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/9404011/diff/1/chrome/browser/policy/device_to...
chrome/browser/policy/device_token_fetcher_unittest.cc:80: void ReadyCallback()
{
ditto

http://codereview.chromium.org/9404011/diff/1/chrome/browser/policy/user_poli...
File chrome/browser/policy/user_policy_cache.cc (right):

http://codereview.chromium.org/9404011/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_cache.cc:123: SetReady();
Are you going to clean up all the existing ready-state stuff in a later CL?

http://codereview.chromium.org/9404011/diff/1/chrome/browser/policy/user_poli...
File chrome/browser/policy/user_policy_cache_unittest.cc (right):

http://codereview.chromium.org/9404011/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_cache_unittest.cc:33: void SetTrue(bool* flag)
{
Use a MOCK_METHOD on UserPolicyCacheTest instead?

[Nikita (slow), 2012-02-20 16:10:23]

http://codereview.chromium.org/9404011/diff/1/chrome/browser/chromeos/login/l...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/9404011/diff/1/chrome/browser/chromeos/login/l...
chrome/browser/chromeos/login/login_utils.cc:772:
connector->InitializeUserPolicy(
What's the cost (and what happens behind the scenes) on any subsequent login
(i.e. when policy has been already fetched)?

Does this also involve policy update as well (it shouldn't) or policy update is
postponed?

http://codereview.chromium.org/9404011/diff/1/chrome/browser/chromeos/login/l...
chrome/browser/chromeos/login/login_utils.cc:789:
base::Bind(&LoginUtilsImpl::OnProfileCreated, AsWeakPtr()));
On 2012/02/16 10:24:11, Mattias Nissler wrote:
> Since user policy is not initialized at this point, I'm worrying that running
> ProfileImpl::OnPrefsLoaded will actually start up lots of pref-consuming code
> without policy in place. I think we need to split profile initialization to
not
> automatically call ProfileImpl::OnPrefsLoaded, s.t. we can create the profile
> and its PrefService and wait for user policy. Once all that has finished we
> should kick off what's now happening in OnPrefsLoaded.

I think this part could be done in a separate CL prior to that one?

[Joao da Silva, 2012-02-20 16:23:15]

Hi Nikita,

 thanks for having a look! Please see the comments inline.

http://codereview.chromium.org/9404011/diff/1/chrome/browser/chromeos/login/l...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/9404011/diff/1/chrome/browser/chromeos/login/l...
chrome/browser/chromeos/login/login_utils.cc:772:
connector->InitializeUserPolicy(
On 2012/02/20 16:10:24, Nikita Kostylev wrote:
> What's the cost (and what happens behind the scenes) on any subsequent login
> (i.e. when policy has been already fetched)?
> 

The cost is always the same. A couple of objects are created and plugged into
places, a URL fetch is started asynchronously, and an asynchronous load of
cached policy is started too. This call won't do any IO itself, and returns
quickly.

|wait_for_policy_fetch| is required precisely because this call can return while
the cache is still being loaded.

> Does this also involve policy update as well (it shouldn't) or policy update
is
> postponed?

An update is triggered by this call but is delayed by 5 seconds, unless
|wait_for_policy_fetch| is true (in that case the fetch starts immediately).

http://codereview.chromium.org/9404011/diff/1/chrome/browser/chromeos/login/l...
chrome/browser/chromeos/login/login_utils.cc:789:
base::Bind(&LoginUtilsImpl::OnProfileCreated, AsWeakPtr()));
On 2012/02/20 16:10:24, Nikita Kostylev wrote:
> On 2012/02/16 10:24:11, Mattias Nissler wrote:
> > Since user policy is not initialized at this point, I'm worrying that
running
> > ProfileImpl::OnPrefsLoaded will actually start up lots of pref-consuming
code
> > without policy in place. I think we need to split profile initialization to
> not
> > automatically call ProfileImpl::OnPrefsLoaded, s.t. we can create the
profile
> > and its PrefService and wait for user policy. Once all that has finished we
> > should kick off what's now happening in OnPrefsLoaded.
> 
> I think this part could be done in a separate CL prior to that one?

Yes. This turned out to be a larger issue and we've been debating how to design
a nicer fix that plugs more clearly into the Profile initialization code. This
CL is now likely to be closed; I'll chime you in again if any changes are
required in LoginUtils.