Reporting of policy errors via host-offline-reason: part 1

Reporting of policy errors via host-offline-reason: part 1

PolicyWatcher.OnPolicyError callback and implementation.
- This new callback enables HostProcess to react to policy errors by
  calling ShutdownHost.
- PolicyWatcher on Linux calls this callback when detecting malformed
  policy files (with retries mechanism to avoid firing this for
  transient file reading problems)

BUG=427512
410050

Committed: https://crrev.com/142f8e1d3e4b75c901f33cb771cd848019630c1e
Cr-Commit-Position: refs/heads/master@{#305069}

Committed: https://crrev.com/0fdd951c7c55120d58944ea94eaa8ddfc8613d23
Cr-Commit-Position: refs/heads/master@{#305658}

[Łukasz Anforowicz, 2014-11-12 21:56:00]

lukasza@chromium.org changed reviewers:
+ lambroslambrou@chromium.org

[Łukasz Anforowicz, 2014-11-12 21:56:00]

Lambros, could you please review the 3 changelists for reporting policy errors
via host-offline-reason?  This is part 1.

Thanks,

Lukasz

[Lambros, 2014-11-13 00:30:27]

https://codereview.chromium.org/722743003/diff/1/remoting/host/it2me/it2me_ho...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/722743003/diff/1/remoting/host/it2me/it2me_ho...
remoting/host/it2me/it2me_host.cc:336: void It2MeHost::OnPolicyError() {
optional: NOTIMPLEMENTED()

https://codereview.chromium.org/722743003/diff/1/remoting/host/it2me/it2me_ho...
remoting/host/it2me/it2me_host.cc:337: // TODO(lukasza): figure out what to do
here...
Maybe keep this simple and just say "Report the policy error to the user."

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
File remoting/host/policy_hack/policy_watcher.cc (right):

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:245: policy_error_callback_.Run();
Depending on what you think should happen, you might want to reset the transient
counter here?

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:250: //                duration
(i.e. reading a partially written file),
nit: Don't indent the whole TODO block. See examples elsewhere for how to format
this.

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:253: //                most likely
still trigger SignalPolicyError)
Is it this class's responsibility to figure out what to do about long-term
policy errors? Is the PolicyErrorCallback not a good-enough interface?

I think it might be better to document in the class header what happens after
SignalPolicyError() is triggered? Will this class continue to report further
errors/updates, or will it stop watching altogether?

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:260: const int MAX_RETRY_COUNT = 5;
nit: kMaxRetryCount

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
File remoting/host/policy_hack/policy_watcher.h (right):

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.h:30: // Called after detecting
malformed policies
nit: Period.

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.h:32: PolicyErrorCallback;
nit: Fits on one line.

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.h:111: // Signals policy error to
whoever is watching this policy watcher
s/whoever is watching this policy watcher/the registered PolicyErrorCallback/

is a bit clearer perhaps? Also, comments end with a period.

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.h:114: // Notes a transient policy
error which shouldn't be reported
I think I would document more fully what is going on here. Something like this:

Called whenever a transient error occurs during reading of policy files. This
will increment a counter, and will trigger a call to SignalPolicyError() only
after a threshold count is reached. The counter is reset whenever policy has
been successfully read.

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
File remoting/host/policy_hack/policy_watcher_unittest.cc (right):

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher_unittest.cc:410: const int
MAX_TRANSIENT_ERROR_RETRIES = 5;
nit: kMaxTransient...

https://codereview.chromium.org/722743003/diff/1/remoting/host/remoting_me2me...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/722743003/diff/1/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:628: // TODO(lukasza): code review please:
the new ShutdownHost call below seems
Please remove this. When requesting a code review, you can add your own inline
comments using the Rietveld web UI, and reviewers will see these comments in the
request.

https://codereview.chromium.org/722743003/diff/1/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:944: base::Bind(
Formatting is a bit odd - see examples elsewhere, or use clang-format. Lines
below should be +4 indented, not +2. And some stuff could be collapsed onto the
previous line.

[Łukasz Anforowicz, 2014-11-13 17:48:09]

Thanks for the feedback Lambros.  I think PatchSet#2 should address it.

Renato - could you please take a peek at remoting_me2me_host.cc in PatchSet#1 -
there is one TODO in the code from you that I think can be addressed via the
end-to-end host-offline-reason changes if we just call
ShutdownHost(kSomeExitCodeThatFitsYourScenario).

https://codereview.chromium.org/722743003/diff/1/remoting/host/it2me/it2me_ho...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/722743003/diff/1/remoting/host/it2me/it2me_ho...
remoting/host/it2me/it2me_host.cc:336: void It2MeHost::OnPolicyError() {
On 2014/11/13 00:30:26, Lambros wrote:
> optional: NOTIMPLEMENTED()

Done.  I hope that the default policy (4 -- [default] LOG(ERROR) at runtime)
from base/logging.h is in effect.

https://codereview.chromium.org/722743003/diff/1/remoting/host/it2me/it2me_ho...
remoting/host/it2me/it2me_host.cc:337: // TODO(lukasza): figure out what to do
here...
On 2014/11/13 00:30:26, Lambros wrote:
> Maybe keep this simple and just say "Report the policy error to the user."

Done.

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
File remoting/host/policy_hack/policy_watcher.cc (right):

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:245: policy_error_callback_.Run();
On 2014/11/13 00:30:26, Lambros wrote:
> Depending on what you think should happen, you might want to reset the
transient
> counter here?

Good point. Done.

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:253: //                most likely
still trigger SignalPolicyError)
On 2014/11/13 00:30:26, Lambros wrote:
> Is it this class's responsibility to figure out what to do about long-term
> policy errors? Is the PolicyErrorCallback not a good-enough interface?
> 
> I think it might be better to document in the class header what happens after
> SignalPolicyError() is triggered? Will this class continue to report further
> errors/updates, or will it stop watching altogether?

Good point on documenting in the header the current behavior of continuing to
process policy files even after reporting the error.

Yes - I guess it is HostProcess's responsibility to deal with long-term policy
errors.

So:
- I am adding a comment to the header file (next to StartWatching method)
- I am removing this TODO
- Do we need to discuss what to do going forward with the long-term failures
(and recovery from long-term failures)?  The current changelist can probably
checked-in as-is (unless you want me to move the TODO to HostProcess class).  I
am not sure how important the long-term-policy-error-autorecovery is + whether
it should be tracked as a separate bug (maybe I should just open one and let
triage team decide?).

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.cc:260: const int MAX_RETRY_COUNT = 5;
On 2014/11/13 00:30:27, Lambros wrote:
> nit: kMaxRetryCount

Done.

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
File remoting/host/policy_hack/policy_watcher.h (right):

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.h:30: // Called after detecting
malformed policies
On 2014/11/13 00:30:27, Lambros wrote:
> nit: Period.

Done.

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.h:32: PolicyErrorCallback;
On 2014/11/13 00:30:27, Lambros wrote:
> nit: Fits on one line.

Done.

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.h:111: // Signals policy error to
whoever is watching this policy watcher
On 2014/11/13 00:30:27, Lambros wrote:
> s/whoever is watching this policy watcher/the registered PolicyErrorCallback/
> 
> is a bit clearer perhaps? Also, comments end with a period.

Done.

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher.h:114: // Notes a transient policy
error which shouldn't be reported
On 2014/11/13 00:30:27, Lambros wrote:
> I think I would document more fully what is going on here. Something like
this:
> 
> Called whenever a transient error occurs during reading of policy files. This
> will increment a counter, and will trigger a call to SignalPolicyError() only
> after a threshold count is reached. The counter is reset whenever policy has
> been successfully read.

Done.

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
File remoting/host/policy_hack/policy_watcher_unittest.cc (right):

https://codereview.chromium.org/722743003/diff/1/remoting/host/policy_hack/po...
remoting/host/policy_hack/policy_watcher_unittest.cc:410: const int
MAX_TRANSIENT_ERROR_RETRIES = 5;
On 2014/11/13 00:30:27, Lambros wrote:
> nit: kMaxTransient...

Done.

https://codereview.chromium.org/722743003/diff/1/remoting/host/remoting_me2me...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/722743003/diff/1/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:628: // TODO(lukasza): code review please:
the new ShutdownHost call below seems
On 2014/11/13 00:30:27, Lambros wrote:
> Please remove this. When requesting a code review, you can add your own inline
> comments using the Rietveld web UI, and reviewers will see these comments in
the
> request.

Done.

https://codereview.chromium.org/722743003/diff/1/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:944: base::Bind(
On 2014/11/13 00:30:27, Lambros wrote:
> Formatting is a bit odd - see examples elsewhere, or use clang-format. Lines
> below should be +4 indented, not +2. And some stuff could be collapsed onto
the
> previous line.

Ok to +4 rather than +2.  Let's discuss the other things - I think single value
or name-of-called-method per line is very readable, but if there is some kind of
policy for keeping things short, I can collapse into less lines.

[Łukasz Anforowicz, 2014-11-13 17:51:15]

https://codereview.chromium.org/722743003/diff/20001/remoting/host/policy_hac...
File remoting/host/policy_hack/policy_watcher.h (right):

https://codereview.chromium.org/722743003/diff/20001/remoting/host/policy_hac...
remoting/host/policy_hack/policy_watcher.h:48: // After calling
|policy_error_callback| PolicyWatcher will continue watching
"will continue watching" is not technically true for Mac implementation... 
Still - I think this comment is fine:

1. Mac implementation says that users expect to have to log out and log back in
for policy^H^H^H pref changes to take effect

2. Setting expectations that both callbacks can be called multiple times is
safest / requires users of PolicyWatchers to be most robust (i.e. the Mac
behavior shouldn't trip the user from C++ memory management perspective)

[kelvinp, 2014-11-13 19:40:56]

kelvinp@chromium.org changed reviewers:
+ kelvinp@chromium.org

[kelvinp, 2014-11-13 19:40:57]

lgtm for it2me changes

https://codereview.chromium.org/722743003/diff/20001/remoting/host/it2me/it2m...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/722743003/diff/20001/remoting/host/it2me/it2m...
remoting/host/it2me/it2me_host.cc:337: // TODO(lukasza): Report the policy error
to the user.
Can you add a bug number for the TODO item?
// TODO(lukasza): Report the policy error to the user (crbug.com/<bug_number>)

[Łukasz Anforowicz, 2014-11-13 19:57:23]

Addressed feedback from Kelvin in patchset#3.

https://codereview.chromium.org/722743003/diff/20001/remoting/host/it2me/it2m...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/722743003/diff/20001/remoting/host/it2me/it2m...
remoting/host/it2me/it2me_host.cc:337: // TODO(lukasza): Report the policy error
to the user.
On 2014/11/13 19:40:57, kelvinp wrote:
> Can you add a bug number for the TODO item?
> // TODO(lukasza): Report the policy error to the user (crbug.com/<bug_number>)

Done.

[Lambros, 2014-11-14 01:42:48]

lgtm, thanks for doing this!

https://codereview.chromium.org/722743003/diff/40001/remoting/host/policy_hac...
File remoting/host/policy_hack/policy_watcher_unittest.cc (right):

https://codereview.chromium.org/722743003/diff/40001/remoting/host/policy_hac...
remoting/host/policy_hack/policy_watcher_unittest.cc:28:
base::Unretained(&mock_policy_callback_));
nit: Wrong indentation. Should line up with first param, or +4 against previous
line.

[Łukasz Anforowicz, 2014-11-14 03:17:18]

Fixed formatting of policy_watcher_unittest.cc

https://codereview.chromium.org/722743003/diff/40001/remoting/host/policy_hac...
File remoting/host/policy_hack/policy_watcher_unittest.cc (right):

https://codereview.chromium.org/722743003/diff/40001/remoting/host/policy_hac...
remoting/host/policy_hack/policy_watcher_unittest.cc:28:
base::Unretained(&mock_policy_callback_));
On 2014/11/14 01:42:47, Lambros wrote:
> nit: Wrong indentation. Should line up with first param, or +4 against
previous
> line.

Ah, right.  I was doing a search & replace of policy_callback ->
policy_updated_callback and that broke the indentation :-)

[Łukasz Anforowicz, 2014-11-20 16:51:14]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2014-11-20 16:52:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/722743003/60001

[commit-bot: I haz the power, 2014-11-20 16:57:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-20 16:57:47]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/84207)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Łukasz Anforowicz, 2014-11-20 16:59:50]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2014-11-20 17:00:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/722743003/80001

[Łukasz Anforowicz, 2014-11-20 17:00:44]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2014-11-20 17:01:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/722743003/80001

[commit-bot: I haz the power, 2014-11-20 17:06:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-20 17:06:26]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/84213)

[Łukasz Anforowicz, 2014-11-20 19:15:46]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2014-11-20 19:17:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/722743003/100001

[commit-bot: I haz the power, 2014-11-20 20:09:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-20 20:09:52]

Patchset 6 (id:??) landed as
https://crrev.com/142f8e1d3e4b75c901f33cb771cd848019630c1e
Cr-Commit-Position: refs/heads/master@{#305069}

[Łukasz Anforowicz, 2014-11-20 22:35:31]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/746723002/ by lukasza@chromium.org.

The reason for reverting is: The reason for reverting is: Reason for reverting -
failures in memory test:
remoting
did not complete
failed 1
stdio
Me2MeNativeMessagingHostTest.UpdateDaemonConfigInvalidConfig.

[Łukasz Anforowicz, 2014-11-21 21:07:13]

Patchset #7 (id:120001) has been deleted

[Łukasz Anforowicz, 2014-11-21 21:26:56]

I could not repro the valgrind crash encountered by the Chromium Mac Valgrind
buildbot
(http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20%28...):

[ RUN      ] Me2MeNativeMessagingHostTest.UpdateDaemonConfigInvalidConfig
[5735:66331:1120/141440:4154254272171:ERROR:me2me_native_messaging_host.cc(64)]
'config' dictionary not found
--5735-- VALGRIND INTERNAL ERROR: Valgrind received a signal 11 (SIGSEGV) -
exiting

I tried and couldn't repro on a Linux machine containing the same patch.  I
tried both valgrind and asan.  Let's see if I can repro the problem by
explicitly asking for "Chromium Mac Valgrind" try bot to try the changes.

[Łukasz Anforowicz, 2014-11-25 17:12:06]

Rachel and Lei have kindly taken a look into the past Valgrind failure and
concluded that it was most likely a Valgrind fluke (Lei pointed out that the
next buildbot run passed even though it didn't include the revert yet). 

I also haven't been able to repro the failure locally (Linux+asan/valgrind,
Mac+asan; Mac+valgrind is unfortunately unsupported on a recent Mac OS) or via
trybots (I've hit unrelated failures).  Therefore I'll just try pushing this
changelist into commit queue and hoping it succeeds this time.

[Łukasz Anforowicz, 2014-11-25 17:12:16]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2014-11-25 17:13:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/722743003/140001

[commit-bot: I haz the power, 2014-11-25 17:47:59]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2014-11-25 17:48:44]

Patchset 7 (id:??) landed as
https://crrev.com/0fdd951c7c55120d58944ea94eaa8ddfc8613d23
Cr-Commit-Position: refs/heads/master@{#305658}