AU: Fix potential issues with premature destruction of HTTP fetchers.
This patch adds a new TransferTerminated callback to the
HttpFetcher class. It fixes two potential memory corruption
issues with premature destruction of HttpFetcher instances:
1. When MultiHttpFetcher completes a range, it terminates
the current fetcher and starts the next one, if any. Change
so that the next fetcher is started when the
TransferTerminated callback is received from the current
fetcher. This prevents the multi fetcher from sending a
TransferComplete/TransferTerminated callbacks before the
underlying fetcher is cleaned up, which may lead to the
fetchers being destroyed prematurely.
2. If the download action fails due to a failed write,
terminate the transfer and then wait for the transfer
terminated callback before notifying the action processor
that the action is complete. Otherwise, the action may get
destroyed before the transfer is actually terminated
possibly leading to memory corruption, etc.
Hopefully these changes fix crosbug.com/8798.
BUG=8798
TEST=unit tests, tested on device with write errors
Change-Id: If416b95625ab31662f2e1308df6bdd1757a2ad78
Review URL: http://codereview.chromium.org/5009009
diff --git a/http_fetcher_unittest.cc b/http_fetcher_unittest.cc
index 30cde95..87dda78 100644
--- a/http_fetcher_unittest.cc
+++ b/http_fetcher_unittest.cc
@@ -196,6 +196,9 @@
EXPECT_EQ(200, fetcher->http_response_code());
g_main_loop_quit(loop_);
}
+ virtual void TransferTerminated(HttpFetcher* fetcher) {
+ ADD_FAILURE();
+ }
GMainLoop* loop_;
};
@@ -264,6 +267,9 @@
virtual void TransferComplete(HttpFetcher* fetcher, bool successful) {
g_main_loop_quit(loop_);
}
+ virtual void TransferTerminated(HttpFetcher* fetcher) {
+ ADD_FAILURE();
+ }
void Unpause() {
CHECK(paused_);
paused_ = false;
@@ -314,9 +320,17 @@
virtual void ReceivedBytes(HttpFetcher* fetcher,
const char* bytes, int length) {}
virtual void TransferComplete(HttpFetcher* fetcher, bool successful) {
- CHECK(false); // We should never get here
+ ADD_FAILURE(); // We should never get here
g_main_loop_quit(loop_);
}
+ virtual void TransferTerminated(HttpFetcher* fetcher) {
+ EXPECT_EQ(fetcher, fetcher_.get());
+ EXPECT_FALSE(once_);
+ EXPECT_TRUE(callback_once_);
+ callback_once_ = false;
+ // |fetcher| can be destroyed during this callback.
+ fetcher_.reset(NULL);
+ }
void TerminateTransfer() {
CHECK(once_);
once_ = false;
@@ -326,7 +340,8 @@
g_main_loop_quit(loop_);
}
bool once_;
- HttpFetcher* fetcher_;
+ bool callback_once_;
+ scoped_ptr<HttpFetcher> fetcher_;
GMainLoop* loop_;
};
@@ -347,11 +362,11 @@
GMainLoop* loop = g_main_loop_new(g_main_context_default(), FALSE);
{
AbortingHttpFetcherTestDelegate delegate;
- scoped_ptr<HttpFetcher> fetcher(this->NewLargeFetcher());
+ delegate.fetcher_.reset(this->NewLargeFetcher());
delegate.once_ = true;
+ delegate.callback_once_ = true;
delegate.loop_ = loop;
- delegate.fetcher_ = fetcher.get();
- fetcher->set_delegate(&delegate);
+ delegate.fetcher_->set_delegate(&delegate);
typename TestFixture::HttpServer server;
this->IgnoreServerAborting(&server);
@@ -361,9 +376,11 @@
g_source_set_callback(timeout_source_, AbortingTimeoutCallback, &delegate,
NULL);
g_source_attach(timeout_source_, NULL);
- fetcher->BeginTransfer(this->BigUrl());
+ delegate.fetcher_->BeginTransfer(this->BigUrl());
g_main_loop_run(loop);
+ CHECK(!delegate.once_);
+ CHECK(!delegate.callback_once_);
g_source_destroy(timeout_source_);
}
g_main_loop_unref(loop);
@@ -381,6 +398,9 @@
EXPECT_EQ(206, fetcher->http_response_code());
g_main_loop_quit(loop_);
}
+ virtual void TransferTerminated(HttpFetcher* fetcher) {
+ ADD_FAILURE();
+ }
string data;
GMainLoop* loop_;
};
@@ -435,6 +455,9 @@
EXPECT_EQ(0, fetcher->http_response_code());
g_main_loop_quit(loop_);
}
+ virtual void TransferTerminated(HttpFetcher* fetcher) {
+ ADD_FAILURE();
+ }
GMainLoop* loop_;
PythonHttpServer* server_;
};
@@ -509,6 +532,9 @@
}
g_main_loop_quit(loop_);
}
+ virtual void TransferTerminated(HttpFetcher* fetcher) {
+ ADD_FAILURE();
+ }
bool expected_successful_;
string data;
GMainLoop* loop_;
@@ -588,14 +614,22 @@
: expected_response_code_(expected_response_code) {}
virtual void ReceivedBytes(HttpFetcher* fetcher,
const char* bytes, int length) {
+ EXPECT_EQ(fetcher, fetcher_.get());
data.append(bytes, length);
}
virtual void TransferComplete(HttpFetcher* fetcher, bool successful) {
+ EXPECT_EQ(fetcher, fetcher_.get());
EXPECT_EQ(expected_response_code_ != 0, successful);
if (expected_response_code_ != 0)
EXPECT_EQ(expected_response_code_, fetcher->http_response_code());
+ // Destroy the fetcher (because we're allowed to).
+ fetcher_.reset(NULL);
g_main_loop_quit(loop_);
}
+ virtual void TransferTerminated(HttpFetcher* fetcher) {
+ ADD_FAILURE();
+ }
+ scoped_ptr<HttpFetcher> fetcher_;
int expected_response_code_;
string data;
GMainLoop* loop_;
@@ -611,16 +645,16 @@
{
MultiHttpFetcherTestDelegate delegate(expected_response_code);
delegate.loop_ = loop;
- scoped_ptr<HttpFetcher> fetcher(fetcher_in);
+ delegate.fetcher_.reset(fetcher_in);
MultiHttpFetcher<LibcurlHttpFetcher>* multi_fetcher =
- dynamic_cast<MultiHttpFetcher<LibcurlHttpFetcher>*>(fetcher.get());
+ dynamic_cast<MultiHttpFetcher<LibcurlHttpFetcher>*>(fetcher_in);
ASSERT_TRUE(multi_fetcher);
multi_fetcher->set_ranges(ranges);
multi_fetcher->SetConnectionAsExpensive(false);
multi_fetcher->SetBuildType(false);
- fetcher->set_delegate(&delegate);
+ multi_fetcher->set_delegate(&delegate);
- StartTransferArgs start_xfer_args = {fetcher.get(), url};
+ StartTransferArgs start_xfer_args = {multi_fetcher, url};
g_timeout_add(0, StartTransfer, &start_xfer_args);
g_main_loop_run(loop);
@@ -633,7 +667,7 @@
}
} // namespace {}
-TYPED_TEST(HttpFetcherTest, MultiHttpFetcherSimplTest) {
+TYPED_TEST(HttpFetcherTest, MultiHttpFetcherSimpleTest) {
if (!this->IsMulti())
return;
typename TestFixture::HttpServer server;
@@ -713,6 +747,9 @@
EXPECT_FALSE(successful);
g_main_loop_quit(loop_);
}
+ virtual void TransferTerminated(HttpFetcher* fetcher) {
+ ADD_FAILURE();
+ }
GMainLoop* loop_;
};