[CrOS Tether] Notify BleConnectionManager when GATT services missing.

When GATT services are unavailable,
BluetoothLowEnergyWeaveClientConnection bubbles an event up to
SecureChannel, which subsequently bubbles an event up to
BleConnectionManager.

This CL will be used in a follow-up CL to work around the GATT
services issue.

Bug: 784968, 672263
Change-Id: I63de1eb025fcef9bec32bd10f3f3956d53c14cca
Reviewed-on: https://chromium-review.googlesource.com/773586Reviewed-by: Jeremy Klein <jlklein@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517189}

chromeos/components/tether/ble_connection_manager.cc

@@ -188,6 +188,11 @@ void BleConnectionManager::ConnectionMetadata::OnMessageSent(
   manager_->SendMessageSentEvent(sequence_number);
 }
 
+void BleConnectionManager::ConnectionMetadata::
+    OnGattCharacteristicsNotAvailable() {
+  // TODO(khorimoto): Work around GATT bug. See crbug.com/784968.
+}
+
 BleConnectionManager::BleConnectionManager(
     cryptauth::CryptAuthService* cryptauth_service,
     scoped_refptr<device::BluetoothAdapter> adapter,

chromeos/components/tether/ble_connection_manager.h

@@ -176,6 +176,7 @@ class BleConnectionManager : public BleScanner::Observer {
                            const std::string& payload) override;
     void OnMessageSent(cryptauth::SecureChannel* secure_channel,
                        int sequence_number) override;
+    void OnGattCharacteristicsNotAvailable() override;
 
    private:
     friend class BleConnectionManagerTest;

components/cryptauth/ble/bluetooth_low_energy_weave_client_connection.cc

@@ -248,6 +248,18 @@ void BluetoothLowEnergyWeaveClientConnection::DestroyConnection(
     RecordBleWeaveConnectionResult(result);
   }
 
+  if (result ==
+          BleWeaveConnectionResult::
+              BLE_WEAVE_CONNECTION_RESULT_TIMEOUT_FINDING_GATT_CHARACTERISTICS ||
+      result ==
+          BleWeaveConnectionResult::
+              BLE_WEAVE_CONNECTION_RESULT_ERROR_FINDING_GATT_CHARACTERISTICS ||
+      result ==
+          BleWeaveConnectionResult::
+              BLE_WEAVE_CONNECTION_RESULT_ERROR_GATT_CHARACTERISTIC_NOT_AVAILABLE) {
+    NotifyGattCharacteristicsNotAvailable();
+  }
+
   if (adapter_) {
     adapter_->RemoveObserver(this);
     adapter_ = nullptr;

components/cryptauth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc

@@ -266,9 +266,33 @@ class MockConnectionObserver : public ConnectionObserver {
   MockConnectionObserver(Connection* connection)
       : connection_(connection),
         num_send_completed_(0),
+        num_gatt_services_unavailable_events_(0),
         delete_on_disconnect_(false),
         delete_on_message_sent_(false) {}
 
+  std::string GetLastDeserializedMessage() {
+    return last_deserialized_message_;
+  }
+
+  bool last_send_success() { return last_send_success_; }
+
+  int num_send_completed() { return num_send_completed_; }
+
+  int num_gatt_services_unavailable_events() {
+    return num_gatt_services_unavailable_events_;
+  }
+
+  bool delete_on_disconnect() { return delete_on_disconnect_; }
+
+  void set_delete_on_disconnect(bool delete_on_disconnect) {
+    delete_on_disconnect_ = delete_on_disconnect;
+  }
+
+  void set_delete_on_message_sent(bool delete_on_message_sent) {
+    delete_on_message_sent_ = delete_on_message_sent;
+  }
+
+  // ConnectionObserver:
   void OnConnectionStatusChanged(Connection* connection,
                                  Connection::Status old_status,
                                  Connection::Status new_status) override {
@@ -290,22 +314,8 @@ class MockConnectionObserver : public ConnectionObserver {
       delete connection_;
   }
 
-  std::string GetLastDeserializedMessage() {
-    return last_deserialized_message_;
-  }
-
-  bool GetLastSendSuccess() { return last_send_success_; }
-
-  int GetNumSendCompleted() { return num_send_completed_; }
-
-  bool delete_on_disconnect() { return delete_on_disconnect_; }
-
-  void set_delete_on_disconnect(bool delete_on_disconnect) {
-    delete_on_disconnect_ = delete_on_disconnect;
-  }
-
-  void set_delete_on_message_sent(bool delete_on_message_sent) {
-    delete_on_message_sent_ = delete_on_message_sent;
+  void OnGattCharacteristicsNotAvailable() override {
+    ++num_gatt_services_unavailable_events_;
   }
 
  private:
@@ -313,6 +323,7 @@ class MockConnectionObserver : public ConnectionObserver {
   std::string last_deserialized_message_;
   bool last_send_success_;
   int num_send_completed_;
+  int num_gatt_services_unavailable_events_;
   bool delete_on_disconnect_;
   bool delete_on_message_sent_;
 };
@@ -333,6 +344,9 @@ class CryptAuthBluetoothLowEnergyWeaveClientConnectionTest
     test_timer_ = nullptr;
     generator_ = nullptr;
     receiver_ = nullptr;
+    has_verified_connection_result_ = false;
+    has_verified_gatt_services_event_ = false;
+    connection_observer_.reset();
 
     adapter_ = base::MakeRefCounted<NiceMock<device::MockBluetoothAdapter>>();
     task_runner_ = base::MakeRefCounted<base::TestSimpleTaskRunner>();
@@ -367,13 +381,15 @@ class CryptAuthBluetoothLowEnergyWeaveClientConnectionTest
         .WillByDefault(Return(tx_characteristic_.get()));
 
     device::BluetoothAdapterFactory::SetAdapterForTesting(adapter_);
-
-    has_verified_connection_result_ = false;
   }
 
   void TearDown() override {
-    connection_observer_.reset();
     ASSERT_TRUE(has_verified_connection_result_);
+    if (connection_observer_ && !has_verified_gatt_services_event_) {
+      EXPECT_EQ(0,
+                connection_observer_->num_gatt_services_unavailable_events());
+    }
+    connection_observer_.reset();
   }
 
   // Creates a BluetoothLowEnergyWeaveClientConnection and verifies it's in
@@ -511,7 +527,7 @@ class CryptAuthBluetoothLowEnergyWeaveClientConnectionTest
     EXPECT_EQ(last_value_written_on_tx_characteristic_, kConnectionRequest);
 
     // OnDidSendMessage is not called.
-    EXPECT_EQ(0, connection_observer_->GetNumSendCompleted());
+    EXPECT_EQ(0, connection_observer_->num_send_completed());
 
     RunWriteCharacteristicSuccessCallback();
 
@@ -604,6 +620,11 @@ class CryptAuthBluetoothLowEnergyWeaveClientConnectionTest
     has_verified_connection_result_ = true;
   }
 
+  void VerifyGattServicesUnavailableEventSent() {
+    EXPECT_EQ(1, connection_observer_->num_gatt_services_unavailable_events());
+    has_verified_gatt_services_event_ = true;
+  }
+
   BluetoothLowEnergyWeaveClientConnection::GattServiceOperationResult
   GattServiceOperationResultSuccessOrFailure(bool success) {
     return success ? BluetoothLowEnergyWeaveClientConnection::
@@ -633,6 +654,7 @@ class CryptAuthBluetoothLowEnergyWeaveClientConnectionTest
   base::MessageLoop message_loop_;
   bool last_wire_message_success_;
   bool has_verified_connection_result_;
+  bool has_verified_gatt_services_event_;
   NiceMock<MockBluetoothLowEnergyWeavePacketGenerator>* generator_;
   NiceMock<MockBluetoothLowEnergyWeavePacketReceiver>* receiver_;
   std::unique_ptr<MockConnectionObserver> connection_observer_;
@@ -789,6 +811,7 @@ TEST_F(CryptAuthBluetoothLowEnergyWeaveClientConnectionTest,
   EXPECT_EQ(connection->sub_status(), SubStatus::DISCONNECTED);
   EXPECT_EQ(connection->status(), Connection::DISCONNECTED);
 
+  VerifyGattServicesUnavailableEventSent();
   VerifyBleWeaveConnectionResult(
       BluetoothLowEnergyWeaveClientConnection::BleWeaveConnectionResult::
           BLE_WEAVE_CONNECTION_RESULT_ERROR_FINDING_GATT_CHARACTERISTICS);
@@ -815,6 +838,7 @@ TEST_F(CryptAuthBluetoothLowEnergyWeaveClientConnectionTest,
   EXPECT_EQ(connection->sub_status(), SubStatus::DISCONNECTED);
   EXPECT_EQ(connection->status(), Connection::DISCONNECTED);
 
+  VerifyGattServicesUnavailableEventSent();
   VerifyBleWeaveConnectionResult(
       BluetoothLowEnergyWeaveClientConnection::BleWeaveConnectionResult::
           BLE_WEAVE_CONNECTION_RESULT_ERROR_GATT_CHARACTERISTIC_NOT_AVAILABLE);
@@ -856,7 +880,7 @@ TEST_F(CryptAuthBluetoothLowEnergyWeaveClientConnectionTest,
   // message |kMaxNumberOfTries| times. There is alredy one EXPECT_CALL for
   // WriteRemoteCharacteristic(_,_,_) in NotifySessionStated, that's why we use
   // |kMaxNumberOfTries-1| in the EXPECT_CALL statement.
-  EXPECT_EQ(0, connection_observer_->GetNumSendCompleted());
+  EXPECT_EQ(0, connection_observer_->num_send_completed());
   EXPECT_CALL(*tx_characteristic_, WriteRemoteCharacteristic(_, _, _))
       .Times(kMaxNumberOfTries - 1)
       .WillRepeatedly(
@@ -951,9 +975,9 @@ TEST_F(CryptAuthBluetoothLowEnergyWeaveClientConnectionTest,
   RunWriteCharacteristicSuccessCallback();
   VerifyGattWriteCharacteristicResult(true /* success */, 2 /* num_writes */);
 
-  EXPECT_EQ(1, connection_observer_->GetNumSendCompleted());
+  EXPECT_EQ(1, connection_observer_->num_send_completed());
   EXPECT_EQ(kSmallMessage, connection_observer_->GetLastDeserializedMessage());
-  EXPECT_TRUE(connection_observer_->GetLastSendSuccess());
+  EXPECT_TRUE(connection_observer_->last_send_success());
 
   connection.reset();
   VerifyBleWeaveConnectionResult(
@@ -1002,9 +1026,9 @@ TEST_F(CryptAuthBluetoothLowEnergyWeaveClientConnectionTest,
   RunWriteCharacteristicSuccessCallback();
   VerifyGattWriteCharacteristicResult(true /* success */, 3 /* num_writes */);
 
-  EXPECT_EQ(1, connection_observer_->GetNumSendCompleted());
+  EXPECT_EQ(1, connection_observer_->num_send_completed());
   EXPECT_EQ(kLargeMessage, connection_observer_->GetLastDeserializedMessage());
-  EXPECT_TRUE(connection_observer_->GetLastSendSuccess());
+  EXPECT_TRUE(connection_observer_->last_send_success());
 
   connection.reset();
   VerifyBleWeaveConnectionResult(
@@ -1038,12 +1062,12 @@ TEST_F(CryptAuthBluetoothLowEnergyWeaveClientConnectionTest,
     VerifyGattWriteCharacteristicResult(false /* success */,
                                         i + 1 /* num_writes */);
     if (i == kMaxNumberOfTries - 1) {
-      EXPECT_EQ(1, connection_observer_->GetNumSendCompleted());
+      EXPECT_EQ(1, connection_observer_->num_send_completed());
       EXPECT_EQ(kSmallMessage,
                 connection_observer_->GetLastDeserializedMessage());
-      EXPECT_FALSE(connection_observer_->GetLastSendSuccess());
+      EXPECT_FALSE(connection_observer_->last_send_success());
     } else {
-      EXPECT_EQ(0, connection_observer_->GetNumSendCompleted());
+      EXPECT_EQ(0, connection_observer_->num_send_completed());
     }
   }
 
@@ -1202,9 +1226,9 @@ TEST_F(CryptAuthBluetoothLowEnergyWeaveClientConnectionTest,
   RunWriteCharacteristicSuccessCallback();
   VerifyGattWriteCharacteristicResult(true /* success */, 2 /* num_writes */);
   task_runner_->RunUntilIdle();
-  EXPECT_EQ(1, connection_observer_->GetNumSendCompleted());
+  EXPECT_EQ(1, connection_observer_->num_send_completed());
   EXPECT_EQ(kSmallMessage, connection_observer_->GetLastDeserializedMessage());
-  EXPECT_TRUE(connection_observer_->GetLastSendSuccess());
+  EXPECT_TRUE(connection_observer_->last_send_success());
 
   // We cannot check if connection's status and sub_status are DISCONNECTED
   // because it has been deleted.
@@ -1439,6 +1463,7 @@ TEST_F(CryptAuthBluetoothLowEnergyWeaveClientConnectionTest,
   EXPECT_EQ(connection->sub_status(), SubStatus::DISCONNECTED);
   EXPECT_EQ(connection->status(), Connection::DISCONNECTED);
 
+  VerifyGattServicesUnavailableEventSent();
   VerifyBleWeaveConnectionResult(
       BluetoothLowEnergyWeaveClientConnection::BleWeaveConnectionResult::
           BLE_WEAVE_CONNECTION_RESULT_TIMEOUT_FINDING_GATT_CHARACTERISTICS);

components/cryptauth/connection.cc

@@ -108,6 +108,11 @@ std::unique_ptr<WireMessage> Connection::DeserializeWireMessage(
   return WireMessage::Deserialize(received_bytes_, is_incomplete_message);
 }
 
+void Connection::NotifyGattCharacteristicsNotAvailable() {
+  for (auto& observer : observers_)
+    observer.OnGattCharacteristicsNotAvailable();
+}
+
 std::string Connection::GetDeviceInfoLogString() {
   std::stringstream ss;
   ss << "{id: \"" << remote_device().GetTruncatedDeviceIdForLogs()

components/cryptauth/connection.h

@@ -92,6 +92,8 @@ class Connection {
   virtual std::unique_ptr<WireMessage> DeserializeWireMessage(
       bool* is_incomplete_message);
 
+  void NotifyGattCharacteristicsNotAvailable();
+
   // Returns a string describing the associated device for logging purposes.
   std::string GetDeviceInfoLogString();
 

components/cryptauth/connection_observer.h

@@ -32,6 +32,13 @@ class ConnectionObserver {
   virtual void OnSendCompleted(const Connection& connection,
                                const WireMessage& message,
                                bool success) {}
+
+  // Called when GATT characteristics are not available. This observer function
+  // is a temporary work-around (see crbug.com/784968).
+  // TODO(khorimoto): This observer function is specific to only one Connection
+  //     implementation, so it is hacky to include it as part of the observer
+  //     for Connection. Remove this work-around when it is no longer necessary.
+  virtual void OnGattCharacteristicsNotAvailable() {}
 };
 
 }  // namespace cryptauth

components/cryptauth/fake_connection.cc

@@ -78,6 +78,10 @@ void FakeConnection::ReceiveMessage(
   pending_payload_.clear();
 }
 
+void FakeConnection::NotifyGattCharacteristicsNotAvailable() {
+  Connection::NotifyGattCharacteristicsNotAvailable();
+}
+
 void FakeConnection::SendMessageImpl(std::unique_ptr<WireMessage> message) {
   CHECK(!current_message_);
   current_message_ = std::move(message);

components/cryptauth/fake_connection.h

@@ -38,6 +38,9 @@ class FakeConnection : public Connection {
   // container WireMessage format.
   void ReceiveMessage(const std::string& feature, const std::string& payload);
 
+  // Notifies observers that GATT characteristics are unavailable.
+  void NotifyGattCharacteristicsNotAvailable();
+
   // Returns the current message in progress of being sent.
   WireMessage* current_message() { return current_message_.get(); }
 

components/cryptauth/fake_secure_channel.cc

@@ -47,6 +47,10 @@ void FakeSecureChannel::CompleteSendingMessage(int sequence_number) {
   }
 }
 
+void FakeSecureChannel::NotifyGattCharacteristicsNotAvailable() {
+  SecureChannel::NotifyGattCharacteristicsNotAvailable();
+}
+
 void FakeSecureChannel::Initialize() {}
 
 int FakeSecureChannel::SendMessage(const std::string& feature,

components/cryptauth/fake_secure_channel.h

@@ -29,6 +29,7 @@ class FakeSecureChannel : public SecureChannel {
   void ChangeStatus(const Status& new_status);
   void ReceiveMessage(const std::string& feature, const std::string& payload);
   void CompleteSendingMessage(int sequence_number);
+  void NotifyGattCharacteristicsNotAvailable();
 
   std::vector<Observer*> observers() { return observers_; }
 

components/cryptauth/secure_channel.cc

@@ -212,6 +212,15 @@ void SecureChannel::OnSendCompleted(const cryptauth::Connection& connection,
   Disconnect();
 }
 
+void SecureChannel::OnGattCharacteristicsNotAvailable() {
+  NotifyGattCharacteristicsNotAvailable();
+}
+
+void SecureChannel::NotifyGattCharacteristicsNotAvailable() {
+  for (auto& observer : observer_list_)
+    observer.OnGattCharacteristicsNotAvailable();
+}
+
 void SecureChannel::TransitionToStatus(const Status& new_status) {
   if (new_status == status_) {
     // Only report changes to state.

components/cryptauth/secure_channel.h

@@ -63,6 +63,14 @@ class SecureChannel : public ConnectionObserver {
     // corresponds to the value returned by an earlier call to SendMessage().
     virtual void OnMessageSent(SecureChannel* secure_channel,
                                int sequence_number) {}
+
+    // Called when GATT characteristics are not available. This observer
+    // function is a temporary work-around (see crbug.com/784968).
+    // TODO(khorimoto): This observer function is specific to only one
+    //     SecureChannel implementation, so it is hacky to include it as part of
+    //     the observer for SecureChannel. Remove this work-around when it is no
+    //     longer necessary.
+    virtual void OnGattCharacteristicsNotAvailable() {}
   };
 
   class Factory {
@@ -110,11 +118,14 @@ class SecureChannel : public ConnectionObserver {
   void OnSendCompleted(const cryptauth::Connection& connection,
                        const cryptauth::WireMessage& wire_message,
                        bool success) override;
+  void OnGattCharacteristicsNotAvailable() override;
 
  protected:
   SecureChannel(std::unique_ptr<Connection> connection,
                 CryptAuthService* cryptauth_service);
 
+  void NotifyGattCharacteristicsNotAvailable();
+
   Status status_;
 
  private:

components/cryptauth/secure_channel_unittest.cc

@@ -47,6 +47,22 @@ class TestObserver final : public SecureChannel::Observer {
   TestObserver(SecureChannel* secure_channel)
       : secure_channel_(secure_channel) {}
 
+  const std::vector<SecureChannelStatusChange>& connection_status_changes() {
+    return connection_status_changes_;
+  }
+
+  const std::vector<ReceivedMessage>& received_messages() {
+    return received_messages_;
+  }
+
+  const std::vector<int>& sent_sequence_numbers() {
+    return sent_sequence_numbers_;
+  }
+
+  int num_gatt_services_unavailable_events() {
+    return num_gatt_services_unavailable_events_;
+  }
+
   // SecureChannel::Observer:
   void OnSecureChannelStatusChanged(
       SecureChannel* secure_channel,
@@ -70,16 +86,8 @@ class TestObserver final : public SecureChannel::Observer {
     sent_sequence_numbers_.push_back(sequence_number);
   }
 
-  const std::vector<SecureChannelStatusChange>& connection_status_changes() {
-    return connection_status_changes_;
-  }
-
-  const std::vector<ReceivedMessage>& received_messages() {
-    return received_messages_;
-  }
-
-  const std::vector<int>& sent_sequence_numbers() {
-    return sent_sequence_numbers_;
+  void OnGattCharacteristicsNotAvailable() override {
+    ++num_gatt_services_unavailable_events_;
   }
 
  private:
@@ -87,6 +95,7 @@ class TestObserver final : public SecureChannel::Observer {
   std::vector<SecureChannelStatusChange> connection_status_changes_;
   std::vector<ReceivedMessage> received_messages_;
   std::vector<int> sent_sequence_numbers_;
+  int num_gatt_services_unavailable_events_ = 0;
 };
 
 // Observer used in the ObserverDeletesChannel test. This Observer deletes the
@@ -154,6 +163,8 @@ class CryptAuthSecureChannelTest : public testing::Test {
         weak_ptr_factory_(this) {}
 
   void SetUp() override {
+    has_verified_gatt_services_event_ = false;
+
     test_authenticator_factory_ = base::MakeUnique<TestAuthenticatorFactory>();
     DeviceToDeviceAuthenticator::Factory::SetInstanceForTesting(
         test_authenticator_factory_.get());
@@ -186,6 +197,9 @@ class CryptAuthSecureChannelTest : public testing::Test {
     // Same with messages being sent.
     if (secure_channel_)
       VerifyNoMessageBeingSent();
+
+    if (!has_verified_gatt_services_event_)
+      EXPECT_EQ(0, test_observer_->num_gatt_services_unavailable_events());
   }
 
   void VerifyConnectionStateChanges(
@@ -331,6 +345,13 @@ class CryptAuthSecureChannelTest : public testing::Test {
     EXPECT_EQ(expected_payload, wire_message->payload());
   }
 
+  void VerifyGattServicesUnavailableEvent() {
+    EXPECT_EQ(1, test_observer_->num_gatt_services_unavailable_events());
+    has_verified_gatt_services_event_ = true;
+  }
+
+  bool has_verified_gatt_services_event_;
+
   // Owned by secure_channel_.
   FakeConnection* fake_connection_;
 
@@ -375,6 +396,21 @@ TEST_F(CryptAuthSecureChannelTest, ConnectionAttemptFails) {
   });
 }
 
+TEST_F(CryptAuthSecureChannelTest, GattServicesUnavailable) {
+  secure_channel_->Initialize();
+  VerifyConnectionStateChanges(std::vector<SecureChannelStatusChange>{
+      {SecureChannel::Status::DISCONNECTED,
+       SecureChannel::Status::CONNECTING}});
+
+  fake_connection_->NotifyGattCharacteristicsNotAvailable();
+  VerifyGattServicesUnavailableEvent();
+
+  fake_connection_->CompleteInProgressConnection(/* success */ false);
+  VerifyConnectionStateChanges(std::vector<SecureChannelStatusChange>{
+      {SecureChannel::Status::CONNECTING,
+       SecureChannel::Status::DISCONNECTED}});
+}
+
 TEST_F(CryptAuthSecureChannelTest, DisconnectBeforeAuthentication) {
   secure_channel_->Initialize();
   VerifyConnectionStateChanges(std::vector<SecureChannelStatusChange> {