Always run PolicyWatcher on UI thread in It2Me host.

remoting/host/policy_hack/policy_watcher.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Most of this code is copied from:
 //   src/chrome/browser/policy/asynchronous_policy_loader.{h,cc}
 
 #include "remoting/host/policy_hack/policy_watcher.h"
 
 #include "base/bind.h"
@@ skipping 77 matching lines @@
 
 policy::PolicyNamespace GetPolicyNamespace() {
   return policy::PolicyNamespace(policy::POLICY_DOMAIN_CHROME, std::string());
 }
 
 }  // namespace
 
 void PolicyWatcher::StartWatching(
     const PolicyUpdatedCallback& policy_updated_callback,
     const PolicyErrorCallback& policy_error_callback) {
-  if (!OnPolicyServiceThread()) {
-    policy_service_task_runner_->PostTask(
-        FROM_HERE,
-        base::Bind(&PolicyWatcher::StartWatching, base::Unretained(this),
-                   policy_updated_callback, policy_error_callback));
-    return;
-  }
+  DCHECK(CalledOnValidThread());
+  DCHECK(!policy_updated_callback.is_null());
+  DCHECK(!policy_error_callback.is_null());

[Łukasz Anforowicz, 2015/01/30 19:50:59]

nit: I wonder if we should/could attempt to catch and prevent calling
StartWatching twice, by doing DCHECK(policy_updated_callback_.is_null()) here.

[Sergey Ulanov, 2015/01/30 20:06:07]

Done.

 
   policy_updated_callback_ = policy_updated_callback;
   policy_error_callback_ = policy_error_callback;
 
   // Listen for future policy changes.
   policy_service_->AddObserver(policy::POLICY_DOMAIN_CHROME, this);
 
   // Process current policy state.
   if (policy_service_->IsInitializationComplete(policy::POLICY_DOMAIN_CHROME)) {
     OnPolicyServiceInitialized(policy::POLICY_DOMAIN_CHROME);
   }
 }
 
-void PolicyWatcher::StopWatching(const base::Closure& stopped_callback) {
-  policy_service_task_runner_->PostTaskAndReply(
-      FROM_HERE, base::Bind(&PolicyWatcher::StopWatchingOnPolicyServiceThread,
-                            base::Unretained(this)),
-      stopped_callback);
-}
-
-void PolicyWatcher::StopWatchingOnPolicyServiceThread() {
-  policy_service_->RemoveObserver(policy::POLICY_DOMAIN_CHROME, this);
-  policy_updated_callback_.Reset();
-  policy_error_callback_.Reset();
-}
-
-bool PolicyWatcher::OnPolicyServiceThread() const {
-  return policy_service_task_runner_->BelongsToCurrentThread();
-}
-
 void PolicyWatcher::UpdatePolicies(
     const base::DictionaryValue* new_policies_raw) {
-  DCHECK(OnPolicyServiceThread());
+  DCHECK(CalledOnValidThread());
 
   transient_policy_error_retry_counter_ = 0;
 
   // Use default values for any missing policies.
   scoped_ptr<base::DictionaryValue> new_policies =
       CopyGoodValuesAndAddDefaults(
           new_policies_raw, default_values_.get(), bad_type_values_.get());
 
   // Find the changed policies.
   scoped_ptr<base::DictionaryValue> changed_policies(
@@ skipping 24 matching lines @@
 
 void PolicyWatcher::SignalTransientPolicyError() {
   const int kMaxRetryCount = 5;
   transient_policy_error_retry_counter_ += 1;
   if (transient_policy_error_retry_counter_ >= kMaxRetryCount) {
     SignalPolicyError();
   }
 }
 
 PolicyWatcher::PolicyWatcher(
-    const scoped_refptr<base::SingleThreadTaskRunner>&
-        policy_service_task_runner,
     policy::PolicyService* policy_service,
     scoped_ptr<policy::PolicyService> owned_policy_service,
     scoped_ptr<policy::ConfigurationPolicyProvider> owned_policy_provider,
     scoped_ptr<policy::SchemaRegistry> owned_schema_registry)
-    : policy_service_task_runner_(policy_service_task_runner),
-      transient_policy_error_retry_counter_(0),
+    : transient_policy_error_retry_counter_(0),
       old_policies_(new base::DictionaryValue()),
       default_values_(new base::DictionaryValue()),
       policy_service_(policy_service),
       owned_schema_registry_(owned_schema_registry.Pass()),
       owned_policy_provider_(owned_policy_provider.Pass()),
       owned_policy_service_(owned_policy_service.Pass()) {
   // Initialize the default values for each policy.
   default_values_->SetBoolean(key::kRemoteAccessHostFirewallTraversal, true);
   default_values_->SetBoolean(key::kRemoteAccessHostRequireTwoFactor, false);
   default_values_->SetBoolean(key::kRemoteAccessHostRequireCurtain, false);
@@ skipping 18 matching lines @@
 
   // Initialize the fall-back values to use for unreadable policies.
   // For most policies these match the defaults.
   bad_type_values_.reset(default_values_->DeepCopy());
   bad_type_values_->SetBoolean(key::kRemoteAccessHostFirewallTraversal, false);
   bad_type_values_->SetBoolean(key::kRemoteAccessHostAllowRelayedConnection,
                                false);
 }
 
 PolicyWatcher::~PolicyWatcher() {
+  // Stop observing |policy_service_| if StartWatching() has been called.
+  if (!policy_updated_callback_.is_null()) {
+    policy_service_->RemoveObserver(policy::POLICY_DOMAIN_CHROME, this);
+  }
+
   if (owned_policy_provider_) {
     owned_policy_provider_->Shutdown();
   }
 }
 
 void PolicyWatcher::OnPolicyUpdated(const policy::PolicyNamespace& ns,
                                     const policy::PolicyMap& previous,
                                     const policy::PolicyMap& current) {
   scoped_ptr<base::DictionaryValue> policy_dict(new base::DictionaryValue());
   for (auto it = current.begin(); it != current.end(); ++it) {
     // TODO(lukasza): Use policy::Schema::Normalize() for schema verification.
     policy_dict->Set(it->first, it->second.value->DeepCopy());
   }
   UpdatePolicies(policy_dict.get());
 }
 
 void PolicyWatcher::OnPolicyServiceInitialized(policy::PolicyDomain domain) {
   policy::PolicyNamespace ns = GetPolicyNamespace();
   const policy::PolicyMap& current = policy_service_->GetPolicies(ns);
   OnPolicyUpdated(ns, current, current);
 }
 
 scoped_ptr<PolicyWatcher> PolicyWatcher::CreateFromPolicyLoader(
-    const scoped_refptr<base::SingleThreadTaskRunner>&
-        policy_service_task_runner,
     scoped_ptr<policy::AsyncPolicyLoader> async_policy_loader) {
   // TODO(lukasza): Schema below should ideally only cover Chromoting-specific
   // policies (expecting perf and maintanability improvement, but no functional
   // impact).
   policy::Schema schema = policy::Schema::Wrap(policy::GetChromeSchemaData());
 
   scoped_ptr<policy::SchemaRegistry> schema_registry(
       new policy::SchemaRegistry());
   schema_registry->RegisterComponent(GetPolicyNamespace(), schema);
 
   scoped_ptr<policy::AsyncPolicyProvider> policy_provider(
       new policy::AsyncPolicyProvider(schema_registry.get(),
                                       async_policy_loader.Pass()));
   policy_provider->Init(schema_registry.get());
 
   policy::PolicyServiceImpl::Providers providers;
   providers.push_back(policy_provider.get());
   scoped_ptr<policy::PolicyService> policy_service(
       new policy::PolicyServiceImpl(providers));
 
   policy::PolicyService* borrowed_policy_service = policy_service.get();
-  return make_scoped_ptr(new PolicyWatcher(
-      policy_service_task_runner, borrowed_policy_service,
-      policy_service.Pass(), policy_provider.Pass(), schema_registry.Pass()));
+  return make_scoped_ptr(
+      new PolicyWatcher(borrowed_policy_service, policy_service.Pass(),
+                        policy_provider.Pass(), schema_registry.Pass()));
 }
 
 scoped_ptr<PolicyWatcher> PolicyWatcher::Create(
     policy::PolicyService* policy_service,
-    const scoped_refptr<base::SingleThreadTaskRunner>& network_task_runner) {
+    const scoped_refptr<base::SingleThreadTaskRunner>& file_task_runner) {
 #if defined(OS_CHROMEOS)
+  // On Chrome OS the PolicyService is owned by the browser.
   DCHECK(policy_service);
   return make_scoped_ptr(
       new PolicyWatcher(content::BrowserThread::GetMessageLoopProxyForThread(
                             content::BrowserThread::UI),
                         policy_service, nullptr, nullptr, nullptr));
-#elif defined(OS_WIN)
+#else  // !defined(OS_CHROMEOS)
   DCHECK(!policy_service);
-  // Always read the Chrome policies (even on Chromium) so that policy
-  // enforcement can't be bypassed by running Chromium.
-  // Note that this comment applies to all of Win/Mac/Posix branches below.
-  static const wchar_t kRegistryKey[] = L"SOFTWARE\\Policies\\Google\\Chrome";
-  return PolicyWatcher::CreateFromPolicyLoader(
-      network_task_runner,
-      policy::PolicyLoaderWin::Create(network_task_runner, kRegistryKey));
+
+  // Create platform-specific PolicyLoader. Always read the Chrome policies
+  // (even on Chromium) so that policy enforcement can't be bypassed by running
+  // Chromium.
+  scoped_ptr<policy::AsyncPolicyLoader> policy_loader;
+#if defined(OS_WIN)
+  policy_loader = policy::PolicyLoaderWin::Create(
+      file_task_runner, L"SOFTWARE\\Policies\\Google\\Chrome");
 #elif defined(OS_MACOSX)
   CFStringRef bundle_id = CFSTR("com.google.Chrome");
-  DCHECK(!policy_service);
-  return PolicyWatcher::CreateFromPolicyLoader(
-      network_task_runner,
-      make_scoped_ptr(new policy::PolicyLoaderMac(
-          network_task_runner,
-          policy::PolicyLoaderMac::GetManagedPolicyPath(bundle_id),
-          new MacPreferences(), bundle_id)));
+  policy_loader.reset(new policy::PolicyLoaderMac(
+      file_task_runner,
+      policy::PolicyLoaderMac::GetManagedPolicyPath(bundle_id),
+      new MacPreferences(), bundle_id));
 #elif defined(OS_POSIX) && !defined(OS_ANDROID)
-  DCHECK(!policy_service);
-  static const base::FilePath::CharType kPolicyDir[] =
-      FILE_PATH_LITERAL("/etc/opt/chrome/policies");
-  return PolicyWatcher::CreateFromPolicyLoader(
-      network_task_runner, make_scoped_ptr(new policy::ConfigDirPolicyLoader(
-                               network_task_runner, base::FilePath(kPolicyDir),
-                               policy::POLICY_SCOPE_MACHINE)));
+  policy_loader.reset(new policy::ConfigDirPolicyLoader(
+      file_task_runner,
+      base::FilePath(FILE_PATH_LITERAL("/etc/opt/chrome/policies")),
+      policy::POLICY_SCOPE_MACHINE));
 #else
 #error OS that is not yet supported by PolicyWatcher code.
 #endif
+
+  return PolicyWatcher::CreateFromPolicyLoader(policy_loader.Pass());
+#endif  // !(OS_CHROMEOS)
 }
 
 }  // namespace policy_hack
 }  // namespace remoting