Revert of Reporting of policy errors via host-offline-reason: part 1 (patchset #6 id:100001 of https://codereview.chromium.org/722743003/)

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

Original issue's description:
> 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}

TBR=lambroslambrou@chromium.org,kelvinp@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=427512 410050

Review URL: https://codereview.chromium.org/746723002

Cr-Commit-Position: refs/heads/master@{#305099}

11 files changed, 18 insertions, 145 deletions

diff --git a/remoting/host/it2me/it2me_host.cc b/remoting/host/it2me/it2me_host.cc
index eb13e47f..afaaf07 100644
--- a/remoting/host/it2me/it2me_host.cc
+++ b/remoting/host/it2me/it2me_host.cc
@@ -67,9 +67,7 @@ void It2MeHost::Connect() {
       host_context_->ui_task_runner()));
 
   // Start monitoring configured policies.
-  policy_watcher_->StartWatching(
-      base::Bind(&It2MeHost::OnPolicyUpdate, this),
-      base::Bind(&It2MeHost::OnPolicyError, this));
+  policy_watcher_->StartWatching(base::Bind(&It2MeHost::OnPolicyUpdate, this));
 
   // Switch to the network thread to start the actual connection.
   host_context_->network_task_runner()->PostTask(
@@ -341,11 +339,6 @@ void It2MeHost::OnPolicyUpdate(scoped_ptr<base::DictionaryValue> policies) {
   }
 }
 
-void It2MeHost::OnPolicyError() {
-  // TODO(lukasza): Report the policy error to the user.  crbug.com/433009
-  NOTIMPLEMENTED();
-}
-
 void It2MeHost::UpdateNatPolicy(bool nat_traversal_enabled) {
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
 
diff --git a/remoting/host/it2me/it2me_host.h b/remoting/host/it2me/it2me_host.h
index 15921c6..b2eb749 100644
--- a/remoting/host/it2me/it2me_host.h
+++ b/remoting/host/it2me/it2me_host.h
@@ -132,9 +132,6 @@ class It2MeHost : public base::RefCountedThreadSafe<It2MeHost>,
   // Called when initial policies are read, and when they change.
   void OnPolicyUpdate(scoped_ptr<base::DictionaryValue> policies);
 
-  // Called when malformed policies are detected.
-  void OnPolicyError();
-
   // Handlers for NAT traversal and host domain policies.
   void UpdateNatPolicy(bool nat_traversal_enabled);
   void UpdateHostDomainPolicy(const std::string& host_domain);
diff --git a/remoting/host/policy_hack/fake_policy_watcher.cc b/remoting/host/policy_hack/fake_policy_watcher.cc
index 57d53ef..809961b 100644
--- a/remoting/host/policy_hack/fake_policy_watcher.cc
+++ b/remoting/host/policy_hack/fake_policy_watcher.cc
@@ -21,10 +21,6 @@ void FakePolicyWatcher::SetPolicies(const base::DictionaryValue* policies) {
   UpdatePolicies(policies);
 }
 
-void FakePolicyWatcher::SignalTransientErrorForTest() {
-  SignalTransientPolicyError();
-}
-
 void FakePolicyWatcher::StartWatchingInternal() {
 }
 
diff --git a/remoting/host/policy_hack/fake_policy_watcher.h b/remoting/host/policy_hack/fake_policy_watcher.h
index 4195440..7faf3a0 100644
--- a/remoting/host/policy_hack/fake_policy_watcher.h
+++ b/remoting/host/policy_hack/fake_policy_watcher.h
@@ -18,8 +18,6 @@ class FakePolicyWatcher : public PolicyWatcher {
 
   void SetPolicies(const base::DictionaryValue* policies);
 
-  void SignalTransientErrorForTest();
-
  protected:
   void StartWatchingInternal() override;
   void StopWatchingInternal() override;
diff --git a/remoting/host/policy_hack/mock_policy_callback.cc b/remoting/host/policy_hack/mock_policy_callback.cc
index 392be25..a069b88 100644
--- a/remoting/host/policy_hack/mock_policy_callback.cc
+++ b/remoting/host/policy_hack/mock_policy_callback.cc
@@ -18,9 +18,5 @@ void MockPolicyCallback::OnPolicyUpdate(
   OnPolicyUpdatePtr(policies.get());
 }
 
-void MockPolicyCallback::OnPolicyError() {
-  OnPolicyErrorPtr();
-}
-
 }  // namespace policy_hack
 }  // namespace remoting
diff --git a/remoting/host/policy_hack/mock_policy_callback.h b/remoting/host/policy_hack/mock_policy_callback.h
index e0e5752..2ae9b0d 100644
--- a/remoting/host/policy_hack/mock_policy_callback.h
+++ b/remoting/host/policy_hack/mock_policy_callback.h
@@ -21,9 +21,6 @@ class MockPolicyCallback {
   MOCK_METHOD1(OnPolicyUpdatePtr, void(const base::DictionaryValue* policies));
   void OnPolicyUpdate(scoped_ptr<base::DictionaryValue> policies);
 
-  MOCK_METHOD0(OnPolicyErrorPtr, void());
-  void OnPolicyError();
-
  private:
   DISALLOW_COPY_AND_ASSIGN(MockPolicyCallback);
 };
diff --git a/remoting/host/policy_hack/policy_watcher.cc b/remoting/host/policy_hack/policy_watcher.cc
index d0f5957..8e30c24 100644
--- a/remoting/host/policy_hack/policy_watcher.cc
+++ b/remoting/host/policy_hack/policy_watcher.cc
@@ -118,7 +118,6 @@ const char PolicyWatcher::kHostDebugOverridePoliciesName[] =
 PolicyWatcher::PolicyWatcher(
     scoped_refptr<base::SingleThreadTaskRunner> task_runner)
     : task_runner_(task_runner),
-      transient_policy_error_retry_counter_(0),
       old_policies_(new base::DictionaryValue()),
       default_values_(new base::DictionaryValue()),
       weak_factory_(this) {
@@ -152,20 +151,16 @@ PolicyWatcher::PolicyWatcher(
 PolicyWatcher::~PolicyWatcher() {
 }
 
-void PolicyWatcher::StartWatching(
-    const PolicyUpdatedCallback& policy_updated_callback,
-    const PolicyErrorCallback& policy_error_callback) {
+void PolicyWatcher::StartWatching(const PolicyCallback& policy_callback) {
   if (!OnPolicyWatcherThread()) {
     task_runner_->PostTask(FROM_HERE,
                            base::Bind(&PolicyWatcher::StartWatching,
                                       base::Unretained(this),
-                                      policy_updated_callback,
-                                      policy_error_callback));
+                                      policy_callback));
     return;
   }
 
-  policy_updated_callback_ = policy_updated_callback;
-  policy_error_callback_ = policy_error_callback;
+  policy_callback_ = policy_callback;
   StartWatchingInternal();
 }
 
@@ -179,8 +174,7 @@ void PolicyWatcher::StopWatching(const base::Closure& stopped_callback) {
 void PolicyWatcher::StopWatchingOnPolicyWatcherThread() {
   StopWatchingInternal();
   weak_factory_.InvalidateWeakPtrs();
-  policy_updated_callback_.Reset();
-  policy_error_callback_.Reset();
+  policy_callback_.Reset();
 }
 
 void PolicyWatcher::ScheduleFallbackReloadTask() {
@@ -209,8 +203,6 @@ void PolicyWatcher::UpdatePolicies(
     const base::DictionaryValue* new_policies_raw) {
   DCHECK(OnPolicyWatcherThread());
 
-  transient_policy_error_retry_counter_ = 0;
-
   // Use default values for any missing policies.
   scoped_ptr<base::DictionaryValue> new_policies =
       CopyGoodValuesAndAddDefaults(
@@ -234,20 +226,7 @@ void PolicyWatcher::UpdatePolicies(
 
   // Notify our client of the changed policies.
   if (!changed_policies->empty()) {
-    policy_updated_callback_.Run(changed_policies.Pass());
-  }
-}
-
-void PolicyWatcher::SignalPolicyError() {
-  transient_policy_error_retry_counter_ = 0;
-  policy_error_callback_.Run();
-}
-
-void PolicyWatcher::SignalTransientPolicyError() {
-  const int kMaxRetryCount = 5;
-  transient_policy_error_retry_counter_ += 1;
-  if (transient_policy_error_retry_counter_ >= kMaxRetryCount) {
-    SignalPolicyError();
+    policy_callback_.Run(changed_policies.Pass());
   }
 }
 
diff --git a/remoting/host/policy_hack/policy_watcher.h b/remoting/host/policy_hack/policy_watcher.h
index dbbfd23..109a110 100644
--- a/remoting/host/policy_hack/policy_watcher.h
+++ b/remoting/host/policy_hack/policy_watcher.h
@@ -26,33 +26,17 @@ class PolicyWatcher {
  public:
   // Called first with all policies, and subsequently with any changed policies.
   typedef base::Callback<void(scoped_ptr<base::DictionaryValue>)>
-      PolicyUpdatedCallback;
-
-  // Called after detecting malformed policies.
-  typedef base::Callback<void()> PolicyErrorCallback;
+      PolicyCallback;
 
   explicit PolicyWatcher(
       scoped_refptr<base::SingleThreadTaskRunner> task_runner);
   virtual ~PolicyWatcher();
 
-  // This guarantees that the |policy_updated_callback| is called at least once
-  // with the current policies.  After that, |policy_updated_callback| will be
-  // called whenever a change to any policy is detected. It will then be called
-  // only with the changed policies.
-  //
-  // |policy_error_callback| will be called when malformed policies are detected
-  // (i.e. wrong type of policy value, or unparseable files under
-  // /etc/opt/chrome/policies/managed).
-  // When called, the |policy_error_callback| is responsible for mitigating the
-  // security risk of running with incorrectly formulated policies (by either
-  // shutting down or locking down the host).
-  // After calling |policy_error_callback| PolicyWatcher will continue watching
-  // for policy changes and will call |policy_updated_callback| when the error
-  // is recovered from and may call |policy_error_callback| when new errors are
-  // found.
-  virtual void StartWatching(
-      const PolicyUpdatedCallback& policy_updated_callback,
-      const PolicyErrorCallback& policy_error_callback);
+  // This guarantees that the |policy_callback| is called at least once with
+  // the current policies.  After that, |policy_callback| will be called
+  // whenever a change to any policy is detected. It will then be called only
+  // with the changed policies.
+  virtual void StartWatching(const PolicyCallback& policy_callback);
 
   // Should be called after StartWatching() before the object is deleted. Calls
   // should wait for |stopped_callback| to be called before deleting it.
@@ -118,15 +102,6 @@ class PolicyWatcher {
   // relevant policies.
   void UpdatePolicies(const base::DictionaryValue* new_policy);
 
-  // Signals policy error to the registered |PolicyErrorCallback|.
-  void SignalPolicyError();
-
-  // 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.
-  void SignalTransientPolicyError();
-
   // Used for time-based reloads in case something goes wrong with the
   // notification system.
   void ScheduleFallbackReloadTask();
@@ -139,9 +114,7 @@ class PolicyWatcher {
   void StopWatchingOnPolicyWatcherThread();
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
 
-  PolicyUpdatedCallback policy_updated_callback_;
-  PolicyErrorCallback policy_error_callback_;
-  int transient_policy_error_retry_counter_;
+  PolicyCallback policy_callback_;
 
   scoped_ptr<base::DictionaryValue> old_policies_;
   scoped_ptr<base::DictionaryValue> default_values_;
diff --git a/remoting/host/policy_hack/policy_watcher_linux.cc b/remoting/host/policy_hack/policy_watcher_linux.cc
index 20724b8..3ffefea 100644
--- a/remoting/host/policy_hack/policy_watcher_linux.cc
+++ b/remoting/host/policy_hack/policy_watcher_linux.cc
@@ -186,7 +186,6 @@ class PolicyWatcherLinux : public PolicyWatcher {
     } else {
       // A failure to load policy definitions is probably temporary, so try
       // again soon.
-      SignalTransientPolicyError();
       ScheduleReloadTask(base::TimeDelta::FromSeconds(kSettleIntervalSeconds));
     }
   }
diff --git a/remoting/host/policy_hack/policy_watcher_unittest.cc b/remoting/host/policy_hack/policy_watcher_unittest.cc
index cb94154..bef254c 100644
--- a/remoting/host/policy_hack/policy_watcher_unittest.cc
+++ b/remoting/host/policy_hack/policy_watcher_unittest.cc
@@ -24,12 +24,8 @@ class PolicyWatcherTest : public testing::Test {
 
   void SetUp() override {
     message_loop_proxy_ = base::MessageLoopProxy::current();
-    policy_updated_callback_ = base::Bind(
-        &MockPolicyCallback::OnPolicyUpdate,
-        base::Unretained(&mock_policy_callback_));
-    policy_error_callback_ = base::Bind(
-        &MockPolicyCallback::OnPolicyError,
-        base::Unretained(&mock_policy_callback_));
+    policy_callback_ = base::Bind(&MockPolicyCallback::OnPolicyUpdate,
+                                  base::Unretained(&mock_policy_callback_));
     policy_watcher_.reset(new FakePolicyWatcher(message_loop_proxy_));
     nat_true_.SetBoolean(PolicyWatcher::kNatPolicyName, true);
     nat_false_.SetBoolean(PolicyWatcher::kNatPolicyName, false);
@@ -98,9 +94,7 @@ class PolicyWatcherTest : public testing::Test {
 
  protected:
   void StartWatching() {
-    policy_watcher_->StartWatching(
-        policy_updated_callback_,
-        policy_error_callback_);
+    policy_watcher_->StartWatching(policy_callback_);
     base::RunLoop().RunUntilIdle();
   }
 
@@ -118,8 +112,7 @@ class PolicyWatcherTest : public testing::Test {
   base::MessageLoop message_loop_;
   scoped_refptr<base::MessageLoopProxy> message_loop_proxy_;
   MockPolicyCallback mock_policy_callback_;
-  PolicyWatcher::PolicyUpdatedCallback policy_updated_callback_;
-  PolicyWatcher::PolicyErrorCallback policy_error_callback_;
+  PolicyWatcher::PolicyCallback policy_callback_;
   scoped_ptr<FakePolicyWatcher> policy_watcher_;
   base::DictionaryValue empty_;
   base::DictionaryValue nat_true_;
@@ -411,42 +404,5 @@ TEST_F(PolicyWatcherTest, UdpPortRange) {
   StopWatching();
 }
 
-const int kMaxTransientErrorRetries = 5;
-
-TEST_F(PolicyWatcherTest, SingleTransientErrorDoesntTriggerErrorCallback) {
-  EXPECT_CALL(mock_policy_callback_, OnPolicyErrorPtr()).Times(0);
-
-  StartWatching();
-  policy_watcher_->SignalTransientErrorForTest();
-  StopWatching();
-}
-
-TEST_F(PolicyWatcherTest, MultipleTransientErrorsTriggerErrorCallback) {
-  EXPECT_CALL(mock_policy_callback_, OnPolicyErrorPtr());
-
-  StartWatching();
-  for (int i = 0; i < kMaxTransientErrorRetries; i++) {
-    policy_watcher_->SignalTransientErrorForTest();
-  }
-  StopWatching();
-}
-
-TEST_F(PolicyWatcherTest, PolicyUpdateResetsTransientErrorsCounter) {
-  testing::InSequence s;
-  EXPECT_CALL(mock_policy_callback_,
-              OnPolicyUpdatePtr(IsPolicies(&nat_true_others_default_)));
-  EXPECT_CALL(mock_policy_callback_, OnPolicyErrorPtr()).Times(0);
-
-  StartWatching();
-  for (int i = 0; i < (kMaxTransientErrorRetries - 1); i++) {
-    policy_watcher_->SignalTransientErrorForTest();
-  }
-  policy_watcher_->SetPolicies(&nat_true_);
-  for (int i = 0; i < (kMaxTransientErrorRetries - 1); i++) {
-    policy_watcher_->SignalTransientErrorForTest();
-  }
-  StopWatching();
-}
-
 }  // namespace policy_hack
 }  // namespace remoting
diff --git a/remoting/host/remoting_me2me_host.cc b/remoting/host/remoting_me2me_host.cc
index d81c4ed..a01122a 100644
--- a/remoting/host/remoting_me2me_host.cc
+++ b/remoting/host/remoting_me2me_host.cc
@@ -233,7 +233,6 @@ class HostProcess
 
   // Handles policy updates, by calling On*PolicyUpdate methods.
   void OnPolicyUpdate(scoped_ptr<base::DictionaryValue> policies);
-  void OnPolicyError();
   void ApplyHostDomainPolicy();
   void ApplyUsernamePolicy();
   bool OnHostDomainPolicyUpdate(base::DictionaryValue* policies);
@@ -529,8 +528,7 @@ void HostProcess::OnConfigUpdated(
     policy_watcher_ = policy_hack::PolicyWatcher::Create(
         nullptr, context_->network_task_runner());
     policy_watcher_->StartWatching(
-        base::Bind(&HostProcess::OnPolicyUpdate, base::Unretained(this)),
-        base::Bind(&HostProcess::OnPolicyError, base::Unretained(this)));
+        base::Bind(&HostProcess::OnPolicyUpdate, base::Unretained(this)));
   } else {
     // Reapply policies that could be affected by a new config.
     ApplyHostDomainPolicy();
@@ -935,15 +933,6 @@ void HostProcess::OnPolicyUpdate(scoped_ptr<base::DictionaryValue> policies) {
   }
 }
 
-void HostProcess::OnPolicyError() {
-  context_->network_task_runner()->PostTask(
-      FROM_HERE,
-      base::Bind(
-          &HostProcess::ShutdownHost,
-          this,
-          kInvalidHostConfigurationExitCode));
-}
-
 void HostProcess::ApplyHostDomainPolicy() {
   HOST_LOG << "Policy sets host domain: " << host_domain_;