Making DeviceCapabilities threadsafe with locking.

chromecast/base/device_capabilities_impl_unittest.cc

Index: chromecast/base/device_capabilities_impl_unittest.cc
diff --git a/chromecast/base/device_capabilities_impl_unittest.cc b/chromecast/base/device_capabilities_impl_unittest.cc
index eab7a9ec19337d6dec30d52cc8e0f10af618fa43..ce676df8fd8203be9008d1d8a3d5cb7f2bfaabce 100644
--- a/chromecast/base/device_capabilities_impl_unittest.cc
+++ b/chromecast/base/device_capabilities_impl_unittest.cc
@@ -6,6 +6,8 @@
 
 #include <string>
 
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "base/values.h"
 #include "chromecast/base/serializers.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -21,6 +23,10 @@ const char kSampleDictionaryCapability[] =
     "  \"dummy_field_int\": 99"
     "}";
 
+void GetSampleDefaultCapability(std::string* key,
+                                scoped_ptr<base::Value>* init_value);
+void TestBasicOperations(DeviceCapabilities* capabilities);
+
 // Simple capability manager that implements the Validator interface. Either
 // accepts or rejects all proposed changes based on |accept_changes| constructor
 // argument.
@@ -58,6 +64,59 @@ class FakeCapabilityManagerSimple : public DeviceCapabilities::Validator {
   const bool accept_changes_;
 };
 
+// Used to test that capabilities/validator can be read and written in
+// Validate() without encountering deadlocks/unexpected behavior.
+class FakeCapabilityManagerComplex : public DeviceCapabilities::Validator {
+ public:
+  FakeCapabilityManagerComplex(DeviceCapabilities* capabilities,
+                               const std::string& key)
+      : DeviceCapabilities::Validator(capabilities), key_(key) {
+    capabilities->Register(key, this);
+  }
+
+  // Unregisters itself as Validator.
+  ~FakeCapabilityManagerComplex() override {
+    capabilities()->Unregister(key_, this);
+  }
+
+  // Runs TestBasicOperations().
+  void Validate(const std::string& path,
+                scoped_ptr<base::Value> proposed_value) override {
+    TestBasicOperations(capabilities());
+  }
+
+ private:
+  const std::string key_;
+};
+
+// Used to test that capabilities/validators can be read and written in
+// OnCapabilitiesChanged() without encountering deadlocks/unexpected behavior.
+class FakeCapabilitiesObserver : public DeviceCapabilities::Observer {
+ public:
+  FakeCapabilitiesObserver(DeviceCapabilities* capabilities)
+      : capabilities_(capabilities), removed_as_observer(false) {
+    capabilities_->AddCapabilitiesObserver(this);
+  }
+
+  ~FakeCapabilitiesObserver() override {
+    if (!removed_as_observer)
+      capabilities_->RemoveCapabilitiesObserver(this);
+  }
+
+  // Runs TestBasicOperations().
+  void OnCapabilitiesChanged(const std::string& path) override {
+    TestBasicOperations(capabilities_);
+    // To prevent infinite loop of SetCapability() -> OnCapabilitiesChanged()
+    // -> SetCapability() -> OnCapabilitiesChanged() etc.
+    capabilities_->RemoveCapabilitiesObserver(this);
+    removed_as_observer = true;
+  }
+
+ private:
+  DeviceCapabilities* const capabilities_;
+  bool removed_as_observer;
+};
+
 // Used to test that OnCapabilitiesChanged() is called when capabilities are
 // modified
 class MockCapabilitiesObserver : public DeviceCapabilities::Observer {
@@ -116,11 +175,55 @@ bool JsonStringEquals(const std::string& json,
   return dict_json.get() && *dict_json == json;
 }
 
+// The function runs through the set of basic operations of DeviceCapabilities.
+// Register validator for sample default capability, reads capability, writes
+// capability, and unregister validator. After it has completed, use
+// AssertBasicOperationsSuccessful() to ensure that all operations completed
+// successfully. Sample default capability should not be added or registered in
+// class before this function is called.
+void TestBasicOperations(DeviceCapabilities* capabilities) {
+  std::string key;
+  scoped_ptr<base::Value> init_value;
+  GetSampleDefaultCapability(&key, &init_value);
+
+  ASSERT_EQ(capabilities->GetCapability(key), nullptr);
+  ASSERT_EQ(capabilities->GetValidator(key), nullptr);
+
+  // Register and write capability
+  FakeCapabilityManagerSimple* manager(new FakeCapabilityManagerSimple(
+      capabilities, key, init_value->CreateDeepCopy(), true));
+  // Read Validator
+  EXPECT_EQ(capabilities->GetValidator(key), manager);
+  // Read Capability
+  EXPECT_TRUE(base::Value::Equals(capabilities->GetCapability(key).get(),
+                                  init_value.get()));
+  // Unregister
+  delete manager;
+
+  // Write capability again. Provides way of checking that this function
+  // ran and was successful.
+  scoped_ptr<base::Value> new_value = GetSampleDefaultCapabilityNewValue();
+  capabilities->SetCapability(key, new_value.Pass());
+}
+
+// See TestBasicOperations() comment.
+void AssertBasicOperationsSuccessful(const DeviceCapabilities* capabilities) {
+  base::RunLoop().RunUntilIdle();
+  std::string key;
+  scoped_ptr<base::Value> init_value;
+  GetSampleDefaultCapability(&key, &init_value);
+  scoped_ptr<base::Value> value = capabilities->GetCapability(key);
+  ASSERT_NE(value, nullptr);
+  scoped_ptr<base::Value> new_value = GetSampleDefaultCapabilityNewValue();
+  EXPECT_TRUE(base::Value::Equals(value.get(), new_value.get()));
+}
+
 }  // namespace
 
 class DeviceCapabilitiesImplTest : public ::testing::Test {
  protected:
   void SetUp() override {
+    message_loop_.reset(new base::MessageLoop(base::MessageLoop::TYPE_IO));
     capabilities_ = DeviceCapabilities::Create();
     mock_capabilities_observer_.reset(new MockCapabilitiesObserver());
     capabilities_->AddCapabilitiesObserver(mock_capabilities_observer_.get());
@@ -136,6 +239,9 @@ class DeviceCapabilitiesImplTest : public ::testing::Test {
   void TearDown() override {
     capabilities_->RemoveCapabilitiesObserver(
         mock_capabilities_observer_.get());
+    mock_capabilities_observer_.reset();
+    capabilities_.reset();
+    message_loop_.reset();
   }
 
   DeviceCapabilities* capabilities() const { return capabilities_.get(); }
@@ -145,6 +251,7 @@ class DeviceCapabilitiesImplTest : public ::testing::Test {
   }
 
  private:
+  scoped_ptr<base::MessageLoop> message_loop_;
   scoped_ptr<DeviceCapabilities> capabilities_;
   scoped_ptr<MockCapabilitiesObserver> mock_capabilities_observer_;
 };
@@ -170,8 +277,7 @@ TEST_F(DeviceCapabilitiesImplTest, Register) {
   scoped_ptr<const std::string> empty_dict_string(
       SerializeToJson(base::DictionaryValue()));
   EXPECT_EQ(capabilities()->GetCapabilitiesString(), *empty_dict_string);
-  const base::Value* value = nullptr;
-  EXPECT_FALSE(capabilities()->GetCapability(key, &value));
+  EXPECT_EQ(capabilities()->GetCapability(key), nullptr);
 }
 
 // Tests Unregister() of a default capability.
@@ -190,8 +296,7 @@ TEST_F(DeviceCapabilitiesImplTest, Unregister) {
   scoped_ptr<const std::string> empty_dict_string(
       SerializeToJson(base::DictionaryValue()));
   EXPECT_EQ(capabilities()->GetCapabilitiesString(), *empty_dict_string);
-  const base::Value* value = nullptr;
-  EXPECT_FALSE(capabilities()->GetCapability(key, &value));
+  EXPECT_EQ(capabilities()->GetCapability(key), nullptr);
 }
 
 // Tests GetCapability() and updating the value through SetCapability().
@@ -202,15 +307,14 @@ TEST_F(DeviceCapabilitiesImplTest, GetCapabilityAndSetCapability) {
   FakeCapabilityManagerSimple manager(capabilities(), key,
                                       init_value->CreateDeepCopy(), true);
 
-  const base::Value* value = nullptr;
-  EXPECT_TRUE(capabilities()->GetCapability(key, &value));
-  EXPECT_TRUE(base::Value::Equals(value, init_value.get()));
+  EXPECT_TRUE(base::Value::Equals(capabilities()->GetCapability(key).get(),
+                                  init_value.get()));
 
   scoped_ptr<base::Value> new_value = GetSampleDefaultCapabilityNewValue();
   capabilities()->SetCapability(key, new_value->CreateDeepCopy());
-  value = nullptr;
-  EXPECT_TRUE(capabilities()->GetCapability(key, &value));
-  EXPECT_TRUE(base::Value::Equals(value, new_value.get()));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(base::Value::Equals(capabilities()->GetCapability(key).get(),
+                                  new_value.get()));
 }
 
 // Tests BluetoothSupported() and updating this value through SetCapability().
@@ -223,6 +327,7 @@ TEST_F(DeviceCapabilitiesImplTest, BluetoothSupportedAndSetCapability) {
   capabilities()->SetCapability(
       DeviceCapabilities::kKeyBluetoothSupported,
       make_scoped_ptr(new base::FundamentalValue(false)));
+  base::RunLoop().RunUntilIdle();
   EXPECT_FALSE(capabilities()->BluetoothSupported());
 }
 
@@ -236,6 +341,7 @@ TEST_F(DeviceCapabilitiesImplTest, DisplaySupportedAndSetCapability) {
   capabilities()->SetCapability(
       DeviceCapabilities::kKeyDisplaySupported,
       make_scoped_ptr(new base::FundamentalValue(false)));
+  base::RunLoop().RunUntilIdle();
   EXPECT_FALSE(capabilities()->DisplaySupported());
 }
 
@@ -249,10 +355,10 @@ TEST_F(DeviceCapabilitiesImplTest, SetCapabilityInvalid) {
                                       init_value->CreateDeepCopy(), false);
 
   capabilities()->SetCapability(key, GetSampleDefaultCapabilityNewValue());
+  base::RunLoop().RunUntilIdle();
 
-  const base::Value* value = nullptr;
-  EXPECT_TRUE(capabilities()->GetCapability(key, &value));
-  EXPECT_TRUE(base::Value::Equals(init_value.get(), value));
+  EXPECT_TRUE(base::Value::Equals(capabilities()->GetCapability(key).get(),
+                                  init_value.get()));
 }
 
 // Test that SetCapability() updates the capabilities string correctly
@@ -268,6 +374,7 @@ TEST_F(DeviceCapabilitiesImplTest, SetCapabilityUpdatesString) {
 
   scoped_ptr<base::Value> new_value = GetSampleDefaultCapabilityNewValue();
   capabilities()->SetCapability(key, new_value->CreateDeepCopy());
+  base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(JsonStringEquals(capabilities()->GetCapabilitiesString(), key,
                                *new_value));
 }
@@ -293,6 +400,7 @@ TEST_F(DeviceCapabilitiesImplTest, SetCapabilityNotifiesObservers) {
 
   // 3rd call
   capabilities()->SetCapability(key, init_value.Pass());
+  base::RunLoop().RunUntilIdle();
 }
 
 // Test adding dynamic capabilities
@@ -301,20 +409,20 @@ TEST_F(DeviceCapabilitiesImplTest, SetCapabilityDynamic) {
   scoped_ptr<base::Value> init_value;
   GetSampleDynamicCapability(&key, &init_value);
 
-  const base::Value* value = nullptr;
-  ASSERT_FALSE(capabilities()->GetCapability(key, &value));
+  ASSERT_EQ(capabilities()->GetCapability(key), nullptr);
   capabilities()->SetCapability(key, init_value->CreateDeepCopy());
+  base::RunLoop().RunUntilIdle();
 
-  EXPECT_TRUE(capabilities()->GetCapability(key, &value));
-  EXPECT_TRUE(base::Value::Equals(init_value.get(), value));
+  EXPECT_TRUE(base::Value::Equals(capabilities()->GetCapability(key).get(),
+                                  init_value.get()));
   EXPECT_TRUE(JsonStringEquals(capabilities()->GetCapabilitiesString(), key,
                                *init_value));
 
   scoped_ptr<base::Value> new_value = GetSampleDynamicCapabilityNewValue();
   capabilities()->SetCapability(key, new_value->CreateDeepCopy());
-  value = nullptr;
-  EXPECT_TRUE(capabilities()->GetCapability(key, &value));
-  EXPECT_TRUE(base::Value::Equals(new_value.get(), value));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(base::Value::Equals(capabilities()->GetCapability(key).get(),
+                                  new_value.get()));
   EXPECT_TRUE(JsonStringEquals(capabilities()->GetCapabilitiesString(), key,
                                *new_value));
 }
@@ -332,20 +440,21 @@ TEST_F(DeviceCapabilitiesImplTest, SetCapabilityDictionary) {
   capabilities()->SetCapability(
       "dummy_dictionary_key.dummy_field_bool",
       make_scoped_ptr(new base::FundamentalValue(false)));
-  const base::Value* value = nullptr;
+  base::RunLoop().RunUntilIdle();
   bool value_bool = true;
-  EXPECT_TRUE(capabilities()->GetCapability(
-      "dummy_dictionary_key.dummy_field_bool", &value));
+  scoped_ptr<base::Value> value =
+      capabilities()->GetCapability("dummy_dictionary_key.dummy_field_bool");
+  ASSERT_NE(value, nullptr);
   EXPECT_TRUE(value->GetAsBoolean(&value_bool));
   EXPECT_FALSE(value_bool);
 
   capabilities()->SetCapability(
       "dummy_dictionary_key.dummy_field_int",
       make_scoped_ptr(new base::FundamentalValue(100)));
-  value = nullptr;
+  base::RunLoop().RunUntilIdle();
   int value_int = 0;
-  EXPECT_TRUE(capabilities()->GetCapability(
-      "dummy_dictionary_key.dummy_field_int", &value));
+  value = capabilities()->GetCapability("dummy_dictionary_key.dummy_field_int");
+  ASSERT_NE(value, nullptr);
   EXPECT_TRUE(value->GetAsInteger(&value_int));
   EXPECT_EQ(value_int, 100);
 }
@@ -363,20 +472,21 @@ TEST_F(DeviceCapabilitiesImplTest, SetCapabilityDictionaryInvalid) {
   capabilities()->SetCapability(
       "dummy_dictionary_key.dummy_field_bool",
       make_scoped_ptr(new base::FundamentalValue(false)));
-  const base::Value* value = nullptr;
+  base::RunLoop().RunUntilIdle();
   bool value_bool = false;
-  EXPECT_TRUE(capabilities()->GetCapability(
-      "dummy_dictionary_key.dummy_field_bool", &value));
+  scoped_ptr<base::Value> value =
+      capabilities()->GetCapability("dummy_dictionary_key.dummy_field_bool");
+  ASSERT_NE(value, nullptr);
   EXPECT_TRUE(value->GetAsBoolean(&value_bool));
   EXPECT_TRUE(value_bool);
 
   capabilities()->SetCapability(
       "dummy_dictionary_key.dummy_field_int",
       make_scoped_ptr(new base::FundamentalValue(100)));
-  value = nullptr;
+  base::RunLoop().RunUntilIdle();
   int value_int = 0;
-  EXPECT_TRUE(capabilities()->GetCapability(
-      "dummy_dictionary_key.dummy_field_int", &value));
+  value = capabilities()->GetCapability("dummy_dictionary_key.dummy_field_int");
+  ASSERT_NE(value, nullptr);
   EXPECT_TRUE(value->GetAsInteger(&value_int));
   EXPECT_EQ(value_int, 99);
 }
@@ -391,17 +501,19 @@ TEST_F(DeviceCapabilitiesImplTest, MergeDictionary) {
   ASSERT_NE(dict_value, nullptr);
 
   capabilities()->MergeDictionary(*dict_value);
+  base::RunLoop().RunUntilIdle();
 
   // First make sure that capabilities get created if they do not exist
-  const base::Value* value = nullptr;
   bool value_bool = false;
-  EXPECT_TRUE(capabilities()->GetCapability("dummy_field_bool", &value));
+  scoped_ptr<base::Value> value =
+      capabilities()->GetCapability("dummy_field_bool");
+  ASSERT_NE(value, nullptr);
   EXPECT_TRUE(value->GetAsBoolean(&value_bool));
   EXPECT_TRUE(value_bool);
 
-  value = nullptr;
   int value_int = 0;
-  EXPECT_TRUE(capabilities()->GetCapability("dummy_field_int", &value));
+  value = capabilities()->GetCapability("dummy_field_int");
+  ASSERT_NE(value, nullptr);
   EXPECT_TRUE(value->GetAsInteger(&value_int));
   EXPECT_EQ(value_int, 99);
 
@@ -411,17 +523,50 @@ TEST_F(DeviceCapabilitiesImplTest, MergeDictionary) {
   ASSERT_TRUE(dict_value->Remove("dummy_field_bool", nullptr));
 
   capabilities()->MergeDictionary(*dict_value);
+  base::RunLoop().RunUntilIdle();
 
-  value = nullptr;
   value_bool = false;
-  EXPECT_TRUE(capabilities()->GetCapability("dummy_field_bool", &value));
+  value = capabilities()->GetCapability("dummy_field_bool");
+  ASSERT_NE(value, nullptr);
   EXPECT_TRUE(value->GetAsBoolean(&value_bool));
   EXPECT_TRUE(value_bool);
 
-  value = nullptr;
-  EXPECT_TRUE(capabilities()->GetCapability("dummy_field_int", &value));
+  value = capabilities()->GetCapability("dummy_field_int");
+  ASSERT_NE(value, nullptr);
   EXPECT_TRUE(value->GetAsInteger(&value_int));
   EXPECT_EQ(value_int, 100);
 }
 
+// Tests that it is safe to call DeviceCapabilities methods in
+// an Observer's OnCapabilitiesChanged() implementation safely with correct
+// behavior and without deadlocking.
+TEST_F(DeviceCapabilitiesImplTest, OnCapabilitiesChangedSafe) {
+  FakeCapabilitiesObserver observer(capabilities());
+
+  // Trigger FakeCapabilitiesObserver::OnCapabilitiesChanged()
+  capabilities()->SetCapability(
+      "dummy_trigger_key", make_scoped_ptr(new base::FundamentalValue(true)));
+  base::RunLoop().RunUntilIdle();
+
+  // Check that FakeCapabilitiesObserver::OnCapabilitiesChanged() ran and that
+  // behavior was successful
+  AssertBasicOperationsSuccessful(capabilities());
+}
+
+// Tests that it is safe to call DeviceCapabilities methods in a Validator's
+// Validate() implementation safely with correct behavior and without
+// deadlocking.
+TEST_F(DeviceCapabilitiesImplTest, ValidateSafe) {
+  FakeCapabilityManagerComplex manager(capabilities(), "dummy_validate_key");
+
+  // Trigger FakeCapabilityManagerComplex::Validate()
+  capabilities()->SetCapability(
+      "dummy_validate_key", make_scoped_ptr(new base::FundamentalValue(true)));
+  base::RunLoop().RunUntilIdle();
+
+  // Check that FakeCapabilityManagerComplex::Validate() ran and that behavior
+  // was successful
+  AssertBasicOperationsSuccessful(capabilities());
+}
+
 }  // namespace chromecast