bluetooth: Fix crash when trying to read or write when operation pending

A return statement was missing so the callback would get called twice.

To avoid future bugs in which callbacks are called unexpectedly, we
add a new parameter to the getters of the mock callbacks. If
Call::EXPECTED is passed then the callback is expected to be called,
otherwise we the callback was not expected to be called and the test
will fail.

We add a TearDown implementation to BluetoothTestBase to check
no unexpected calls have been made to callbacks.

BUG=557571

Review URL: https://codereview.chromium.org/1465863003

Cr-Commit-Position: refs/heads/master@{#361040}

device/bluetooth/bluetooth_adapter_unittest.cc

@@ -139,7 +139,6 @@ class TestPairingDelegate : public BluetoothDevice::PairingDelegate {
   void AuthorizePairing(BluetoothDevice* device) override {}
 };
 
-
 TEST(BluetoothAdapterTest, NoDefaultPairingDelegate) {
   scoped_refptr<BluetoothAdapter> adapter = new TestBluetoothAdapter();
 
@@ -465,9 +464,8 @@ TEST_F(BluetoothTest, DiscoverySession) {
   EXPECT_TRUE(discovery_sessions_[0]->IsActive());
 
   ResetEventCounts();
-  discovery_sessions_[0]->Stop(GetCallback(), GetErrorCallback());
-  EXPECT_EQ(1, callback_count_);
-  EXPECT_EQ(0, error_callback_count_);
+  discovery_sessions_[0]->Stop(GetCallback(Call::EXPECTED),
+                               GetErrorCallback(Call::NOT_EXPECTED));
   EXPECT_FALSE(adapter_->IsDiscovering());
   EXPECT_FALSE(discovery_sessions_[0]->IsActive());
 }
@@ -488,7 +486,7 @@ TEST_F(BluetoothTest, NoPermissions) {
     return;
   }
 
-  StartLowEnergyDiscoverySession();
+  StartLowEnergyDiscoverySessionExpectedToFail();
 
   EXPECT_EQ(0, callback_count_);
   EXPECT_EQ(1, error_callback_count_);

device/bluetooth/bluetooth_gatt_service_unittest.cc

@@ -26,10 +26,10 @@ TEST_F(BluetoothGattServiceTest, GetIdentifier) {
   // 2 devices to verify unique IDs across them.
   BluetoothDevice* device1 = DiscoverLowEnergyDevice(3);
   BluetoothDevice* device2 = DiscoverLowEnergyDevice(4);
-  device1->CreateGattConnection(GetGattConnectionCallback(),
-                                GetConnectErrorCallback());
-  device2->CreateGattConnection(GetGattConnectionCallback(),
-                                GetConnectErrorCallback());
+  device1->CreateGattConnection(GetGattConnectionCallback(Call::EXPECTED),
+                                GetConnectErrorCallback(Call::NOT_EXPECTED));
+  device2->CreateGattConnection(GetGattConnectionCallback(Call::EXPECTED),
+                                GetConnectErrorCallback(Call::NOT_EXPECTED));
   SimulateGattConnection(device1);
   SimulateGattConnection(device2);
 
@@ -62,8 +62,8 @@ TEST_F(BluetoothGattServiceTest, GetUUID) {
   InitWithFakeAdapter();
   StartLowEnergyDiscoverySession();
   BluetoothDevice* device = DiscoverLowEnergyDevice(3);
-  device->CreateGattConnection(GetGattConnectionCallback(),
-                               GetConnectErrorCallback());
+  device->CreateGattConnection(GetGattConnectionCallback(Call::EXPECTED),
+                               GetConnectErrorCallback(Call::NOT_EXPECTED));
   SimulateGattConnection(device);
 
   // Create multiple instances with the same UUID.
@@ -84,8 +84,8 @@ TEST_F(BluetoothGattServiceTest, GetCharacteristics_FindNone) {
   InitWithFakeAdapter();
   StartLowEnergyDiscoverySession();
   BluetoothDevice* device = DiscoverLowEnergyDevice(3);
-  device->CreateGattConnection(GetGattConnectionCallback(),
-                               GetConnectErrorCallback());
+  device->CreateGattConnection(GetGattConnectionCallback(Call::EXPECTED),
+                               GetConnectErrorCallback(Call::NOT_EXPECTED));
   SimulateGattConnection(device);
 
   // Simulate a service, with no Characteristics:
@@ -103,8 +103,8 @@ TEST_F(BluetoothGattServiceTest, GetCharacteristics_and_GetCharacteristic) {
   InitWithFakeAdapter();
   StartLowEnergyDiscoverySession();
   BluetoothDevice* device = DiscoverLowEnergyDevice(3);
-  device->CreateGattConnection(GetGattConnectionCallback(),
-                               GetConnectErrorCallback());
+  device->CreateGattConnection(GetGattConnectionCallback(Call::EXPECTED),
+                               GetConnectErrorCallback(Call::NOT_EXPECTED));
   SimulateGattConnection(device);
 
   // Simulate a service, with several Characteristics:

device/bluetooth/bluetooth_remote_gatt_characteristic_android.cc

@@ -144,7 +144,10 @@ void BluetoothRemoteGattCharacteristicAndroid::ReadRemoteCharacteristic(
     const ValueCallback& callback,
     const ErrorCallback& error_callback) {
   if (read_pending_ || write_pending_) {
-    error_callback.Run(BluetoothGattService::GATT_ERROR_IN_PROGRESS);
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE, base::Bind(error_callback,
+                              BluetoothGattService::GATT_ERROR_IN_PROGRESS));
+    return;
   }
 
   if (!Java_ChromeBluetoothRemoteGattCharacteristic_readRemoteCharacteristic(
@@ -166,7 +169,10 @@ void BluetoothRemoteGattCharacteristicAndroid::WriteRemoteCharacteristic(
     const base::Closure& callback,
     const ErrorCallback& error_callback) {
   if (read_pending_ || write_pending_) {
-    error_callback.Run(BluetoothGattService::GATT_ERROR_IN_PROGRESS);
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE, base::Bind(error_callback,
+                              BluetoothGattService::GATT_ERROR_IN_PROGRESS));
+    return;
   }
 
   JNIEnv* env = AttachCurrentThread();

device/bluetooth/test/bluetooth_test.cc

@@ -34,10 +34,27 @@ void BluetoothTestBase::StartLowEnergyDiscoverySession() {
   adapter_->StartDiscoverySessionWithFilter(
       make_scoped_ptr(new BluetoothDiscoveryFilter(
           BluetoothDiscoveryFilter::Transport::TRANSPORT_LE)),
-      GetDiscoverySessionCallback(), GetErrorCallback());
+      GetDiscoverySessionCallback(Call::EXPECTED),
+      GetErrorCallback(Call::NOT_EXPECTED));
   base::RunLoop().RunUntilIdle();
 }
 
+void BluetoothTestBase::StartLowEnergyDiscoverySessionExpectedToFail() {
+  adapter_->StartDiscoverySessionWithFilter(
+      make_scoped_ptr(new BluetoothDiscoveryFilter(
+          BluetoothDiscoveryFilter::Transport::TRANSPORT_LE)),
+      GetDiscoverySessionCallback(Call::NOT_EXPECTED),
+      GetErrorCallback(Call::EXPECTED));
+  base::RunLoop().RunUntilIdle();
+}
+
+void BluetoothTestBase::TearDown() {
+  EXPECT_EQ(expected_success_callback_calls_, actual_success_callback_calls_);
+  EXPECT_EQ(expected_error_callback_calls_, actual_error_callback_calls_);
+  EXPECT_FALSE(unexpected_success_callback_);
+  EXPECT_FALSE(unexpected_error_callback_);
+}
+
 bool BluetoothTestBase::DenyPermission() {
   return false;
 }
@@ -52,92 +69,156 @@ void BluetoothTestBase::DeleteDevice(BluetoothDevice* device) {
   adapter_->DeleteDeviceForTesting(device->GetAddress());
 }
 
-void BluetoothTestBase::Callback() {
+void BluetoothTestBase::Callback(Call expected) {
   ++callback_count_;
+
+  if (expected == Call::EXPECTED)
+    ++actual_success_callback_calls_;
+  else
+    unexpected_success_callback_ = true;
 }
 
 void BluetoothTestBase::DiscoverySessionCallback(
+    Call expected,
     scoped_ptr<BluetoothDiscoverySession> discovery_session) {
   ++callback_count_;
   discovery_sessions_.push_back(discovery_session.release());
+
+  if (expected == Call::EXPECTED)
+    ++actual_success_callback_calls_;
+  else
+    unexpected_success_callback_ = true;
 }
 
 void BluetoothTestBase::GattConnectionCallback(
+    Call expected,
     scoped_ptr<BluetoothGattConnection> connection) {
   ++callback_count_;
   gatt_connections_.push_back(connection.release());
+
+  if (expected == Call::EXPECTED)
+    ++actual_success_callback_calls_;
+  else
+    unexpected_success_callback_ = true;
 }
 
 void BluetoothTestBase::NotifyCallback(
+    Call expected,
     scoped_ptr<BluetoothGattNotifySession> notify_session) {
   ++callback_count_;
   notify_sessions_.push_back(notify_session.release());
+
+  if (expected == Call::EXPECTED)
+    ++actual_success_callback_calls_;
+  else
+    unexpected_success_callback_ = true;
 }
 
-void BluetoothTestBase::ReadValueCallback(const std::vector<uint8>& value) {
+void BluetoothTestBase::ReadValueCallback(Call expected,
+                                          const std::vector<uint8>& value) {
   ++callback_count_;
   last_read_value_ = value;
+
+  if (expected == Call::EXPECTED)
+    ++actual_success_callback_calls_;
+  else
+    unexpected_success_callback_ = true;
 }
 
-void BluetoothTestBase::ErrorCallback() {
+void BluetoothTestBase::ErrorCallback(Call expected) {
   ++error_callback_count_;
+
+  if (expected == Call::EXPECTED)
+    ++actual_error_callback_calls_;
+  else
+    unexpected_error_callback_ = true;
 }
 
 void BluetoothTestBase::ConnectErrorCallback(
+    Call expected,
     enum BluetoothDevice::ConnectErrorCode error_code) {
   ++error_callback_count_;
   last_connect_error_code_ = error_code;
+
+  if (expected == Call::EXPECTED)
+    ++actual_error_callback_calls_;
+  else
+    unexpected_error_callback_ = true;
 }
 
 void BluetoothTestBase::GattErrorCallback(
+    Call expected,
     BluetoothGattService::GattErrorCode error_code) {
   ++error_callback_count_;
   last_gatt_error_code_ = error_code;
+
+  if (expected == Call::EXPECTED)
+    ++actual_error_callback_calls_;
+  else
+    unexpected_error_callback_ = true;
 }
 
-base::Closure BluetoothTestBase::GetCallback() {
-  return base::Bind(&BluetoothTestBase::Callback, weak_factory_.GetWeakPtr());
+base::Closure BluetoothTestBase::GetCallback(Call expected) {
+  if (expected == Call::EXPECTED)
+    ++expected_success_callback_calls_;
+  return base::Bind(&BluetoothTestBase::Callback, weak_factory_.GetWeakPtr(),
+                    expected);
 }
 
 BluetoothAdapter::DiscoverySessionCallback
-BluetoothTestBase::GetDiscoverySessionCallback() {
+BluetoothTestBase::GetDiscoverySessionCallback(Call expected) {
+  if (expected == Call::EXPECTED)
+    ++expected_success_callback_calls_;
   return base::Bind(&BluetoothTestBase::DiscoverySessionCallback,
-                    weak_factory_.GetWeakPtr());
+                    weak_factory_.GetWeakPtr(), expected);
 }
 
 BluetoothDevice::GattConnectionCallback
-BluetoothTestBase::GetGattConnectionCallback() {
+BluetoothTestBase::GetGattConnectionCallback(Call expected) {
+  if (expected == Call::EXPECTED)
+    ++expected_success_callback_calls_;
   return base::Bind(&BluetoothTestBase::GattConnectionCallback,
-                    weak_factory_.GetWeakPtr());
+                    weak_factory_.GetWeakPtr(), expected);
 }
 
 BluetoothGattCharacteristic::NotifySessionCallback
-BluetoothTestBase::GetNotifyCallback() {
+BluetoothTestBase::GetNotifyCallback(Call expected) {
+  if (expected == Call::EXPECTED)
+    ++expected_success_callback_calls_;
   return base::Bind(&BluetoothTestBase::NotifyCallback,
-                    weak_factory_.GetWeakPtr());
+                    weak_factory_.GetWeakPtr(), expected);
 }
 
 BluetoothGattCharacteristic::ValueCallback
-BluetoothTestBase::GetReadValueCallback() {
+BluetoothTestBase::GetReadValueCallback(Call expected) {
+  if (expected == Call::EXPECTED)
+    ++expected_success_callback_calls_;
   return base::Bind(&BluetoothTestBase::ReadValueCallback,
-                    weak_factory_.GetWeakPtr());
+                    weak_factory_.GetWeakPtr(), expected);
 }
 
-BluetoothAdapter::ErrorCallback BluetoothTestBase::GetErrorCallback() {
+BluetoothAdapter::ErrorCallback BluetoothTestBase::GetErrorCallback(
+    Call expected) {
+  if (expected == Call::EXPECTED)
+    ++expected_error_callback_calls_;
   return base::Bind(&BluetoothTestBase::ErrorCallback,
-                    weak_factory_.GetWeakPtr());
+                    weak_factory_.GetWeakPtr(), expected);
 }
 
 BluetoothDevice::ConnectErrorCallback
-BluetoothTestBase::GetConnectErrorCallback() {
+BluetoothTestBase::GetConnectErrorCallback(Call expected) {
+  if (expected == Call::EXPECTED)
+    ++expected_error_callback_calls_;
   return base::Bind(&BluetoothTestBase::ConnectErrorCallback,
-                    weak_factory_.GetWeakPtr());
+                    weak_factory_.GetWeakPtr(), expected);
 }
 
 base::Callback<void(BluetoothGattService::GattErrorCode)>
-BluetoothTestBase::GetGattErrorCallback() {
+BluetoothTestBase::GetGattErrorCallback(Call expected) {
+  if (expected == Call::EXPECTED)
+    ++expected_error_callback_calls_;
   return base::Bind(&BluetoothTestBase::GattErrorCallback,
-                    weak_factory_.GetWeakPtr());
+                    weak_factory_.GetWeakPtr(), expected);
 }
 
 void BluetoothTestBase::ResetEventCounts() {

device/bluetooth/test/bluetooth_test.h

@@ -29,6 +29,8 @@ class BluetoothDevice;
 // BluetoothTest.
 class BluetoothTestBase : public testing::Test {
  public:
+  enum class Call { EXPECTED, NOT_EXPECTED };
+
   static const std::string kTestAdapterName;
   static const std::string kTestAdapterAddress;
 
@@ -46,10 +48,20 @@ class BluetoothTestBase : public testing::Test {
   BluetoothTestBase();
   ~BluetoothTestBase() override;
 
+  // Checks that no unexpected calls have been made to callbacks.
+  // Overrides of this method should always call the parent's class method.
+  void TearDown() override;
+
   // Calls adapter_->StartDiscoverySessionWithFilter with Low Energy transport,
-  // and this fixture's callbacks. Then RunLoop().RunUntilIdle().
+  // and this fixture's callbacks expecting success.
+  // Then RunLoop().RunUntilIdle().
   void StartLowEnergyDiscoverySession();
 
+  // Calls adapter_->StartDiscoverySessionWithFilter with Low Energy transport,
+  // and this fixture's callbacks expecting error.
+  // Then RunLoop().RunUntilIdle().
+  void StartLowEnergyDiscoverySessionExpectedToFail();
+
   // Check if Low Energy is available. On Mac, we require OS X >= 10.10.
   virtual bool PlatformSupportsLowEnergy() = 0;
 
@@ -148,31 +160,38 @@ class BluetoothTestBase : public testing::Test {
   virtual void DeleteDevice(BluetoothDevice* device);
 
   // Callbacks that increment |callback_count_|, |error_callback_count_|:
-  void Callback();
-  void DiscoverySessionCallback(scoped_ptr<BluetoothDiscoverySession>);
-  void GattConnectionCallback(scoped_ptr<BluetoothGattConnection>);
-  void NotifyCallback(scoped_ptr<BluetoothGattNotifySession>);
-  void ReadValueCallback(const std::vector<uint8>& value);
-  void ErrorCallback();
-  void ConnectErrorCallback(enum BluetoothDevice::ConnectErrorCode);
-  void GattErrorCallback(BluetoothGattService::GattErrorCode);
+  void Callback(Call expected);
+  void DiscoverySessionCallback(Call expected,
+                                scoped_ptr<BluetoothDiscoverySession>);
+  void GattConnectionCallback(Call expected,
+                              scoped_ptr<BluetoothGattConnection>);
+  void NotifyCallback(Call expected, scoped_ptr<BluetoothGattNotifySession>);
+  void ReadValueCallback(Call expected, const std::vector<uint8>& value);
+  void ErrorCallback(Call expected);
+  void ConnectErrorCallback(Call expected,
+                            enum BluetoothDevice::ConnectErrorCode);
+  void GattErrorCallback(Call expected, BluetoothGattService::GattErrorCode);
 
   // Accessors to get callbacks bound to this fixture:
-  base::Closure GetCallback();
-  BluetoothAdapter::DiscoverySessionCallback GetDiscoverySessionCallback();
-  BluetoothDevice::GattConnectionCallback GetGattConnectionCallback();
-  BluetoothGattCharacteristic::NotifySessionCallback GetNotifyCallback();
-  BluetoothGattCharacteristic::ValueCallback GetReadValueCallback();
-  BluetoothAdapter::ErrorCallback GetErrorCallback();
-  BluetoothDevice::ConnectErrorCallback GetConnectErrorCallback();
+  base::Closure GetCallback(Call expected);
+  BluetoothAdapter::DiscoverySessionCallback GetDiscoverySessionCallback(
+      Call expected);
+  BluetoothDevice::GattConnectionCallback GetGattConnectionCallback(
+      Call expected);
+  BluetoothGattCharacteristic::NotifySessionCallback GetNotifyCallback(
+      Call expected);
+  BluetoothGattCharacteristic::ValueCallback GetReadValueCallback(
+      Call expected);
+  BluetoothAdapter::ErrorCallback GetErrorCallback(Call expected);
+  BluetoothDevice::ConnectErrorCallback GetConnectErrorCallback(Call expected);
   base::Callback<void(BluetoothGattService::GattErrorCode)>
-  GetGattErrorCallback();
+  GetGattErrorCallback(Call expected);
 
   // Reset all event count members to 0.
   void ResetEventCounts();
 
   // A Message loop is required by some implementations that will PostTasks and
-  // by base::RunLoop().RunUntilIdle() use in this fixuture.
+  // by base::RunLoop().RunUntilIdle() use in this fixture.
   base::MessageLoop message_loop_;
 
   scoped_refptr<BluetoothAdapter> adapter_;
@@ -184,6 +203,7 @@ class BluetoothTestBase : public testing::Test {
   std::vector<uint8> last_read_value_;
   std::vector<uint8> last_write_value_;
   BluetoothGattService::GattErrorCode last_gatt_error_code_;
+
   int callback_count_ = 0;
   int error_callback_count_ = 0;
   int gatt_connection_attempts_ = 0;
@@ -192,6 +212,16 @@ class BluetoothTestBase : public testing::Test {
   int gatt_notify_characteristic_attempts_ = 0;
   int gatt_read_characteristic_attempts_ = 0;
   int gatt_write_characteristic_attempts_ = 0;
+
+  // The following values are used to make sure the correct callbacks
+  // have been called. They are not reset when calling ResetEventCounts().
+  int expected_success_callback_calls_ = 0;
+  int expected_error_callback_calls_ = 0;
+  int actual_success_callback_calls_ = 0;
+  int actual_error_callback_calls_ = 0;
+  bool unexpected_success_callback_ = false;
+  bool unexpected_error_callback_ = false;
+
   base::WeakPtrFactory<BluetoothTestBase> weak_factory_;
 };