Fast Pair: Retry metadata requests when eligible.

Retry device metadata requests on network status changes when
an initial request fails in a way that could indicate a network issue.

Change-Id: I87f5e781cebd60b9ac01a0159e3082fc077c6d76
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3411240
Reviewed-by: Shane Fitzpatrick <[email protected]>
Commit-Queue: Jon Mann <[email protected]>
Auto-Submit: Jon Mann <[email protected]>
Cr-Commit-Position: refs/heads/main@{#967427}

Author: Jon Mann

ash/quick_pair/repository/fake_fast_pair_repository.cc

@@ -43,6 +43,11 @@ bool FakeFastPairRepository::HasKeyForDevice(const std::string& mac_address) {
 void FakeFastPairRepository::GetDeviceMetadata(
     const std::string& hex_model_id,
     DeviceMetadataCallback callback) {
+  if (!is_network_connected_) {
+    std::move(callback).Run(/*device=*/nullptr, /*has_retryable_error=*/true);
+    return;
+  }
+
   std::string normalized_id = base::ToUpperASCII(hex_model_id);
   if (data_.contains(normalized_id)) {
     std::move(callback).Run(data_[normalized_id].get(),

ash/quick_pair/repository/fake_fast_pair_repository.h

@@ -45,6 +45,10 @@ class FakeFastPairRepository : public FastPairRepository {
 
   bool HasKeyForDevice(const std::string& mac_address);
 
+  void set_is_network_connected(bool is_connected) {
+    is_network_connected_ = is_connected;
+  }
+
   // FastPairRepository::
   void GetDeviceMetadata(const std::string& hex_model_id,
                          DeviceMetadataCallback callback) override;
@@ -64,6 +68,7 @@ class FakeFastPairRepository : public FastPairRepository {
  private:
   static void SetInstance(FastPairRepository* instance);
 
+  bool is_network_connected_ = true;
   base::flat_map<std::string, std::unique_ptr<DeviceMetadata>> data_;
   base::flat_map<std::string, std::vector<uint8_t>> saved_account_keys_;
   absl::optional<PairingMetadata> check_account_key_result_;

ash/quick_pair/scanning/BUILD.gn

@@ -29,6 +29,7 @@ source_set("scanning") {
     "//ash/services/quick_pair",
     "//ash/services/quick_pair/public/cpp",
     "//base",
+    "//chromeos/network",
     "//device/bluetooth",
     "//device/bluetooth/public/cpp",
   ]
@@ -76,6 +77,8 @@ source_set("unit_tests") {
     "//ash/services/quick_pair",
     "//ash/services/quick_pair:test_support",
     "//base/test:test_support",
+    "//chromeos/network:test_support",
+    "//dbus:test_support",
     "//device/bluetooth",
     "//device/bluetooth:mocks",
     "//mojo/public/cpp/bindings",

ash/quick_pair/scanning/fast_pair/fast_pair_discoverable_scanner.cc

@@ -25,6 +25,8 @@
 #include "base/containers/flat_set.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/strings/string_util.h"
+#include "chromeos/network/network_handler.h"
+#include "chromeos/network/network_state_handler.h"
 #include "device/bluetooth//bluetooth_adapter.h"
 #include "device/bluetooth/bluetooth_device.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
@@ -49,9 +51,14 @@ FastPairDiscoverableScanner::FastPairDiscoverableScanner(
       found_callback_(std::move(found_callback)),
       lost_callback_(std::move(lost_callback)) {
   observation_.Observe(scanner_.get());
+  chromeos::NetworkHandler::Get()->network_state_handler()->AddObserver(
+      this, FROM_HERE);
 }
 
-FastPairDiscoverableScanner::~FastPairDiscoverableScanner() = default;
+FastPairDiscoverableScanner::~FastPairDiscoverableScanner() {
+  chromeos::NetworkHandler::Get()->network_state_handler()->RemoveObserver(
+      this, FROM_HERE);
+}
 
 void FastPairDiscoverableScanner::OnDeviceFound(
     device::BluetoothDevice* device) {
@@ -116,18 +123,26 @@ void FastPairDiscoverableScanner::OnDeviceMetadataRetrieved(
     const std::string model_id,
     DeviceMetadata* device_metadata,
     bool has_retryable_error) {
+  if (has_retryable_error) {
+    pending_devices_address_to_model_id_[address] = model_id;
+    QP_LOG(WARNING) << __func__
+                    << ": Could not retrieve metadata for id: " << model_id
+                    << ". Waiting for network change before retry.";
+    return;
+  }
+
   if (!device_metadata) {
     QP_LOG(WARNING) << __func__
-                    << ": Could not get metadata for id: " << model_id
+                    << ": No metadata available for id: " << model_id
                     << ". Ignoring this advertisement";
     return;
   }
 
-  QP_LOG(VERBOSE) << __func__ << ": Id: " << model_id;
-
   auto device = base::MakeRefCounted<Device>(model_id, address,
                                              Protocol::kFastPairInitial);
 
+  QP_LOG(VERBOSE) << __func__ << ": Id: " << model_id;
+
   // Anti-spoofing keys were introduced in Fast Pair v2, so if this isn't
   // available then the device is v1.
   if (device_metadata->GetDetails()
@@ -182,6 +197,9 @@ void FastPairDiscoverableScanner::OnDeviceLost(
     device::BluetoothDevice* device) {
   QP_LOG(VERBOSE) << __func__ << ": " << device->GetNameForDisplay();
 
+  // No need to retry fetching metadata for devices that are no longer in range.
+  pending_devices_address_to_model_id_.erase(device->GetAddress());
+
   // If we have an in-progress attempt to parse for this device, removing it
   // from this map will ensure the result is ignored.
   model_id_parse_attempts_.erase(device->GetAddress());
@@ -231,5 +249,25 @@ void FastPairDiscoverableScanner::OnUtilityProcessStopped(
                      weak_pointer_factory_.GetWeakPtr(), address));
 }
 
+void FastPairDiscoverableScanner::DefaultNetworkChanged(
+    const chromeos::NetworkState* network) {
+  // Only retry when we have an active connected network.
+  if (!network || !network->IsConnectedState()) {
+    return;
+  }
+
+  auto it = pending_devices_address_to_model_id_.begin();
+  while (it != pending_devices_address_to_model_id_.end()) {
+    FastPairRepository::Get()->GetDeviceMetadata(
+        /*model_id=*/it->second,
+        base::BindOnce(&FastPairDiscoverableScanner::OnDeviceMetadataRetrieved,
+                       weak_pointer_factory_.GetWeakPtr(),
+                       /*address=*/it->first,
+                       /*model_id=*/it->second));
+
+    pending_devices_address_to_model_id_.erase(it);
+  }
+}
+
 }  // namespace quick_pair
 }  // namespace ash

ash/quick_pair/scanning/fast_pair/fast_pair_discoverable_scanner.h

@@ -14,6 +14,7 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/scoped_observation.h"
+#include "chromeos/network/network_state_handler_observer.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
 
 namespace device {
@@ -35,7 +36,9 @@ using DeviceCallback = base::RepeatingCallback<void(scoped_refptr<Device>)>;
 // and invokes the |found_callback| when it finds a device within the
 // appropriate range.  |lost_callback| will be invoked when that device is lost
 // to the bluetooth adapter.
-class FastPairDiscoverableScanner final : public FastPairScanner::Observer {
+class FastPairDiscoverableScanner final
+    : public FastPairScanner::Observer,
+      public chromeos::NetworkStateHandlerObserver {
  public:
   FastPairDiscoverableScanner(scoped_refptr<FastPairScanner> scanner,
                               scoped_refptr<device::BluetoothAdapter> adapter,
@@ -50,6 +53,9 @@ class FastPairDiscoverableScanner final : public FastPairScanner::Observer {
   void OnDeviceFound(device::BluetoothDevice* device) override;
   void OnDeviceLost(device::BluetoothDevice* device) override;
 
+  // chromeos::NetworkStateHandlerObserver:
+  void DefaultNetworkChanged(const chromeos::NetworkState* network) override;
+
  private:
   void OnModelIdRetrieved(const std::string& address,
                           const absl::optional<std::string>& model_id);
@@ -68,6 +74,7 @@ class FastPairDiscoverableScanner final : public FastPairScanner::Observer {
   scoped_refptr<device::BluetoothAdapter> adapter_;
   DeviceCallback found_callback_;
   DeviceCallback lost_callback_;
+  base::flat_map<std::string, std::string> pending_devices_address_to_model_id_;
   base::flat_map<std::string, scoped_refptr<Device>> notified_devices_;
   base::flat_map<std::string, int> model_id_parse_attempts_;
   base::ScopedObservation<FastPairScanner, FastPairScanner::Observer>

ash/quick_pair/scanning/fast_pair/fast_pair_discoverable_scanner_unittest.cc

@@ -27,6 +27,9 @@
 #include "base/test/bind.h"
 #include "base/test/mock_callback.h"
 #include "base/test/task_environment.h"
+#include "chromeos/network/network_handler.h"
+#include "chromeos/network/network_state_handler.h"
+#include "chromeos/network/network_state_test_helper.h"
 #include "device/bluetooth/bluetooth_adapter.h"
 #include "device/bluetooth/bluetooth_device.h"
 #include "device/bluetooth/test/mock_bluetooth_adapter.h"
@@ -45,6 +48,7 @@ namespace quick_pair {
 class FastPairDiscoverableScannerTest : public testing::Test {
  public:
   void SetUp() override {
+    chromeos::NetworkHandler::Initialize();
     repository_ = std::make_unique<FakeFastPairRepository>();
 
     nearby::fastpair::Device metadata;
@@ -81,6 +85,12 @@ class FastPairDiscoverableScannerTest : public testing::Test {
         lost_device_callback_.Get());
   }
 
+  void TearDown() override {
+    testing::Test::TearDown();
+    discoverable_scanner_.reset();
+    chromeos::NetworkHandler::Shutdown();
+  }
+
   MockQuickPairProcessManager* mock_process_manager() {
     return static_cast<MockQuickPairProcessManager*>(process_manager_.get());
   }
@@ -119,6 +129,7 @@ class FastPairDiscoverableScannerTest : public testing::Test {
   }
 
   base::test::SingleThreadTaskEnvironment task_enviornment_;
+  chromeos::NetworkStateTestHelper helper_{/*use_defaults=*/true};
   scoped_refptr<FakeFastPairScanner> scanner_;
   std::unique_ptr<FakeFastPairRepository> repository_;
   std::unique_ptr<FastPairDiscoverableScanner> discoverable_scanner_;
@@ -179,6 +190,38 @@ TEST_F(FastPairDiscoverableScannerTest, InvokesLostCallbackAfterFound_v1) {
   base::RunLoop().RunUntilIdle();
 }
 
+TEST_F(FastPairDiscoverableScannerTest,
+       InvokesFoundCallback_AfterNetworkAvailable) {
+  device::BluetoothDevice* device = GetDevice(kValidModelId);
+  repository_->set_is_network_connected(false);
+
+  EXPECT_CALL(found_device_callback_, Run).Times(0);
+  scanner_->NotifyDeviceFound(device);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_CALL(found_device_callback_, Run).Times(1);
+  repository_->set_is_network_connected(true);
+  discoverable_scanner_->DefaultNetworkChanged(
+      helper_.network_state_handler()->DefaultNetwork());
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(FastPairDiscoverableScannerTest,
+       NoFoundCallback_AfterDeviceLostAndNetworkAvailable) {
+  device::BluetoothDevice* device = GetDevice(kValidModelId);
+  repository_->set_is_network_connected(false);
+
+  EXPECT_CALL(found_device_callback_, Run).Times(0);
+  scanner_->NotifyDeviceFound(device);
+  base::RunLoop().RunUntilIdle();
+
+  scanner_->NotifyDeviceLost(device);
+  repository_->set_is_network_connected(true);
+  discoverable_scanner_->DefaultNetworkChanged(
+      helper_.network_state_handler()->DefaultNetwork());
+  base::RunLoop().RunUntilIdle();
+}
+
 TEST_F(FastPairDiscoverableScannerTest, InvokesLostCallbackAfterFound_v2) {
   nearby::fastpair::Device metadata;
   metadata.set_trigger_distance(2);