Skip to content

Commit

Permalink
[CrOS Cellular] Add wait for inhibit property change.
Browse files Browse the repository at this point in the history 
This CL adds a wait for inhibit property to change before
returning inhibit lock from CellularInhbitor class. This makes
Chrome inhibit logic more robust against cases where Modem
Manager may still be busy inhibiting when an uninhbit call
is made.

Bug: 1093185, b/172077101
Change-Id: I92bbc6a8fda011bcf47c3031379d915203e367fb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2792914
Commit-Queue: Azeem Arshad <azeemarshad@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#869033}
  • Loading branch information
Azeem Arshad authored and Chromium LUCI CQ committed Apr 3, 2021
1 parent d64168a commit c590260
Show file tree
Hide file tree
Showing 6 changed files with 160 additions and 36 deletions.
22 changes: 22 additions & 0 deletions chromeos/dbus/shill/fake_shill_device_client.cc
View file
Expand Up @@ -13,6 +13,7 @@
#include "base/location.h"
#include "base/logging.h"
#include "base/memory/ptr_util.h"
#include "base/optional.h"
#include "base/single_thread_task_runner.h"
#include "base/strings/stringprintf.h"
#include "base/threading/thread_task_runner_handle.h"
Expand Down Expand Up @@ -93,6 +94,22 @@ void FakeShillDeviceClient::SetProperty(const dbus::ObjectPath& device_path,
const base::Value& value,
base::OnceClosure callback,
ErrorCallback error_callback) {
if (property_change_delay_.has_value()) {
// Return callback immediately and set property after delay.
std::move(callback).Run();
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
FROM_HERE,
base::BindOnce(
&FakeShillDeviceClient::SetPropertyInternal,
weak_ptr_factory_.GetWeakPtr(), device_path, name, value.Clone(),
/*callback=*/base::DoNothing::Once<>(),
/*error_callback=*/
base::DoNothing::Once<const std::string&, const std::string&>(),
/*notify_changed=*/true),
*property_change_delay_);
return;
}

SetPropertyInternal(device_path, name, value, std::move(callback),
std::move(error_callback),
/*notify_changed=*/true);
Expand Down Expand Up @@ -505,6 +522,11 @@ void FakeShillDeviceClient::SetSimulateUninhibitScanning(
simulate_uninhibit_scanning_ = simulate_uninhibit_scanning;
}

void FakeShillDeviceClient::SetPropertyChangeDelay(
base::Optional<base::TimeDelta> time_delay) {
property_change_delay_ = time_delay;
}

// Private Methods -------------------------------------------------------------

FakeShillDeviceClient::SimLockStatus FakeShillDeviceClient::GetSimLockStatus(
Expand Down
6 changes: 6 additions & 0 deletions chromeos/dbus/shill/fake_shill_device_client.h
View file
Expand Up @@ -113,6 +113,8 @@ class COMPONENT_EXPORT(SHILL_CLIENT) FakeShillDeviceClient
const std::string& device_path,
const std::string& error_name) override;
void SetSimulateUninhibitScanning(bool simulate_uninhibit_scanning) override;
void SetPropertyChangeDelay(
base::Optional<base::TimeDelta> time_delay) override;

static const char kSimPuk[];
static const char kDefaultSimPin[];
Expand Down Expand Up @@ -185,6 +187,10 @@ class COMPONENT_EXPORT(SHILL_CLIENT) FakeShillDeviceClient
// during normal operation.
bool simulate_uninhibit_scanning_ = true;

// When set, causes SetProperty call to return immediately and delay the value
// change by given amount.
base::Optional<base::TimeDelta> property_change_delay_;

// Note: This should remain the last member so it'll be destroyed and
// invalidate its weak pointers before any other members are destroyed.
base::WeakPtrFactory<FakeShillDeviceClient> weak_ptr_factory_{this};
Expand Down
4 changes: 4 additions & 0 deletions chromeos/dbus/shill/shill_device_client.h
View file
Expand Up @@ -67,6 +67,10 @@ class COMPONENT_EXPORT(SHILL_CLIENT) ShillDeviceClient {
// an Inhibit operation is complete.
virtual void SetSimulateUninhibitScanning(
bool simulate_uninhibit_scanning) = 0;
// Adds a delay before a SetProperty call will result in property value
// change.
virtual void SetPropertyChangeDelay(
base::Optional<base::TimeDelta> time_delay) = 0;

protected:
virtual ~TestInterface() {}
Expand Down
105 changes: 78 additions & 27 deletions chromeos/network/cellular_inhibitor.cc
View file
Expand Up @@ -10,6 +10,7 @@
#include "base/bind.h"
#include "base/callback_helpers.h"
#include "base/threading/thread_task_runner_handle.h"
#include "base/time/time.h"
#include "chromeos/network/device_state.h"
#include "chromeos/network/network_device_handler.h"
#include "chromeos/network/network_state_handler.h"
Expand Down Expand Up @@ -43,6 +44,10 @@ CellularInhibitor::InhibitLock::~InhibitLock() {
std::move(unlock_callback_).Run();
}

// static
const base::TimeDelta CellularInhibitor::kInhibitPropertyChangeTimeout =
base::TimeDelta::FromSeconds(5);

CellularInhibitor::CellularInhibitor() = default;

CellularInhibitor::~CellularInhibitor() {
Expand Down Expand Up @@ -96,6 +101,7 @@ void CellularInhibitor::DeviceListChanged() {

void CellularInhibitor::DevicePropertiesUpdated(const DeviceState* device) {
CheckScanningIfNeeded();
CheckInhibitPropertyIfNeeded();
}

const DeviceState* CellularInhibitor::GetCellularDevice() const {
Expand Down Expand Up @@ -133,20 +139,19 @@ void CellularInhibitor::ProcessRequests() {
if (state_ != State::kIdle)
return;

uninhibit_attempts_so_far_ = 0;
TransitionToState(State::kInhibiting);
SetInhibitProperty(/*new_inhibit_value=*/true,
base::BindOnce(&CellularInhibitor::OnInhibit,
weak_ptr_factory_.GetWeakPtr()));
SetInhibitProperty();
}

void CellularInhibitor::OnInhibit(bool success) {
DCHECK(state_ == State::kInhibiting);
DCHECK(state_ == State::kWaitForInhibit || state_ == State::kInhibiting);

if (success) {
TransitionToState(State::kInhibited);
std::unique_ptr<InhibitLock> lock = std::make_unique<InhibitLock>(
base::BindOnce(&CellularInhibitor::AttemptUninhibit,
weak_ptr_factory_.GetWeakPtr(), /*attempts_so_far=*/0));
weak_ptr_factory_.GetWeakPtr()));
std::move(inhibit_requests_.front()->inhibit_callback).Run(std::move(lock));
return;
}
Expand All @@ -155,27 +160,26 @@ void CellularInhibitor::OnInhibit(bool success) {
PopRequestAndProcessNext();
}

void CellularInhibitor::AttemptUninhibit(size_t attempts_so_far) {
void CellularInhibitor::AttemptUninhibit() {
DCHECK(state_ == State::kInhibited);
TransitionToState(State::kUninhibiting);
SetInhibitProperty(
/*new_inhibit_value=*/false,
base::BindOnce(&CellularInhibitor::OnUninhibit,
weak_ptr_factory_.GetWeakPtr(), attempts_so_far));
SetInhibitProperty();
}

void CellularInhibitor::OnUninhibit(size_t attempts_so_far, bool success) {
DCHECK(state_ == State::kUninhibiting);
void CellularInhibitor::OnUninhibit(bool success) {
DCHECK(state_ == State::kWaitForUninhibit || state_ == State::kUninhibiting);

if (!success) {
base::TimeDelta retry_delay = kUninhibitRetryDelay * (1 << attempts_so_far);
base::TimeDelta retry_delay =
kUninhibitRetryDelay * (1 << uninhibit_attempts_so_far_);
NET_LOG(DEBUG) << "Uninhibit Failed. Retrying in " << retry_delay;
TransitionToState(State::kInhibited);
uninhibit_attempts_so_far_++;

base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
FROM_HERE,
base::BindOnce(&CellularInhibitor::AttemptUninhibit,
weak_ptr_factory_.GetWeakPtr(), attempts_so_far + 1),
weak_ptr_factory_.GetWeakPtr()),
retry_delay);
return;
}
Expand Down Expand Up @@ -231,45 +235,86 @@ void CellularInhibitor::PopRequestAndProcessNext() {
ProcessRequests();
}

void CellularInhibitor::SetInhibitProperty(bool new_inhibit_value,
SuccessCallback callback) {
void CellularInhibitor::SetInhibitProperty() {
DCHECK(state_ == State::kInhibiting || state_ == State::kUninhibiting);
const DeviceState* cellular_device = GetCellularDevice();
if (!cellular_device) {
std::move(callback).Run(false);
ReturnSetInhibitPropertyResult(/*success=*/false);
return;
}

bool new_inhibit_value = state_ == State::kInhibiting;

// If the new value is already set, return early.
if (cellular_device->inhibited() == new_inhibit_value) {
std::move(callback).Run(true);
ReturnSetInhibitPropertyResult(/*success=*/true);
return;
}

auto repeating_callback =
base::AdaptCallbackForRepeating(std::move(callback));
network_device_handler_->SetDeviceProperty(
cellular_device->path(), shill::kInhibitedProperty,
base::Value(new_inhibit_value),
base::BindOnce(&CellularInhibitor::OnSetPropertySuccess,
weak_ptr_factory_.GetWeakPtr(), repeating_callback),
weak_ptr_factory_.GetWeakPtr()),
base::BindOnce(&CellularInhibitor::OnSetPropertyError,
weak_ptr_factory_.GetWeakPtr(), repeating_callback,
weak_ptr_factory_.GetWeakPtr(),
/*attempted_inhibit=*/new_inhibit_value));
}

void CellularInhibitor::OnSetPropertySuccess(
const base::RepeatingCallback<void(bool)>& success_callback) {
std::move(success_callback).Run(true);
void CellularInhibitor::OnSetPropertySuccess() {
if (state_ == State::kInhibiting) {
TransitionToState(State::kWaitForInhibit);
} else if (state_ == State::kUninhibiting) {
TransitionToState(State::kWaitForUninhibit);
}
set_inhibit_timer_.Start(
FROM_HERE, kInhibitPropertyChangeTimeout,
base::BindOnce(&CellularInhibitor::OnInhibitPropertyChangeTimeout,
weak_ptr_factory_.GetWeakPtr()));
CheckInhibitPropertyIfNeeded();
}

void CellularInhibitor::OnSetPropertyError(
const base::RepeatingCallback<void(bool)>& success_callback,
bool attempted_inhibit,
const std::string& error_name,
std::unique_ptr<base::DictionaryValue> error_data) {
NET_LOG(ERROR) << (attempted_inhibit ? "Inhibit" : "Uninhibit")
<< "CellularScanning() failed: " << error_name;
std::move(success_callback).Run(false);
ReturnSetInhibitPropertyResult(/*success=*/false);
}

void CellularInhibitor::ReturnSetInhibitPropertyResult(bool success) {
set_inhibit_timer_.Stop();
if (state_ == State::kInhibiting || state_ == State::kWaitForInhibit) {
OnInhibit(success);
return;
}
if (state_ == State::kUninhibiting || state_ == State::kWaitForUninhibit) {
OnUninhibit(success);
}
}

void CellularInhibitor::CheckInhibitPropertyIfNeeded() {
if (state_ != State::kWaitForInhibit && state_ != State::kWaitForUninhibit)
return;

const DeviceState* cellular_device = GetCellularDevice();
if (!cellular_device)
return;

if (state_ == State::kWaitForInhibit && !cellular_device->inhibited())
return;

if (state_ == State::kWaitForUninhibit && cellular_device->inhibited())
return;

ReturnSetInhibitPropertyResult(/*success=*/true);
}

void CellularInhibitor::OnInhibitPropertyChangeTimeout() {
NET_LOG(EVENT) << "Timeout waiting for inhibit property change state_"
<< state_;
ReturnSetInhibitPropertyResult(/*success=*/false);
}

std::ostream& operator<<(std::ostream& stream,
Expand All @@ -281,12 +326,18 @@ std::ostream& operator<<(std::ostream& stream,
case CellularInhibitor::State::kInhibiting:
stream << "[Inhibiting]";
break;
case CellularInhibitor::State::kWaitForInhibit:
stream << "[Waiting for Inhibit property set]";
break;
case CellularInhibitor::State::kInhibited:
stream << "[Inhibited]";
break;
case CellularInhibitor::State::kUninhibiting:
stream << "[Uninhibiting]";
break;
case CellularInhibitor::State::kWaitForUninhibit:
stream << "[Waiting for Inhibit property clear]";
break;
case CellularInhibitor::State::kWaitingForScanningToStart:
stream << "[Waiting for scanning to start]";
break;
Expand Down
27 changes: 20 additions & 7 deletions chromeos/network/cellular_inhibitor.h
View file
Expand Up @@ -10,6 +10,7 @@
#include "base/memory/weak_ptr.h"
#include "base/observer_list.h"
#include "base/observer_list_types.h"
#include "base/timer/timer.h"
#include "chromeos/network/network_handler_callbacks.h"
#include "chromeos/network/network_state_handler_observer.h"

Expand Down Expand Up @@ -91,6 +92,10 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) CellularInhibitor
void NotifyInhibitStateChanged();

private:
// Timeout after which an inhibit property change is considered to be failed.
static const base::TimeDelta kInhibitPropertyChangeTimeout;
friend class CellularInhibitorTest;

struct InhibitRequest {
InhibitRequest(InhibitReason inhibit_reason,
InhibitCallback inhibit_callback);
Expand All @@ -105,8 +110,10 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) CellularInhibitor
enum class State {
kIdle,
kInhibiting,
kWaitForInhibit,
kInhibited,
kUninhibiting,
kWaitForUninhibit,
kWaitingForScanningToStart,
kWaitingForScanningToStop,
};
Expand All @@ -121,33 +128,39 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) CellularInhibitor
void TransitionToState(State state);
void ProcessRequests();
void OnInhibit(bool success);
void AttemptUninhibit(size_t attempts_so_far);
void OnUninhibit(size_t attempts_so_far, bool success);
void AttemptUninhibit();
void OnUninhibit(bool success);

void CheckScanningIfNeeded();
void CheckForScanningStarted();
bool HasScanningStarted();
void CheckForScanningStopped();
bool HasScanningStopped();

void CheckInhibitPropertyIfNeeded();
void CheckForInhibit();
void CheckForUninhibit();
void OnInhibitPropertyChangeTimeout();

void PopRequestAndProcessNext();

using SuccessCallback = base::OnceCallback<void(bool)>;
void SetInhibitProperty(bool new_inhibit_value, SuccessCallback callback);
void OnSetPropertySuccess(
const base::RepeatingCallback<void(bool)>& success_callback);
void SetInhibitProperty();
void OnSetPropertySuccess();
void OnSetPropertyError(
const base::RepeatingCallback<void(bool)>& success_callback,
bool attempted_inhibit,
const std::string& error_name,
std::unique_ptr<base::DictionaryValue> error_data);
void ReturnSetInhibitPropertyResult(bool success);

NetworkStateHandler* network_state_handler_ = nullptr;
NetworkDeviceHandler* network_device_handler_ = nullptr;

State state_ = State::kIdle;
base::queue<std::unique_ptr<InhibitRequest>> inhibit_requests_;

size_t uninhibit_attempts_so_far_ = 0;
base::OneShotTimer set_inhibit_timer_;

base::ObserverList<Observer> observer_list_;

base::WeakPtrFactory<CellularInhibitor> weak_ptr_factory_{this};
Expand Down

0 comments on commit c590260

Please sign in to comment.