[bluetooth] Handle adapter destruction during discovery callbacks

BluetoothAdapter::OnDiscoveryChangeComplete() needs to be able to handle
the case where a callback destroys the BluetoothAdapter.

This issue was not noticed in tests because TestBluetoothAdapter takes a
reference to the BluetoothAdapter when executing these callbacks. It
has been updated to use a WeakPtr as the real backends do.

I noticed that both BluetoothAdapter and nearly all of its subclasses
have a WeakPtrFactory. The factory in the base class has been removed
and subclasses are required to provide their own.

Bug: 1025067
Change-Id: I91f952e01fd3bda618455f294ce0cba2e2a7dad1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1922298
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Ovidio de Jesús Ruiz-Henríquez <odejesush@chromium.org>
Cr-Commit-Position: refs/heads/master@{#716465}

device/bluetooth/bluetooth_adapter.cc

@@ -37,7 +37,7 @@ base::WeakPtr<BluetoothAdapter> BluetoothAdapter::CreateAdapter(
 #endif  // !defined(OS_CHROMEOS) && !defined(OS_WIN) && !defined(OS_MACOSX)
 
 base::WeakPtr<BluetoothAdapter> BluetoothAdapter::GetWeakPtrForTesting() {
-  return weak_ptr_factory_.GetWeakPtr();
+  return GetWeakPtr();
 }
 
 #if defined(OS_LINUX)
@@ -446,8 +446,15 @@ void BluetoothAdapter::OnDiscoveryChangeComplete(
     UMABluetoothDiscoverySessionOutcome outcome) {
   UpdateDiscoveryState(is_error);
 
+  // Take a weak reference to |this| in case a callback frees the adapter.
+  base::WeakPtr<BluetoothAdapter> self = GetWeakPtr();
+
   if (is_error) {
     NotifyDiscoveryError(std::move(callbacks_awaiting_response_));
+
+    if (!self)
+      return;
+
     discovery_request_pending_ = false;
     ProcessDiscoveryQueue();
     return;
@@ -455,16 +462,20 @@ void BluetoothAdapter::OnDiscoveryChangeComplete(
 
   current_discovery_filter_.CopyFrom(filter_being_set_);
 
-  while (!callbacks_awaiting_response_.empty()) {
+  auto callbacks_awaiting_response = std::move(callbacks_awaiting_response_);
+  while (!callbacks_awaiting_response.empty()) {
     std::unique_ptr<StartOrStopDiscoveryCallback> callbacks =
-        std::move(callbacks_awaiting_response_.front());
-    callbacks_awaiting_response_.pop();
+        std::move(callbacks_awaiting_response.front());
+    callbacks_awaiting_response.pop();
     if (callbacks->start_callback)
       std::move(callbacks->start_callback).Run();
-
     if (callbacks->stop_callback)
       std::move(callbacks->stop_callback).Run();
   }
+
+  if (!self)
+    return;
+
   discovery_request_pending_ = false;
   ProcessDiscoveryQueue();
 }
@@ -501,14 +512,13 @@ void BluetoothAdapter::ProcessDiscoveryQueue() {
     internal_discovery_state_ = DiscoveryState::kStopping;
     discovery_request_pending_ = true;
     StopScan(base::BindOnce(&BluetoothAdapter::OnDiscoveryChangeComplete,
-                            weak_ptr_factory_.GetWeakPtr()));
+                            GetWeakPtr()));
 
     return;
   }
 
-  auto result_callback =
-      base::BindOnce(&BluetoothAdapter::OnDiscoveryChangeComplete,
-                     weak_ptr_factory_.GetWeakPtr());
+  auto result_callback = base::BindOnce(
+      &BluetoothAdapter::OnDiscoveryChangeComplete, GetWeakPtr());
   auto new_desired_filter = GetMergedDiscoveryFilter();
   discovery_request_pending_ = true;
   filter_being_set_.CopyFrom(*new_desired_filter.get());

device/bluetooth/bluetooth_adapter.h

@@ -671,6 +671,8 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothAdapter
   BluetoothAdapter();
   virtual ~BluetoothAdapter();
 
+  virtual base::WeakPtr<BluetoothAdapter> GetWeakPtr() = 0;
+
   // This method calls into platform specific logic on macOS and Android where
   // pending SetPowered() callbacks need to be stored explicitly.
   virtual bool SetPoweredImpl(bool powered) = 0;
@@ -818,10 +820,6 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothAdapter
 
   // enum used to track our internal discovery state.
   DiscoveryState internal_discovery_state_ = DiscoveryState::kIdle;
-
-  // Note: This should remain the last member so it'll be destroyed and
-  // invalidate its weak pointers before any other members are destroyed.
-  base::WeakPtrFactory<BluetoothAdapter> weak_ptr_factory_{this};
 };
 
 }  // namespace device

device/bluetooth/bluetooth_adapter_android.cc

@@ -295,6 +295,10 @@ void BluetoothAdapterAndroid::PurgeTimedOutDevices() {
   }
 }
 
+base::WeakPtr<BluetoothAdapter> BluetoothAdapterAndroid::GetWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
 bool BluetoothAdapterAndroid::SetPoweredImpl(bool powered) {
   return Java_ChromeBluetoothAdapter_setPowered(AttachCurrentThread(),
                                                 j_adapter_, powered);

device/bluetooth/bluetooth_adapter_android.h

@@ -112,6 +112,7 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothAdapterAndroid final
   ~BluetoothAdapterAndroid() override;
 
   // BluetoothAdapter:
+  base::WeakPtr<BluetoothAdapter> GetWeakPtr() override;
   bool SetPoweredImpl(bool powered) override;
   void StartScanWithFilter(
       std::unique_ptr<BluetoothDiscoveryFilter> discovery_filter,

device/bluetooth/bluetooth_adapter_mac.h

@@ -124,6 +124,7 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothAdapterMac
       base::RepeatingCallback<bool(const std::string& address)>;
 
   // BluetoothAdapter override:
+  base::WeakPtr<BluetoothAdapter> GetWeakPtr() override;
   bool SetPoweredImpl(bool powered) override;
   void RemovePairingDelegateInternal(
       device::BluetoothDevice::PairingDelegate* pairing_delegate) override;

device/bluetooth/bluetooth_adapter_mac.mm

@@ -302,6 +302,10 @@ void BluetoothAdapterMac::DeviceConnected(IOBluetoothDevice* device) {
   ClassicDeviceAdded(device);
 }
 
+base::WeakPtr<BluetoothAdapter> BluetoothAdapterMac::GetWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
 bool BluetoothAdapterMac::SetPoweredImpl(bool powered) {
   power_state_function_.Run(base::strict_cast<int>(powered));
   return true;

device/bluetooth/bluetooth_adapter_unittest.cc

@@ -60,7 +60,7 @@ void AddDeviceFilterWithUUID(BluetoothDiscoveryFilter* filter,
 
 namespace {
 
-class TestBluetoothAdapter : public BluetoothAdapter {
+class TestBluetoothAdapter final : public BluetoothAdapter {
  public:
   TestBluetoothAdapter() = default;
 
@@ -151,6 +151,11 @@ class TestBluetoothAdapter : public BluetoothAdapter {
     run_loop_quit.Run();
   }
 
+  void set_discovery_session_outcome(
+      UMABluetoothDiscoverySessionOutcome outcome) {
+    discovery_session_outcome_ = outcome;
+  }
+
   void StopDiscoverySession(base::Closure run_loop_quit) {
     discovery_sessions_holder_.front()->Stop(
         base::BindRepeating(&TestBluetoothAdapter::OnRemoveDiscoverySession,
@@ -238,12 +243,17 @@ class TestBluetoothAdapter : public BluetoothAdapter {
 
   bool SetPoweredImpl(bool powered) override { return false; }
 
+  base::WeakPtr<BluetoothAdapter> GetWeakPtr() override {
+    return weak_ptr_factory_.GetWeakPtr();
+  }
+
   void StartScanWithFilter(
       std::unique_ptr<BluetoothDiscoveryFilter> discovery_filter,
       DiscoverySessionResultCallback callback) override {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
-        base::BindOnce(&TestBluetoothAdapter::SetFilter, this,
+        base::BindOnce(&TestBluetoothAdapter::SetFilter,
+                       weak_ptr_factory_.GetWeakPtr(),
                        std::move(discovery_filter), std::move(callback)));
   }
 
@@ -251,22 +261,27 @@ class TestBluetoothAdapter : public BluetoothAdapter {
                     DiscoverySessionResultCallback callback) override {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
-        base::BindOnce(&TestBluetoothAdapter::SetFilter, this,
+        base::BindOnce(&TestBluetoothAdapter::SetFilter,
+                       weak_ptr_factory_.GetWeakPtr(),
                        std::move(discovery_filter), std::move(callback)));
   }
 
   void SetFilter(std::unique_ptr<BluetoothDiscoveryFilter> discovery_filter,
                  DiscoverySessionResultCallback callback) {
+    bool is_error = discovery_session_outcome_ !=
+                    UMABluetoothDiscoverySessionOutcome::SUCCESS;
+    if (!is_error) {
       is_discovering_ = true;
       current_filter->CopyFrom(*discovery_filter.get());
-    std::move(callback).Run(/*is_error=*/false,
-                            UMABluetoothDiscoverySessionOutcome::SUCCESS);
+    }
+    std::move(callback).Run(is_error, discovery_session_outcome_);
   }
 
   void StopScan(DiscoverySessionResultCallback callback) override {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(&TestBluetoothAdapter::FakeOSStopScan, this,
-                                  std::move(callback)));
+        FROM_HERE,
+        base::BindOnce(&TestBluetoothAdapter::FakeOSStopScan,
+                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }
 
   void FakeOSStopScan(DiscoverySessionResultCallback callback) {
@@ -285,6 +300,13 @@ class TestBluetoothAdapter : public BluetoothAdapter {
     base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
                                                   std::move(callback));
   }
+
+  UMABluetoothDiscoverySessionOutcome discovery_session_outcome_ =
+      UMABluetoothDiscoverySessionOutcome::SUCCESS;
+
+  // This must be the last field in the class so that weak pointers are
+  // invalidated first.
+  base::WeakPtrFactory<TestBluetoothAdapter> weak_ptr_factory_{this};
 };
 
 class TestPairingDelegate : public BluetoothDevice::PairingDelegate {
@@ -640,6 +662,30 @@ TEST_F(BluetoothAdapterTest, GetMergedDiscoveryFilterAllFields) {
   adapter_->CleanupSessions();
 }
 
+TEST_F(BluetoothAdapterTest, StartDiscoverySession_Destroy) {
+  base::RunLoop loop;
+  adapter_->StartDiscoverySession(
+      base::BindLambdaForTesting(
+          [&](std::unique_ptr<BluetoothDiscoverySession> session) {
+            adapter_.reset();
+            loop.Quit();
+          }),
+      base::DoNothing());
+  loop.Run();
+}
+
+TEST_F(BluetoothAdapterTest, StartDiscoverySessionError_Destroy) {
+  base::RunLoop loop;
+  adapter_->set_discovery_session_outcome(
+      UMABluetoothDiscoverySessionOutcome::FAILED);
+  adapter_->StartDiscoverySession(base::DoNothing(),
+                                  base::BindLambdaForTesting([&]() {
+                                    adapter_.reset();
+                                    loop.Quit();
+                                  }));
+  loop.Run();
+}
+
 // TODO(scheib): Enable BluetoothTest fixture tests on all platforms.
 #if defined(OS_ANDROID) || defined(OS_MACOSX)
 #define MAYBE_ConstructDefaultAdapter ConstructDefaultAdapter

device/bluetooth/bluetooth_adapter_win.cc

@@ -279,6 +279,10 @@ void BluetoothAdapterWin::DevicesPolled(
   }
 }
 
+base::WeakPtr<BluetoothAdapter> BluetoothAdapterWin::GetWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
 // BluetoothAdapterWin should override SetPowered() instead.
 bool BluetoothAdapterWin::SetPoweredImpl(bool powered) {
   NOTREACHED();

device/bluetooth/bluetooth_adapter_win.h

@@ -116,6 +116,7 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothAdapterWin
   ~BluetoothAdapterWin() override;
 
   // BluetoothAdapter:
+  base::WeakPtr<BluetoothAdapter> GetWeakPtr() override;
   bool SetPoweredImpl(bool powered) override;
   void UpdateFilter(std::unique_ptr<BluetoothDiscoveryFilter> discovery_filter,
                     DiscoverySessionResultCallback callback) override;

device/bluetooth/bluetooth_adapter_winrt.cc

@@ -841,6 +841,10 @@ void BluetoothAdapterWinrt::CompleteInit(
   }
 }
 
+base::WeakPtr<BluetoothAdapter> BluetoothAdapterWinrt::GetWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
 bool BluetoothAdapterWinrt::SetPoweredImpl(bool powered) {
   // Due to an issue on WoW64 we might fail to obtain the radio in
   // OnGetRadio(). This is why it can be null here.

device/bluetooth/bluetooth_adapter_winrt.h

@@ -92,6 +92,7 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothAdapterWinrt : public BluetoothAdapter {
           radio_statics);
 
   // BluetoothAdapter:
+  base::WeakPtr<BluetoothAdapter> GetWeakPtr() override;
   bool SetPoweredImpl(bool powered) override;
   void UpdateFilter(std::unique_ptr<BluetoothDiscoveryFilter> discovery_filter,
                     DiscoverySessionResultCallback callback) override;

device/bluetooth/bluez/bluetooth_adapter_bluez.cc

@@ -1476,6 +1476,10 @@ void BluetoothAdapterBlueZ::OnPropertyChangeCompleted(
   }
 }
 
+base::WeakPtr<BluetoothAdapter> BluetoothAdapterBlueZ::GetWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
 // BluetoothAdapterBlueZ should override SetPowered() instead.
 bool BluetoothAdapterBlueZ::SetPoweredImpl(bool powered) {
   NOTREACHED();

device/bluetooth/bluez/bluetooth_adapter_bluez.h

@@ -74,7 +74,7 @@ class BluetoothPairingBlueZ;
 //
 // When adding methods to this class verify shutdown behavior in
 // BluetoothBlueZTest, Shutdown.
-class DEVICE_BLUETOOTH_EXPORT BluetoothAdapterBlueZ
+class DEVICE_BLUETOOTH_EXPORT BluetoothAdapterBlueZ final
     : public device::BluetoothAdapter,
       public bluez::BluetoothAdapterClient::Observer,
       public bluez::BluetoothDeviceClient::Observer,
@@ -372,6 +372,7 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothAdapterBlueZ
                                  bool success);
 
   // BluetoothAdapter:
+  base::WeakPtr<BluetoothAdapter> GetWeakPtr() override;
   bool SetPoweredImpl(bool powered) override;
   void StartScanWithFilter(
       std::unique_ptr<device::BluetoothDiscoveryFilter> discovery_filter,

device/bluetooth/cast/bluetooth_adapter_cast.cc

@@ -180,6 +180,11 @@ BluetoothLocalGattService* BluetoothAdapterCast::GetGattService(
   return nullptr;
 }
 
+base::WeakPtr<BluetoothAdapter> BluetoothAdapterCast::GetWeakPtr() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  return weak_factory_.GetWeakPtr();
+}
+
 bool BluetoothAdapterCast::SetPoweredImpl(bool powered) {
   NOTREACHED() << "This method is not invoked when SetPowered() is overridden.";
   return true;
@@ -247,11 +252,6 @@ void BluetoothAdapterCast::RemovePairingDelegateInternal(
   NOTIMPLEMENTED();
 }
 
-base::WeakPtr<BluetoothAdapterCast> BluetoothAdapterCast::GetWeakPtr() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return weak_factory_.GetWeakPtr();
-}
-
 void BluetoothAdapterCast::OnConnectChanged(
     scoped_refptr<chromecast::bluetooth::RemoteDevice> device,
     bool connected) {

device/bluetooth/cast/bluetooth_adapter_cast.h

@@ -89,6 +89,7 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothAdapterCast
       const AdvertisementErrorCallback& error_callback) override;
   BluetoothLocalGattService* GetGattService(
       const std::string& identifier) const override;
+  base::WeakPtr<BluetoothAdapter> GetWeakPtr() override;
   bool SetPoweredImpl(bool powered) override;
   void StartScanWithFilter(
       std::unique_ptr<device::BluetoothDiscoveryFilter> discovery_filter,
@@ -100,12 +101,6 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothAdapterCast
   void RemovePairingDelegateInternal(
       BluetoothDevice::PairingDelegate* pairing_delegate) override;
 
-  // Return a WeakPtr for this class. Must be called on the sequence on which
-  // this class was created.
-  // TODO(slan): Remove this once this class talks to a dedicated Bluetooth
-  // service (b/76155468)
-  base::WeakPtr<BluetoothAdapterCast> GetWeakPtr();
-
   // |factory_cb| is used to inject a factory method from ChromecastService into
   // this class. It will be invoked when Create() is called.
   // TODO(slan): Remove this once this class talks to a dedicated Bluetooth

device/bluetooth/test/fake_central.cc

@@ -584,6 +584,10 @@ device::BluetoothLocalGattService* FakeCentral::GetGattService(
   return nullptr;
 }
 
+base::WeakPtr<device::BluetoothAdapter> FakeCentral::GetWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
 bool FakeCentral::SetPoweredImpl(bool powered) {
   NOTREACHED();
   return false;

device/bluetooth/test/fake_central.h

@@ -27,7 +27,8 @@ class FakeRemoteGattService;
 // device/bluetooth/bluetooth_adapter.h.
 //
 // Not intended for direct use by clients.  See README.md.
-class FakeCentral : public mojom::FakeCentral, public device::BluetoothAdapter {
+class FakeCentral final : public mojom::FakeCentral,
+                          public device::BluetoothAdapter {
  public:
   FakeCentral(mojom::CentralState state,
               mojo::PendingReceiver<mojom::FakeCentral> receiver);
@@ -183,6 +184,7 @@ class FakeCentral : public mojom::FakeCentral, public device::BluetoothAdapter {
 #endif
   device::BluetoothLocalGattService* GetGattService(
       const std::string& identifier) const override;
+  base::WeakPtr<BluetoothAdapter> GetWeakPtr() override;
   bool SetPoweredImpl(bool powered) override;
   void UpdateFilter(
       std::unique_ptr<device::BluetoothDiscoveryFilter> discovery_filter,
@@ -214,6 +216,7 @@ class FakeCentral : public mojom::FakeCentral, public device::BluetoothAdapter {
 
   mojom::CentralState state_;
   mojo::Receiver<mojom::FakeCentral> receiver_;
+  base::WeakPtrFactory<FakeCentral> weak_ptr_factory_{this};
 };
 
 }  // namespace bluetooth

device/bluetooth/test/mock_bluetooth_adapter.cc

@@ -43,6 +43,10 @@ void MockBluetoothAdapter::Shutdown() {
 }
 #endif
 
+base::WeakPtr<BluetoothAdapter> MockBluetoothAdapter::GetWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
 bool MockBluetoothAdapter::SetPoweredImpl(bool powered) {
   return false;
 }

device/bluetooth/test/mock_bluetooth_adapter.h

@@ -132,6 +132,7 @@ class MockBluetoothAdapter : public BluetoothAdapter {
   }
 
  protected:
+  base::WeakPtr<BluetoothAdapter> GetWeakPtr() override;
   bool SetPoweredImpl(bool powered) override;
   void StartScanWithFilter(
       std::unique_ptr<BluetoothDiscoveryFilter> discovery_filter,
@@ -158,6 +159,10 @@ class MockBluetoothAdapter : public BluetoothAdapter {
                void(BluetoothDevice::PairingDelegate* pairing_delegate));
 
   std::vector<std::unique_ptr<MockBluetoothDevice>> mock_devices_;
+
+  // This must be the last field in the class so that weak pointers are
+  // invalidated first.
+  base::WeakPtrFactory<MockBluetoothAdapter> weak_ptr_factory_{this};
 };
 
 }  // namespace device