Fix getting notified twice after subscribe to notifications and call readValue

The CL:
https://codereview.chromium.org/2718583002
adds code that sends an event on the readValue callback. So if subscribes
to notifications and then calls readValue, will get notified twice. This
CL fixes this issue.

BUG=697702

Review-Url: https://codereview.chromium.org/2728623004
Cr-Commit-Position: refs/heads/master@{#455652}

device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc

@@ -202,7 +202,6 @@ void BluetoothRemoteGattCharacteristicAndroid::OnRead(
   if (status == 0  // android.bluetooth.BluetoothGatt.GATT_SUCCESS
       && !read_callback.is_null()) {
     base::android::JavaByteArrayToByteVector(env, value, &value_);
-    adapter_->NotifyGattCharacteristicValueChanged(this, value_);
     read_callback.Run(value_);
   } else if (!read_error_callback.is_null()) {
     read_error_callback.Run(

device/bluetooth/bluetooth_remote_gatt_characteristic_mac.h

@@ -74,8 +74,8 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothRemoteGattCharacteristicMac
   // Called by the BluetoothRemoteGattServiceMac instance when the
   // characteristics value has been read.
   void DidUpdateValue(NSError* error);
-  // Updates value_ and notifies the adapter of the new value.
-  void UpdateValueAndNotify();
+  // Updates value_.
+  void UpdateValue();
   // Called by the BluetoothRemoteGattServiceMac instance when the
   // characteristics value has been written.
   void DidWriteValue(NSError* error);

device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm

@@ -257,10 +257,12 @@ void BluetoothRemoteGattCharacteristicMac::DidUpdateValue(NSError* error) {
       callbacks.second.Run(error_code);
       return;
     }
-    UpdateValueAndNotify();
+    UpdateValue();
     callbacks.first.Run(value_);
   } else if (IsNotifying()) {
-    UpdateValueAndNotify();
+    UpdateValue();
+    gatt_service_->GetMacAdapter()->NotifyGattCharacteristicValueChanged(
+        this, value_);
   } else {
     // In case of buggy device, nothing should be done if receiving extra
     // read confirmation.
@@ -269,12 +271,10 @@ void BluetoothRemoteGattCharacteristicMac::DidUpdateValue(NSError* error) {
   }
 }
 
-void BluetoothRemoteGattCharacteristicMac::UpdateValueAndNotify() {
+void BluetoothRemoteGattCharacteristicMac::UpdateValue() {
   NSData* nsdata_value = cb_characteristic_.get().value;
   const uint8_t* buffer = static_cast<const uint8_t*>(nsdata_value.bytes);
   value_.assign(buffer, buffer + nsdata_value.length);
-  gatt_service_->GetMacAdapter()->NotifyGattCharacteristicValueChanged(this,
-                                                                       value_);
 }
 
 void BluetoothRemoteGattCharacteristicMac::DidWriteValue(NSError* error) {

device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc

@@ -526,14 +526,14 @@ static void test_callback(
     BluetoothRemoteGattCharacteristic::ValueCallback callback,
     const TestBluetoothAdapterObserver& callback_observer,
     const std::vector<uint8_t>& value) {
-  EXPECT_EQ(1, callback_observer.gatt_characteristic_value_changed_count());
+  EXPECT_EQ(0, callback_observer.gatt_characteristic_value_changed_count());
   callback.Run(value);
 }
 
-// Tests that ReadRemoteCharacteristic results in a
+// Tests that ReadRemoteCharacteristic doesn't result in a
 // GattCharacteristicValueChanged call.
 TEST_F(BluetoothRemoteGattCharacteristicTest,
-       ReadRemoteCharacteristic_GattCharacteristicValueChanged) {
+       ReadRemoteCharacteristic_GattCharacteristicValueChangedNotCalled) {
   if (!PlatformSupportsLowEnergy()) {
     LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
     return;
@@ -552,12 +552,17 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
   SimulateGattCharacteristicRead(characteristic1_, test_vector);
   base::RunLoop().RunUntilIdle();
 
-  EXPECT_EQ(1, observer.gatt_characteristic_value_changed_count());
-  EXPECT_EQ(characteristic1_->GetIdentifier(),
-            observer.last_gatt_characteristic_id());
-  EXPECT_EQ(characteristic1_->GetUUID(),
-            observer.last_gatt_characteristic_uuid());
-  EXPECT_EQ(test_vector, observer.last_changed_characteristic_value());
+  EXPECT_EQ(0, observer.gatt_characteristic_value_changed_count());
+// TODO(https://crbug.com/699694): Remove this #if once the bug on Windows is
+// fixed.
+#if defined(OS_WIN)
+  EXPECT_FALSE(observer.last_gatt_characteristic_id().empty());
+  EXPECT_TRUE(observer.last_gatt_characteristic_uuid().IsValid());
+#else
+  EXPECT_TRUE(observer.last_gatt_characteristic_id().empty());
+  EXPECT_FALSE(observer.last_gatt_characteristic_uuid().IsValid());
+#endif  // defined(OS_WIN)
+  EXPECT_TRUE(observer.last_changed_characteristic_value().empty());
 }
 #endif  // defined(OS_ANDROID) || defined(OS_MACOSX) || defined(OS_WIN)
 

device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc

@@ -397,9 +397,6 @@ void BluetoothRemoteGattCharacteristicWin::
     for (ULONG i = 0; i < value->DataSize; i++)
       characteristic_value_.push_back(value->Data[i]);
 
-    parent_service_->GetWinAdapter()->NotifyGattCharacteristicValueChanged(
-        this, characteristic_value_);
-
     callbacks.first.Run(characteristic_value_);
   }
   characteristic_value_read_or_write_in_progress_ = false;

device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc

@@ -1101,8 +1101,7 @@ TEST_F(BluetoothGattBlueZTest, GattCharacteristicValue) {
   EXPECT_EQ(BluetoothRemoteGattService::GATT_ERROR_FAILED, last_service_error_);
   EXPECT_EQ(0, observer.gatt_characteristic_value_changed_count());
 
-  // Issue a read request. A successful read results in a
-  // CharacteristicValueChanged notification.
+  // Issue a read request.
   characteristic = service->GetCharacteristic(
       fake_bluetooth_gatt_characteristic_client_->GetBodySensorLocationPath()
           .value());
@@ -1119,7 +1118,7 @@ TEST_F(BluetoothGattBlueZTest, GattCharacteristicValue) {
                  base::Unretained(this)));
   EXPECT_EQ(2, success_callback_count_);
   EXPECT_EQ(4, error_callback_count_);
-  EXPECT_EQ(1, observer.gatt_characteristic_value_changed_count());
+  EXPECT_EQ(0, observer.gatt_characteristic_value_changed_count());
   EXPECT_TRUE(ValuesEqual(characteristic->GetValue(), last_read_value_));
 
   // Test long-running actions.
@@ -1143,7 +1142,7 @@ TEST_F(BluetoothGattBlueZTest, GattCharacteristicValue) {
   // tne next one.
   EXPECT_EQ(2, success_callback_count_);
   EXPECT_EQ(4, error_callback_count_);
-  EXPECT_EQ(1, observer.gatt_characteristic_value_changed_count());
+  EXPECT_EQ(0, observer.gatt_characteristic_value_changed_count());
 
   // Next read should error because IN_PROGRESS
   characteristic->ReadRemoteCharacteristic(
@@ -1157,7 +1156,7 @@ TEST_F(BluetoothGattBlueZTest, GattCharacteristicValue) {
 
   // But previous call finished.
   EXPECT_EQ(3, success_callback_count_);
-  EXPECT_EQ(2, observer.gatt_characteristic_value_changed_count());
+  EXPECT_EQ(0, observer.gatt_characteristic_value_changed_count());
   EXPECT_TRUE(ValuesEqual(characteristic->GetValue(), last_read_value_));
   fake_bluetooth_gatt_characteristic_client_->SetExtraProcessing(0);
 
@@ -1172,7 +1171,7 @@ TEST_F(BluetoothGattBlueZTest, GattCharacteristicValue) {
   EXPECT_EQ(6, error_callback_count_);
   EXPECT_EQ(BluetoothRemoteGattService::GATT_ERROR_NOT_AUTHORIZED,
             last_service_error_);
-  EXPECT_EQ(2, observer.gatt_characteristic_value_changed_count());
+  EXPECT_EQ(0, observer.gatt_characteristic_value_changed_count());
   fake_bluetooth_gatt_characteristic_client_->SetAuthorized(true);
 
   // Test unauthenticated / needs login.
@@ -1186,7 +1185,7 @@ TEST_F(BluetoothGattBlueZTest, GattCharacteristicValue) {
   EXPECT_EQ(7, error_callback_count_);
   EXPECT_EQ(BluetoothRemoteGattService::GATT_ERROR_NOT_PAIRED,
             last_service_error_);
-  EXPECT_EQ(2, observer.gatt_characteristic_value_changed_count());
+  EXPECT_EQ(0, observer.gatt_characteristic_value_changed_count());
   fake_bluetooth_gatt_characteristic_client_->SetAuthenticated(true);
 }
 

device/bluetooth/bluez/bluetooth_remote_gatt_characteristic_bluez.cc

@@ -45,6 +45,7 @@ BluetoothRemoteGattCharacteristicBlueZ::BluetoothRemoteGattCharacteristicBlueZ(
     : BluetoothGattCharacteristicBlueZ(object_path),
       has_notify_session_(false),
       service_(service),
+      num_of_characteristic_value_read_in_progress_(0),
       weak_ptr_factory_(this) {
   VLOG(1) << "Creating remote GATT characteristic with identifier: "
           << GetIdentifier() << ", UUID: " << GetUUID().canonical_value();
@@ -184,11 +185,15 @@ void BluetoothRemoteGattCharacteristicBlueZ::ReadRemoteCharacteristic(
           << GetIdentifier() << ", UUID: " << GetUUID().canonical_value()
           << ".";
 
+  DCHECK_GE(num_of_characteristic_value_read_in_progress_, 0);
+  ++num_of_characteristic_value_read_in_progress_;
+
   bluez::BluezDBusManager::Get()
       ->GetBluetoothGattCharacteristicClient()
-      ->ReadValue(object_path(), callback,
-                  base::Bind(&BluetoothRemoteGattCharacteristicBlueZ::OnError,
-                             weak_ptr_factory_.GetWeakPtr(), error_callback));
+      ->ReadValue(
+          object_path(), callback,
+          base::Bind(&BluetoothRemoteGattCharacteristicBlueZ::OnReadError,
+                     weak_ptr_factory_.GetWeakPtr(), error_callback));
 }
 
 void BluetoothRemoteGattCharacteristicBlueZ::WriteRemoteCharacteristic(
@@ -201,9 +206,10 @@ void BluetoothRemoteGattCharacteristicBlueZ::WriteRemoteCharacteristic(
 
   bluez::BluezDBusManager::Get()
       ->GetBluetoothGattCharacteristicClient()
-      ->WriteValue(object_path(), value, callback,
-                   base::Bind(&BluetoothRemoteGattCharacteristicBlueZ::OnError,
-                              weak_ptr_factory_.GetWeakPtr(), error_callback));
+      ->WriteValue(
+          object_path(), value, callback,
+          base::Bind(&BluetoothRemoteGattCharacteristicBlueZ::OnWriteError,
+                     weak_ptr_factory_.GetWeakPtr(), error_callback));
 }
 
 void BluetoothRemoteGattCharacteristicBlueZ::SubscribeToNotifications(
@@ -353,7 +359,19 @@ void BluetoothRemoteGattCharacteristicBlueZ::OnStopNotifyError(
   OnStopNotifySuccess(callback);
 }
 
-void BluetoothRemoteGattCharacteristicBlueZ::OnError(
+void BluetoothRemoteGattCharacteristicBlueZ::OnReadError(
+    const ErrorCallback& error_callback,
+    const std::string& error_name,
+    const std::string& error_message) {
+  VLOG(1) << "Operation failed: " << error_name
+          << ", message: " << error_message;
+  --num_of_characteristic_value_read_in_progress_;
+  DCHECK_GE(num_of_characteristic_value_read_in_progress_, 0);
+  error_callback.Run(
+      BluetoothGattServiceBlueZ::DBusErrorToServiceError(error_name));
+}
+
+void BluetoothRemoteGattCharacteristicBlueZ::OnWriteError(
     const ErrorCallback& error_callback,
     const std::string& error_name,
     const std::string& error_message) {

device/bluetooth/bluez/bluetooth_remote_gatt_characteristic_bluez.h

@@ -107,11 +107,17 @@ class BluetoothRemoteGattCharacteristicBlueZ
                          const std::string& error_name,
                          const std::string& error_message);
 
-  // Called by dbus:: on unsuccessful completion of a request to read or write
+  // Called by dbus:: on unsuccessful completion of a request to read
   // the characteristic value.
-  void OnError(const ErrorCallback& error_callback,
-               const std::string& error_name,
-               const std::string& error_message);
+  void OnReadError(const ErrorCallback& error_callback,
+                   const std::string& error_name,
+                   const std::string& error_message);
+
+  // Called by dbus:: on unsuccessful completion of a request to write
+  // the characteristic value.
+  void OnWriteError(const ErrorCallback& error_callback,
+                    const std::string& error_name,
+                    const std::string& error_message);
 
   // True, if there exists a Bluez notify session.
   bool has_notify_session_;
@@ -130,6 +136,9 @@ class BluetoothRemoteGattCharacteristicBlueZ
   // The GATT service this GATT characteristic belongs to.
   BluetoothRemoteGattServiceBlueZ* service_;
 
+  // Number of gatt read requests in progress.
+  int num_of_characteristic_value_read_in_progress_;
+
   // Note: This should remain the last member so it'll be destroyed and
   // invalidate its weak pointers before any other members are destroyed.
   base::WeakPtrFactory<BluetoothRemoteGattCharacteristicBlueZ>

device/bluetooth/bluez/bluetooth_remote_gatt_service_bluez.cc

@@ -243,9 +243,15 @@ void BluetoothRemoteGattServiceBlueZ::GattCharacteristicPropertyChanged(
 
   if (property_name == properties->flags.name())
     NotifyServiceChanged();
-  else if (property_name == properties->value.name())
-    GetAdapter()->NotifyGattCharacteristicValueChanged(
-        iter->second, properties->value.value());
+  else if (property_name == properties->value.name()) {
+    DCHECK_GE(iter->second->num_of_characteristic_value_read_in_progress_, 0);
+    if (iter->second->num_of_characteristic_value_read_in_progress_ > 0) {
+      --iter->second->num_of_characteristic_value_read_in_progress_;
+    } else {
+      GetAdapter()->NotifyGattCharacteristicValueChanged(
+          iter->second, properties->value.value());
+    }
+  }
 }
 
 }  // namespace bluez