New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 823686 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 16
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Mac
Pri: 2
Type: Bug


Participants' hotlists:
Hotlist-1


Sign in to add a comment

BluetoothAdapter invokes callbacks synchronously

Project Member Reported by engedy@chromium.org, Mar 20 2018

Issue description

Unlike 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.
 
Cc: ortuno@chromium.org
Components: IO>Bluetooth

Comment 2 by engedy@chromium.org, Mar 20 2018

To clarify, probably each method, not just AddDiscoverySession, should either guarantee this on all platforms or not.
Project Member

Comment 3 by bugdroid1@chromium.org, 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

Comment 4 by ortuno@chromium.org, 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 :)
Project Member

Comment 5 by bugdroid1@chromium.org, 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

Comment 6 by engedy@chromium.org, Mar 23 2018

Components: Blink>WebAuthentication
Labels: OS-Linux OS-Mac
Summary: BluetoothAdapter invokes callbacks synchronously (was: BluetoothAdapterMac invokes callback synchronously)
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()

Comment 7 by engedy@chromium.org, Mar 31 2018

Labels: Hotlist-WebAuthnFixit
Project Member

Comment 8 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)

Sign in to add a comment