Fix reentrancy in BluetoothDeviceWinrt::ClearGattServices()

This change fixes a crash due to reentrancy in the ClearGattServices()
method when called during disconnect. Destroying a GATT service object
and associated characteristic and descriptor objects can run callbacks
for pending read or write operations. If these themselves attempt to
close the connection an unsafe reentrancy into std::map::clear() will
occur.

This change reverts the temporary fix for this issue.

Bug: 950204
Change-Id: I3b1028389c58dc49e5aefc9ab77fd6be51d86fb3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1597345
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@{#657379}

device/bluetooth/bluetooth_device_winrt.cc

@@ -615,7 +615,12 @@ void BluetoothDeviceWinrt::OnGattDiscoveryComplete(bool success) {
 }
 
 void BluetoothDeviceWinrt::ClearGattServices() {
-  gatt_services_.clear();
+  // Clearing |gatt_services_| can trigger callbacks. Move the existing
+  // objects into a local variable to avoid re-entrancy into clear().
+  GattServiceMap temp_gatt_services;
+  temp_gatt_services.swap(gatt_services_);
+  temp_gatt_services.clear();
+
   device_uuids_.ClearServiceUUIDs();
   SetGattServicesDiscoveryComplete(false);
 }

device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc

@@ -3334,6 +3334,44 @@ TEST_F(
             last_gatt_error_code_);
 }
 
+// Tests that closing the GATT connection when a characteristic value write
+// fails due to a disconnect is safe.
+#if defined(OS_ANDROID) || defined(OS_MACOSX)
+#define MAYBE_WriteWithoutResponseOnlyCharacteristic_CloseConnectionDuringDisconnect \
+  WriteWithoutResponseOnlyCharacteristic_CloseConnectionDuringDisconnect
+#else
+#define MAYBE_WriteWithoutResponseOnlyCharacteristic_CloseConnectionDuringDisconnect \
+  DISABLED_WriteWithoutResponseOnlyCharacteristic_CloseConnectionDuringDisconnect
+#endif
+#if defined(OS_WIN)
+TEST_P(BluetoothRemoteGattCharacteristicTestWinrtOnly,
+       WriteWithoutResponseOnlyCharacteristic_CloseConnectionDuringDisconnect) {
+#else
+TEST_F(
+    BluetoothRemoteGattCharacteristicTest,
+    MAYBE_WriteWithoutResponseOnlyCharacteristic_CloseConnectionDuringDisconnect) {
+#endif
+  if (!PlatformSupportsLowEnergy()) {
+    LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
+    return;
+  }
+  ASSERT_NO_FATAL_FAILURE(FakeCharacteristicBoilerplate(
+      BluetoothRemoteGattCharacteristic::PROPERTY_WRITE_WITHOUT_RESPONSE));
+
+  base::RunLoop loop;
+  characteristic1_->WriteRemoteCharacteristic(
+      std::vector<uint8_t>(), GetCallback(Call::NOT_EXPECTED),
+      base::BindLambdaForTesting(
+          [&](BluetoothGattService::GattErrorCode error_code) {
+            EXPECT_EQ(BluetoothRemoteGattService::GATT_ERROR_FAILED,
+                      error_code);
+            gatt_connections_[0]->Disconnect();
+            loop.Quit();
+          }));
+  SimulateDeviceBreaksConnection(adapter_->GetDevices()[0]);
+  loop.Run();
+}
+
 #if defined(OS_MACOSX)
 #define MAYBE_WriteWithoutResponseOnlyCharacteristic_DisconnectCalledDuringWriteRemoteCharacteristic \
   WriteWithoutResponseOnlyCharacteristic_DisconnectCalledDuringWriteRemoteCharacteristic

device/fido/ble/fido_ble_connection.cc

@@ -185,11 +185,6 @@ FidoBleConnection::FidoBleConnection(
 }
 
 FidoBleConnection::~FidoBleConnection() {
-  // Workaround for crbug.com/950204, avoids re-entrancy bugs triggered by
-  // closing a GATT connection while handling changes to the GATT connection
-  // state.
-  base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE,
-                                                  std::move(connection_));
   adapter_->RemoveObserver(this);
 }
 

device/fido/ble/fido_ble_connection_unittest.cc

@@ -123,12 +123,6 @@ class FidoBleConnectionTest : public ::testing::Test {
     BluetoothAdapterFactory::SetAdapterForTesting(adapter_);
   }
 
-  void TearDown() override {
-    // Workaround for the workaround for crbug.com/950204 to allow the
-    // MockBluetoothGattConnection to be destroyed before the test exits.
-    scoped_task_environment_.RunUntilIdle();
-  }
-
   BluetoothAdapter* adapter() { return adapter_.get(); }
   MockBluetoothDevice* device() { return fido_device_; }
 

device/fido/cable/fido_cable_handshake_handler_fuzzer.cc

@@ -9,8 +9,6 @@
 
 #include "base/containers/span.h"
 #include "base/memory/ref_counted.h"
-#include "base/no_destructor.h"
-#include "base/test/scoped_task_environment.h"
 #include "device/bluetooth/test/mock_bluetooth_adapter.h"
 #include "device/fido/cable/fido_cable_device.h"
 #include "device/fido/cable/fido_cable_handshake_handler.h"
@@ -35,7 +33,6 @@ constexpr char kTestDeviceAddress[] = "Fake_Address";
 }  // namespace
 
 extern "C" int LLVMFuzzerTestOneInput(const uint8_t* raw_data, size_t size) {
-  static const base::NoDestructor<base::test::ScopedTaskEnvironment> task_env;
   auto data_span = base::make_span(raw_data, size);
   auto adapter =
       base::MakeRefCounted<::testing::NiceMock<device::MockBluetoothAdapter>>();