Commit adf1da58 authored by Scott Violet's avatar Scott Violet Committed by Commit Bot

Revert "bluetooth: Query for devices every second in TrayBluetoothHelper"

This reverts commit e45a6ef0.

Reason for revert: This triggers DCHECKs on device, so reverting. Here's the stack:

[13255:13255:1127/111259.548287:FATAL:tray_bluetooth_helper.cc(50)] Check failed: !timer_.IsRunning().
#0 0x5abde8c02cef base::debug::StackTrace::StackTrace()
#1 0x5abde8b586cb logging::LogMessage::~LogMessage()
#2 0x5abdeb50f0a5 ash::TrayBluetoothHelper::StartOrStopRefreshingDeviceList()
#3 0x5abde9b4ac48 device::BluetoothAdapter::NotifyAdapterPoweredChanged()
#4 0x5abde9b55a5f bluez::BluetoothAdapterBlueZ::SetAdapter()
#5 0x5abde9b7d3d5 bluez::BluetoothAdapterClientImpl::ObjectAdded()
#6 0x5abde9a471be dbus::ObjectManager::AddInterface()
#7 0x5abde9a46d2e dbus::ObjectManager::UpdateObject()
#8 0x5abde9a457fb dbus::ObjectManager::OnGetManagedObjects()
#9 0x5abde505efdc _ZN4base8internal7InvokerINS0_9BindStateIMN8chromeos14BiodClientImplEFvPN4dbus6SignalEEJNS_7WeakPtrIS4_EEEEEFvS7_EE3RunEPNS0_13BindStateBaseES7_
#10 0x5abde9a486bf dbus::ObjectProxy::OnCallMethod()
#11 0x5abde9a4cb43 _ZN4base8internal7InvokerINS0_9BindStateIMN4dbus11ObjectProxyEFvRKNSt3__112basic_stringIcNS5_11char_traitsIcEENS5_9allocatorIcEEEESD_NS_12OnceCallbackIFvPNS3_8ResponseEEEESG_PNS3_13ErrorResponseEEJ13scoped_refptrIS4_ESB_SB_SI_EEEFvSG_SK_EE7RunOnceEPNS0_13BindStateBaseESG_SK_
#12 0x5abde9a49074 dbus::ObjectProxy::RunResponseOrErrorCallback()
#13 0x5abde9a4db5e _ZN4base8internal13FunctorTraitsIMN4dbus11ObjectProxyEFvNS3_19ReplyCallbackHolderENS_9TimeTicksEPNS2_8ResponseEPNS2_13ErrorResponseEEvE6InvokeISB_13scoped_refptrIS3_EJS4_S5_S7_S9_EEEvT_OT0_DpOT1_
#14 0x5abde9a4da80 _ZN4base8internal7InvokerINS0_9BindStateIMN4dbus11ObjectProxyEFvNS4_19ReplyCallbackHolderENS_9TimeTicksEPNS3_8ResponseEPNS3_13ErrorResponseEEJ13scoped_refptrIS4_ES5_S6_S8_SA_EEEFvvEE7RunOnceEPNS0_13BindStateBaseE
#15 0x5abde8bd218b base::(anonymous namespace)::PostTaskAndReplyRelay::RunTaskAndPostReply()
#16 0x5abde8bd260b _ZN4base8internal7InvokerINS0_9BindStateIPFvNS_12_GLOBAL__N_121PostTaskAndReplyRelayEEJS4_EEEFvvEE7RunOnceEPNS0_13BindStateBaseE
#17 0x5abde8c28597 base::debug::TaskAnnotator::RunTask()
#18 0x5abde8b6242f base::MessageLoopImpl::RunTask()
#19 0x5abde8b62ae2 base::MessageLoopImpl::DoWork()
#20 0x5abde8c24189 base::MessagePumpLibevent::Run()
#21 0x5abde8b61f05 base::MessageLoopImpl::Run()
#22 0x5abde8b8c576 base::RunLoop::Run()
#23 0x5abde86a22e5 ChromeBrowserMainParts::MainMessageLoopRun()
#24 0x5abde5eebb04 content::BrowserMainLoop::RunMainMessageLoopParts()
#25 0x5abde5eee503 content::BrowserMainRunnerImpl::Run()
#26 0x5abde5ee828f content::BrowserMain()


Original change's description:
> bluetooth: Query for devices every second in TrayBluetoothHelper
> 
> Before TrayBluetoothHelper would get notified of any changes to devices.
> This caused problems in busy environments because TrayBluetoothHelper
> would get notified hundreds or even thousands of times per second of
> devices changes.
> 
> This CL changes to a "pull" pattern where TrayBluetootHelper queries
> for devices every second. The devices are then cached so that clients
> of TrayBluetoothHelper can access them synchronously.
> 
> TrayBluetoothHelper queries for devices as long as the state is
> kPoweredOn. If the state changes, the device list is cleared.
> 
> Bug: 882346
> 
> TEST=The following actions should be performed:
> 
> 1. Turn bluetooth on and off in system tray.
> 2. Open device list in system menu.
> 3. Connect to a device.
> 4. See that the pod feature button in the system tray indicates a
>    device is connected when connected to a device.
> 
> Change-Id: Id9028546a627f260527aa737f51c032f74a447ec
> Reviewed-on: https://chromium-review.googlesource.com/c/1347638
> Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
> Reviewed-by: Tetsui Ohkubo <tetsui@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#611069}

TBR=ortuno@chromium.org,tetsui@chromium.org

Change-Id: I5b5ca3f227ab7616321047b3a8ade17bfa30b29a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 882346
Reviewed-on: https://chromium-review.googlesource.com/c/1352340Reviewed-by: default avatarScott Violet <sky@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611246}
parent 59d9ba11
......@@ -99,7 +99,7 @@ void BluetoothFeaturePodController::UpdateButton() {
Shell::Get()->tray_bluetooth_helper()->GetAvailableBluetoothDevices()) {
if (device->connection_state ==
BluetoothDeviceInfo::ConnectionState::kConnected) {
connected_devices.push_back(device->Clone());
connected_devices.push_back(std::move(device));
}
}
......
......@@ -3,19 +3,11 @@
// found in the LICENSE file.
#include "ash/system/bluetooth/tray_bluetooth_helper.h"
#include "base/time/time.h"
using device::mojom::BluetoothSystem;
namespace ash {
namespace {
constexpr base::TimeDelta kUpdateFrequencyMs =
base::TimeDelta::FromMilliseconds(1000);
} // namespace
TrayBluetoothHelper::TrayBluetoothHelper() = default;
TrayBluetoothHelper::~TrayBluetoothHelper() = default;
......@@ -28,11 +20,6 @@ void TrayBluetoothHelper::RemoveObserver(Observer* observer) {
observers_.RemoveObserver(observer);
}
const BluetoothDeviceList& TrayBluetoothHelper::GetAvailableBluetoothDevices()
const {
return cached_devices_;
}
bool TrayBluetoothHelper::IsBluetoothStateAvailable() {
switch (GetBluetoothState()) {
case BluetoothSystem::State::kUnsupported:
......@@ -45,36 +32,6 @@ bool TrayBluetoothHelper::IsBluetoothStateAvailable() {
}
}
void TrayBluetoothHelper::StartOrStopRefreshingDeviceList() {
if (GetBluetoothState() == BluetoothSystem::State::kPoweredOn) {
DCHECK(!timer_.IsRunning());
UpdateDeviceCache();
timer_.Start(FROM_HERE, kUpdateFrequencyMs, this,
&TrayBluetoothHelper::UpdateDeviceCache);
return;
}
timer_.Stop();
cached_devices_.clear();
NotifyBluetoothDeviceListChanged();
}
void TrayBluetoothHelper::UpdateDeviceCache() {
GetBluetoothDevices(
base::BindOnce(&TrayBluetoothHelper::OnGetBluetoothDevices,
weak_ptr_factory_.GetWeakPtr()));
}
void TrayBluetoothHelper::OnGetBluetoothDevices(BluetoothDeviceList devices) {
cached_devices_ = std::move(devices);
NotifyBluetoothDeviceListChanged();
}
void TrayBluetoothHelper::NotifyBluetoothDeviceListChanged() {
for (auto& observer : observers_)
observer.OnBluetoothDeviceListChanged();
}
void TrayBluetoothHelper::NotifyBluetoothSystemStateChanged() {
for (auto& observer : observers_)
observer.OnBluetoothSystemStateChanged();
......@@ -85,4 +42,9 @@ void TrayBluetoothHelper::NotifyBluetoothScanStateChanged() {
observer.OnBluetoothScanStateChanged();
}
void TrayBluetoothHelper::NotifyBluetoothDeviceListChanged() {
for (auto& observer : observers_)
observer.OnBluetoothDeviceListChanged();
}
} // namespace ash
......@@ -24,7 +24,7 @@ using BluetoothDeviceList = std::vector<device::mojom::BluetoothDeviceInfoPtr>;
// This is a temporary virtual class used during the migration to the new
// BluetoothSystem Mojo interface. Once the migration is over, we'll
// de-virtualize this class and remove its legacy implementation.
class ASH_EXPORT TrayBluetoothHelper {
class TrayBluetoothHelper {
public:
class Observer : public base::CheckedObserver {
public:
......@@ -48,7 +48,7 @@ class ASH_EXPORT TrayBluetoothHelper {
virtual void Initialize() = 0;
// Returns a list of available bluetooth devices.
const BluetoothDeviceList& GetAvailableBluetoothDevices() const;
virtual BluetoothDeviceList GetAvailableBluetoothDevices() const = 0;
// Requests bluetooth start discovering devices, which happens asynchronously.
virtual void StartBluetoothDiscovering() = 0;
......@@ -75,40 +75,13 @@ class ASH_EXPORT TrayBluetoothHelper {
virtual bool HasBluetoothDiscoverySession() = 0;
protected:
using GetBluetoothDevicesCallback =
base::OnceCallback<void(BluetoothDeviceList)>;
// Using a "push" pattern where the underlying API notifies of device changes
// is undesireable because there are hundreds or sometimes thousands of
// changes per second. This could result in significantly slowing down the UI.
// To avoid this we use a pull pattern where we retrieve the device list every
// second and notify observers.
//
// Implementations of TrayBluetoothHelper should call this whenever the state
// changes.
void StartOrStopRefreshingDeviceList();
void NotifyBluetoothSystemStateChanged();
void NotifyBluetoothScanStateChanged();
virtual void GetBluetoothDevices(
GetBluetoothDevicesCallback callback) const = 0;
void NotifyBluetoothDeviceListChanged();
base::ObserverList<Observer> observers_;
private:
void UpdateDeviceCache();
void OnGetBluetoothDevices(BluetoothDeviceList devices);
void NotifyBluetoothDeviceListChanged();
// List of cached devices. Updated every second.
BluetoothDeviceList cached_devices_;
// Timer used to update |cached_devices_|.
base::RepeatingTimer timer_;
base::WeakPtrFactory<TrayBluetoothHelper> weak_ptr_factory_{this};
DISALLOW_COPY_AND_ASSIGN(TrayBluetoothHelper);
};
......
......@@ -48,6 +48,12 @@ void TrayBluetoothHelperExperimental::Initialize() {
base::Unretained(this)));
}
BluetoothDeviceList
TrayBluetoothHelperExperimental::GetAvailableBluetoothDevices() const {
NOTIMPLEMENTED();
return BluetoothDeviceList();
}
void TrayBluetoothHelperExperimental::StartBluetoothDiscovering() {
bluetooth_system_ptr_->StartScan(base::DoNothing());
}
......@@ -75,19 +81,10 @@ bool TrayBluetoothHelperExperimental::HasBluetoothDiscoverySession() {
device::mojom::BluetoothSystem::ScanState::kScanning;
}
void TrayBluetoothHelperExperimental::GetBluetoothDevices(
GetBluetoothDevicesCallback callback) const {
NOTIMPLEMENTED();
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(std::move(callback), BluetoothDeviceList()));
}
void TrayBluetoothHelperExperimental::OnStateChanged(
device::mojom::BluetoothSystem::State state) {
cached_state_ = state;
NotifyBluetoothSystemStateChanged();
StartOrStopRefreshingDeviceList();
}
void TrayBluetoothHelperExperimental::OnScanStateChanged(
......
......@@ -31,13 +31,13 @@ class TrayBluetoothHelperExperimental
// TrayBluetoothHelper:
void Initialize() override;
BluetoothDeviceList GetAvailableBluetoothDevices() const override;
void StartBluetoothDiscovering() override;
void StopBluetoothDiscovering() override;
void ConnectToBluetoothDevice(const std::string& address) override;
device::mojom::BluetoothSystem::State GetBluetoothState() override;
void SetBluetoothEnabled(bool enabled) override;
bool HasBluetoothDiscoverySession() override;
void GetBluetoothDevices(GetBluetoothDevicesCallback callback) const override;
// device::mojom::BluetoothSystemClient
void OnStateChanged(device::mojom::BluetoothSystem::State state) override;
......
......@@ -114,8 +114,6 @@ void TrayBluetoothHelperLegacy::InitializeOnAdapterReady(
adapter_ = adapter;
CHECK(adapter_);
adapter_->AddObserver(this);
StartOrStopRefreshingDeviceList();
}
void TrayBluetoothHelperLegacy::Initialize() {
......@@ -124,6 +122,20 @@ void TrayBluetoothHelperLegacy::Initialize() {
weak_ptr_factory_.GetWeakPtr()));
}
BluetoothDeviceList TrayBluetoothHelperLegacy::GetAvailableBluetoothDevices()
const {
device::BluetoothAdapter::DeviceList devices =
device::FilterBluetoothDeviceList(adapter_->GetDevices(),
device::BluetoothFilterType::KNOWN,
kMaximumDevicesShown);
BluetoothDeviceList device_list;
for (device::BluetoothDevice* device : devices)
device_list.push_back(GetBluetoothDeviceInfo(device));
return device_list;
}
void TrayBluetoothHelperLegacy::StartBluetoothDiscovering() {
if (HasBluetoothDiscoverySession()) {
LOG(WARNING) << "Already have active Bluetooth device discovery session.";
......@@ -197,20 +209,6 @@ bool TrayBluetoothHelperLegacy::HasBluetoothDiscoverySession() {
return discovery_session_ && discovery_session_->IsActive();
}
void TrayBluetoothHelperLegacy::GetBluetoothDevices(
GetBluetoothDevicesCallback callback) const {
BluetoothDeviceList device_list;
device::BluetoothAdapter::DeviceList devices =
device::FilterBluetoothDeviceList(adapter_->GetDevices(),
device::BluetoothFilterType::KNOWN,
kMaximumDevicesShown);
for (device::BluetoothDevice* device : devices)
device_list.push_back(GetBluetoothDeviceInfo(device));
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(std::move(callback), std::move(device_list)));
}
////////////////////////////////////////////////////////////////////////////////
// BluetoothAdapter::Observer:
......@@ -218,14 +216,12 @@ void TrayBluetoothHelperLegacy::AdapterPresentChanged(
device::BluetoothAdapter* adapter,
bool present) {
NotifyBluetoothSystemStateChanged();
StartOrStopRefreshingDeviceList();
}
void TrayBluetoothHelperLegacy::AdapterPoweredChanged(
device::BluetoothAdapter* adapter,
bool powered) {
NotifyBluetoothSystemStateChanged();
StartOrStopRefreshingDeviceList();
}
void TrayBluetoothHelperLegacy::AdapterDiscoveringChanged(
......@@ -234,6 +230,21 @@ void TrayBluetoothHelperLegacy::AdapterDiscoveringChanged(
NotifyBluetoothScanStateChanged();
}
void TrayBluetoothHelperLegacy::DeviceAdded(device::BluetoothAdapter* adapter,
device::BluetoothDevice* device) {
NotifyBluetoothDeviceListChanged();
}
void TrayBluetoothHelperLegacy::DeviceChanged(device::BluetoothAdapter* adapter,
device::BluetoothDevice* device) {
NotifyBluetoothDeviceListChanged();
}
void TrayBluetoothHelperLegacy::DeviceRemoved(device::BluetoothAdapter* adapter,
device::BluetoothDevice* device) {
NotifyBluetoothDeviceListChanged();
}
void TrayBluetoothHelperLegacy::OnStartDiscoverySession(
std::unique_ptr<device::BluetoothDiscoverySession> discovery_session) {
// If the discovery session was returned after a request to stop discovery
......
......@@ -38,13 +38,13 @@ class ASH_EXPORT TrayBluetoothHelperLegacy
// TrayBluetoothHelper:
void Initialize() override;
BluetoothDeviceList GetAvailableBluetoothDevices() const override;
void StartBluetoothDiscovering() override;
void StopBluetoothDiscovering() override;
void ConnectToBluetoothDevice(const std::string& address) override;
device::mojom::BluetoothSystem::State GetBluetoothState() override;
void SetBluetoothEnabled(bool enabled) override;
bool HasBluetoothDiscoverySession() override;
void GetBluetoothDevices(GetBluetoothDevicesCallback callback) const override;
// BluetoothAdapter::Observer:
void AdapterPresentChanged(device::BluetoothAdapter* adapter,
......@@ -53,6 +53,12 @@ class ASH_EXPORT TrayBluetoothHelperLegacy
bool powered) override;
void AdapterDiscoveringChanged(device::BluetoothAdapter* adapter,
bool discovering) override;
void DeviceAdded(device::BluetoothAdapter* adapter,
device::BluetoothDevice* device) override;
void DeviceChanged(device::BluetoothAdapter* adapter,
device::BluetoothDevice* device) override;
void DeviceRemoved(device::BluetoothAdapter* adapter,
device::BluetoothDevice* device) override;
private:
void OnStartDiscoverySession(
......
......@@ -7,7 +7,6 @@
#include <string>
#include <vector>
#include "ash/system/bluetooth/tray_bluetooth_helper.h"
#include "ash/test/ash_test_base.h"
#include "base/test/scoped_feature_list.h"
#include "dbus/object_path.h"
......@@ -44,10 +43,6 @@ TEST_F(TrayBluetoothHelperLegacyTest, Basics) {
static_cast<FakeBluetoothAdapterClient*>(
BluezDBusManager::Get()->GetBluetoothAdapterClient());
adapter_client->SetSimulationIntervalMs(0);
adapter_client
->GetProperties(
dbus::ObjectPath(bluez::FakeBluetoothAdapterClient::kAdapterPath))
->powered.ReplaceValue(true);
FakeBluetoothDeviceClient* device_client =
static_cast<FakeBluetoothDeviceClient*>(
......@@ -64,11 +59,11 @@ TEST_F(TrayBluetoothHelperLegacyTest, Basics) {
TrayBluetoothHelperLegacy helper;
helper.Initialize();
RunAllPendingInMessageLoop();
EXPECT_EQ(device::mojom::BluetoothSystem::State::kPoweredOn,
EXPECT_EQ(device::mojom::BluetoothSystem::State::kPoweredOff,
helper.GetBluetoothState());
EXPECT_FALSE(helper.HasBluetoothDiscoverySession());
const BluetoothDeviceList& devices = helper.GetAvailableBluetoothDevices();
BluetoothDeviceList devices = helper.GetAvailableBluetoothDevices();
// The devices are fake in tests, so don't assume any particular number.
EXPECT_FALSE(devices.empty());
EXPECT_TRUE(ExistInFilteredDevices(
......@@ -139,10 +134,6 @@ TEST_F(TrayBluetoothHelperLegacyTest, UnfilteredBluetoothDevices) {
static_cast<FakeBluetoothAdapterClient*>(
BluezDBusManager::Get()->GetBluetoothAdapterClient());
adapter_client->SetSimulationIntervalMs(0);
adapter_client
->GetProperties(
dbus::ObjectPath(bluez::FakeBluetoothAdapterClient::kAdapterPath))
->powered.ReplaceValue(true);
FakeBluetoothDeviceClient* device_client =
static_cast<FakeBluetoothDeviceClient*>(
......@@ -159,7 +150,7 @@ TEST_F(TrayBluetoothHelperLegacyTest, UnfilteredBluetoothDevices) {
helper.Initialize();
base::RunLoop().RunUntilIdle();
const BluetoothDeviceList& devices = helper.GetAvailableBluetoothDevices();
BluetoothDeviceList devices = helper.GetAvailableBluetoothDevices();
// The devices are fake in tests, so don't assume any particular number.
EXPECT_TRUE(ExistInFilteredDevices(
FakeBluetoothDeviceClient::kDisplayPinCodeAddress, devices));
......
......@@ -22,6 +22,8 @@ using device::mojom::BluetoothDeviceInfoPtr;
namespace ash {
const int kUpdateFrequencyMs = 1000;
namespace {
// Updates bluetooth device |device| in the |list|. If it is new, append to the
......@@ -89,17 +91,27 @@ void UnifiedBluetoothDetailedViewController::OnBluetoothSystemStateChanged() {
if (bluetooth_state == BluetoothSystem::State::kPoweredOn) {
// If Bluetooth was just turned on, start discovering.
Shell::Get()->tray_bluetooth_helper()->StartBluetoothDiscovering();
} else {
// Otherwise stop updating the list of devices.
timer_.Stop();
}
UpdateDeviceListAndUI();
}
void UnifiedBluetoothDetailedViewController::OnBluetoothScanStateChanged() {
// To avoid delaying showing devices, update the device list and UI
// immediately.
UpdateDeviceListAndUI();
}
void UnifiedBluetoothDetailedViewController::OnBluetoothDeviceListChanged() {
UpdateDeviceListAndUI();
if (Shell::Get()->tray_bluetooth_helper()->HasBluetoothDiscoverySession()) {
// Update the device list and UI every |kUpdateFrequencyMs|.
timer_.Start(
FROM_HERE, base::TimeDelta::FromMilliseconds(kUpdateFrequencyMs), this,
&UnifiedBluetoothDetailedViewController::UpdateDeviceListAndUI);
return;
}
timer_.Stop();
}
void UnifiedBluetoothDetailedViewController::UpdateDeviceListAndUI() {
......@@ -139,28 +151,25 @@ void UnifiedBluetoothDetailedViewController::UpdateBluetoothDeviceList() {
std::set<std::string> new_paired_not_connected_devices;
std::set<std::string> new_discovered_not_paired_devices;
for (const auto& device :
for (auto& device :
Shell::Get()->tray_bluetooth_helper()->GetAvailableBluetoothDevices()) {
auto device_clone = device->Clone();
if (device->connection_state ==
BluetoothDeviceInfo::ConnectionState::kConnecting) {
new_connecting_devices.insert(device->address);
UpdateBluetoothDeviceListHelper(&connecting_devices_,
std::move(device_clone));
UpdateBluetoothDeviceListHelper(&connecting_devices_, std::move(device));
} else if (device->connection_state ==
BluetoothDeviceInfo::ConnectionState::kConnected &&
device->is_paired) {
new_connected_devices.insert(device->address);
UpdateBluetoothDeviceListHelper(&connected_devices_,
std::move(device_clone));
UpdateBluetoothDeviceListHelper(&connected_devices_, std::move(device));
} else if (device->is_paired) {
new_paired_not_connected_devices.insert(device->address);
UpdateBluetoothDeviceListHelper(&paired_not_connected_devices_,
std::move(device_clone));
std::move(device));
} else {
new_discovered_not_paired_devices.insert(device->address);
UpdateBluetoothDeviceListHelper(&discovered_not_paired_devices_,
std::move(device_clone));
std::move(device));
}
}
RemoveObsoleteBluetoothDevicesFromList(&connecting_devices_,
......
......@@ -36,7 +36,6 @@ class UnifiedBluetoothDetailedViewController
// BluetoothObserver:
void OnBluetoothSystemStateChanged() override;
void OnBluetoothScanStateChanged() override;
void OnBluetoothDeviceListChanged() override;
private:
void UpdateDeviceListAndUI();
......@@ -51,6 +50,9 @@ class UnifiedBluetoothDetailedViewController
BluetoothDeviceList paired_not_connected_devices_;
BluetoothDeviceList discovered_not_paired_devices_;
// Timer used to limit the update frequency.
base::RepeatingTimer timer_;
DISALLOW_COPY_AND_ASSIGN(UnifiedBluetoothDetailedViewController);
};
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment