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/download_action_unittest.cc b/download_action_unittest.cc
index 2f37dfb..8135841 100644
--- a/download_action_unittest.cc
+++ b/download_action_unittest.cc
@@ -49,9 +49,11 @@
g_main_loop_quit(loop_);
vector<char> found_data;
ASSERT_TRUE(utils::ReadFile(path_, &found_data));
- ASSERT_EQ(expected_data_.size(), found_data.size());
- for (unsigned i = 0; i < expected_data_.size(); i++) {
- EXPECT_EQ(expected_data_[i], found_data[i]);
+ if (expected_code_ != kActionCodeDownloadWriteError) {
+ ASSERT_EQ(expected_data_.size(), found_data.size());
+ for (unsigned i = 0; i < expected_data_.size(); i++) {
+ EXPECT_EQ(expected_data_[i], found_data[i]);
+ }
}
processing_done_called_ = true;
}
@@ -74,6 +76,24 @@
ActionExitCode expected_code_;
};
+class TestDirectFileWriter : public DirectFileWriter {
+ public:
+ TestDirectFileWriter() : fail_write_(0), current_write_(0) {}
+ void set_fail_write(int fail_write) { fail_write_ = fail_write; }
+
+ virtual ssize_t Write(const void* bytes, size_t count) {
+ if (++current_write_ == fail_write_) {
+ return -EINVAL;
+ }
+ return DirectFileWriter::Write(bytes, count);
+ }
+
+ private:
+ // If positive, fail on the |fail_write_| call to Write.
+ int fail_write_;
+ int current_write_;
+};
+
struct EntryPointArgs {
const vector<char> *data;
GMainLoop *loop;
@@ -98,12 +118,14 @@
void TestWithData(const vector<char>& data,
bool hash_test,
bool size_test,
+ int fail_write,
bool use_download_delegate) {
GMainLoop *loop = g_main_loop_new(g_main_context_default(), FALSE);
// TODO(adlr): see if we need a different file for build bots
ScopedTempFile output_temp_file;
- DirectFileWriter writer;
+ TestDirectFileWriter writer;
+ writer.set_fail_write(fail_write);
// We pull off the first byte from data and seek past it.
@@ -134,8 +156,7 @@
if (data.size() > kMockHttpFetcherChunkSize)
EXPECT_CALL(download_delegate,
BytesReceived(1 + kMockHttpFetcherChunkSize, _));
-
- EXPECT_CALL(download_delegate, BytesReceived(_, _)).Times(AtLeast(1));
+ EXPECT_CALL(download_delegate, BytesReceived(_, _)).Times(AtLeast(1));
EXPECT_CALL(download_delegate, SetDownloadStatus(false)).Times(1);
}
ActionExitCode expected_code = kActionCodeSuccess;
@@ -143,6 +164,8 @@
expected_code = kActionCodeDownloadHashMismatchError;
else if (size_test)
expected_code = kActionCodeDownloadSizeMismatchError;
+ else if (fail_write > 0)
+ expected_code = kActionCodeDownloadWriteError;
DownloadActionTestProcessorDelegate delegate(expected_code);
delegate.loop_ = loop;
delegate.expected_data_ = vector<char>(data.begin() + 1, data.end());
@@ -168,6 +191,7 @@
TestWithData(small,
false, // hash_test
false, // size_test
+ 0, // fail_write
true); // use_download_delegate
}
@@ -176,14 +200,26 @@
char c = '0';
for (unsigned int i = 0; i < big.size(); i++) {
big[i] = c;
- if ('9' == c)
- c = '0';
- else
- c++;
+ c = ('9' == c) ? '0' : c + 1;
}
TestWithData(big,
false, // hash_test
false, // size_test
+ 0, // fail_write
+ true); // use_download_delegate
+}
+
+TEST(DownloadActionTest, FailWriteTest) {
+ vector<char> big(5 * kMockHttpFetcherChunkSize);
+ char c = '0';
+ for (unsigned int i = 0; i < big.size(); i++) {
+ big[i] = c;
+ c = ('9' == c) ? '0' : c + 1;
+ }
+ TestWithData(big,
+ false, // hash_test
+ false, // size_test
+ 2, // fail_write
true); // use_download_delegate
}
@@ -194,6 +230,7 @@
TestWithData(small,
true, // hash_test
false, // size_test
+ 0, // fail_write
true); // use_download_delegate
}
@@ -203,6 +240,7 @@
TestWithData(small,
false, // hash_test
true, // size_test
+ 0, // fail_write
true); // use_download_delegate
}
@@ -213,6 +251,7 @@
TestWithData(small,
false, // hash_test
false, // size_test
+ 0, // fail_write
false); // use_download_delegate
}