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..4031b84e5163477c848514db577281b3aad956e0 100644
--- a/chromecast/base/device_capabilities_impl.h
+++ b/chromecast/base/device_capabilities_impl.h
@@ -6,10 +6,17 @@
 #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/memory/weak_ptr.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 +29,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 +39,92 @@ 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.
+  class ImmutableCapabilitiesData
+      : public base::RefCountedThreadSafe<ImmutableCapabilitiesData> {
+   public:
+    ImmutableCapabilitiesData();
+    explicit ImmutableCapabilitiesData(
+        scoped_ptr<const base::DictionaryValue> dictionary);
+
+    const base::DictionaryValue* dictionary() const {
+      return dictionary_.get();
+    }
+
+    const std::string* json_string() const { return json_string_.get(); }

[byungchul, 2015/10/28 01:09:43]

Can they be null? Otherwise, const&

[esum, 2015/10/28 04:16:27]

Done.

+
+   private:
+    friend class base::RefCountedThreadSafe<ImmutableCapabilitiesData>;
+
+    ~ImmutableCapabilitiesData();
+
+    const scoped_ptr<const base::DictionaryValue> dictionary_;
+    const scoped_ptr<const std::string> json_string_;
+
+    DISALLOW_COPY_AND_ASSIGN(ImmutableCapabilitiesData);
+  };
+
+  class ValidatorInfo : public base::SupportsWeakPtr<ValidatorInfo> {
+   public:
+    explicit ValidatorInfo(Validator* validator);
+    ~ValidatorInfo();
+
+    Validator* validator() const { return validator_; }
+
+    base::SingleThreadTaskRunner* task_runner() const {

[byungchul, 2015/10/28 01:09:43]

Please comment why it is not a scoped_refptr

[esum, 2015/10/28 04:16:27]

Done. Was just trying to avoid a call to AddRef(), and const
scoped_refptr<base::SingleThreadTaskRunner>& looked too verbose.

[byungchul, 2015/10/28 16:04:18]

It depends. Sometimes returning ref-counted object without scoped_refptr has an
edge case when it is destroy after function returns since it doesn't increase
ref-count.
But, in this case, it would be fine because ValidatorInfo holds scoped_refptr
and task_runner is accessed only within lock and ValidatorInfo cannot be
destroyed when task_runner is accessed.

+      return task_runner_.get();
+    }
+
+    void Validate(const std::string& path,
+                  scoped_ptr<base::Value> proposed_value) const;
+
+   private:
+    Validator* const validator_;
+    // TaskRunner of thread that validator_ was registered on
+    const scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+    DISALLOW_COPY_AND_ASSIGN(ValidatorInfo);
+  };
+
   // For DeviceCapabilitiesImpl()
   friend class DeviceCapabilities;
   // For SetValidatedValueInternal()
   friend class DeviceCapabilities::Validator;
 
-  // Map from capability key to corresponding Validator. Gets updated
-  // in Register()/Unregister().
-  typedef base::hash_map<std::string, Validator*> ValidatorMap;
+  // Map from capability key to corresponding ValidatorInfo. Gets updated
+  // in Register()/Unregister(). A ValidatorInfo* is used because
+  // SupportsWeakPtr does not allow ValidatorInfo to be copyable. This means
+  // that the code is responsible for deleting the ValidatorInfo* before an
+  // entry is removed.
+  typedef base::hash_map<std::string, ValidatorInfo*> ValidatorMap;
 
   // 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;
+  scoped_refptr<ImmutableCapabilitiesData> GetCapabilitiesDataReference() const;
+
+  // Defined locking order if acquiring both locks at same time from a thread
+  // to prevent deadlocks:
+  // 1) validation_lock_
+  // 2) capabilities_lock_

[byungchul, 2015/10/28 01:09:43]

Any chances to lock both at a time?

[esum, 2015/10/28 04:16:27]

Not currently. Just put this here as precaution/reminder if it came up.
Removing it.

+
+  // Lock for all access to capabilities related members (capabilities_data_)
+  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<ImmutableCapabilitiesData> capabilities_data_;
+  // TaskRunner for capability writes. All internal writes to
+  // capabilities_data_ 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);
 };