[SecureChannel Bluetooth] Ignore DeviceChanged event right after device is disconnected |
|||||||||||
Issue descriptionFor context, see https://groups.google.com/a/google.com/d/topic/better-together-dev/nHWZg-7fme4/discussion. We are currently trying to connect to a device right after the connection to it is lost, which will always fail and waste one of our retry attempts. It also seems like it may put Bluetooth in a weird state sometimes. According to Gio, we should: "Once the device becomes disconnected, ignore the first DeviceChanged event. That event is being dispatched because the device disconnected, not because we received an advertisement. If the device is still around, we'll soon get another DeviceChanged event and we can connect then."
,
Oct 24
,
Oct 24
,
Oct 26
ortuno@/rkc@: Is this the sort of solution you were thinking of? https://chromium-review.googlesource.com/c/chromium/src/+/1302675/1/chromeos/services/secure_channel/ble_scanner_impl.cc (Note that the CL is a WIP and is not ready for a review.)
,
Oct 28
yup, I think that makes sense.
,
Nov 22
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1feb34d303cbd035e8f4b678afe0523f484d5007 commit 1feb34d303cbd035e8f4b678afe0523f484d5007 Author: Kyle Horimoto <khorimoto@chromium.org> Date: Thu Nov 22 05:14:01 2018 [CrOS MultiDevice] Add DeviceConnectedStateChanged() observer callback. This callback allows Bluetooth clients on Chrome OS to listen for connection state changes (i.e., when a device goes from connected to disconnected or vice versa). A follow-up CL will add handling for this event in the SecureChannel service to prevent us from accidentally attempting to connect to a disconnecting device (see https://crbug.com/898334 ). Bug: 898334 Change-Id: Icc18f44106d12a881518f0ece3f0a6e9dd715c0b Reviewed-on: https://chromium-review.googlesource.com/c/1347204 Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org> Commit-Queue: Kyle Horimoto <khorimoto@chromium.org> Cr-Commit-Position: refs/heads/master@{#610307} [modify] https://crrev.com/1feb34d303cbd035e8f4b678afe0523f484d5007/device/bluetooth/bluetooth_adapter.h [modify] https://crrev.com/1feb34d303cbd035e8f4b678afe0523f484d5007/device/bluetooth/bluez/bluetooth_adapter_bluez.cc [modify] https://crrev.com/1feb34d303cbd035e8f4b678afe0523f484d5007/device/bluetooth/bluez/bluetooth_adapter_bluez.h [modify] https://crrev.com/1feb34d303cbd035e8f4b678afe0523f484d5007/device/bluetooth/bluez/bluetooth_bluez_unittest.cc [modify] https://crrev.com/1feb34d303cbd035e8f4b678afe0523f484d5007/device/bluetooth/test/test_bluetooth_adapter_observer.cc [modify] https://crrev.com/1feb34d303cbd035e8f4b678afe0523f484d5007/device/bluetooth/test/test_bluetooth_adapter_observer.h
,
Nov 27
I've written up a CL which theoretically should fix things: https://chromium-review.googlesource.com/c/chromium/src/+/1351608 However, it doesn't work as it cannot pick up any advertisements. ortuno@ says that there is an issue with the "advertisement received" event, so my CL will have to wait until that issue has been fixed.
,
Nov 27
,
Dec 4
,
Dec 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/af8d33b48b8a6d7685584a4682ae39cd848217a5 commit af8d33b48b8a6d7685584a4682ae39cd848217a5 Author: Kyle Horimoto <khorimoto@chromium.org> Date: Tue Dec 04 00:50:17 2018 [CrOS MultiDevice] Fix BLE scan result handling. Previously, we would attempt a BLE scan any time that we got a DeviceChanged() or DeviceAdded() callback. However, the DeviceChanged() callback was invoked when a device was in the process of disconnecting, meaning that we would see a disconnecting device, then try to start a connection immediately. This resulted in spurious connection requests which could sometime cause a deadlock in underlying Bluetooth code. Additionally, we would rely on the DeviceChanged() and DeviceRemoved() callbacks to listen for Bluetooth disconnections, which also resulted in error-prone disconnections. This CL updates both call sites to use the updated DeviceAdvertisementReceived() and DeviceConnectedStateChanged() callbacks instead. Bug: 898334 Change-Id: I743e51e6e47194649ab6852966b9d892c3ef334d Reviewed-on: https://chromium-review.googlesource.com/c/1359478 Commit-Queue: Kyle Horimoto <khorimoto@chromium.org> Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org> Cr-Commit-Position: refs/heads/master@{#613358} [modify] https://crrev.com/af8d33b48b8a6d7685584a4682ae39cd848217a5/chromeos/services/secure_channel/ble_scanner_impl.cc [modify] https://crrev.com/af8d33b48b8a6d7685584a4682ae39cd848217a5/chromeos/services/secure_channel/ble_scanner_impl.h [modify] https://crrev.com/af8d33b48b8a6d7685584a4682ae39cd848217a5/chromeos/services/secure_channel/ble_scanner_impl_unittest.cc [modify] https://crrev.com/af8d33b48b8a6d7685584a4682ae39cd848217a5/components/cryptauth/ble/bluetooth_low_energy_weave_client_connection.cc [modify] https://crrev.com/af8d33b48b8a6d7685584a4682ae39cd848217a5/components/cryptauth/ble/bluetooth_low_energy_weave_client_connection.h [modify] https://crrev.com/af8d33b48b8a6d7685584a4682ae39cd848217a5/components/cryptauth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc
,
Dec 4
,
Dec 5
Your change meets the bar and is auto-approved for M72. Please go ahead and merge the CL to branch 3626 manually. Please contact milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 5
,
Dec 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dc6efc1135fc8d4890b29b3d0fb9426b34bc2b30 commit dc6efc1135fc8d4890b29b3d0fb9426b34bc2b30 Author: Kyle Horimoto <khorimoto@chromium.org> Date: Wed Dec 05 18:57:32 2018 [CrOS MultiDevice] Fix BLE scan result handling. Previously, we would attempt a BLE scan any time that we got a DeviceChanged() or DeviceAdded() callback. However, the DeviceChanged() callback was invoked when a device was in the process of disconnecting, meaning that we would see a disconnecting device, then try to start a connection immediately. This resulted in spurious connection requests which could sometime cause a deadlock in underlying Bluetooth code. Additionally, we would rely on the DeviceChanged() and DeviceRemoved() callbacks to listen for Bluetooth disconnections, which also resulted in error-prone disconnections. This CL updates both call sites to use the updated DeviceAdvertisementReceived() and DeviceConnectedStateChanged() callbacks instead. Bug: 898334 Change-Id: I743e51e6e47194649ab6852966b9d892c3ef334d Reviewed-on: https://chromium-review.googlesource.com/c/1359478 Commit-Queue: Kyle Horimoto <khorimoto@chromium.org> Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org> Cr-Commit-Position: refs/heads/master@{#613358} (cherry picked from commit af8d33b48b8a6d7685584a4682ae39cd848217a5) TBR: ortuno@chromium.org Change-Id: I743e51e6e47194649ab6852966b9d892c3ef334d Reviewed-on: https://chromium-review.googlesource.com/c/1363685 Reviewed-by: Kyle Horimoto <khorimoto@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#84} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/dc6efc1135fc8d4890b29b3d0fb9426b34bc2b30/chromeos/services/secure_channel/ble_scanner_impl.cc [modify] https://crrev.com/dc6efc1135fc8d4890b29b3d0fb9426b34bc2b30/chromeos/services/secure_channel/ble_scanner_impl.h [modify] https://crrev.com/dc6efc1135fc8d4890b29b3d0fb9426b34bc2b30/chromeos/services/secure_channel/ble_scanner_impl_unittest.cc [modify] https://crrev.com/dc6efc1135fc8d4890b29b3d0fb9426b34bc2b30/components/cryptauth/ble/bluetooth_low_energy_weave_client_connection.cc [modify] https://crrev.com/dc6efc1135fc8d4890b29b3d0fb9426b34bc2b30/components/cryptauth/ble/bluetooth_low_energy_weave_client_connection.h [modify] https://crrev.com/dc6efc1135fc8d4890b29b3d0fb9426b34bc2b30/components/cryptauth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc
,
Dec 19
Here's a summary of the rules that were executed: - OnlyMergeApprovedChange: Rule Failed -- Revision dc6efc1135fc8d4890b29b3d0fb9426b34bc2b30 was merged to refs/branch-heads/3626 branch with no merge approval from a TPM! Please explain why this change was merged to the branch! - AcknowledgeMerge: Notification Required --
,
Dec 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dc6efc1135fc8d4890b29b3d0fb9426b34bc2b30 Commit: dc6efc1135fc8d4890b29b3d0fb9426b34bc2b30 Author: khorimoto@chromium.org Commiter: khorimoto@chromium.org Date: 2018-12-05 18:57:32 +0000 UTC [CrOS MultiDevice] Fix BLE scan result handling. Previously, we would attempt a BLE scan any time that we got a DeviceChanged() or DeviceAdded() callback. However, the DeviceChanged() callback was invoked when a device was in the process of disconnecting, meaning that we would see a disconnecting device, then try to start a connection immediately. This resulted in spurious connection requests which could sometime cause a deadlock in underlying Bluetooth code. Additionally, we would rely on the DeviceChanged() and DeviceRemoved() callbacks to listen for Bluetooth disconnections, which also resulted in error-prone disconnections. This CL updates both call sites to use the updated DeviceAdvertisementReceived() and DeviceConnectedStateChanged() callbacks instead. Bug: 898334 Change-Id: I743e51e6e47194649ab6852966b9d892c3ef334d Reviewed-on: https://chromium-review.googlesource.com/c/1359478 Commit-Queue: Kyle Horimoto <khorimoto@chromium.org> Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org> Cr-Commit-Position: refs/heads/master@{#613358} (cherry picked from commit af8d33b48b8a6d7685584a4682ae39cd848217a5) TBR: ortuno@chromium.org Change-Id: I743e51e6e47194649ab6852966b9d892c3ef334d Reviewed-on: https://chromium-review.googlesource.com/c/1363685 Reviewed-by: Kyle Horimoto <khorimoto@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#84} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} |
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by jlklein@chromium.org
, Oct 23