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.cc b/download_action.cc
index c17d7e2..d06fb79 100644
--- a/download_action.cc
+++ b/download_action.cc
@@ -25,6 +25,7 @@
     : prefs_(prefs),
       writer_(NULL),
       http_fetcher_(http_fetcher),
+      code_(kActionCodeSuccess),
       delegate_(NULL),
       bytes_received_(0) {}
 
@@ -101,10 +102,12 @@
     LOG_IF(WARNING, writer_->Close() != 0) << "Error closing the writer.";
     writer_ = NULL;
   }
-  http_fetcher_->TerminateTransfer();
   if (delegate_) {
     delegate_->SetDownloadStatus(false);  // Set to inactive.
   }
+  // Terminates the transfer. The action is terminated, if necessary, when the
+  // TransferTerminated callback is received.
+  http_fetcher_->TerminateTransfer();
 }
 
 void DownloadAction::SeekToOffset(off_t offset) {
@@ -119,8 +122,11 @@
     delegate_->BytesReceived(bytes_received_, install_plan_.size);
   if (writer_ && writer_->Write(bytes, length) < 0) {
     LOG(ERROR) << "Write error -- terminating processing.";
+    // Don't tell the action processor that the action is complete until we get
+    // the TransferTerminated callback. Otherwise, this and the HTTP fetcher
+    // objects may get destroyed before all callbacks are complete.
+    code_ = kActionCodeDownloadWriteError;
     TerminateProcessing();
-    processor_->ActionComplete(this, kActionCodeDownloadWriteError);
     return;
   }
   // DeltaPerformer checks the hashes for delta updates.
@@ -194,4 +200,10 @@
   processor_->ActionComplete(this, code);
 }
 
+void DownloadAction::TransferTerminated(HttpFetcher *fetcher) {
+  if (code_ != kActionCodeSuccess) {
+    processor_->ActionComplete(this, code_);
+  }
+}
+
 };  // namespace {}
diff --git a/download_action.h b/download_action.h
index d0ac175..6106740 100644
--- a/download_action.h
+++ b/download_action.h
@@ -88,6 +88,7 @@
                              const char* bytes, int length);
   virtual void SeekToOffset(off_t offset);
   virtual void TransferComplete(HttpFetcher *fetcher, bool successful);
+  virtual void TransferTerminated(HttpFetcher *fetcher);
 
   DownloadActionDelegate* delegate() const { return delegate_; }
   void set_delegate(DownloadActionDelegate* delegate) {
@@ -124,6 +125,10 @@
   // Used to find the hash of the bytes downloaded
   OmahaHashCalculator omaha_hash_calculator_;
 
+  // Used by TransferTerminated to figure if this action terminated itself or
+  // was terminated by the action processor.
+  ActionExitCode code_;
+
   // For reporting status to outsiders
   DownloadActionDelegate* delegate_;
   uint64_t bytes_received_;
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
 }
 
diff --git a/http_fetcher.h b/http_fetcher.h
index d0f9cb2..6d8608b 100644
--- a/http_fetcher.h
+++ b/http_fetcher.h
@@ -45,11 +45,13 @@
   // Downloading should resume from this offset
   virtual void SetOffset(off_t offset) = 0;
 
-  // Begins the transfer to the specified URL.
+  // Begins the transfer to the specified URL. This fetcher instance should not
+  // be destroyed until either TransferComplete, or TransferTerminated is
+  // called.
   virtual void BeginTransfer(const std::string& url) = 0;
 
-  // Aborts the transfer. TransferComplete() will not be called on the
-  // delegate.
+  // Aborts the transfer. The transfer may not abort right away -- delegate's
+  // TransferTerminated() will be called when the transfer is actually done.
   virtual void TerminateTransfer() = 0;
 
   // If data is coming in too quickly, you can call Pause() to pause the
@@ -97,9 +99,14 @@
   // Called if the fetcher seeks to a particular offset.
   virtual void SeekToOffset(off_t offset) {}
 
-  // Called when the transfer has completed successfully or been somehow
-  // aborted.
+  // Called when the transfer has completed successfully or been aborted through
+  // means other than TerminateTransfer. It's OK to destroy the |fetcher| object
+  // in this callback.
   virtual void TransferComplete(HttpFetcher* fetcher, bool successful) = 0;
+
+  // Called when the transfer has been aborted through TerminateTransfer. It's
+  // OK to destroy the |fetcher| object in this callback.
+  virtual void TransferTerminated(HttpFetcher* fetcher) {}
 };
 
 }  // namespace chromeos_update_engine
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_;
 };
 
diff --git a/libcurl_http_fetcher.cc b/libcurl_http_fetcher.cc
index d5358bd..b65d6b1 100644
--- a/libcurl_http_fetcher.cc
+++ b/libcurl_http_fetcher.cc
@@ -28,6 +28,8 @@
 }  // namespace {}
 
 LibcurlHttpFetcher::~LibcurlHttpFetcher() {
+  LOG_IF(ERROR, transfer_in_progress_)
+      << "Destroying the fetcher while a transfer is in progress.";
   CleanUp();
 }
 
@@ -135,11 +137,20 @@
   CurlPerformOnce();
 }
 
+void LibcurlHttpFetcher::ForceTransferTermination() {
+  CleanUp();
+  if (delegate_) {
+    // Note that after the callback returns this object may be destroyed.
+    delegate_->TransferTerminated(this);
+  }
+}
+
 void LibcurlHttpFetcher::TerminateTransfer() {
-  if (in_write_callback_)
+  if (in_write_callback_) {
     terminate_requested_ = true;
-  else
-    CleanUp();
+  } else {
+    ForceTransferTermination();
+  }
 }
 
 void LibcurlHttpFetcher::CurlPerformOnce() {
@@ -152,7 +163,7 @@
   while (CURLM_CALL_MULTI_PERFORM == retcode) {
     retcode = curl_multi_perform(curl_multi_handle_, &running_handles);
     if (terminate_requested_) {
-      CleanUp();
+      ForceTransferTermination();
       return;
     }
   }
diff --git a/libcurl_http_fetcher.h b/libcurl_http_fetcher.h
index 3a50bda..b3ba518 100644
--- a/libcurl_http_fetcher.h
+++ b/libcurl_http_fetcher.h
@@ -48,8 +48,8 @@
   // Begins the transfer if it hasn't already begun.
   virtual void BeginTransfer(const std::string& url);
 
-  // If the transfer is in progress, aborts the transfer early.
-  // The transfer cannot be resumed.
+  // If the transfer is in progress, aborts the transfer early. The transfer
+  // cannot be resumed.
   virtual void TerminateTransfer();
 
   // Suspend the transfer by calling curl_easy_pause(CURLPAUSE_ALL).
@@ -136,6 +136,11 @@
   // curl(m) handles, io_channels_, timeout_source_.
   void CleanUp();
 
+  // Force terminate the transfer. This will invoke the delegate's (if any)
+  // TransferTerminated callback so, after returning, this fetcher instance may
+  // be destroyed.
+  void ForceTransferTermination();
+
   // Returns whether or not the current network connection is considered
   // expensive.
   bool ConnectionIsExpensive() const;
diff --git a/mock_http_fetcher.cc b/mock_http_fetcher.cc
index eec550f..1a88b94 100644
--- a/mock_http_fetcher.cc
+++ b/mock_http_fetcher.cc
@@ -41,6 +41,11 @@
                                   data_.size() - sent_size_);
     CHECK(delegate_);
     delegate_->ReceivedBytes(this, &data_[sent_size_], chunk_size);
+    // We may get terminated in the callback.
+    if (sent_size_ == data_.size()) {
+      LOG(INFO) << "Terminated in the ReceivedBytes callback.";
+      return timeout_source_;
+    }
     sent_size_ += chunk_size;
     CHECK_LE(sent_size_, data_.size());
     if (sent_size_ == data_.size()) {
@@ -81,6 +86,7 @@
 // If the transfer is in progress, aborts the transfer early.
 // The transfer cannot be resumed.
 void MockHttpFetcher::TerminateTransfer() {
+  LOG(INFO) << "Terminating transfer.";
   sent_size_ = data_.size();
   // kill any timeout
   if (timeout_source_) {
@@ -88,6 +94,7 @@
     g_source_destroy(timeout_source_);
     timeout_source_ = NULL;
   }
+  delegate_->TransferTerminated(this);
 }
 
 void MockHttpFetcher::Pause() {
diff --git a/multi_http_fetcher.h b/multi_http_fetcher.h
index 692054d..533998b 100644
--- a/multi_http_fetcher.h
+++ b/multi_http_fetcher.h
@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "update_engine/http_fetcher.h"
+#include "update_engine/utils.h"
 
 // This class is a simple wrapper around an HttpFetcher. The client
 // specifies a vector of byte ranges. MultiHttpFetcher will fetch bytes
@@ -26,6 +27,7 @@
 
   MultiHttpFetcher()
       : sent_transfer_complete_(false),
+        pending_next_fetcher_(false),
         current_index_(0),
         bytes_received_this_fetcher_(0) {}
   ~MultiHttpFetcher() {}
@@ -56,11 +58,41 @@
     StartTransfer();
   }
 
-  void TerminateTransfer() {
-    if (current_index_ < fetchers_.size())
-      fetchers_[current_index_]->TerminateTransfer();
+  void TransferTerminated(HttpFetcher* fetcher) {
+    LOG(INFO) << "Received transfer terminated.";
+    if (pending_next_fetcher_) {
+      pending_next_fetcher_ = false;
+      if (++current_index_ >= ranges_.size()) {
+        SendTransferComplete(fetcher, true);
+      } else {
+        StartTransfer();
+      }
+      return;
+    }
     current_index_ = ranges_.size();
     sent_transfer_complete_ = true;  // a fib
+    if (delegate_) {
+      // Note that after the callback returns this object may be destroyed.
+      delegate_->TransferTerminated(this);
+    }
+  }
+
+  void TerminateTransfer() {
+    // If the current fetcher is already being terminated, just wait for its
+    // TransferTerminated callback.
+    if (pending_next_fetcher_) {
+      pending_next_fetcher_ = false;
+      return;
+    }
+    // If there's a current active fetcher terminate it and wait for its
+    // TransferTerminated callback.
+    if (current_index_ < fetchers_.size()) {
+      fetchers_[current_index_]->TerminateTransfer();
+      return;
+    }
+    // Transfer is terminated before it got started and before any ranges were
+    // added.
+    TransferTerminated(this);
   }
 
   void Pause() {
@@ -127,15 +159,10 @@
     fetchers_[current_index_]->BeginTransfer(url_);
   }
 
-  void ReceivedBytes(HttpFetcher* fetcher,
-                     const char* bytes,
-                     int length) {
-    if (current_index_ >= ranges_.size())
-      return;
-    if (fetcher != fetchers_[current_index_].get()) {
-      LOG(WARNING) << "Received bytes from invalid fetcher";
-      return;
-    }
+  void ReceivedBytes(HttpFetcher* fetcher, const char* bytes, int length) {
+    TEST_AND_RETURN(current_index_ < ranges_.size());
+    TEST_AND_RETURN(fetcher == fetchers_[current_index_].get());
+    TEST_AND_RETURN(!pending_next_fetcher_);
     off_t next_size = length;
     if (ranges_[current_index_].second >= 0) {
       next_size = std::min(next_size,
@@ -149,23 +176,22 @@
     bytes_received_this_fetcher_ += length;
     if (ranges_[current_index_].second >= 0 &&
         bytes_received_this_fetcher_ >= ranges_[current_index_].second) {
-      fetchers_[current_index_]->TerminateTransfer();
-      current_index_++;
-      if (current_index_ == ranges_.size()) {
-        SendTransferComplete(fetchers_[current_index_ - 1].get(), true);
-      } else {
-        StartTransfer();
-      }
+      // Terminates the current fetcher. Waits for its TransferTerminated
+      // callback before starting the next fetcher so that we don't end up
+      // signalling the delegate that the whole multi-transfer is complete
+      // before all fetchers are really done and cleaned up.
+      pending_next_fetcher_ = true;
+      fetcher->TerminateTransfer();
     }
   }
 
   void TransferComplete(HttpFetcher* fetcher, bool successful) {
-    LOG(INFO) << "Received transfer complete";
+    TEST_AND_RETURN(!pending_next_fetcher_);
+    LOG(INFO) << "Received transfer complete.";
     if (current_index_ >= ranges_.size()) {
       SendTransferComplete(fetcher, true);
       return;
     }
-
     if (ranges_[current_index_].second < 0) {
       // We're done with the current operation
       current_index_++;
@@ -177,27 +203,28 @@
       }
       return;
     }
-
     if (bytes_received_this_fetcher_ < ranges_[current_index_].second) {
       LOG(WARNING) << "Received insufficient bytes from fetcher. "
                    << "Ending early";
       SendTransferComplete(fetcher, false);
       return;
-    } else {
-      LOG(INFO) << "Got spurious TransferComplete. Ingoring.";
     }
+    LOG(INFO) << "Got spurious TransferComplete. Ignoring.";
   }
 
   // If true, do not send any more data or TransferComplete to the delegate.
   bool sent_transfer_complete_;
 
+  // If true, the next fetcher needs to be started when TransferTerminated is
+  // received from the current fetcher.
+  bool pending_next_fetcher_;
+
   RangesVect ranges_;
   std::vector<std::tr1::shared_ptr<BaseHttpFetcher> > fetchers_;
 
   RangesVect::size_type current_index_;  // index into ranges_, fetchers_
   off_t bytes_received_this_fetcher_;
 
- private:
   DISALLOW_COPY_AND_ASSIGN(MultiHttpFetcher);
 };