Reland "bluetooth/winrt: Observe discovery failures"

This reverts commit a361d30d.

Reason for revert: Fixed test failure from crbug.com/1097635

Original change's description:
> Revert "bluetooth/winrt: Observe discovery failures"
>
> This reverts commit e44abb7d.
>
> Reason for revert: Fails on Win DBG builder.
>
> Original change's description:
> > bluetooth/winrt: Observe discovery failures
> >
> > Change BluetoothAdapterWinrt to subscribe to
> > BluetoothLEAdvertisementWatcher::Stopped events. The event fires if
> > scanning for LE advertisements has been cancelled by the application or
> > due to an error. Once the event is received, mark all active
> > BluetoothDiscoverySession instances inactive, which will cause
> > BluetoothAdapter::IsDiscovering() to return false.
> >
> > Also make BluetoothAdapterWinrt call Observer::DiscoveringChanged().
> >
> > Change-Id: If4abf65f09b4d20b89b53adb63dad89939ddcceb
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2255549
> > Commit-Queue: Martin Kreichgauer <martinkr@google.com>
> > Commit-Queue: Martin Kreichgauer <martinkr@chromium.org>
> > Reviewed-by: Reilly Grant <reillyg@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#780539}
>
> TBR=reillyg@chromium.org,martinkr@chromium.org
> BUG: 1097635
> # Not skipping CQ checks because original CL landed > 1 day ago.
>
> Change-Id: I5b2e012bee60c94d735f95aa49cf90db2adff016
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2255946
> Reviewed-by: Fergal Daly <fergal@chromium.org>
> Commit-Queue: Fergal Daly <fergal@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#780660}

TBR=reillyg@chromium.org,martinkr@google.com,fergal@chromium.org,martinkr@chromium.org

# Not skipping CQ checks because this is a reland.

Change-Id: Ia924fe22558e323d4b56813ec8018de228e5a36d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2257564Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Cr-Commit-Position: refs/heads/master@{#781940}

device/bluetooth/bluetooth_adapter_unittest.cc

@@ -132,12 +132,6 @@ class TestBluetoothAdapter final : public BluetoothAdapter {
 
   void TestErrorCallback() {}
 
-  void TestOnStartDiscoverySession(
-      std::unique_ptr<device::BluetoothDiscoverySession> discovery_session) {
-    ++callback_count_;
-    discovery_sessions_holder_.push(std::move(discovery_session));
-  }
-
   void OnStartDiscoverySessionQuitLoop(
       base::Closure run_loop_quit,
       std::unique_ptr<device::BluetoothDiscoverySession> discovery_session) {
@@ -724,7 +718,7 @@ TEST_F(BluetoothTest, MAYBE_ConstructDefaultAdapter) {
   EXPECT_EQ(adapter_->IsPowered(), adapter_->IsPowered());
   EXPECT_FALSE(adapter_->IsDiscoverable());
   EXPECT_FALSE(adapter_->IsDiscovering());
-}
+}  // namespace device
 
 // TODO(scheib): Enable BluetoothTest fixture tests on all platforms.
 #if defined(OS_ANDROID) || defined(OS_MACOSX)
@@ -1421,6 +1415,34 @@ TEST_F(BluetoothTest, MAYBE_TogglePowerBeforeScan) {
   EXPECT_TRUE(discovery_sessions_[0]->IsActive());
 }
 
+#if defined(OS_WIN)
+TEST_P(BluetoothTestWinrtOnly, DiscoverySessionFailure) {
+  if (!PlatformSupportsLowEnergy()) {
+    LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
+    return;
+  }
+
+  InitWithFakeAdapter();
+  TestBluetoothAdapterObserver observer(adapter_);
+  EXPECT_FALSE(adapter_->IsDiscovering());
+
+  StartLowEnergyDiscoverySession();
+  EXPECT_EQ(1, callback_count_);
+  EXPECT_EQ(0, error_callback_count_);
+  EXPECT_TRUE(adapter_->IsDiscovering());
+  EXPECT_EQ(1, observer.discovering_changed_count());
+  EXPECT_TRUE(observer.last_discovering());
+  ASSERT_EQ((size_t)1, discovery_sessions_.size());
+  EXPECT_TRUE(discovery_sessions_[0]->IsActive());
+
+  SimulateLowEnergyDiscoveryFailure();
+  EXPECT_FALSE(adapter_->IsDiscovering());
+  EXPECT_FALSE(discovery_sessions_[0]->IsActive());
+  EXPECT_EQ(2, observer.discovering_changed_count());
+  EXPECT_FALSE(observer.last_discovering());
+}
+#endif  // defined(OS_WIN)
+
 #if defined(OS_MACOSX)
 #define MAYBE_TurnOffAdapterWithConnectedDevice \
   TurnOffAdapterWithConnectedDevice

device/bluetooth/bluetooth_adapter_winrt.cc

@@ -51,6 +51,7 @@ namespace {
 namespace uwp {
 using ABI::Windows::Devices::Bluetooth::BluetoothAdapter;
 }  // namespace uwp
+using ABI::Windows::Devices::Bluetooth::BluetoothError;
 using ABI::Windows::Devices::Bluetooth::IBluetoothAdapter;
 using ABI::Windows::Devices::Bluetooth::IBluetoothAdapterStatics;
 using ABI::Windows::Devices::Bluetooth::IID_IBluetoothAdapterStatics;
@@ -64,15 +65,12 @@ using ABI::Windows::Devices::Bluetooth::Advertisement::
     BluetoothLEAdvertisementWatcherStatus_Aborted;
 using ABI::Windows::Devices::Bluetooth::Advertisement::
     BluetoothLEManufacturerData;
-using ABI::Windows::Devices::Bluetooth::Advertisement::BluetoothLEScanningMode;
 using ABI::Windows::Devices::Bluetooth::Advertisement::
     BluetoothLEScanningMode_Active;
 using ABI::Windows::Devices::Bluetooth::Advertisement::
     IBluetoothLEAdvertisement;
 using ABI::Windows::Devices::Bluetooth::Advertisement::
     IBluetoothLEAdvertisementDataSection;
-using ABI::Windows::Devices::Bluetooth::Advertisement::
-    IBluetoothLEAdvertisementPublisherFactory;
 using ABI::Windows::Devices::Bluetooth::Advertisement::
     IBluetoothLEAdvertisementReceivedEventArgs;
 using ABI::Windows::Devices::Bluetooth::Advertisement::
@@ -931,12 +929,12 @@ void BluetoothAdapterWinrt::StartScanWithFilter(
     return;
   }
 
-  auto advertisement_received_token = AddTypedEventHandler(
+  advertisement_received_token_ = AddTypedEventHandler(
       ble_advertisement_watcher_.Get(),
       &IBluetoothLEAdvertisementWatcher::add_Received,
       base::BindRepeating(&BluetoothAdapterWinrt::OnAdvertisementReceived,
                           weak_ptr_factory_.GetWeakPtr()));
-  if (!advertisement_received_token) {
+  if (!advertisement_received_token_) {
     ui_task_runner_->PostTask(
         FROM_HERE,
         base::BindOnce(std::move(callback), /*is_error=*/true,
@@ -944,13 +942,25 @@ void BluetoothAdapterWinrt::StartScanWithFilter(
     return;
   }
 
-  advertisement_received_token_ = *advertisement_received_token;
+  advertisement_watcher_stopped_token_ = AddTypedEventHandler(
+      ble_advertisement_watcher_.Get(),
+      &IBluetoothLEAdvertisementWatcher::add_Stopped,
+      base::BindRepeating(&BluetoothAdapterWinrt::OnAdvertisementWatcherStopped,
+                          weak_ptr_factory_.GetWeakPtr()));
+  if (!advertisement_watcher_stopped_token_) {
+    RemoveAdvertisementWatcherEventHandlers();
+    ui_task_runner_->PostTask(
+        FROM_HERE,
+        base::BindOnce(std::move(callback), /*is_error=*/true,
+                       UMABluetoothDiscoverySessionOutcome::UNKNOWN));
+    return;
+  }
 
   hr = ble_advertisement_watcher_->Start();
   if (FAILED(hr)) {
     BLUETOOTH_LOG(ERROR) << "Starting the Advertisement Watcher failed: "
                          << logging::SystemErrorCodeToString(hr);
-    RemoveAdvertisementReceivedHandler();
+    RemoveAdvertisementWatcherEventHandlers();
     ui_task_runner_->PostTask(
         FROM_HERE,
         base::BindOnce(std::move(callback), /*is_error=*/true,
@@ -967,7 +977,7 @@ void BluetoothAdapterWinrt::StartScanWithFilter(
     BLUETOOTH_LOG(ERROR)
         << "Starting Advertisement Watcher failed, it is in the Aborted "
            "state.";
-    RemoveAdvertisementReceivedHandler();
+    RemoveAdvertisementWatcherEventHandlers();
     ui_task_runner_->PostTask(
         FROM_HERE,
         base::BindOnce(std::move(callback), /*is_error=*/true,
@@ -975,6 +985,10 @@ void BluetoothAdapterWinrt::StartScanWithFilter(
     return;
   }
 
+  for (auto& observer : observers_) {
+    observer.AdapterDiscoveringChanged(this, /*discovering=*/true);
+  }
+
   ui_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(std::move(callback), false,
                                 UMABluetoothDiscoverySessionOutcome::SUCCESS));
@@ -983,7 +997,7 @@ void BluetoothAdapterWinrt::StartScanWithFilter(
 void BluetoothAdapterWinrt::StopScan(DiscoverySessionResultCallback callback) {
   DCHECK_EQ(NumDiscoverySessions(), 0);
 
-  RemoveAdvertisementReceivedHandler();
+  RemoveAdvertisementWatcherEventHandlers();
   HRESULT hr = ble_advertisement_watcher_->Stop();
   if (FAILED(hr)) {
     BLUETOOTH_LOG(ERROR) << "Stopped the Advertisement Watcher failed: "
@@ -995,8 +1009,14 @@ void BluetoothAdapterWinrt::StopScan(DiscoverySessionResultCallback callback) {
     return;
   }
 
-  for (auto& device : devices_)
+  for (auto& device : devices_) {
     device.second->ClearAdvertisementData();
+  }
+
+  for (auto& observer : observers_) {
+    observer.AdapterDiscoveringChanged(this, /*discovering=*/false);
+  }
+
   ble_advertisement_watcher_.Reset();
   ui_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(std::move(callback), /*is_error=*/false,
@@ -1345,6 +1365,22 @@ void BluetoothAdapterWinrt::OnAdvertisementReceived(
   }
 }
 
+void BluetoothAdapterWinrt::OnAdvertisementWatcherStopped(
+    ABI::Windows::Devices::Bluetooth::Advertisement::
+        IBluetoothLEAdvertisementWatcher* watcher,
+    ABI::Windows::Devices::Bluetooth::Advertisement::
+        IBluetoothLEAdvertisementWatcherStoppedEventArgs* args) {
+  BluetoothError error;
+  HRESULT hr = args->get_Error(&error);
+  if (FAILED(hr)) {
+    BLUETOOTH_LOG(ERROR) << "get_Error() failed: " << hr;
+    return;
+  }
+  BLUETOOTH_LOG(DEBUG) << "OnAdvertisementWatcherStopped() error=" << error;
+
+  MarkDiscoverySessionsAsInactive();
+}
+
 void BluetoothAdapterWinrt::OnRegisterAdvertisement(
     BluetoothAdvertisement* advertisement,
     const CreateAdvertisementCallback& callback) {
@@ -1417,13 +1453,23 @@ void BluetoothAdapterWinrt::TryRemovePoweredRadioEventHandlers() {
   }
 }
 
-void BluetoothAdapterWinrt::RemoveAdvertisementReceivedHandler() {
+void BluetoothAdapterWinrt::RemoveAdvertisementWatcherEventHandlers() {
   DCHECK(ble_advertisement_watcher_);
-  HRESULT hr = ble_advertisement_watcher_->remove_Received(
-      advertisement_received_token_);
-  if (FAILED(hr)) {
-    BLUETOOTH_LOG(ERROR) << "Removing the Received Handler failed: "
-                         << logging::SystemErrorCodeToString(hr);
+  if (advertisement_received_token_) {
+    HRESULT hr = ble_advertisement_watcher_->remove_Received(
+        *advertisement_received_token_);
+    if (FAILED(hr)) {
+      BLUETOOTH_LOG(ERROR) << "Removing the Received Handler failed: "
+                           << logging::SystemErrorCodeToString(hr);
+    }
+  }
+  if (advertisement_watcher_stopped_token_) {
+    HRESULT hr = ble_advertisement_watcher_->remove_Stopped(
+        *advertisement_watcher_stopped_token_);
+    if (FAILED(hr)) {
+      BLUETOOTH_LOG(ERROR) << "Removing the Stopped Handler failed: "
+                           << logging::SystemErrorCodeToString(hr);
+    }
   }
 }
 

device/bluetooth/bluetooth_adapter_winrt.h

@@ -198,7 +198,13 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothAdapterWinrt : public BluetoothAdapter {
       ABI::Windows::Devices::Bluetooth::Advertisement::
           IBluetoothLEAdvertisementWatcher* watcher,
       ABI::Windows::Devices::Bluetooth::Advertisement::
-          IBluetoothLEAdvertisementReceivedEventArgs* received);
+          IBluetoothLEAdvertisementReceivedEventArgs* args);
+
+  void OnAdvertisementWatcherStopped(
+      ABI::Windows::Devices::Bluetooth::Advertisement::
+          IBluetoothLEAdvertisementWatcher* watcher,
+      ABI::Windows::Devices::Bluetooth::Advertisement::
+          IBluetoothLEAdvertisementWatcherStoppedEventArgs* args);
 
   void OnRegisterAdvertisement(BluetoothAdvertisement* advertisement,
                                const CreateAdvertisementCallback& callback);
@@ -212,7 +218,7 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothAdapterWinrt : public BluetoothAdapter {
 
   void TryRemovePoweredRadioEventHandlers();
 
-  void RemoveAdvertisementReceivedHandler();
+  void RemoveAdvertisementWatcherEventHandlers();
 
   bool is_initialized_ = false;
   bool radio_access_allowed_ = false;
@@ -237,7 +243,8 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothAdapterWinrt : public BluetoothAdapter {
 
   std::vector<scoped_refptr<BluetoothAdvertisement>> pending_advertisements_;
 
-  EventRegistrationToken advertisement_received_token_;
+  base::Optional<EventRegistrationToken> advertisement_received_token_;
+  base::Optional<EventRegistrationToken> advertisement_watcher_stopped_token_;
   Microsoft::WRL::ComPtr<ABI::Windows::Devices::Bluetooth::Advertisement::
                              IBluetoothLEAdvertisementWatcher>
       ble_advertisement_watcher_;

device/bluetooth/test/bluetooth_test.cc

@@ -116,6 +116,10 @@ BluetoothDevice* BluetoothTestBase::SimulateLowEnergyDevice(
   return nullptr;
 }
 
+void BluetoothTestBase::SimulateLowEnergyDiscoveryFailure() {
+  NOTIMPLEMENTED();
+}
+
 BluetoothDevice* BluetoothTestBase::SimulateClassicDevice() {
   NOTIMPLEMENTED();
   return nullptr;

device/bluetooth/test/bluetooth_test.h

@@ -274,6 +274,10 @@ class BluetoothTestBase : public testing::Test {
 
   virtual BluetoothDevice* SimulateLowEnergyDevice(int device_ordinal);
 
+  // Simulates a signal by the OS that an ongoing discovery aborted because of
+  // some unexpected error.
+  virtual void SimulateLowEnergyDiscoveryFailure();
+
   // Simulates a connected low energy device. Used before starting a low energy
   // discovey session.
   virtual void SimulateConnectedLowEnergyDevice(

device/bluetooth/test/bluetooth_test_win.cc

@@ -853,6 +853,15 @@ BluetoothDevice* BluetoothTestWinrt::SimulateLowEnergyDevice(
   return adapter_->GetDevice(data.address);
 }
 
+void BluetoothTestWinrt::SimulateLowEnergyDiscoveryFailure() {
+  static_cast<TestBluetoothAdapterWinrt*>(adapter_.get())
+      ->watcher()
+      ->SimulateDiscoveryError();
+
+  // Spin until the WatcherStopped event fires.
+  base::RunLoop().RunUntilIdle();
+}
+
 void BluetoothTestWinrt::SimulateDevicePaired(BluetoothDevice* device,
                                               bool is_paired) {
   auto* const ble_device =

device/bluetooth/test/bluetooth_test_win.h

@@ -182,6 +182,7 @@ class BluetoothTestWinrt
   void SimulateAdapterPoweredOn() override;
   void SimulateAdapterPoweredOff() override;
   BluetoothDevice* SimulateLowEnergyDevice(int device_ordinal) override;
+  void SimulateLowEnergyDiscoveryFailure() override;
   void SimulateDevicePaired(BluetoothDevice* device, bool is_paired) override;
   void SimulatePairingPinCode(BluetoothDevice* device,
                               std::string pin_code) override;

device/bluetooth/test/fake_bluetooth_le_advertisement_watcher_winrt.cc

@@ -12,6 +12,9 @@ namespace device {
 
 namespace {
 
+using ABI::Windows::Devices::Bluetooth::BluetoothError;
+using ABI::Windows::Devices::Bluetooth::BluetoothError_OtherError;
+using ABI::Windows::Devices::Bluetooth::IBluetoothSignalStrengthFilter;
 using ABI::Windows::Devices::Bluetooth::Advertisement::
     BluetoothLEAdvertisementReceivedEventArgs;
 using ABI::Windows::Devices::Bluetooth::Advertisement::
@@ -27,14 +30,36 @@ using ABI::Windows::Devices::Bluetooth::Advertisement::
 using ABI::Windows::Devices::Bluetooth::Advertisement::BluetoothLEScanningMode;
 using ABI::Windows::Devices::Bluetooth::Advertisement::
     IBluetoothLEAdvertisementFilter;
-using ABI::Windows::Devices::Bluetooth::Advertisement::
-    IBluetoothLEAdvertisementReceivedEventArgs;
-using ABI::Windows::Devices::Bluetooth::IBluetoothSignalStrengthFilter;
-using ABI::Windows::Foundation::TimeSpan;
 using ABI::Windows::Foundation::ITypedEventHandler;
-using Microsoft::WRL::ComPtr;
+using ABI::Windows::Foundation::TimeSpan;
 using Microsoft::WRL::Make;
 
+class FakeBluetoothLEAdvertisementWatcherStoppedEventArgsWinrt
+    : public Microsoft::WRL::RuntimeClass<
+          Microsoft::WRL::RuntimeClassFlags<
+              Microsoft::WRL::WinRt | Microsoft::WRL::InhibitRoOriginateError>,
+          ABI::Windows::Devices::Bluetooth::Advertisement::
+              IBluetoothLEAdvertisementWatcherStoppedEventArgs> {
+ public:
+  FakeBluetoothLEAdvertisementWatcherStoppedEventArgsWinrt(BluetoothError error)
+      : error_(error) {}
+  FakeBluetoothLEAdvertisementWatcherStoppedEventArgsWinrt(
+      const FakeBluetoothLEAdvertisementWatcherStoppedEventArgsWinrt&) = delete;
+  FakeBluetoothLEAdvertisementWatcherStoppedEventArgsWinrt& operator=(
+      const FakeBluetoothLEAdvertisementWatcherStoppedEventArgsWinrt&) = delete;
+  ~FakeBluetoothLEAdvertisementWatcherStoppedEventArgsWinrt() override {}
+
+  // IBluetoothLEAdvertisementWatcherStoppedEventArgs:
+  IFACEMETHODIMP get_Error(
+      ABI::Windows::Devices::Bluetooth::BluetoothError* value) override {
+    *value = error_;
+    return S_OK;
+  }
+
+ private:
+  BluetoothError error_;
+};
+
 }  // namespace
 
 FakeBluetoothLEAdvertisementWatcherWinrt::
@@ -113,13 +138,13 @@ HRESULT FakeBluetoothLEAdvertisementWatcherWinrt::add_Received(
     ITypedEventHandler<BluetoothLEAdvertisementWatcher*,
                        BluetoothLEAdvertisementReceivedEventArgs*>* handler,
     EventRegistrationToken* token) {
-  handler_ = handler;
+  received_handler_ = handler;
   return S_OK;
 }
 
 HRESULT FakeBluetoothLEAdvertisementWatcherWinrt::remove_Received(
     EventRegistrationToken token) {
-  handler_.Reset();
+  received_handler_.Reset();
   return S_OK;
 }
 
@@ -128,18 +153,20 @@ HRESULT FakeBluetoothLEAdvertisementWatcherWinrt::add_Stopped(
                        BluetoothLEAdvertisementWatcherStoppedEventArgs*>*
         handler,
     EventRegistrationToken* token) {
-  return E_NOTIMPL;
+  stopped_handler_ = handler;
+  return S_OK;
 }
 
 HRESULT FakeBluetoothLEAdvertisementWatcherWinrt::remove_Stopped(
     EventRegistrationToken token) {
-  return E_NOTIMPL;
+  stopped_handler_.Reset();
+  return S_OK;
 }
 
 void FakeBluetoothLEAdvertisementWatcherWinrt::SimulateLowEnergyDevice(
     const BluetoothTestBase::LowEnergyDeviceData& device_data) {
-  if (handler_) {
-    handler_->Invoke(
+  if (received_handler_) {
+    received_handler_->Invoke(
         this, Make<FakeBluetoothLEAdvertisementReceivedEventArgsWinrt>(
                   device_data.rssi, device_data.address,
                   Make<FakeBluetoothLEAdvertisementWinrt>(
@@ -150,4 +177,13 @@ void FakeBluetoothLEAdvertisementWatcherWinrt::SimulateLowEnergyDevice(
   }
 }
 
+void FakeBluetoothLEAdvertisementWatcherWinrt::SimulateDiscoveryError() {
+  if (stopped_handler_) {
+    stopped_handler_->Invoke(
+        this, Make<FakeBluetoothLEAdvertisementWatcherStoppedEventArgsWinrt>(
+                  BluetoothError_OtherError)
+                  .Get());
+  }
+}
+
 }  // namespace device

device/bluetooth/test/fake_bluetooth_le_advertisement_watcher_winrt.h

@@ -77,6 +77,7 @@ class FakeBluetoothLEAdvertisementWatcherWinrt
 
   void SimulateLowEnergyDevice(
       const BluetoothTestBase::LowEnergyDeviceData& device_data);
+  void SimulateDiscoveryError();
 
  private:
   ABI::Windows::Devices::Bluetooth::Advertisement::
@@ -89,7 +90,14 @@ class FakeBluetoothLEAdvertisementWatcherWinrt
           BluetoothLEAdvertisementWatcher*,
       ABI::Windows::Devices::Bluetooth::Advertisement::
           BluetoothLEAdvertisementReceivedEventArgs*>>
-      handler_;
+      received_handler_;
+
+  Microsoft::WRL::ComPtr<ABI::Windows::Foundation::ITypedEventHandler<
+      ABI::Windows::Devices::Bluetooth::Advertisement::
+          BluetoothLEAdvertisementWatcher*,
+      ABI::Windows::Devices::Bluetooth::Advertisement::
+          BluetoothLEAdvertisementWatcherStoppedEventArgs*>>
+      stopped_handler_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeBluetoothLEAdvertisementWatcherWinrt);
 };