shill: DHCPConfig: Reorder callback and lease expiration timer
The lease expiration timer should be started before calling the
"UpdateProperties" method, since the latter may end up stopping
the DHCPConfig instance. Otherwise, we may inadevertently start
an expiration timer with no running DHCP process.
The concept of having an expiration timer is attractive, so that
even with a static IP configuration (the plausible reason that
the config would be stopped) it might be attractive to
periodically restart the lease acquisition process. However it
probably makes sense to use a different timer for this purpose.
BUG=chromium:364735
TEST=Unit tests, repro of bug steps
Change-Id: I252dbeb29fc9daf6f2522ebed8c3886df2b50bb7
Reviewed-on: https://chromium-review.googlesource.com/195560
Reviewed-by: mukesh agrawal <quiche@chromium.org>
Commit-Queue: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
diff --git a/dhcp_config_unittest.cc b/dhcp_config_unittest.cc
index c449ae3..f3aa275 100644
--- a/dhcp_config_unittest.cc
+++ b/dhcp_config_unittest.cc
@@ -31,6 +31,7 @@
using testing::_;
using testing::AnyNumber;
using testing::ContainsRegex;
+using testing::InvokeWithoutArgs;
using testing::Mock;
using testing::Return;
using testing::SetArgumentPointee;
@@ -73,6 +74,10 @@
config_->minijail_ = NULL;
}
+ void StopInstance() {
+ config_->Stop("In test");
+ }
+
DHCPConfigRefPtr CreateMockMinijailConfig(const string &hostname,
const string &lease_suffix,
bool arp_gateway,
@@ -486,6 +491,41 @@
}
}
+TEST_F(DHCPConfigCallbackTest, StoppedDuringFailureCallback) {
+ DHCPConfig::Configuration conf;
+ conf[DHCPConfig::kConfigurationKeyIPAddress].writer().append_uint32(
+ 0x01020304);
+ // Stop the DHCP config while it is calling the failure callback. We
+ // need to ensure that no callbacks are left running inadvertently as
+ // a result.
+ EXPECT_CALL(*this, FailureCallback(ConfigRef()))
+ .WillOnce(InvokeWithoutArgs(this, &DHCPConfigTest::StopInstance));
+ config_->ProcessEventSignal(DHCPConfig::kReasonFail, conf);
+ EXPECT_TRUE(Mock::VerifyAndClearExpectations(this));
+ EXPECT_TRUE(config_->lease_acquisition_timeout_callback_.IsCancelled());
+ EXPECT_TRUE(config_->lease_expiration_callback_.IsCancelled());
+}
+
+TEST_F(DHCPConfigCallbackTest, StoppedDuringSuccessCallback) {
+ DHCPConfig::Configuration conf;
+ conf[DHCPConfig::kConfigurationKeyIPAddress].writer().append_uint32(
+ 0x01020304);
+ const uint32 kLeaseTime = 1;
+ conf[DHCPConfig::kConfigurationKeyLeaseTime].writer().append_uint32(
+ kLeaseTime);
+ // Stop the DHCP config while it is calling the success callback. This
+ // can happen if the device has a static IP configuration and releases
+ // the lease after accepting other network parameters from the DHCP
+ // IPConfig properties. We need to ensure that no callbacks are left
+ // running inadvertently as a result.
+ EXPECT_CALL(*this, SuccessCallback(ConfigRef()))
+ .WillOnce(InvokeWithoutArgs(this, &DHCPConfigTest::StopInstance));
+ config_->ProcessEventSignal(DHCPConfig::kReasonBound, conf);
+ EXPECT_TRUE(Mock::VerifyAndClearExpectations(this));
+ EXPECT_TRUE(config_->lease_acquisition_timeout_callback_.IsCancelled());
+ EXPECT_TRUE(config_->lease_expiration_callback_.IsCancelled());
+}
+
TEST_F(DHCPConfigCallbackTest, ProcessEventSignalUnknown) {
DHCPConfig::Configuration conf;
conf[DHCPConfig::kConfigurationKeyIPAddress].writer().append_uint32(