bluetooth: Invalidate connection objects if a connection fails and add histograms

DidFailToConnectGatt is sometimes called when there are valid Connection
objects so we need to invalidate them.

Also adds histograms to understand what types of error can we expect in android.

BUG=577741

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

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

4 files changed, 43 insertions, 76 deletions

diff --git a/device/bluetooth/bluetooth_device.cc b/device/bluetooth/bluetooth_device.cc
index 05532cc..c85af11 100644
--- a/device/bluetooth/bluetooth_device.cc
+++ b/device/bluetooth/bluetooth_device.cc
@@ -303,6 +303,10 @@ void BluetoothDevice::DidConnectGatt() {
 }
 
 void BluetoothDevice::DidFailToConnectGatt(ConnectErrorCode error) {
+  // Connection request should only be made if there are no active
+  // connections.
+  DCHECK(gatt_connections_.empty());
+
   for (const auto& error_callback : create_gatt_connection_error_callbacks_)
     error_callback.Run(error);
   create_gatt_connection_success_callbacks_.clear();
@@ -311,13 +315,8 @@ void BluetoothDevice::DidFailToConnectGatt(ConnectErrorCode error) {
 
 void BluetoothDevice::DidDisconnectGatt() {
   // Pending calls to connect GATT are not expected, if they were then
-  // DidFailToConnectGatt should be called. But in case callbacks exist
-  // flush them to ensure a consistent state.
-  if (create_gatt_connection_error_callbacks_.size() > 0) {
-    VLOG(1) << "Unexpected / unexplained DidDisconnectGatt call while "
-               "create_gatt_connection_error_callbacks_ are pending.";
-  }
-  DidFailToConnectGatt(ERROR_FAILED);
+  // DidFailToConnectGatt should have been called.
+  DCHECK(create_gatt_connection_error_callbacks_.empty());
 
   // Invalidate all BluetoothGattConnection objects.
   for (BluetoothGattConnection* connection : gatt_connections_) {
diff --git a/device/bluetooth/bluetooth_device_android.cc b/device/bluetooth/bluetooth_device_android.cc
index 71ceb66..253680b 100644
--- a/device/bluetooth/bluetooth_device_android.cc
+++ b/device/bluetooth/bluetooth_device_android.cc
@@ -8,6 +8,7 @@
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
+#include "base/metrics/sparse_histogram.h"
 #include "base/strings/stringprintf.h"
 #include "device/bluetooth/bluetooth_adapter_android.h"
 #include "device/bluetooth/bluetooth_remote_gatt_service_android.h"
@@ -17,6 +18,20 @@ using base::android::AttachCurrentThread;
 using base::android::AppendJavaStringArrayToStringVector;
 
 namespace device {
+namespace {
+void RecordConnectionSuccessResult(int32_t status) {
+  UMA_HISTOGRAM_SPARSE_SLOWLY("Bluetooth.Android.GATTConnection.Success.Result",
+                              status);
+}
+void RecordConnectionFailureResult(int32_t status) {
+  UMA_HISTOGRAM_SPARSE_SLOWLY("Bluetooth.Android.GATTConnection.Failure.Result",
+                              status);
+}
+void RecordConnectionTerminatedResult(int32_t status) {
+  UMA_HISTOGRAM_SPARSE_SLOWLY(
+      "Bluetooth.Android.GATTConnection.Disconnected.Result", status);
+}
+}  // namespace
 
 BluetoothDeviceAndroid* BluetoothDeviceAndroid::Create(
     BluetoothAdapterAndroid* adapter,
@@ -208,36 +223,21 @@ void BluetoothDeviceAndroid::OnConnectionStateChange(
     bool connected) {
   gatt_connected_ = connected;
   if (gatt_connected_) {
+    RecordConnectionSuccessResult(status);
     DidConnectGatt();
+  } else if (!create_gatt_connection_error_callbacks_.empty()) {
+    // We assume that if there are any pending connection callbacks there
+    // was a failed connection attempt.
+    RecordConnectionFailureResult(status);
+    // TODO(ortuno): Return an error code based on |status|
+    // http://crbug.com/578191
+    DidFailToConnectGatt(ERROR_FAILED);
   } else {
+    // Otherwise an existing connection was terminated.
+    RecordConnectionTerminatedResult(status);
     gatt_services_.clear();
     SetGattServicesDiscoveryComplete(false);
-
-    switch (status) {   // Constants are from android.bluetooth.BluetoothGatt.
-      case 0x0000008f:  // GATT_CONNECTION_CONGESTED
-        return DidFailToConnectGatt(ERROR_CONNECTION_CONGESTED);
-      case 0x00000101:  // GATT_FAILURE
-        return DidFailToConnectGatt(ERROR_FAILED);
-      case 0x00000005:  // GATT_INSUFFICIENT_AUTHENTICATION
-        return DidFailToConnectGatt(ERROR_AUTH_FAILED);
-      case 0x0000000f:  // GATT_INSUFFICIENT_ENCRYPTION
-        return DidFailToConnectGatt(ERROR_INSUFFICIENT_ENCRYPTION);
-      case 0x0000000d:  // GATT_INVALID_ATTRIBUTE_LENGTH
-        return DidFailToConnectGatt(ERROR_ATTRIBUTE_LENGTH_INVALID);
-      case 0x00000007:  // GATT_INVALID_OFFSET
-        return DidFailToConnectGatt(ERROR_OFFSET_INVALID);
-      case 0x00000002:  // GATT_READ_NOT_PERMITTED
-        return DidFailToConnectGatt(ERROR_READ_NOT_PERMITTED);
-      case 0x00000006:  // GATT_REQUEST_NOT_SUPPORTED
-        return DidFailToConnectGatt(ERROR_REQUEST_NOT_SUPPORTED);
-      case 0x00000000:  // GATT_SUCCESS
-        return DidDisconnectGatt();
-      case 0x00000003:  // GATT_WRITE_NOT_PERMITTED
-        return DidFailToConnectGatt(ERROR_WRITE_NOT_PERMITTED);
-      default:
-        VLOG(1) << "Unhandled status: " << status;
-        return DidFailToConnectGatt(ERROR_UNKNOWN);
-    }
+    DidDisconnectGatt();
   }
 }
 
diff --git a/device/bluetooth/bluetooth_device_unittest.cc b/device/bluetooth/bluetooth_device_unittest.cc
index 69410b5..4fc30e4 100644
--- a/device/bluetooth/bluetooth_device_unittest.cc
+++ b/device/bluetooth/bluetooth_device_unittest.cc
@@ -457,7 +457,11 @@ TEST_F(BluetoothTest, BluetoothGattConnection_ErrorAfterConnection) {
   EXPECT_EQ(1, gatt_connection_attempts_);
   SimulateGattConnectionError(device, BluetoothDevice::ERROR_AUTH_FAILED);
   SimulateGattConnectionError(device, BluetoothDevice::ERROR_FAILED);
-  EXPECT_EQ(BluetoothDevice::ERROR_AUTH_FAILED, last_connect_error_code_);
+  // TODO: Change to ERROR_AUTH_FAILED. We should be getting a callback
+  // only with the first error, but our android framework doesn't yet
+  // support sending different errors.
+  // http://crbug.com/578191
+  EXPECT_EQ(BluetoothDevice::ERROR_FAILED, last_connect_error_code_);
   for (BluetoothGattConnection* connection : gatt_connections_)
     EXPECT_FALSE(connection->IsConnected());
 }
diff --git a/device/bluetooth/test/bluetooth_test_android.cc b/device/bluetooth/test/bluetooth_test_android.cc
index 353822d..e23a835 100644
--- a/device/bluetooth/test/bluetooth_test_android.cc
+++ b/device/bluetooth/test/bluetooth_test_android.cc
@@ -84,52 +84,16 @@ void BluetoothTestAndroid::SimulateGattConnection(BluetoothDevice* device) {
 
 void BluetoothTestAndroid::SimulateGattConnectionError(
     BluetoothDevice* device,
-    BluetoothDevice::ConnectErrorCode error) {
-  int android_error_value = 0;
-  switch (error) {  // Constants are from android.bluetooth.BluetoothGatt.
-    case BluetoothDevice::ERROR_ATTRIBUTE_LENGTH_INVALID:
-      android_error_value = 0x0000000d;  // GATT_INVALID_ATTRIBUTE_LENGTH
-      break;
-    case BluetoothDevice::ERROR_AUTH_FAILED:
-      android_error_value = 0x00000005;  // GATT_INSUFFICIENT_AUTHENTICATION
-      break;
-    case BluetoothDevice::ERROR_CONNECTION_CONGESTED:
-      android_error_value = 0x0000008f;  // GATT_CONNECTION_CONGESTED
-      break;
-    case BluetoothDevice::ERROR_FAILED:
-      android_error_value = 0x00000101;  // GATT_FAILURE
-      break;
-    case BluetoothDevice::ERROR_INSUFFICIENT_ENCRYPTION:
-      android_error_value = 0x0000000f;  // GATT_INSUFFICIENT_ENCRYPTION
-      break;
-    case BluetoothDevice::ERROR_OFFSET_INVALID:
-      android_error_value = 0x00000007;  // GATT_INVALID_OFFSET
-      break;
-    case BluetoothDevice::ERROR_READ_NOT_PERMITTED:
-      android_error_value = 0x00000002;  // GATT_READ_NOT_PERMITTED
-      break;
-    case BluetoothDevice::ERROR_REQUEST_NOT_SUPPORTED:
-      android_error_value = 0x00000006;  // GATT_REQUEST_NOT_SUPPORTED
-      break;
-    case BluetoothDevice::ERROR_WRITE_NOT_PERMITTED:
-      android_error_value = 0x00000003;  // GATT_WRITE_NOT_PERMITTED
-      break;
-    case BluetoothDevice::ERROR_AUTH_CANCELED:
-    case BluetoothDevice::ERROR_AUTH_REJECTED:
-    case BluetoothDevice::ERROR_AUTH_TIMEOUT:
-    case BluetoothDevice::ERROR_INPROGRESS:
-    case BluetoothDevice::ERROR_UNKNOWN:
-    case BluetoothDevice::ERROR_UNSUPPORTED_DEVICE:
-    case BluetoothDevice::NUM_CONNECT_ERROR_CODES:
-      NOTREACHED() << "No translation for error code: " << error;
-  }
-
+    BluetoothDevice::ConnectErrorCode) {
   BluetoothDeviceAndroid* device_android =
       static_cast<BluetoothDeviceAndroid*>(device);
 
   Java_FakeBluetoothDevice_connectionStateChange(
       AttachCurrentThread(), device_android->GetJavaObject().obj(),
-      android_error_value,
+      // TODO(ortuno): Add all types of errors Android can produce. For now we
+      // just return a timeout error.
+      // http://crbug.com/578191
+      0x08,    // Connection Timeout from Bluetooth Spec.
       false);  // connected
 }
 
@@ -139,7 +103,7 @@ void BluetoothTestAndroid::SimulateGattDisconnection(BluetoothDevice* device) {
 
   Java_FakeBluetoothDevice_connectionStateChange(
       AttachCurrentThread(), device_android->GetJavaObject().obj(),
-      0,       // android.bluetooth.BluetoothGatt.GATT_SUCCESS
+      0x13,    // Connection terminate by peer user from Bluetooth Spec.
       false);  // disconnected
 }