shill: Ensure valid cellular service before using it.
Cellular::OnConnectFailed() and CellularCapabilityGSM::OnConnectReply()
may be called when there are no cellular service. This CL checks to
make sure the service is valid before using it.
BUG=chromium-os:31016
TEST=Unit tests, network_3GModemControl
Change-Id: I79ffeccf80f79c5cbe62ce93c6feaabf86ae5ea1
Reviewed-on: https://gerrit.chromium.org/gerrit/23042
Commit-Ready: Thieu Le <thieule@chromium.org>
Reviewed-by: Thieu Le <thieule@chromium.org>
Tested-by: Thieu Le <thieule@chromium.org>
diff --git a/cellular.cc b/cellular.cc
index 1f8bfb9..e3cdcb2 100644
--- a/cellular.cc
+++ b/cellular.cc
@@ -432,7 +432,8 @@
}
void Cellular::OnConnectFailed(const Error &error) {
- service_->SetFailure(Service::kFailureUnknown);
+ if (service_)
+ service_->SetFailure(Service::kFailureUnknown);
}
void Cellular::Disconnect(Error *error) {
diff --git a/cellular.h b/cellular.h
index 14a6584..99b6ed4 100644
--- a/cellular.h
+++ b/cellular.h
@@ -209,6 +209,7 @@
FRIEND_TEST(CellularTest, CreateService);
FRIEND_TEST(CellularTest, Connect);
FRIEND_TEST(CellularTest, ConnectFailure);
+ FRIEND_TEST(CellularTest, ConnectFailureNoService);
FRIEND_TEST(CellularTest, DisableModem);
FRIEND_TEST(CellularTest, Disconnect);
FRIEND_TEST(CellularTest, ModemStateChangeDisable);
diff --git a/cellular_capability_classic.h b/cellular_capability_classic.h
index 3f41e25..b18a85c 100644
--- a/cellular_capability_classic.h
+++ b/cellular_capability_classic.h
@@ -158,6 +158,7 @@
FRIEND_TEST(CellularTest, StartLinked);
FRIEND_TEST(CellularTest, Connect);
FRIEND_TEST(CellularTest, ConnectFailure);
+ FRIEND_TEST(CellularTest, ConnectFailureNoService);
FRIEND_TEST(CellularTest, Disconnect);
FRIEND_TEST(CellularTest, ModemStateChangeEnable);
FRIEND_TEST(CellularTest, ModemStateChangeDisable);
diff --git a/cellular_capability_gsm.cc b/cellular_capability_gsm.cc
index 6042415..016e288 100644
--- a/cellular_capability_gsm.cc
+++ b/cellular_capability_gsm.cc
@@ -261,8 +261,13 @@
void CellularCapabilityGSM::OnConnectReply(const ResultCallback &callback,
const Error &error) {
- if (error.IsFailure()) {
- cellular()->service()->ClearLastGoodApn();
+ CellularServiceRefPtr service = cellular()->service();
+ if (!service) {
+ // The service could have been deleted before our Connect() request
+ // completes if the modem was enabled and then quickly disabled.
+ apn_try_list_.clear();
+ } else if (error.IsFailure()) {
+ service->ClearLastGoodApn();
// The APN that was just tried (and failed) is still at the
// front of the list, about to be removed. If the list is empty
// after that, try one last time without an APN. This may succeed
@@ -278,7 +283,7 @@
return;
}
} else if (!apn_try_list_.empty()) {
- cellular()->service()->SetLastGoodApn(apn_try_list_.front());
+ service->SetLastGoodApn(apn_try_list_.front());
apn_try_list_.clear();
}
if (!callback.is_null())
diff --git a/cellular_capability_gsm_unittest.cc b/cellular_capability_gsm_unittest.cc
index 3fe2277..75912ae 100644
--- a/cellular_capability_gsm_unittest.cc
+++ b/cellular_capability_gsm_unittest.cc
@@ -44,6 +44,10 @@
return arg.IsSuccess();
}
+MATCHER(IsFailure, "") {
+ return arg.IsFailure();
+}
+
class CellularCapabilityGSMTest : public testing::Test {
public:
CellularCapabilityGSMTest()
@@ -168,6 +172,11 @@
callback.Run(info, Error());
}
+ void InvokeConnectFail(DBusPropertiesMap props, Error *error,
+ const ResultCallback &callback, int timeout) {
+ callback.Run(Error(Error::kOperationFailed));
+ }
+
MOCK_METHOD1(TestCallback, void(const Error &error));
protected:
@@ -251,11 +260,15 @@
cellular_->provider_db_ = provider_db_;
}
- void SetupCommonStartModemExpectations() {
+ void SetupCommonProxiesExpectations() {
EXPECT_CALL(*proxy_, set_state_changed_callback(_));
EXPECT_CALL(*network_proxy_, set_signal_quality_callback(_));
EXPECT_CALL(*network_proxy_, set_network_mode_callback(_));
EXPECT_CALL(*network_proxy_, set_registration_info_callback(_));
+ }
+
+ void SetupCommonStartModemExpectations() {
+ SetupCommonProxiesExpectations();
EXPECT_CALL(*proxy_, Enable(_, _, _, CellularCapability::kTimeoutEnable))
.WillOnce(Invoke(this, &CellularCapabilityGSMTest::InvokeEnable));
@@ -277,6 +290,10 @@
EXPECT_CALL(*this, TestCallback(IsSuccess()));
}
+ void InitProxies() {
+ capability_->InitProxies();
+ }
+
NiceMockControl control_;
EventDispatcher dispatcher_;
MockMetrics metrics_;
@@ -885,4 +902,24 @@
dispatcher_.DispatchPendingEvents();
}
+TEST_F(CellularCapabilityGSMTest, ConnectFailureNoService) {
+ // Make sure we don't crash if the connect failed and there is no
+ // CellularService object. This can happen if the modem is enabled and
+ // then quickly disabled.
+ SetupCommonProxiesExpectations();
+ EXPECT_CALL(*simple_proxy_,
+ Connect(_, _, _, CellularCapabilityGSM::kTimeoutConnect))
+ .WillOnce(Invoke(this, &CellularCapabilityGSMTest::InvokeConnectFail));
+ EXPECT_CALL(*this, TestCallback(IsFailure()));
+ SetProxyFactory();
+ InitProxies();
+ EXPECT_FALSE(capability_->cellular()->service());
+ Error error;
+ DBusPropertiesMap props;
+ capability_->Connect(props, &error,
+ Bind(&CellularCapabilityGSMTest::TestCallback,
+ Unretained(this)));
+}
+
+
} // namespace shill
diff --git a/cellular_unittest.cc b/cellular_unittest.cc
index 6d3cc0c..f5db7d7 100644
--- a/cellular_unittest.cc
+++ b/cellular_unittest.cc
@@ -243,10 +243,15 @@
callback.Run(Error());
}
void InvokeConnectFail(DBusPropertiesMap props, Error *error,
- const ResultCallback &callback, int timeout) {
+ const ResultCallback &callback, int timeout) {
EXPECT_EQ(Service::kStateAssociating, device_->service_->state());
callback.Run(Error(Error::kNotOnHomeNetwork));
}
+ void InvokeConnectFailNoService(DBusPropertiesMap props, Error *error,
+ const ResultCallback &callback, int timeout) {
+ device_->service_ = NULL;
+ callback.Run(Error(Error::kNotOnHomeNetwork));
+ }
void InvokeDisconnect(Error *error, const ResultCallback &callback,
int timeout) {
if (!callback.is_null())
@@ -603,6 +608,24 @@
EXPECT_EQ(Service::kStateFailure, device_->service_->state());
}
+TEST_F(CellularTest, ConnectFailureNoService) {
+ // Make sure we don't crash if the connect failed and there is no
+ // CellularService object. This can happen if the modem is enabled and
+ // then quick disabled.
+ SetCellularType(Cellular::kTypeCDMA);
+ device_->state_ = Cellular::kStateRegistered;
+ device_->service_ = new CellularService(
+ &control_interface_, &dispatcher_, &metrics_, &manager_, device_);
+ EXPECT_CALL(
+ *simple_proxy_,
+ Connect(_, _, _, CellularCapability::kTimeoutConnect))
+ .WillOnce(Invoke(this, &CellularTest::InvokeConnectFailNoService));
+ EXPECT_CALL(manager_, UpdateService(_));
+ GetCapabilityClassic()->simple_proxy_.reset(simple_proxy_.release());
+ Error error;
+ device_->Connect(&error);
+}
+
TEST_F(CellularTest, ModemStateChangeEnable) {
EXPECT_CALL(*simple_proxy_,
GetModemStatus(_, _, CellularCapability::kTimeoutDefault))