Read CrOS install attributes cache on startup.

Read CrOS install attributes cache on startup.

This works around slow startup of cryptohomed, which results in Chrome being
unable to read install attributes at startup, which in turn renders
enterprise-devices to appear non-enterprise on the login screen.

BUG=chromium-os:37367
TEST=Manual: Enroll device, reboot, check enrollment is signaled on login screen.


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

[Mattias Nissler (ping if slow), 2013-01-17 19:30:50]

Joao, can you review?

[Joao da Silva, 2013-01-17 19:53:25]

Scary stuff, lgtm. I trust Will knows about this :-)

https://codereview.chromium.org/11734005/diff/9001/chrome/app/policy/cloud_po...
File chrome/app/policy/cloud_policy_codegen.gyp (right):

https://codereview.chromium.org/11734005/diff/9001/chrome/app/policy/cloud_po...
chrome/app/policy/cloud_policy_codegen.gyp:110:
'<(proto_rel_path)/install_attributes.proto',
This (and chrome_device_policy.proto) are chromeos-only. Let's split this target
in another CL.

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/brow...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.cc:132:
FilePath(policy::EnterpriseInstallAttributes::kCacheFilePath));
device_cloud_policy_store_chromeos_unittest.cc also creates an instance of EIA.

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/ente...
File chrome/browser/policy/enterprise_install_attributes.cc (right):

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/ente...
chrome/browser/policy/enterprise_install_attributes.cc:111:
attr_map.insert(std::make_pair(entry->name(),
#include <utility>

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/ente...
chrome/browser/policy/enterprise_install_attributes.cc:180: return
device_locked_ && !registration_user_.empty();
CHECK(attrs_read_)?

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/ente...
chrome/browser/policy/enterprise_install_attributes.cc:220: attrs_read_ = true;
It's possible that locking the device fails and this flag is never set; the
getters will then CHECK.

Then again, ReadCacheFile is always called after creation; is the flag meant to
enforce that ReadCacheFile() is called before other operations?

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/ente...
File chrome/browser/policy/enterprise_install_attributes.h (right):

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/ente...
chrome/browser/policy/enterprise_install_attributes.h:16: class FilePath;
file_path.h already included

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/ente...
chrome/browser/policy/enterprise_install_attributes.h:44: void
ReadCacheFile(const FilePath& cache_file);
Why the argument and the static const? Just ReadCacheFile()? Or did you mean to
write tests for this? :-)

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/prot...
File chrome/browser/policy/proto/install_attributes.proto (right):

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/prot...
chrome/browser/policy/proto/install_attributes.proto:11: message
SerializedInstallAttributes {
Please comment where this was taken from.

[Mattias Nissler (ping if slow), 2013-01-17 20:34:42]

Addressed your comments and wrote another unit test. PTAL.

And yes, Will most certainly knows about this ;)

https://codereview.chromium.org/11734005/diff/9001/chrome/app/policy/cloud_po...
File chrome/app/policy/cloud_policy_codegen.gyp (right):

https://codereview.chromium.org/11734005/diff/9001/chrome/app/policy/cloud_po...
chrome/app/policy/cloud_policy_codegen.gyp:110:
'<(proto_rel_path)/install_attributes.proto',
On 2013/01/17 19:53:25, Joao da Silva wrote:
> This (and chrome_device_policy.proto) are chromeos-only. Let's split this
target
> in another CL.

Yes, I was planning to move the CrOS-specific policy code into a subdir anyways.

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/brow...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/brow...
chrome/browser/policy/browser_policy_connector.cc:132:
FilePath(policy::EnterpriseInstallAttributes::kCacheFilePath));
On 2013/01/17 19:53:25, Joao da Silva wrote:
> device_cloud_policy_store_chromeos_unittest.cc also creates an instance of
EIA.

That test is good because it locks the device immediately (which reads the
attrs).

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/ente...
File chrome/browser/policy/enterprise_install_attributes.cc (right):

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/ente...
chrome/browser/policy/enterprise_install_attributes.cc:111:
attr_map.insert(std::make_pair(entry->name(),
On 2013/01/17 19:53:25, Joao da Silva wrote:
> #include <utility>

Done.

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/ente...
chrome/browser/policy/enterprise_install_attributes.cc:180: return
device_locked_ && !registration_user_.empty();
On 2013/01/17 19:53:25, Joao da Silva wrote:
> CHECK(attrs_read_)?

I removed the entire attrs_read_ stuff, wasn't much useful.

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/ente...
chrome/browser/policy/enterprise_install_attributes.cc:220: attrs_read_ = true;
On 2013/01/17 19:53:25, Joao da Silva wrote:
> It's possible that locking the device fails and this flag is never set; the
> getters will then CHECK.
> 
> Then again, ReadCacheFile is always called after creation; is the flag meant
to
> enforce that ReadCacheFile() is called before other operations?

The flag was meant to catch reads before ReadCacheFile(), but now that I'm
calling this immediately after construction, there's really no point in
attrs_read_, so I removed it.

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/ente...
File chrome/browser/policy/enterprise_install_attributes.h (right):

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/ente...
chrome/browser/policy/enterprise_install_attributes.h:16: class FilePath;
On 2013/01/17 19:53:25, Joao da Silva wrote:
> file_path.h already included

Done.

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/ente...
chrome/browser/policy/enterprise_install_attributes.h:44: void
ReadCacheFile(const FilePath& cache_file);
On 2013/01/17 19:53:25, Joao da Silva wrote:
> Why the argument and the static const? Just ReadCacheFile()? Or did you mean
to
> write tests for this? :-)

Wrote the test :)

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/prot...
File chrome/browser/policy/proto/install_attributes.proto (right):

https://codereview.chromium.org/11734005/diff/9001/chrome/browser/policy/prot...
chrome/browser/policy/proto/install_attributes.proto:11: message
SerializedInstallAttributes {
On 2013/01/17 19:53:25, Joao da Silva wrote:
> Please comment where this was taken from.

Done.

[Joao da Silva, 2013-01-17 20:47:13]

slgtm

[Will Drewry, 2013-01-17 21:17:44]

Drive-by LGTM

Just a minor nit. Skimmed the change and looks good on the whole!

https://codereview.chromium.org/11734005/diff/19001/chrome/browser/policy/ent...
File chrome/browser/policy/enterprise_install_attributes.cc (right):

https://codereview.chromium.org/11734005/diff/19001/chrome/browser/policy/ent...
chrome/browser/policy/enterprise_install_attributes.cc:90: char buf[16384];
Hrm it worries me that this assumes an install attrs max size since if it grows
wildly in a release, we'll end up breaking the cache silently :/  Would
ReadFileToString() be a safer option since we consider the cache somewhat safe? 
(I realize it could be too large and cause issues there...)

just a nit.

[Mattias Nissler (ping if slow), 2013-01-17 23:08:51]

https://codereview.chromium.org/11734005/diff/19001/chrome/browser/policy/ent...
File chrome/browser/policy/enterprise_install_attributes.cc (right):

https://codereview.chromium.org/11734005/diff/19001/chrome/browser/policy/ent...
chrome/browser/policy/enterprise_install_attributes.cc:90: char buf[16384];
On 2013/01/17 21:17:44, Will Drewry wrote:
> Hrm it worries me that this assumes an install attrs max size since if it
grows
> wildly in a release, we'll end up breaking the cache silently :/  Would
> ReadFileToString() be a safer option since we consider the cache somewhat
safe? 
> (I realize it could be too large and cause issues there...)
> 
> just a nit.

The reason I didn't use ReadFileToString was exactly to guard against reading
/dev/zero ;)

I think we're good here because:
(1) 16k should be plenty (famous last words!)
(2) We can always increase the buffer size when we start writing more data. Old
versions are not going to see data size increase, since this is immutable after
initial lock after all.

[commit-bot: I haz the power, 2013-01-17 23:12:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/11734005/19001

[commit-bot: I haz the power, 2013-01-18 00:27:56]

Change committed as 177547