Commit 73dc567c authored by khorimoto's avatar khorimoto Committed by Commit Bot

[CrOS Tether] Disconnect cleanly from active Tether networks when the user...

[CrOS Tether] Disconnect cleanly from active Tether networks when the user logs out or the Tether component is killed.

Under normal circumstances, when a user disconnects from a Tether network, we start up an asynchronous connection flow. However, when these two edge cases occur, the Tether component is destroyed synchronously, and the full disconnection flow cannot complete (namely, the old Wi-Fi network supporting the Tether network is not removed). This CL fixes this problem by persisting the disconnecting Wi-Fi network GUID and removing it next time the Tether componentn starts up.

This CL also:
(1) Changes TetherDisconnector to an interface and moves the implementation to TetherDisconnectorImpl to make testing easier.
(2) Updates the shutdown flow for the Tether component by leaving the Tether TechnologyState intact during shutdown to prevent UI jank.

BUG=672263

Review-Url: https://codereview.chromium.org/2975483002
Cr-Commit-Position: refs/heads/master@{#486072}
parent 81dd1a25
......@@ -135,6 +135,8 @@ void TetherService::Shutdown() {
shut_down_ = true;
// Remove all observers. This ensures that once Shutdown() is called, no more
// calls to UpdateTetherTechnologyState() will be triggered.
power_manager_client_->RemoveObserver(this);
session_manager_client_->RemoveObserver(this);
cryptauth_service_->GetCryptAuthDeviceManager()->RemoveObserver(this);
......@@ -143,7 +145,10 @@ void TetherService::Shutdown() {
adapter_->RemoveObserver(this);
registrar_.RemoveAll();
UpdateTetherTechnologyState();
// Shut down the feature. Note that this does not change Tether's technology
// state in NetworkStateHandler because doing so could cause visual jank just
// as the user logs out.
StopTether();
}
void TetherService::SuspendImminent() {
......
......@@ -233,10 +233,12 @@ TEST_F(TetherServiceTest, TestShutdown) {
ShutdownTetherService();
EXPECT_EQ(
chromeos::NetworkStateHandler::TechnologyState::TECHNOLOGY_UNAVAILABLE,
network_state_handler()->GetTechnologyState(
chromeos::NetworkTypePattern::Tether()));
// The TechnologyState should not have changed due to Shutdown() being called.
// If it had changed, any settings UI that was previously open would have
// shown visual jank.
EXPECT_EQ(chromeos::NetworkStateHandler::TechnologyState::TECHNOLOGY_ENABLED,
network_state_handler()->GetTechnologyState(
chromeos::NetworkTypePattern::Tether()));
EXPECT_FALSE(test_initializer_delegate_->is_tether_running());
}
......
......@@ -60,8 +60,9 @@ static_library("tether") {
"pref_names.h",
"tether_connector.cc",
"tether_connector.h",
"tether_disconnector.cc",
"tether_disconnector.h",
"tether_disconnector_impl.cc",
"tether_disconnector_impl.h",
"tether_host_fetcher.cc",
"tether_host_fetcher.h",
"tether_host_response_recorder.cc",
......@@ -159,7 +160,7 @@ source_set("unit_tests") {
"network_configuration_remover_unittest.cc",
"network_connection_handler_tether_delegate_unittest.cc",
"tether_connector_unittest.cc",
"tether_disconnector_unittest.cc",
"tether_disconnector_impl_unittest.cc",
"tether_host_fetcher_unittest.cc",
"tether_host_response_recorder_unittest.cc",
"tether_network_disconnection_handler_unittest.cc",
......
......@@ -20,12 +20,17 @@ class FakeTetherHostFetcher : public TetherHostFetcher {
public:
FakeTetherHostFetcher(std::vector<cryptauth::RemoteDevice> tether_hosts,
bool synchronously_reply_with_results);
FakeTetherHostFetcher(bool synchronously_reply_with_results);
explicit FakeTetherHostFetcher(bool synchronously_reply_with_results);
~FakeTetherHostFetcher() override;
void set_synchronously_reply_with_results(
bool synchronously_reply_with_results) {
synchronously_reply_with_results_ = synchronously_reply_with_results;
}
void SetTetherHosts(const std::vector<cryptauth::RemoteDevice> tether_hosts);
// If |sychronously_reply_with_results| is false, calling this method will
// If |synchronously_reply_with_results_| is false, calling this method will
// actually invoke the callbacks.
void InvokePendingCallbacks();
......
......@@ -18,7 +18,7 @@
#include "chromeos/components/tether/network_connection_handler_tether_delegate.h"
#include "chromeos/components/tether/notification_presenter.h"
#include "chromeos/components/tether/tether_connector.h"
#include "chromeos/components/tether/tether_disconnector.h"
#include "chromeos/components/tether/tether_disconnector_impl.h"
#include "chromeos/components/tether/tether_host_fetcher.h"
#include "chromeos/components/tether/tether_host_response_recorder.h"
#include "chromeos/components/tether/tether_network_disconnection_handler.h"
......@@ -94,6 +94,7 @@ void Initializer::Shutdown() {
void Initializer::RegisterProfilePrefs(PrefRegistrySimple* registry) {
ActiveHost::RegisterPrefs(registry);
TetherHostResponseRecorder::RegisterPrefs(registry);
TetherDisconnectorImpl::RegisterPrefs(registry);
}
Initializer::Initializer(
......@@ -222,11 +223,11 @@ void Initializer::OnBluetoothAdapterAdvertisingIntervalSet(
network_configuration_remover_ =
base::MakeUnique<NetworkConfigurationRemover>(
network_state_handler_, managed_network_configuration_handler_);
tether_disconnector_ = base::MakeUnique<TetherDisconnector>(
tether_disconnector_ = base::MakeUnique<TetherDisconnectorImpl>(
network_connection_handler_, network_state_handler_, active_host_.get(),
ble_connection_manager_.get(), network_configuration_remover_.get(),
tether_connector_.get(), device_id_tether_network_guid_map_.get(),
tether_host_fetcher_.get());
tether_host_fetcher_.get(), pref_service_);
tether_network_disconnection_handler_ =
base::MakeUnique<TetherNetworkDisconnectionHandler>(
active_host_.get(), network_state_handler_,
......
......@@ -87,15 +87,7 @@ class MockTetherConnector : public TetherConnector {
class MockTetherDisconnector : public TetherDisconnector {
public:
MockTetherDisconnector()
: TetherDisconnector(nullptr /* network_connection_handler */,
nullptr /* network_state_handler */,
nullptr /* active_host */,
nullptr /* ble_connection_manager */,
nullptr /* network_configuration_remover */,
nullptr /* tether_connector */,
nullptr /* device_id_tether_network_guid_map */,
nullptr /* tether_host_fetcher */) {}
MockTetherDisconnector() : TetherDisconnector() {}
~MockTetherDisconnector() override {}
MOCK_METHOD3(
......
......@@ -24,6 +24,9 @@ const char kTetherNetworkGuid[] = "tether.tether_network_id";
const char kWifiNetworkGuid[] = "tether.wifi_network_id";
const char kDisconnectingWifiNetworkGuid[] =
"tether.disconnecting_wifi_network_id";
} // namespace prefs
} // namespace tether
......
......@@ -39,6 +39,17 @@ extern const char kTetherNetworkGuid[];
// value at this key is "".
extern const char kWifiNetworkGuid[];
// The Wi-Fi network GUID that is currently being disconnected. When
// disconnecting under normal circumstances, this value is set when a
// disconnection is initiated and is cleared when a disconnection completes.
// However, when a disconnection is triggered by the user logging out, the
// disconnection flow cannot complete before Chrome shuts down (due to the
// asynchronous nature of the network stack), so this GUID remains in prefs.
// When the Tether component starts up again (the next time the user logs in),
// this GUID is fetched, the associated network configuration is removed, and
// the GUID is cleared from prefs.
extern const char kDisconnectingWifiNetworkGuid[];
} // namespace prefs
} // namespace tether
......
......@@ -5,9 +5,7 @@
#ifndef CHROMEOS_COMPONENTS_TETHER_TETHER_DISCONNECTOR_H_
#define CHROMEOS_COMPONENTS_TETHER_TETHER_DISCONNECTOR_H_
#include <memory>
#include <string>
#include <unordered_map>
#include "base/callback_forward.h"
#include "base/macros.h"
......@@ -15,84 +13,26 @@
#include "chromeos/components/tether/disconnect_tethering_operation.h"
#include "chromeos/network/network_handler_callbacks.h"
namespace base {
class DictionaryValue;
}
namespace chromeos {
class NetworkConnectionHandler;
class NetworkStateHandler;
namespace tether {
class ActiveHost;
class BleConnectionManager;
class DeviceIdTetherNetworkGuidMap;
class NetworkConfigurationRemover;
class TetherConnector;
class TetherHostFetcher;
class TetherDisconnector : public DisconnectTetheringOperation::Observer {
// Disconnects from an active Tether connection.
class TetherDisconnector {
public:
TetherDisconnector(
NetworkConnectionHandler* network_connection_handler,
NetworkStateHandler* network_state_handler,
ActiveHost* active_host,
BleConnectionManager* ble_connection_manager,
NetworkConfigurationRemover* network_configuration_remover,
TetherConnector* tether_connector,
DeviceIdTetherNetworkGuidMap* device_id_tether_network_guid_map,
TetherHostFetcher* tether_host_fetcher);
virtual ~TetherDisconnector();
TetherDisconnector() {}
virtual ~TetherDisconnector() {}
// Disconnects from the network with GUID |tether_network_guid|. This GUID
// must correspond to an active (i.e., connecting/connected) Tether network.
// If disconnection fails, |error_callback| is invoked with a
// NetworkConnectionHandler error value.
virtual void DisconnectFromNetwork(
const std::string& tether_network_guid,
const base::Closure& success_callback,
const network_handler::StringResultCallback& error_callback);
// DisconnectTetheringOperation::Observer:
void OnOperationFinished(const std::string& device_id, bool success) override;
const network_handler::StringResultCallback& error_callback) = 0;
private:
friend class TetherDisconnectorTest;
void DisconnectActiveWifiConnection(
const std::string& tether_network_guid,
const std::string& wifi_network_guid,
const base::Closure& success_callback,
const network_handler::StringResultCallback& error_callback);
void OnSuccessfulWifiDisconnect(
const std::string& wifi_network_guid,
const base::Closure& success_callback,
const network_handler::StringResultCallback& error_callback);
void OnFailedWifiDisconnect(
const std::string& wifi_network_guid,
const base::Closure& success_callback,
const network_handler::StringResultCallback& error_callback,
const std::string& error_name,
std::unique_ptr<base::DictionaryValue> error_data);
void CleanUpAfterWifiDisconnection(
bool success,
const std::string& wifi_network_guid,
const base::Closure& success_callback,
const network_handler::StringResultCallback& error_callback);
void OnTetherHostFetched(
const std::string& device_id,
std::unique_ptr<cryptauth::RemoteDevice> tether_host);
NetworkConnectionHandler* network_connection_handler_;
NetworkStateHandler* network_state_handler_;
ActiveHost* active_host_;
BleConnectionManager* ble_connection_manager_;
NetworkConfigurationRemover* network_configuration_remover_;
TetherConnector* tether_connector_;
DeviceIdTetherNetworkGuidMap* device_id_tether_network_guid_map_;
TetherHostFetcher* tether_host_fetcher_;
std::unique_ptr<DisconnectTetheringOperation> disconnect_tethering_operation_;
base::WeakPtrFactory<TetherDisconnector> weak_ptr_factory_;
DISALLOW_COPY_AND_ASSIGN(TetherDisconnector);
};
......
......@@ -2,24 +2,41 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "chromeos/components/tether/tether_disconnector.h"
#include "chromeos/components/tether/tether_disconnector_impl.h"
#include "base/values.h"
#include "chromeos/components/tether/active_host.h"
#include "chromeos/components/tether/device_id_tether_network_guid_map.h"
#include "chromeos/components/tether/network_configuration_remover.h"
#include "chromeos/components/tether/pref_names.h"
#include "chromeos/components/tether/tether_connector.h"
#include "chromeos/components/tether/tether_host_fetcher.h"
#include "chromeos/network/network_connection_handler.h"
#include "chromeos/network/network_state.h"
#include "chromeos/network/network_state_handler.h"
#include "components/prefs/pref_registry_simple.h"
#include "components/prefs/pref_service.h"
#include "components/proximity_auth/logging/logging.h"
namespace chromeos {
namespace tether {
TetherDisconnector::TetherDisconnector(
namespace {
void OnDisconnectError(const std::string& error_name) {
PA_LOG(WARNING) << "Error disconnecting from Tether network during shutdown; "
<< "Error name: " << error_name;
}
} // namespace
// static
void TetherDisconnectorImpl::RegisterPrefs(PrefRegistrySimple* registry) {
registry->RegisterStringPref(prefs::kDisconnectingWifiNetworkGuid, "");
}
TetherDisconnectorImpl::TetherDisconnectorImpl(
NetworkConnectionHandler* network_connection_handler,
NetworkStateHandler* network_state_handler,
ActiveHost* active_host,
......@@ -27,7 +44,8 @@ TetherDisconnector::TetherDisconnector(
NetworkConfigurationRemover* network_configuration_remover,
TetherConnector* tether_connector,
DeviceIdTetherNetworkGuidMap* device_id_tether_network_guid_map,
TetherHostFetcher* tether_host_fetcher)
TetherHostFetcher* tether_host_fetcher,
PrefService* pref_service)
: network_connection_handler_(network_connection_handler),
network_state_handler_(network_state_handler),
active_host_(active_host),
......@@ -36,14 +54,34 @@ TetherDisconnector::TetherDisconnector(
tether_connector_(tether_connector),
device_id_tether_network_guid_map_(device_id_tether_network_guid_map),
tether_host_fetcher_(tether_host_fetcher),
weak_ptr_factory_(this) {}
pref_service_(pref_service),
weak_ptr_factory_(this) {
std::string disconnecting_wifi_guid_from_previous_session =
pref_service_->GetString(prefs::kDisconnectingWifiNetworkGuid);
if (!disconnecting_wifi_guid_from_previous_session.empty()) {
// If a previous disconnection attempt was aborted before it could be fully
// completed, clean up the leftover network configuration.
network_configuration_remover_->RemoveNetworkConfiguration(
disconnecting_wifi_guid_from_previous_session);
pref_service_->ClearPref(prefs::kDisconnectingWifiNetworkGuid);
}
}
TetherDisconnector::~TetherDisconnector() {
TetherDisconnectorImpl::~TetherDisconnectorImpl() {
if (disconnect_tethering_operation_)
disconnect_tethering_operation_->RemoveObserver(this);
std::string active_tether_guid = active_host_->GetTetherNetworkGuid();
if (!active_tether_guid.empty()) {
PA_LOG(INFO) << "There was an active Tether connection during Tether "
<< "shutdown. Initiating disconnection from network with GUID "
<< "\"" << active_tether_guid << "\"";
DisconnectFromNetwork(active_tether_guid, base::Bind(&base::DoNothing),
base::Bind(&OnDisconnectError));
}
}
void TetherDisconnector::DisconnectFromNetwork(
void TetherDisconnectorImpl::DisconnectFromNetwork(
const std::string& tether_network_guid,
const base::Closure& success_callback,
const network_handler::StringResultCallback& error_callback) {
......@@ -92,7 +130,7 @@ void TetherDisconnector::DisconnectFromNetwork(
success_callback, error_callback);
}
void TetherDisconnector::DisconnectActiveWifiConnection(
void TetherDisconnectorImpl::DisconnectActiveWifiConnection(
const std::string& tether_network_guid,
const std::string& wifi_network_guid,
const base::Closure& success_callback,
......@@ -105,15 +143,25 @@ void TetherDisconnector::DisconnectActiveWifiConnection(
// transition which needs to be fixed.
active_host_->SetActiveHostDisconnected();
// Before starting disconnection, log the disconnecting Wi-Fi GUID to prefs.
// Under normal circumstances, the GUID will be cleared as part of
// CleanUpAfterWifiDisconnection(). However, when the user logs out,
// this TetherDisconnectorImpl instance will be deleted before one of the
// callbacks passed below to DisconnectNetwork() can be called, and the
// GUID will remain in prefs until the next time the user logs in, at which
// time the associated network configuration can be removed.
pref_service_->Set(prefs::kDisconnectingWifiNetworkGuid,
base::Value(wifi_network_guid));
const NetworkState* wifi_network_state =
network_state_handler_->GetNetworkStateFromGuid(wifi_network_guid);
if (wifi_network_state) {
network_connection_handler_->DisconnectNetwork(
wifi_network_state->path(),
base::Bind(&TetherDisconnector::OnSuccessfulWifiDisconnect,
base::Bind(&TetherDisconnectorImpl::OnSuccessfulWifiDisconnect,
weak_ptr_factory_.GetWeakPtr(), wifi_network_guid,
success_callback, error_callback),
base::Bind(&TetherDisconnector::OnFailedWifiDisconnect,
base::Bind(&TetherDisconnectorImpl::OnFailedWifiDisconnect,
weak_ptr_factory_.GetWeakPtr(), wifi_network_guid,
success_callback, error_callback));
} else {
......@@ -129,12 +177,12 @@ void TetherDisconnector::DisconnectActiveWifiConnection(
device_id_tether_network_guid_map_->GetDeviceIdForTetherNetworkGuid(
tether_network_guid);
tether_host_fetcher_->FetchTetherHost(
device_id, base::Bind(&TetherDisconnector::OnTetherHostFetched,
device_id, base::Bind(&TetherDisconnectorImpl::OnTetherHostFetched,
weak_ptr_factory_.GetWeakPtr(), device_id));
}
void TetherDisconnector::OnOperationFinished(const std::string& device_id,
bool success) {
void TetherDisconnectorImpl::OnOperationFinished(const std::string& device_id,
bool success) {
if (success) {
PA_LOG(INFO) << "Successfully sent DisconnectTetheringRequest to device "
<< "with ID "
......@@ -152,7 +200,7 @@ void TetherDisconnector::OnOperationFinished(const std::string& device_id,
disconnect_tethering_operation_.reset();
}
void TetherDisconnector::OnSuccessfulWifiDisconnect(
void TetherDisconnectorImpl::OnSuccessfulWifiDisconnect(
const std::string& wifi_network_guid,
const base::Closure& success_callback,
const network_handler::StringResultCallback& error_callback) {
......@@ -162,7 +210,7 @@ void TetherDisconnector::OnSuccessfulWifiDisconnect(
success_callback, error_callback);
}
void TetherDisconnector::OnFailedWifiDisconnect(
void TetherDisconnectorImpl::OnFailedWifiDisconnect(
const std::string& wifi_network_guid,
const base::Closure& success_callback,
const network_handler::StringResultCallback& error_callback,
......@@ -174,20 +222,21 @@ void TetherDisconnector::OnFailedWifiDisconnect(
success_callback, error_callback);
}
void TetherDisconnector::CleanUpAfterWifiDisconnection(
void TetherDisconnectorImpl::CleanUpAfterWifiDisconnection(
bool success,
const std::string& wifi_network_guid,
const base::Closure& success_callback,
const network_handler::StringResultCallback& error_callback) {
network_configuration_remover_->RemoveNetworkConfiguration(wifi_network_guid);
pref_service_->ClearPref(prefs::kDisconnectingWifiNetworkGuid);
if (success)
success_callback.Run();
else
error_callback.Run(NetworkConnectionHandler::kErrorDisconnectFailed);
network_configuration_remover_->RemoveNetworkConfiguration(wifi_network_guid);
}
void TetherDisconnector::OnTetherHostFetched(
void TetherDisconnectorImpl::OnTetherHostFetched(
const std::string& device_id,
std::unique_ptr<cryptauth::RemoteDevice> tether_host) {
if (!tether_host) {
......
// Copyright 2017 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef CHROMEOS_COMPONENTS_TETHER_TETHER_DISCONNECTOR_IMPL_H_
#define CHROMEOS_COMPONENTS_TETHER_TETHER_DISCONNECTOR_IMPL_H_
#include <memory>
#include <string>
#include <unordered_map>
#include "base/callback_forward.h"
#include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "chromeos/components/tether/disconnect_tethering_operation.h"
#include "chromeos/components/tether/tether_disconnector.h"
#include "chromeos/network/network_handler_callbacks.h"
class PrefRegistrySimple;
class PrefService;
namespace base {
class DictionaryValue;
}
namespace chromeos {
class NetworkConnectionHandler;
class NetworkStateHandler;
namespace tether {
class ActiveHost;
class BleConnectionManager;
class DeviceIdTetherNetworkGuidMap;
class NetworkConfigurationRemover;
class TetherConnector;
class TetherHostFetcher;
class TetherDisconnectorImpl : public TetherDisconnector,
public DisconnectTetheringOperation::Observer {
public:
// Registers the prefs used by this class to the given |registry|.
static void RegisterPrefs(PrefRegistrySimple* registry);
TetherDisconnectorImpl(
NetworkConnectionHandler* network_connection_handler,
NetworkStateHandler* network_state_handler,
ActiveHost* active_host,
BleConnectionManager* ble_connection_manager,
NetworkConfigurationRemover* network_configuration_remover,
TetherConnector* tether_connector,
DeviceIdTetherNetworkGuidMap* device_id_tether_network_guid_map,
TetherHostFetcher* tether_host_fetcher,
PrefService* pref_service);
~TetherDisconnectorImpl() override;
void DisconnectFromNetwork(
const std::string& tether_network_guid,
const base::Closure& success_callback,
const network_handler::StringResultCallback& error_callback) override;
// DisconnectTetheringOperation::Observer:
void OnOperationFinished(const std::string& device_id, bool success) override;
private:
friend class TetherDisconnectorImplTest;
void DisconnectActiveWifiConnection(
const std::string& tether_network_guid,
const std::string& wifi_network_guid,
const base::Closure& success_callback,
const network_handler::StringResultCallback& error_callback);
void OnSuccessfulWifiDisconnect(
const std::string& wifi_network_guid,
const base::Closure& success_callback,
const network_handler::StringResultCallback& error_callback);
void OnFailedWifiDisconnect(
const std::string& wifi_network_guid,
const base::Closure& success_callback,
const network_handler::StringResultCallback& error_callback,
const std::string& error_name,
std::unique_ptr<base::DictionaryValue> error_data);
void CleanUpAfterWifiDisconnection(
bool success,
const std::string& wifi_network_guid,
const base::Closure& success_callback,
const network_handler::StringResultCallback& error_callback);
void OnTetherHostFetched(
const std::string& device_id,
std::unique_ptr<cryptauth::RemoteDevice> tether_host);
NetworkConnectionHandler* network_connection_handler_;
NetworkStateHandler* network_state_handler_;
ActiveHost* active_host_;
BleConnectionManager* ble_connection_manager_;
NetworkConfigurationRemover* network_configuration_remover_;
TetherConnector* tether_connector_;
DeviceIdTetherNetworkGuidMap* device_id_tether_network_guid_map_;
TetherHostFetcher* tether_host_fetcher_;
PrefService* pref_service_;
std::unique_ptr<DisconnectTetheringOperation> disconnect_tethering_operation_;
base::WeakPtrFactory<TetherDisconnectorImpl> weak_ptr_factory_;
DISALLOW_COPY_AND_ASSIGN(TetherDisconnectorImpl);
};
} // namespace tether
} // namespace chromeos
#endif // CHROMEOS_COMPONENTS_TETHER_TETHER_DISCONNECTOR_IMPL_H_
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