Skip to content

Commit

Permalink
[CrOS Tether] Invoke any pending connection callbacks on shutdown.
Browse files Browse the repository at this point in the history 
This ensures that there is no possibility for pending network connection
or disconnection requests when shutdown occurs. Leaving these pending
could leave the network stack in a bad state.

The original CL landed as [1] and was reverted as [2] due to an ASAN bot
failure. I've uploaded the original CL as patch 1 on this CL, and my fix
as patch 2.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/656570
[2] https://chromium-review.googlesource.com/c/chromium/src/+/659377

[email protected]

(cherry picked from commit 7dd2ddd)

Bug: 761541, 672263
Change-Id: I3e9beb0ad62fb92ca202f391229dbc46c9110b18
Reviewed-on: https://chromium-review.googlesource.com/663673
Reviewed-by: Ryan Hansberry <[email protected]>
Commit-Queue: Ryan Hansberry <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#501397}
Reviewed-on: https://chromium-review.googlesource.com/663856
Reviewed-by: Kyle Horimoto <[email protected]>
Cr-Commit-Position: refs/branch-heads/3202@{crosswalk-project#186}
Cr-Branched-From: fa6a5d8-refs/heads/master@{#499098}
  • Loading branch information
Kyle Horimoto authored and Kyle Horimoto committed Sep 12, 2017
1 parent dd91637 commit 7103446
Show file tree
Hide file tree
Showing 3 changed files with 154 additions and 27 deletions.
73 changes: 60 additions & 13 deletions chromeos/components/tether/network_connection_handler_tether_delegate.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "chromeos/components/tether/network_connection_handler_tether_delegate.h"

#include "base/bind_helpers.h"
#include "base/stl_util.h"
#include "chromeos/components/tether/active_host.h"
#include "chromeos/components/tether/tether_connector.h"
#include "chromeos/components/tether/tether_disconnector.h"
Expand All @@ -14,6 +15,27 @@ namespace chromeos {

namespace tether {

namespace {

void OnFailedDisconnectionFromPreviousHost(
const std::string& tether_network_guid,
const std::string& error_name) {
PA_LOG(ERROR) << "Failed to disconnect from previously-active host. "
<< "GUID: " << tether_network_guid << ", Error: " << error_name;
}

} // namespace

NetworkConnectionHandlerTetherDelegate::Callbacks::Callbacks(
const base::Closure& success_callback,
const network_handler::StringResultCallback& error_callback)
: success_callback(success_callback), error_callback(error_callback) {}

NetworkConnectionHandlerTetherDelegate::Callbacks::Callbacks(
const Callbacks& other) = default;

NetworkConnectionHandlerTetherDelegate::Callbacks::~Callbacks() {}

NetworkConnectionHandlerTetherDelegate::NetworkConnectionHandlerTetherDelegate(
NetworkConnectionHandler* network_connection_handler,
ActiveHost* active_host,
Expand All @@ -29,15 +51,29 @@ NetworkConnectionHandlerTetherDelegate::NetworkConnectionHandlerTetherDelegate(

NetworkConnectionHandlerTetherDelegate::
~NetworkConnectionHandlerTetherDelegate() {
// If there are still pending callbacks, invoke them here. It should never be
// possible for the Tether component to shut down with pending callbacks.
for (const auto& entry : request_num_to_callbacks_map_) {
entry.second.error_callback.Run(
NetworkConnectionHandler::kErrorConnectFailed);
}

network_connection_handler_->SetTetherDelegate(nullptr);
}

void NetworkConnectionHandlerTetherDelegate::DisconnectFromNetwork(
const std::string& tether_network_guid,
const base::Closure& success_callback,
const network_handler::StringResultCallback& error_callback) {
tether_disconnector_->DisconnectFromNetwork(tether_network_guid,
success_callback, error_callback);
int request_num = next_request_num_++;
request_num_to_callbacks_map_.emplace(
request_num, Callbacks(success_callback, error_callback));
tether_disconnector_->DisconnectFromNetwork(
tether_network_guid,
base::Bind(&NetworkConnectionHandlerTetherDelegate::OnRequestSuccess,
weak_ptr_factory_.GetWeakPtr(), request_num),
base::Bind(&NetworkConnectionHandlerTetherDelegate::OnRequestError,
weak_ptr_factory_.GetWeakPtr(), request_num));
}

void NetworkConnectionHandlerTetherDelegate::ConnectToNetwork(
Expand All @@ -63,21 +99,32 @@ void NetworkConnectionHandlerTetherDelegate::ConnectToNetwork(
<< previous_host_guid << ".";
DisconnectFromNetwork(
previous_host_guid, base::Bind(&base::DoNothing),
base::Bind(&NetworkConnectionHandlerTetherDelegate::
OnFailedDisconnectionFromPreviousHost,
weak_ptr_factory_.GetWeakPtr(), previous_host_guid));
base::Bind(&OnFailedDisconnectionFromPreviousHost, previous_host_guid));
}

tether_connector_->ConnectToNetwork(tether_network_guid, success_callback,
error_callback);
int request_num = next_request_num_++;
request_num_to_callbacks_map_.emplace(
request_num, Callbacks(success_callback, error_callback));
tether_connector_->ConnectToNetwork(
tether_network_guid,
base::Bind(&NetworkConnectionHandlerTetherDelegate::OnRequestSuccess,
weak_ptr_factory_.GetWeakPtr(), request_num),
base::Bind(&NetworkConnectionHandlerTetherDelegate::OnRequestError,
weak_ptr_factory_.GetWeakPtr(), request_num));
}

void NetworkConnectionHandlerTetherDelegate::
OnFailedDisconnectionFromPreviousHost(
const std::string& tether_network_guid,
const std::string& error_name) {
PA_LOG(ERROR) << "Failed to disconnect from previously-active host. "
<< "GUID: " << tether_network_guid << ", Error: " << error_name;
void NetworkConnectionHandlerTetherDelegate::OnRequestSuccess(int request_num) {
DCHECK(base::ContainsKey(request_num_to_callbacks_map_, request_num));
request_num_to_callbacks_map_.at(request_num).success_callback.Run();
request_num_to_callbacks_map_.erase(request_num);
}

void NetworkConnectionHandlerTetherDelegate::OnRequestError(
int request_num,
const std::string& error_name) {
DCHECK(base::ContainsKey(request_num_to_callbacks_map_, request_num));
request_num_to_callbacks_map_.at(request_num).error_callback.Run(error_name);
request_num_to_callbacks_map_.erase(request_num);
}

} // namespace tether
Expand Down
24 changes: 21 additions & 3 deletions chromeos/components/tether/network_connection_handler_tether_delegate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,12 @@
#ifndef CHROMEOS_COMPONENTS_TETHER_NETWORK_CONNECTION_HANDLER_TETHER_DELEGATE_H_
#define CHROMEOS_COMPONENTS_TETHER_NETWORK_CONNECTION_HANDLER_TETHER_DELEGATE_H_

#include <unordered_map>

#include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "chromeos/network/network_connection_handler.h"
#include "chromeos/network/network_handler_callbacks.h"

namespace chromeos {

Expand Down Expand Up @@ -41,15 +44,30 @@ class NetworkConnectionHandlerTetherDelegate
const network_handler::StringResultCallback& error_callback) override;

private:
void OnFailedDisconnectionFromPreviousHost(
const std::string& tether_network_guid,
const std::string& error_name);
struct Callbacks {
public:
Callbacks(const base::Closure& success_callback,
const network_handler::StringResultCallback& error_callback);
Callbacks(const Callbacks& other);
~Callbacks();

base::Closure success_callback;
network_handler::StringResultCallback error_callback;
};

void OnRequestSuccess(int request_num);
void OnRequestError(int request_num, const std::string& error_name);

NetworkConnectionHandler* network_connection_handler_;
ActiveHost* active_host_;
TetherConnector* tether_connector_;
TetherDisconnector* tether_disconnector_;

// Cache request callbacks in a map so that if the callbacks do not occur by
// the time the object is deleted, all callbacks are invoked.
int next_request_num_ = 0;
std::unordered_map<int, Callbacks> request_num_to_callbacks_map_;

base::WeakPtrFactory<NetworkConnectionHandlerTetherDelegate>
weak_ptr_factory_;

Expand Down
84 changes: 73 additions & 11 deletions chromeos/components/tether/network_connection_handler_tether_delegate_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,28 @@ namespace tether {

namespace {

const char kSuccessResult[] = "success";

// Does nothing when a connection is requested.
class DummyTetherConnector : public FakeTetherConnector {
public:
// TetherConnector:
void ConnectToNetwork(
const std::string& tether_network_guid,
const base::Closure& success_callback,
const network_handler::StringResultCallback& error_callback) override {}
};

// Does nothing when a disconnection is requested.
class DummyTetherDisconnector : public FakeTetherDisconnector {
public:
// TetherDisconnector:
void DisconnectFromNetwork(
const std::string& tether_network_guid,
const base::Closure& success_callback,
const network_handler::StringResultCallback& error_callback) override {}
};

class TestNetworkConnectionHandler : public NetworkConnectionHandler {
public:
TestNetworkConnectionHandler() : NetworkConnectionHandler() {}
Expand Down Expand Up @@ -70,7 +92,7 @@ class NetworkConnectionHandlerTetherDelegateTest : public testing::Test {
NetworkConnectionHandlerTetherDelegateTest() {}

void SetUp() override {
error_occurred_during_test_ = false;
result_.clear();

test_network_connection_handler_ =
base::WrapUnique(new TestNetworkConnectionHandler());
Expand All @@ -84,27 +106,41 @@ class NetworkConnectionHandlerTetherDelegateTest : public testing::Test {
fake_tether_connector_.get(), fake_tether_disconnector_.get());
}

void TearDown() override {
// No more callbacks should occur after deletion.
delegate_.reset();
EXPECT_EQ(std::string(), GetResultAndReset());
}

void CallTetherConnect(const std::string& guid) {
test_network_connection_handler_->CallTetherConnect(
guid, base::Closure(),
guid,
base::Bind(&NetworkConnectionHandlerTetherDelegateTest::OnSuccess,
base::Unretained(this)),
base::Bind(&NetworkConnectionHandlerTetherDelegateTest::OnError,
base::Unretained(this)));
}

void CallTetherDisconnect(const std::string& guid) {
test_network_connection_handler_->CallTetherDisconnect(
guid, base::Closure(),
guid,
base::Bind(&NetworkConnectionHandlerTetherDelegateTest::OnSuccess,
base::Unretained(this)),
base::Bind(&NetworkConnectionHandlerTetherDelegateTest::OnError,
base::Unretained(this)));
}

void OnSuccess() { result_ = kSuccessResult; }

void OnError(const std::string& error,
std::unique_ptr<base::DictionaryValue> error_data) {
error_occurred_during_test_ = true;
result_ = error;
}

void VerifyErrorExpected(bool expected) {
EXPECT_EQ(expected, error_occurred_during_test_);
std::string GetResultAndReset() {
std::string result;
result.swap(result_);
return result;
}

std::unique_ptr<TestNetworkConnectionHandler>
Expand All @@ -113,7 +149,7 @@ class NetworkConnectionHandlerTetherDelegateTest : public testing::Test {
std::unique_ptr<FakeTetherConnector> fake_tether_connector_;
std::unique_ptr<FakeTetherDisconnector> fake_tether_disconnector_;

bool error_occurred_during_test_;
std::string result_;

std::unique_ptr<NetworkConnectionHandlerTetherDelegate> delegate_;

Expand All @@ -126,7 +162,7 @@ TEST_F(NetworkConnectionHandlerTetherDelegateTest,
CallTetherConnect("tetherNetworkGuid");
EXPECT_EQ("tetherNetworkGuid",
fake_tether_connector_->last_connected_tether_network_guid());
VerifyErrorExpected(false);
EXPECT_EQ(kSuccessResult, GetResultAndReset());
}

TEST_F(NetworkConnectionHandlerTetherDelegateTest,
Expand All @@ -138,7 +174,7 @@ TEST_F(NetworkConnectionHandlerTetherDelegateTest,
fake_tether_connector_->last_connected_tether_network_guid().empty());
EXPECT_TRUE(fake_tether_disconnector_->last_disconnected_tether_network_guid()
.empty());
VerifyErrorExpected(true);
EXPECT_EQ(NetworkConnectionHandler::kErrorConnected, GetResultAndReset());
}

TEST_F(NetworkConnectionHandlerTetherDelegateTest,
Expand All @@ -151,14 +187,40 @@ TEST_F(NetworkConnectionHandlerTetherDelegateTest,
fake_tether_disconnector_->last_disconnected_tether_network_guid());
EXPECT_EQ("newTetherNetworkGuid",
fake_tether_connector_->last_connected_tether_network_guid());
VerifyErrorExpected(false);
EXPECT_EQ(kSuccessResult, GetResultAndReset());
}

TEST_F(NetworkConnectionHandlerTetherDelegateTest, TestDisconnect) {
CallTetherDisconnect("tetherNetworkGuid");
EXPECT_EQ("tetherNetworkGuid",
fake_tether_disconnector_->last_disconnected_tether_network_guid());
VerifyErrorExpected(false);
EXPECT_EQ(kSuccessResult, GetResultAndReset());
}

TEST_F(NetworkConnectionHandlerTetherDelegateTest,
TestPendingCallbacksInvokedWhenDeleted) {
delegate_.reset();

// Use "dummy" connector/disconnector.
std::unique_ptr<DummyTetherConnector> dummy_connector =
base::WrapUnique(new DummyTetherConnector());
std::unique_ptr<DummyTetherDisconnector> dummy_disconnector =
base::WrapUnique(new DummyTetherDisconnector());

test_network_connection_handler_ =
base::WrapUnique(new TestNetworkConnectionHandler());
delegate_ = base::MakeUnique<NetworkConnectionHandlerTetherDelegate>(
test_network_connection_handler_.get(), fake_active_host_.get(),
dummy_connector.get(), dummy_disconnector.get());

CallTetherConnect("tetherNetworkGuid");

// No callbacks should have been invoked.
EXPECT_TRUE(result_.empty());

// Now, delete the delegate. It should fire the error callback.
delegate_.reset();
EXPECT_EQ(NetworkConnectionHandler::kErrorConnectFailed, GetResultAndReset());
}

} // namespace tether
Expand Down

0 comments on commit 7103446

Please sign in to comment.