BluetoothAdapter invokes callbacks synchronously |
||||
Issue descriptionUnlike BT implementations on other platforms, BluetoothAdapterMac::AddDiscoverySession sometimes invokes |callback| synchronously, violating an invariant of FidoDiscovery that Start() never finishes synchronously. We should add PostTasks preferably to BluetoothAdapterMac so that it is consistent with other platforms.
,
Mar 20 2018
To clarify, probably each method, not just AddDiscoverySession, should either guarantee this on all platforms or not.
,
Mar 20 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/66353c23a32b3ada6a26699e4e8420397613c31b commit 66353c23a32b3ada6a26699e4e8420397613c31b Author: Balazs Engedy <engedy@chromium.org> Date: Tue Mar 20 15:41:07 2018 Apply Occam's razor to U2fDiscovery. No production code is calling U2fDiscovery::Stop, and all observer method implementations are no-ops. Similarly, no U2fDiscovery has more than a single observer added, and the only time that observer is removed is when both the discovery and that observer (the owner) dies. It sounds like we are not going to make use of this functionality in the near future, so this CL removes: -- the ability to Stop() U2fDiscoveries, and -- the ability to have more than a single Observer per U2fDiscovery. The CL also moves slightly more functionality into the U2fDiscovery base class from derived classes, most importantly with the goal of making sure the fake and production implementations function consistently in the following areas: -- U2fDiscovery now keeps track of whether Start NotifyDiscoveryStarted have been called, and exposes corresponding getters. -- It is now enforced that Start() and NotifyDiscoveryStarted() can be called once each, and in this order, as well as that NotifyDeviceAdded cannot be called before Start() is called. -- It is now enforced that Observer::DiscoveryStarted is never invoked synchronously while Start() is still on the call stack. Bug: 822244 , 823686 Change-Id: Ia029646d43481a1a2ca58452577727f96d6aade1 Reviewed-on: https://chromium-review.googlesource.com/964301 Commit-Queue: Balazs Engedy <engedy@chromium.org> Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org> Cr-Commit-Position: refs/heads/master@{#544379} [modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fake_fido_discovery.cc [modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fake_fido_discovery.h [modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fake_fido_discovery_unittest.cc [modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fido_ble_discovery.cc [modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fido_ble_discovery.h [modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fido_ble_discovery_unittest.cc [modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fido_discovery.cc [modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fido_discovery.h [modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fido_discovery_unittest.cc [modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fido_hid_discovery.cc [modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fido_hid_discovery.h [modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/fido_hid_discovery_unittest.cc [modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/scoped_virtual_u2f_device.cc [modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/u2f_request.cc [modify] https://crrev.com/66353c23a32b3ada6a26699e4e8420397613c31b/device/fido/u2f_request.h
,
Mar 21 2018
Definitely a bug on our part. We've been moving to always running callbacks asynchronously but there are some parts that still need to be moved. Patches always welcomed :)
,
Mar 21 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8a4ace9241e4a589b6c3c74a0870cbb5cbe67a24 commit 8a4ace9241e4a589b6c3c74a0870cbb5cbe67a24 Author: Jan Wilken Dörrie <jdoerrie@chromium.org> Date: Wed Mar 21 11:33:33 2018 [bluetooth] Make BluetoothAdapterMac::AddDiscoverySession() async This change adds calls to PostTask to BluetoothAdapterMac's AddDiscoverySession(), so that its callbacks are run asynchronously. Bug: 823686 Change-Id: If480e4cec792d1aee38fb7586d010aa3df009ca6 Reviewed-on: https://chromium-review.googlesource.com/970645 Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org> Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org> Cr-Commit-Position: refs/heads/master@{#544668} [modify] https://crrev.com/8a4ace9241e4a589b6c3c74a0870cbb5cbe67a24/device/bluetooth/bluetooth_adapter_mac.mm [modify] https://crrev.com/8a4ace9241e4a589b6c3c74a0870cbb5cbe67a24/device/bluetooth/bluetooth_adapter_mac_unittest.mm
,
Mar 23 2018
Looks like this affects Linux too. Querying the adapter and starting a discovery both return synchronously if a discovery is already running: base::debug::StackTrace::StackTrace() device::FidoDiscovery::NotifyDiscoveryStarted() device::FidoBleDiscovery::OnStartDiscoverySessionWithFilter() _ZN4base8internal7InvokerINS0_9BindStateIMN6device17FidoBleConnectionEFvNSt3__110unique_ptrINS3_23BluetoothGattConnectionENS5_14default_deleteIS7_EEEEEJNS_7WeakPtrIS4_EEEEEFvSA_EE7RunImplIRKSC_RKNS5_5tupleIJSE_EEEJLm0EEEEvOT_OT0_NS5_16integer_sequenceImJXspT1_EEEEOSA_ device::BluetoothAdapter::OnStartDiscoverySession() _ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIRKMN6device16BluetoothAdapterEFvNSt3__110unique_ptrINS4_24BluetoothDiscoveryFilterENS6_14default_deleteIS8_EEEERKNS_17RepeatingCallbackIFvNS7_INS4_25BluetoothDiscoverySessionENS9_ISD_EEEEEEEERKNS_7WeakPtrIS5_EEJSB_SJ_EEEvOT_OT0_DpOT1_ _ZN4base8internal7InvokerINS0_9BindStateIMN6device16BluetoothAdapterEFvNSt3__110unique_ptrINS3_24BluetoothDiscoveryFilterENS5_14default_deleteIS7_EEEERKNS_17RepeatingCallbackIFvNS6_INS3_25BluetoothDiscoverySessionENS8_ISC_EEEEEEEEJNS_7WeakPtrIS4_EENS0_13PassedWrapperISA_EESG_EEEFvvEE3RunEPNS0_13BindStateBaseE bluez::BluetoothAdapterBlueZ::SetDiscoveryFilter() bluez::BluetoothAdapterBlueZ::AddDiscoverySession() device::BluetoothAdapter::StartDiscoverySessionWithFilter() device::FidoBleDiscovery::OnSetPowered() device::FidoBleDiscovery::OnGetAdapter() _ZN4base8internal7InvokerINS0_9BindStateIMN6device17FidoBleConnectionEFv13scoped_refptrINS3_16BluetoothAdapterEEEJNS_7WeakPtrIS4_EEEEEFvS7_EE7RunImplIRKS9_RKNSt3__15tupleIJSB_EEEJLm0EEEEvOT_OT0_NSI_16integer_sequenceImJXspT1_EEEEOS7_ device::BluetoothAdapterFactory::GetAdapter() device::FidoBleDiscovery::StartInternal() device::FidoDiscovery::Start() device::U2fRequest::Start() device::U2fRegister::TryRegistration() content::AuthenticatorImpl::MakeCredential()
,
Mar 31 2018
,
Oct 26
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/20bb4123925613cc1b972af92f825b4d2d02fc29 commit 20bb4123925613cc1b972af92f825b4d2d02fc29 Author: Jun Choi <hongjunchoi@chromium.org> Date: Fri Oct 26 22:22:21 2018 Start FidoDeviceDiscovery asynchronously Callback for BluetoothAdapterFactory::GetAdapter() may be invoked synchronously for ChromeOS devices. This results in discovery of FidoBleDevices that were already known to BluetoothAdapter not being notified to UI embedder, and WebAuthN UI would not show new Bluetooth devices on device selection view --even when FidoBleDevices are discovered. To avoid such hairpinning, invoke FidoDeviceDiscovery::StartInternal asynchronously. Bug: 823686 Change-Id: Idc6c4d1d12d76a15118f15366e734325ba1819c0 Reviewed-on: https://chromium-review.googlesource.com/c/1300798 Commit-Queue: Jun Choi <hongjunchoi@chromium.org> Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org> Cr-Commit-Position: refs/heads/master@{#603216} [modify] https://crrev.com/20bb4123925613cc1b972af92f825b4d2d02fc29/device/fido/ble/fido_ble_discovery_base.cc [modify] https://crrev.com/20bb4123925613cc1b972af92f825b4d2d02fc29/device/fido/ble/fido_ble_discovery_unittest.cc [modify] https://crrev.com/20bb4123925613cc1b972af92f825b4d2d02fc29/device/fido/fido_device_discovery.cc [modify] https://crrev.com/20bb4123925613cc1b972af92f825b4d2d02fc29/device/fido/fido_device_discovery_unittest.cc
,
Jan 16
|
||||
►
Sign in to add a comment |
||||
Comment 1 by jdoerrie@chromium.org
, Mar 20 2018Components: IO>Bluetooth