Making DeviceCapabilities threadsafe with locking.

chromecast/base/device_capabilities_impl.h

Index: chromecast/base/device_capabilities_impl.h
diff --git a/chromecast/base/device_capabilities_impl.h b/chromecast/base/device_capabilities_impl.h
index e58774cf088d51ed221fc4374cbb6ae5e943394e..c191f04d631771df4aadf451ad5e4d825247faed 100644
--- a/chromecast/base/device_capabilities_impl.h
+++ b/chromecast/base/device_capabilities_impl.h
@@ -6,10 +6,16 @@
 #define CHROMECAST_BASE_DEVICE_CAPABILITIES_IMPL_H_
 
 #include "base/containers/hash_tables.h"
-#include "base/observer_list.h"
-#include "base/threading/thread_checker.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/observer_list_threadsafe.h"
+#include "base/synchronization/lock.h"
 #include "chromecast/base/device_capabilities.h"
 
+namespace base {
+class SingleThreadTaskRunner;
+}
+
 namespace chromecast {
 
 class DeviceCapabilitiesImpl : public DeviceCapabilities {
@@ -22,10 +28,9 @@ class DeviceCapabilitiesImpl : public DeviceCapabilities {
   Validator* GetValidator(const std::string& key) const override;
   bool BluetoothSupported() const override;
   bool DisplaySupported() const override;
-  bool GetCapability(const std::string& path,
-                     const base::Value** out_value) const override;
-  const std::string& GetCapabilitiesString() const override;
-  const base::DictionaryValue* GetCapabilities() const override;
+  scoped_ptr<base::Value> GetCapability(const std::string& path) const override;
+  std::string GetCapabilitiesString() const override;
+  scoped_ptr<base::DictionaryValue> GetCapabilities() const override;
   void SetCapability(const std::string& path,
                      scoped_ptr<base::Value> proposed_value) override;
   void MergeDictionary(const base::DictionaryValue& dict_value) override;
@@ -33,28 +38,80 @@ class DeviceCapabilitiesImpl : public DeviceCapabilities {
   void RemoveCapabilitiesObserver(Observer* observer) override;
 
  private:
+  // Customized wrapper class to make capabilities-related members immutable
+  // and RefCountedThreadSafe. Not using RefCountedData class since it requires
+  // a copy-constructable template parameter, which DictionaryValue does not
+  // meet.
+  template <class T>
+  struct ImmutableRefCounted
+      : public base::RefCountedThreadSafe<ImmutableRefCounted<T>> {
+   public:
+    ImmutableRefCounted();
+    explicit ImmutableRefCounted(scoped_ptr<const T> data_arg);
+
+    // Using scoped_ptr instead of raw data member (T) because DictionaryValue
+    // and string usage in this class tends to get passed by scoped_ptr.
+    const scoped_ptr<const T> data;

[byungchul, 2015/10/27 20:38:22]

This struct is not copyable. Please make this a class. And, a simple getter
instead of direct member var access.

[esum, 2015/10/28 00:29:51]

Done.

+
+   private:
+    friend class base::RefCountedThreadSafe<ImmutableRefCounted<T>>;
+
+    ~ImmutableRefCounted() {}
+
+    DISALLOW_COPY_AND_ASSIGN(ImmutableRefCounted);
+  };
+
+  struct ValidatorInfo {
+    explicit ValidatorInfo(Validator* validator_arg);
+    ~ValidatorInfo();
+
+    Validator* const validator;
+    // TaskRunner of thread that validator_ was registered on
+    const scoped_refptr<base::SingleThreadTaskRunner> task_runner;
+  };
+
   // For DeviceCapabilitiesImpl()
   friend class DeviceCapabilities;
   // For SetValidatedValueInternal()
   friend class DeviceCapabilities::Validator;
 
-  // Map from capability key to corresponding Validator. Gets updated
+  // Map from capability key to corresponding ValidatorInfo. Gets updated
   // in Register()/Unregister().
-  typedef base::hash_map<std::string, Validator*> ValidatorMap;
+  typedef base::hash_map<std::string, ValidatorInfo> ValidatorMap;
+  typedef ImmutableRefCounted<base::DictionaryValue>
+      ImmutableRefCountedDictionary;
+  typedef ImmutableRefCounted<std::string> ImmutableRefCountedString;
 
   // Internal constructor used by static DeviceCapabilities::Create*() methods.
   DeviceCapabilitiesImpl();
 
-  void SetValidatedValueInternal(const std::string& path,
-                                 scoped_ptr<base::Value> new_value) override;
-  void UpdateStrAndNotifyChanged(const std::string& path);
+  void SetValidatedValue(const std::string& path,
+                         scoped_ptr<base::Value> new_value) override;
+  void RunValidateOnCurrentThread(const std::string& path,
+                                  scoped_ptr<base::Value> proposed_value,
+                                  Validator* validator);
+  scoped_refptr<ImmutableRefCountedDictionary> GetCapabilitiesReference() const;
+  scoped_refptr<ImmutableRefCountedString> GetCapabilitiesStrReference() const;
+
+  // Defined locking order if acquiring both locks at same time from a thread
+  // to prevent deadlocks:
+  // 1) validation_lock_
+  // 2) capabilities_lock_
+
+  // Lock for all access to capabilities related members (capabilities_ and
+  // capabilities_str_)
+  mutable base::Lock capabilities_lock_;
+  // Lock for all access to validation members (validator_map_)
+  mutable base::Lock validation_lock_;
 
-  scoped_ptr<base::DictionaryValue> capabilities_;
-  scoped_ptr<const std::string> capabilities_str_;
+  scoped_refptr<ImmutableRefCountedDictionary> capabilities_;
+  scoped_refptr<ImmutableRefCountedString> capabilities_str_;
+  // TaskRunner for capability writes. All internal writes to capabilities_
+  // must occur on task_runner_for_writes_'s thread.
+  const scoped_refptr<base::SingleThreadTaskRunner> task_runner_for_writes_;
 
   ValidatorMap validator_map_;
-  base::ObserverList<Observer> observer_list_;
-  base::ThreadChecker thread_checker_;
+  const scoped_refptr<base::ObserverListThreadSafe<Observer>> observer_list_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceCapabilitiesImpl);
 };