Fix terminating a transfer while resolving proxies.
Calling TerminateTransfer() on an HttpFetcher should cancel the current
transfer regardless of where it is sitting. If TerminateTransfer() was
called right after BeginTransfer(), the fetcher would be waiting for
the proxy resolution callback which will kick the download. This patch
fixes this case by releasing the proxy callback when terminating a
transfer.
Bug: 34178297
Test: Added a unittest to trigger this case.
Change-Id: I282d04995bd0d03f9a469c80c1e263f9902e4be2
diff --git a/common/http_fetcher.cc b/common/http_fetcher.cc
index e592cc5..4fd1082 100644
--- a/common/http_fetcher.cc
+++ b/common/http_fetcher.cc
@@ -26,10 +26,7 @@
namespace chromeos_update_engine {
HttpFetcher::~HttpFetcher() {
- if (no_resolver_idle_id_ != MessageLoop::kTaskIdNull) {
- MessageLoop::current()->CancelTask(no_resolver_idle_id_);
- no_resolver_idle_id_ = MessageLoop::kTaskIdNull;
- }
+ CancelProxyResolution();
}
void HttpFetcher::SetPostData(const void* data, size_t size,
@@ -59,23 +56,42 @@
base::Unretained(this)));
return true;
}
- return proxy_resolver_->GetProxiesForUrl(
+ proxy_request_ = proxy_resolver_->GetProxiesForUrl(
url, base::Bind(&HttpFetcher::ProxiesResolved, base::Unretained(this)));
+ if (proxy_request_ == kProxyRequestIdNull) {
+ callback_.reset();
+ return false;
+ }
+ return true;
}
void HttpFetcher::NoProxyResolverCallback() {
+ no_resolver_idle_id_ = MessageLoop::kTaskIdNull;
ProxiesResolved(deque<string>());
}
void HttpFetcher::ProxiesResolved(const deque<string>& proxies) {
- no_resolver_idle_id_ = MessageLoop::kTaskIdNull;
+ proxy_request_ = kProxyRequestIdNull;
if (!proxies.empty())
SetProxies(proxies);
- CHECK_NE(static_cast<Closure*>(nullptr), callback_.get());
+ CHECK(callback_.get()) << "ProxiesResolved but none pending.";
Closure* callback = callback_.release();
// This may indirectly call back into ResolveProxiesForUrl():
callback->Run();
delete callback;
}
+bool HttpFetcher::CancelProxyResolution() {
+ bool ret = false;
+ if (no_resolver_idle_id_ != MessageLoop::kTaskIdNull) {
+ ret = MessageLoop::current()->CancelTask(no_resolver_idle_id_);
+ no_resolver_idle_id_ = MessageLoop::kTaskIdNull;
+ }
+ if (proxy_request_ && proxy_resolver_) {
+ ret = proxy_resolver_->CancelProxyRequest(proxy_request_) || ret;
+ proxy_request_ = kProxyRequestIdNull;
+ }
+ return ret;
+}
+
} // namespace chromeos_update_engine
diff --git a/common/http_fetcher.h b/common/http_fetcher.h
index 9f81879..fb15689 100644
--- a/common/http_fetcher.h
+++ b/common/http_fetcher.h
@@ -134,6 +134,11 @@
ProxyResolver* proxy_resolver() const { return proxy_resolver_; }
protected:
+ // Cancels a proxy resolution in progress. The callback passed to
+ // ResolveProxiesForUrl() will not be called. Returns whether there was a
+ // pending proxy resolution to be canceled.
+ bool CancelProxyResolution();
+
// The URL we're actively fetching from
std::string url_;
@@ -170,6 +175,10 @@
// |proxy_resolver_|.
void NoProxyResolverCallback();
+ // Stores the ongoing proxy request id if there is one, otherwise
+ // kProxyRequestIdNull.
+ ProxyRequestId proxy_request_{kProxyRequestIdNull};
+
DISALLOW_COPY_AND_ASSIGN(HttpFetcher);
};
diff --git a/common/http_fetcher_unittest.cc b/common/http_fetcher_unittest.cc
index 11a20e9..e3f3c6d 100644
--- a/common/http_fetcher_unittest.cc
+++ b/common/http_fetcher_unittest.cc
@@ -432,7 +432,6 @@
}
void TransferTerminated(HttpFetcher* fetcher) override {
- ADD_FAILURE();
times_transfer_terminated_called_++;
MessageLoop::current()->BreakLoop();
}
@@ -468,6 +467,7 @@
fetcher.get(),
this->test_.SmallUrl(server->GetPort())));
this->loop_.Run();
+ EXPECT_EQ(0, delegate.times_transfer_terminated_called_);
}
TYPED_TEST(HttpFetcherTest, SimpleBigTest) {
@@ -483,6 +483,7 @@
fetcher.get(),
this->test_.BigUrl(server->GetPort())));
this->loop_.Run();
+ EXPECT_EQ(0, delegate.times_transfer_terminated_called_);
}
// Issue #9648: when server returns an error HTTP response, the fetcher needs to
@@ -508,13 +509,13 @@
this->loop_.Run();
// Make sure that no bytes were received.
- CHECK_EQ(delegate.times_received_bytes_called_, 0);
- CHECK_EQ(fetcher->GetBytesDownloaded(), static_cast<size_t>(0));
+ EXPECT_EQ(0, delegate.times_received_bytes_called_);
+ EXPECT_EQ(0U, fetcher->GetBytesDownloaded());
// Make sure that transfer completion was signaled once, and no termination
// was signaled.
- CHECK_EQ(delegate.times_transfer_complete_called_, 1);
- CHECK_EQ(delegate.times_transfer_terminated_called_, 0);
+ EXPECT_EQ(1, delegate.times_transfer_complete_called_);
+ EXPECT_EQ(0, delegate.times_transfer_terminated_called_);
}
TYPED_TEST(HttpFetcherTest, ExtraHeadersInRequestTest) {
@@ -706,6 +707,32 @@
this->loop_.CancelTask(task_id);
}
+TYPED_TEST(HttpFetcherTest, TerminateTransferWhileResolvingProxyTest) {
+ if (this->test_.IsMock() || !this->test_.IsHttpSupported())
+ return;
+ MockProxyResolver mock_resolver;
+ unique_ptr<HttpFetcher> fetcher(this->test_.NewLargeFetcher(&mock_resolver));
+
+ HttpFetcherTestDelegate delegate;
+ fetcher->set_delegate(&delegate);
+
+ EXPECT_CALL(mock_resolver, GetProxiesForUrl(_, _)).WillOnce(Return(123));
+ fetcher->BeginTransfer("http://fake_url");
+ // Run the message loop until idle. This must call the MockProxyResolver with
+ // the request.
+ while (this->loop_.RunOnce(false)) {
+ }
+ testing::Mock::VerifyAndClearExpectations(&mock_resolver);
+
+ EXPECT_CALL(mock_resolver, CancelProxyRequest(123)).WillOnce(Return(true));
+
+ // Terminate the transfer right before the proxy resolution response.
+ fetcher->TerminateTransfer();
+ EXPECT_EQ(0, delegate.times_received_bytes_called_);
+ EXPECT_EQ(0, delegate.times_transfer_complete_called_);
+ EXPECT_EQ(1, delegate.times_transfer_terminated_called_);
+}
+
namespace {
class FlakyHttpFetcherTestDelegate : public HttpFetcherDelegate {
public:
