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

Issue 777610 link

Starred by 0 users
Status: Fixed
Owner:
khorimoto@chromium.org
Closed: Dec 2017
Cc:
█ jlklein@chromium.org
khorimoto@chromium.org
jhawkins@chromium.org
hansberry@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Devices can get stuck in advertisement queue if fields are updated

Project Member Reported by khorimoto@chromium.org, Oct 23 2017 
Repro:
(1) Start trying to connect to a device - for example, by performing a host scan.
(2) Trigger some sort of update to the back-end. For example, re-register a device such that its "last updated timestamp" field changes its value.

Expected: The device connection completes and is cleaned up correctly.

Actual: The changed field results in the two RemoteDevice objects not being equal. Thus, the bookkeeping in BleConnectionManager, BleScanner, and BleAdvertisementDeviceQueue gets out of sync since there could be two entries corresponding to the same RemoteDevice. Connections will continue until the TetherComponent is destroyed.

Comment 1 by khorimoto@chromium.org, Oct 25 2017 

I have an in-progress CL here: https://chromium-review.googlesource.com/c/chromium/src/+/738467.

I'll be OOO for the next 10 days, so I won't be able to complete this before I return.

Comment 2 by khorimoto@chromium.org, Oct 25 2017

Status: Started (was: Assigned)

Comment 3 by khorimoto@chromium.org, Nov 14 2017

Labels: -M-63 M-64
I'm moving this issue to M-64. The CL is fairly complex, and I'm not comfortable with merging it into M-63 this late in the game, especially since the issue is rare and recoverable when it occurs.
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 6 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cdfd4cb7c2b8974abee8bf4b443f597050599815

commit cdfd4cb7c2b8974abee8bf4b443f597050599815
Author: Kyle Horimoto <khorimoto@google.com>
Date: Wed Dec 06 00:47:55 2017

[CrOS Tether] Utilize RemoteDeviceCapability within TetherHostFetcher.

This change makes it possible to return RemoteDevice objects
synchronously instead of requiring an IPC call to create them. This
change is needed to support [1], since that CL requires that a
BluetoothDevice object be used immediately (i.e., synchronously),
requiring that a RemoteDevice object is also available synchronously.

This CL also moves ownership of TetherHostFetcher from TetherComponent
to TetherService, ensuring that RemoteDevice objects necessary for
TetherComponent are generated before the component is started up.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/738467

Bug:  777610 , 672263
Change-Id: I3c74af3cb8964d32e3bd30298849af1e2477c77a
Reviewed-on: https://chromium-review.googlesource.com/804395
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Reviewed-by: Jeremy Klein <jlklein@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521922}
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chrome/browser/chromeos/tether/tether_service.cc
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chrome/browser/chromeos/tether/tether_service.h
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chrome/browser/chromeos/tether/tether_service_unittest.cc
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chrome/browser/cryptauth/chrome_cryptauth_service.h
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chromeos/components/tether/BUILD.gn
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chromeos/components/tether/active_host_unittest.cc
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chromeos/components/tether/asynchronous_shutdown_object_container_impl.cc
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chromeos/components/tether/asynchronous_shutdown_object_container_impl.h
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chromeos/components/tether/asynchronous_shutdown_object_container_impl_unittest.cc
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chromeos/components/tether/disconnect_tethering_request_sender_impl_unittest.cc
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chromeos/components/tether/fake_tether_host_fetcher.cc
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chromeos/components/tether/fake_tether_host_fetcher.h
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chromeos/components/tether/host_scanner.cc
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chromeos/components/tether/host_scanner_unittest.cc
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chromeos/components/tether/tether_component_impl.cc
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chromeos/components/tether/tether_component_impl.h
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chromeos/components/tether/tether_component_impl_unittest.cc
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chromeos/components/tether/tether_connector_impl_unittest.cc
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chromeos/components/tether/tether_host_fetcher.cc
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chromeos/components/tether/tether_host_fetcher.h
[add] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chromeos/components/tether/tether_host_fetcher_impl.cc
[add] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chromeos/components/tether/tether_host_fetcher_impl.h
[add] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/chromeos/components/tether/tether_host_fetcher_impl_unittest.cc
[delete] https://crrev.com/5ab0c368f8ae52fdb29b337e889f1728897526ac/chromeos/components/tether/tether_host_fetcher_unittest.cc
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/components/cryptauth/cryptauth_service.h
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/components/cryptauth/fake_remote_device_provider.cc
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/components/cryptauth/fake_remote_device_provider.h
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/components/cryptauth/remote_device_provider.cc
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/components/cryptauth/remote_device_provider.h
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/components/cryptauth/remote_device_provider_impl.cc
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/components/cryptauth/remote_device_provider_impl.h
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/components/cryptauth/remote_device_provider_impl_unittest.cc
[modify] https://crrev.com/cdfd4cb7c2b8974abee8bf4b443f597050599815/components/cryptauth/secure_message_delegate.h
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 6 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/957fdda58a383af3e093286d6345b725d5b8588b

commit 957fdda58a383af3e093286d6345b725d5b8588b
Author: Kyle Horimoto <khorimoto@google.com>
Date: Wed Dec 06 03:03:47 2017

[CrOS Tether] Refactor BLE code to key by device ID.

Previously, much of the BLE code was keyed by RemoteDevice instead. This
can cause subtle race conditions; for example:
  (1) Register remote device A.
  (2) CryptAuth sync occurs; device A is updated.
  (3) Register remote device A again. Since device fields have changed,
      this would be considered a different device.
  (4) Multiple connections are attempted to the same device.

These issues are recoverable, since the MessageTransferOperation which
registered the original device (before the update) will eventually
finish and unregister the device; however, during this time period,
connections are prone to errors.

Bug:  777610 , 672263
Change-Id: I9e6ed9c2ad69cb318863268f041db7b5f5ec3bd5
Reviewed-on: https://chromium-review.googlesource.com/738467
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Jeremy Klein <jlklein@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521974}
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/ad_hoc_ble_advertiser.h
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/ad_hoc_ble_advertiser_impl.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/ad_hoc_ble_advertiser_impl.h
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/ad_hoc_ble_advertiser_impl_unittest.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/asynchronous_shutdown_object_container_impl.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/ble_advertisement_device_queue.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/ble_advertisement_device_queue.h
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/ble_advertisement_device_queue_unittest.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/ble_advertiser.h
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/ble_advertiser_impl.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/ble_advertiser_impl.h
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/ble_advertiser_impl_unittest.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/ble_connection_manager.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/ble_connection_manager.h
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/ble_connection_manager_unittest.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/ble_scanner.h
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/ble_scanner_impl.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/ble_scanner_impl.h
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/ble_scanner_impl_unittest.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/connect_tethering_operation_unittest.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/disconnect_tethering_operation_unittest.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/fake_ad_hoc_ble_advertiser.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/fake_ad_hoc_ble_advertiser.h
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/fake_ble_advertiser.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/fake_ble_advertiser.h
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/fake_ble_connection_manager.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/fake_ble_connection_manager.h
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/fake_ble_scanner.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/fake_ble_scanner.h
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/host_scanner_operation_unittest.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/keep_alive_operation_unittest.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/message_transfer_operation.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/message_transfer_operation.h
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/chromeos/components/tether/message_transfer_operation_unittest.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/components/cryptauth/ble/ble_advertisement_generator.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/components/cryptauth/ble/ble_advertisement_generator.h
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/components/cryptauth/ble/ble_advertisement_generator_unittest.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/components/cryptauth/ble/fake_ble_advertisement_generator.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/components/cryptauth/ble/fake_ble_advertisement_generator.h
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/components/cryptauth/foreground_eid_generator.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/components/cryptauth/foreground_eid_generator.h
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/components/cryptauth/foreground_eid_generator_unittest.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/components/cryptauth/mock_foreground_eid_generator.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/components/cryptauth/mock_foreground_eid_generator.h
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/components/cryptauth/mock_remote_beacon_seed_fetcher.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/components/cryptauth/mock_remote_beacon_seed_fetcher.h
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/components/cryptauth/remote_beacon_seed_fetcher.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/components/cryptauth/remote_beacon_seed_fetcher.h
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/components/cryptauth/remote_beacon_seed_fetcher_unittest.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/components/cryptauth/remote_device.cc
[modify] https://crrev.com/957fdda58a383af3e093286d6345b725d5b8588b/components/cryptauth/remote_device.h

Comment 6 by khorimoto@chromium.org, Dec 6 2017

Labels: -M-64 M-65
Status: Fixed (was: Started)
Closing the issue, but re-targeting it to M-65. After consulting with jlklein@, we feel that this issue is too risky to merge to M-64 since it touches so many files.
►

Sign in to add a comment