Added retry support to AttestationPolicyObserver.

Added retry support to AttestationPolicyObserver.

It is currently possible for this observer to run before the cryptohome
dbus interface is ready.  If the policy does not change, the observer
only runs again on the next login.  For users who only login immediately
after boot, this means the observer may never succeed.  This CL
implements a simple mechanism to reschedule on failure.

BUG=chromium:253646
TEST=unit

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

[Mattias Nissler (ping if slow), 2013-07-02 08:18:07]

FWIW, we've seen similar problems in a different context. When Will debugged
http://crbug.com/189681 he found that enrollment timed out on (synchronous) DBus
calls because cryptohome took too long to initialize/respond. The long-term plan
to fix this is making cryptohome send DBus ignals once it becomes ready to do
various things (see http://crbug.com/255013) and it seems that would be a better
way to solve your problem as well. No need for you to take this on now (would
you be interested?), but putting a TODO to that effect would be good.

https://chromiumcodereview.appspot.com/18053006/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right):

https://chromiumcodereview.appspot.com/18053006/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:155:
Reschedule();
Why is this needed?

https://chromiumcodereview.appspot.com/18053006/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:273:
Reschedule();
This is probably not a good idea. You'll hammer the server with 100 requests
even in cases where the return status indicates that the client is at fault (for
example, HTTP 400 bad request).

[dkrahn, 2013-07-02 21:00:01]

https://chromiumcodereview.appspot.com/18053006/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/attestation/attestation_policy_observer.cc (right):

https://chromiumcodereview.appspot.com/18053006/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:155:
Reschedule();
On 2013/07/02 08:18:07, Mattias Nissler wrote:
> Why is this needed?

I haven't seen this fail but the idea was to reschedule on any failure that may
succeed later.  Since I'm revisiting the upload failure case, I'll remove this
for now and take a closer look.

https://chromiumcodereview.appspot.com/18053006/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/attestation/attestation_policy_observer.cc:273:
Reschedule();
On 2013/07/02 08:18:07, Mattias Nissler wrote:
> This is probably not a good idea. You'll hammer the server with 100 requests
> even in cases where the return status indicates that the client is at fault
(for
> example, HTTP 400 bad request).

You're right.  This may be more work than I thought, I want to get the
retry-on-dbus-error in so I'll punt this to a separate CL.

[dkrahn, 2013-07-02 21:52:19]

Filed https://code.google.com/p/chromium/issues/detail?id=256841 for handling
upload errors and https://code.google.com/p/chromium/issues/detail?id=256845 for
adding cryptohome events.

I'm open to helping clean up the cryptohome interface, I still have to nail down
my priorities for Q3.

[Mattias Nissler (ping if slow), 2013-07-03 15:20:29]

LGTM w/ a nit.

Regarding the cryptohome DBus interface cleanup, that would be a nice project I
guess. You may also want to ask Will for what cleanups he has in mind for
cryptohome in general, in case they tie in with this.

https://chromiumcodereview.appspot.com/18053006/diff/5001/chrome/browser/chro...
File chrome/browser/chromeos/attestation/attestation_policy_observer.h (right):

https://chromiumcodereview.appspot.com/18053006/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/attestation/attestation_policy_observer.h:88: //
Reschedules a policy check (i.e. a call to Start) for a later time.
nit: Can you add a TODO here saying that this should rather wait for a
cryptohome DBus signal and reference the bug?

[dkrahn, 2013-07-03 18:07:45]

https://chromiumcodereview.appspot.com/18053006/diff/5001/chrome/browser/chro...
File chrome/browser/chromeos/attestation/attestation_policy_observer.h (right):

https://chromiumcodereview.appspot.com/18053006/diff/5001/chrome/browser/chro...
chrome/browser/chromeos/attestation/attestation_policy_observer.h:88: //
Reschedules a policy check (i.e. a call to Start) for a later time.
On 2013/07/03 15:20:29, Mattias Nissler wrote:
> nit: Can you add a TODO here saying that this should rather wait for a
> cryptohome DBus signal and reference the bug?

Done.

[commit-bot: I haz the power, 2013-07-03 18:08:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dkrahn@google.com/18053006/12001

[commit-bot: I haz the power, 2013-07-03 20:04:23]

Change committed as 210029