shill: AsyncConnection: Call callback last
..since it may actually end up deleting the AsyncConnection.
For example, this happens in the HTTPRequest callback:
HTTPRequest::OnConnectCompletion(false, -1)
HTTPRequest::SendStatus(kResultConnectionFailure)
result_callback.Run() in the case of portal detector resolves to...
PortalDetector::RequestResultCallback()
PortalDetector::CompleteAttempt()
PortalDetector::Stop()
request_.reset()
this frees the AsyncConnection as well, since it is owned by the
HTTPRequest. As such this is only safe if OnConnectCompletion does
not do anything more with |this| when the callback returns.
BUG=chromium-os:34655
TEST=Unit tests
Change-Id: I37056c5f2a330409920f117ebeb4e40c5366593e
Reviewed-on: https://gerrit.chromium.org/gerrit/33800
Reviewed-by: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
Commit-Ready: Paul Stewart <pstew@chromium.org>
diff --git a/async_connection.cc b/async_connection.cc
index e897b1c..85c4f14 100644
--- a/async_connection.cc
+++ b/async_connection.cc
@@ -102,16 +102,21 @@
void AsyncConnection::OnConnectCompletion(int fd) {
CHECK_EQ(fd_, fd);
+ bool success = false;
+ int returned_fd = -1;
if (sockets_->GetSocketError(fd_) != 0) {
error_ = sockets_->ErrorString();
PLOG(ERROR) << "Async GetSocketError returns failure";
- callback_.Run(false, -1);
} else {
- callback_.Run(true, fd_); // Passes ownership
+ returned_fd = fd_;
fd_ = -1;
+ success = true;
}
Stop();
+
+ // Run the callback last, since it may end up freeing this instance.
+ callback_.Run(success, returned_fd); // Passes ownership
}
} // namespace shill
diff --git a/async_connection_unittest.cc b/async_connection_unittest.cc
index ee9a41e..ba05699 100644
--- a/async_connection_unittest.cc
+++ b/async_connection_unittest.cc
@@ -39,19 +39,23 @@
class AsyncConnectionTest : public Test {
public:
AsyncConnectionTest()
- : async_connection_(kInterfaceName, &dispatcher_, &sockets_,
- callback_target_.callback()),
+ : async_connection_(
+ new AsyncConnection(kInterfaceName, &dispatcher_, &sockets_,
+ callback_target_.callback())),
address_(IPAddress::kFamilyIPv4) { }
virtual void SetUp() {
EXPECT_TRUE(address_.SetAddressFromString(kConnectAddress));
}
virtual void TearDown() {
- if (async_connection_.fd_ >= 0) {
+ if (async_connection_.get() && async_connection_->fd_ >= 0) {
EXPECT_CALL(sockets(), Close(kSocketFD))
.WillOnce(Return(0));
}
}
+ void InvokeFreeConnection(bool /*success*/, int /*fd*/) {
+ async_connection_.reset();
+ }
protected:
class ConnectCallbackTarget {
@@ -68,14 +72,14 @@
};
void ExpectReset() {
- EXPECT_STREQ(kInterfaceName, async_connection_.interface_name_.c_str());
- EXPECT_EQ(&dispatcher_, async_connection_.dispatcher_);
- EXPECT_EQ(&sockets_, async_connection_.sockets_);
+ EXPECT_STREQ(kInterfaceName, async_connection_->interface_name_.c_str());
+ EXPECT_EQ(&dispatcher_, async_connection_->dispatcher_);
+ EXPECT_EQ(&sockets_, async_connection_->sockets_);
EXPECT_TRUE(callback_target_.callback().
- Equals(async_connection_.callback_));
- EXPECT_EQ(-1, async_connection_.fd_);
- EXPECT_FALSE(async_connection_.connect_completion_callback_.is_null());
- EXPECT_FALSE(async_connection_.connect_completion_handler_.get());
+ Equals(async_connection_->callback_));
+ EXPECT_EQ(-1, async_connection_->fd_);
+ EXPECT_FALSE(async_connection_->connect_completion_callback_.is_null());
+ EXPECT_FALSE(async_connection_->connect_completion_handler_.get());
}
void StartConnection() {
@@ -96,14 +100,14 @@
}
void OnConnectCompletion(int fd) {
- async_connection_.OnConnectCompletion(fd);
+ async_connection_->OnConnectCompletion(fd);
}
- AsyncConnection &async_connection() { return async_connection_; }
+ AsyncConnection &async_connection() { return *async_connection_.get(); }
StrictMock<MockSockets> &sockets() { return sockets_; }
MockEventDispatcher &dispatcher() { return dispatcher_; }
const IPAddress &address() { return address_; }
- int fd() { return async_connection_.fd_; }
- void set_fd(int fd) { async_connection_.fd_ = fd; }
+ int fd() { return async_connection_->fd_; }
+ void set_fd(int fd) { async_connection_->fd_ = fd; }
StrictMock<ConnectCallbackTarget> &callback_target() {
return callback_target_;
}
@@ -112,7 +116,7 @@
MockEventDispatcher dispatcher_;
StrictMock<MockSockets> sockets_;
StrictMock<ConnectCallbackTarget> callback_target_;
- AsyncConnection async_connection_;
+ scoped_ptr<AsyncConnection> async_connection_;
IPAddress address_;
};
@@ -248,4 +252,26 @@
ExpectReset();
}
+TEST_F(AsyncConnectionTest, FreeOnSuccessCallback) {
+ StartConnection();
+ EXPECT_CALL(sockets(), GetSocketError(kSocketFD))
+ .WillOnce(Return(0));
+ EXPECT_CALL(callback_target(), CallTarget(true, kSocketFD))
+ .WillOnce(Invoke(this, &AsyncConnectionTest::InvokeFreeConnection));
+ OnConnectCompletion(kSocketFD);
+}
+
+TEST_F(AsyncConnectionTest, FreeOnFailureCallback) {
+ StartConnection();
+ EXPECT_CALL(sockets(), GetSocketError(kSocketFD))
+ .WillOnce(Return(1));
+ EXPECT_CALL(callback_target(), CallTarget(false, -1))
+ .WillOnce(Invoke(this, &AsyncConnectionTest::InvokeFreeConnection));
+ EXPECT_CALL(sockets(), Error())
+ .WillOnce(Return(kErrorNumber));
+ EXPECT_CALL(sockets(), Close(kSocketFD))
+ .WillOnce(Return(0));
+ OnConnectCompletion(kSocketFD);
+}
+
} // namespace shill
diff --git a/http_request.cc b/http_request.cc
index 4a142d6..98d9020 100644
--- a/http_request.cc
+++ b/http_request.cc
@@ -186,6 +186,7 @@
<< server_hostname_
<< ": "
<< server_async_connection_->error();
+ // |this| could be freed as a result of calling SendStatus().
SendStatus(kResultConnectionFailure);
return;
}