Partially integrate filename determination with resumption.

Also substantially increased test coverage.

BUG=7648
R=asanka@chromium.org


Review URL: https://chromiumcodereview.appspot.com/12308011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@188562 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 737 insertions, 133 deletions

diff --git a/content/browser/download/byte_stream.cc b/content/browser/download/byte_stream.cc
index ee442d4..26224ad 100644
--- a/content/browser/download/byte_stream.cc
+++ b/content/browser/download/byte_stream.cc
@@ -403,12 +403,8 @@ void ByteStreamReaderImpl::MaybeUpdateInput() {
 
 }  // namespace
 
-// The fraction of the buffer that must be ready to send on the input
-// before we ship data to the output.
-const int ByteStreamWriter::kFractionBufferBeforeSending = 3;
 
-// The fraction of the buffer that must have been consumed on the output
-// before we update the input window.
+const int ByteStreamWriter::kFractionBufferBeforeSending = 3;
 const int ByteStreamReader::kFractionReadBeforeWindowUpdate = 3;
 
 ByteStreamReader::~ByteStreamReader() { }
diff --git a/content/browser/download/download_browsertest.cc b/content/browser/download/download_browsertest.cc
index 9e8921c..a62cd89 100644
--- a/content/browser/download/download_browsertest.cc
+++ b/content/browser/download/download_browsertest.cc
@@ -19,6 +19,7 @@
 #include "content/public/browser/power_save_blocker.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/download_test_observer.h"
+#include "content/public/test/test_file_error_injector.h"
 #include "content/public/test/test_utils.h"
 #include "content/shell/shell.h"
 #include "content/shell/shell_browser_context.h"
@@ -349,23 +350,113 @@ class TestShellDownloadManagerDelegate : public ShellDownloadManagerDelegate {
   std::vector<DownloadOpenDelayedCallback> delayed_callbacks_;
 };
 
-// Filter for handling resumption; don't return true until
-// we see first a transition to IN_PROGRESS then a transition to
-// some final state.  Since this is a stateful filter, we
-// must provide a flag in which to store the state; that flag
-// must be false on initialization.  The flag must be the first argument
-// so that Bind() can be used to produce the callback expected by
-// DownloadUpdatedObserver.
-bool DownloadResumptionFilter(bool* state_flag, DownloadItem* download) {
-  if (*state_flag && DownloadItem::IN_PROGRESS != download->GetState()) {
-    return true;
+// Record all state transitions and byte counts on the observed download.
+class RecordingDownloadObserver : DownloadItem::Observer {
+ public:
+  struct RecordStruct {
+    DownloadItem::DownloadState state;
+    int bytes_received;
+  };
+
+  typedef std::vector<RecordStruct> RecordVector;
+
+  RecordingDownloadObserver(DownloadItem* download)
+      : download_(download) {
+    last_state_.state = download->GetState();
+    last_state_.bytes_received = download->GetReceivedBytes();
+    download_->AddObserver(this);
   }
 
-  if (DownloadItem::IN_PROGRESS == download->GetState())
-    *state_flag = true;
+  virtual ~RecordingDownloadObserver() {
+    RemoveObserver();
+  }
+
+  void CompareToExpectedRecord(const RecordStruct expected[], size_t size) {
+    EXPECT_EQ(size, record_.size());
+    int min = size > record_.size() ? record_.size() : size;
+    for (int i = 0; i < min; ++i) {
+      EXPECT_EQ(expected[i].state, record_[i].state) << "Iteration " << i;
+      EXPECT_EQ(expected[i].bytes_received, record_[i].bytes_received)
+          << "Iteration " << i;
+    }
+  }
+
+ private:
+  virtual void OnDownloadUpdated(DownloadItem* download) OVERRIDE {
+    DCHECK_EQ(download_, download);
+    DownloadItem::DownloadState state = download->GetState();
+    int bytes = download->GetReceivedBytes();
+    if (last_state_.state != state || last_state_.bytes_received > bytes) {
+      last_state_.state = state;
+      last_state_.bytes_received = bytes;
+      record_.push_back(last_state_);
+    }
+  }
+
+  virtual void OnDownloadDestroyed(DownloadItem* download) OVERRIDE {
+    DCHECK_EQ(download_, download);
+    RemoveObserver();
+  }
+
+  void RemoveObserver() {
+    if (download_) {
+      download_->RemoveObserver(this);
+      download_ = NULL;
+    }
+  }
+
+  DownloadItem* download_;
+  RecordStruct last_state_;
+  RecordVector record_;
+};
+
+// Get the next created download.
+class DownloadCreateObserver : DownloadManager::Observer {
+ public:
+  DownloadCreateObserver(DownloadManager* manager)
+      : manager_(manager),
+        item_(NULL),
+        waiting_(false) {
+    manager_->AddObserver(this);
+  }
+
+  ~DownloadCreateObserver() {
+    if (manager_)
+      manager_->RemoveObserver(this);
+    manager_ = NULL;
+  }
+
+  virtual void ManagerGoingDown(DownloadManager* manager) {
+    DCHECK_EQ(manager_, manager);
+    manager_->RemoveObserver(this);
+    manager_ = NULL;
+  }
+
+  virtual void OnDownloadCreated(DownloadManager* manager,
+                                 DownloadItem* download) {
+    if (!item_)
+      item_ = download;
+
+    if (waiting_)
+      MessageLoopForUI::current()->Quit();
+  }
+
+  DownloadItem* WaitForFinished() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    if (!item_) {
+      waiting_ = true;
+      RunMessageLoop();
+      waiting_ = false;
+    }
+    return item_;
+  }
+
+ private:
+  DownloadManager* manager_;
+  DownloadItem* item_;
+  bool waiting_;
+};
 
-  return false;
-}
 
 // Filter for waiting for intermediate file rename.
 bool IntermediateFileRenameFilter(DownloadItem* download) {
@@ -377,10 +468,36 @@ bool DataReceivedFilter(int number_of_bytes, DownloadItem* download) {
   return download->GetReceivedBytes() >= number_of_bytes;
 }
 
+// Filter for download completion.
+bool DownloadCompleteFilter(DownloadItem* download) {
+  return download->GetState() == DownloadItem::COMPLETE;
+}
+
+// Filter for saving the size of the download when the first IN_PROGRESS
+// is hit.
+bool InitialSizeFilter(int* download_size, DownloadItem* download) {
+  if (download->GetState() != DownloadItem::IN_PROGRESS)
+    return false;
+
+  *download_size = download->GetReceivedBytes();
+  return true;
+}
+
 }  // namespace
 
 class DownloadContentTest : public ContentBrowserTest {
  protected:
+  // An initial send from a website of at least this size will not be
+  // help up by buffering in the underlying downloads ByteStream data
+  // transfer.  This is important because on resumption tests we wait
+  // until we've gotten the data we expect before allowing the test server
+  // to send its reset, to get around hard close semantics on the Windows
+  // socket layer implementation.
+  int GetSafeBufferChunk() const {
+    return (DownloadResourceHandler::kDownloadByteStreamSize /
+       ByteStreamWriter::kFractionBufferBeforeSending) + 1;
+  }
+
   virtual void SetUpOnMainThread() OVERRIDE {
     ASSERT_TRUE(downloads_directory_.CreateUniqueTempDir());
 
@@ -417,10 +534,10 @@ class DownloadContentTest : public ContentBrowserTest {
 
   // Create a DownloadTestObserverInProgress that will wait for the
   // specified number of downloads to start.
-  DownloadTestObserver* CreateInProgressWaiter(
+  DownloadCreateObserver* CreateInProgressWaiter(
       Shell* shell, int num_downloads) {
     DownloadManager* download_manager = DownloadManagerForShell(shell);
-    return new DownloadTestObserverInProgress(download_manager, num_downloads);
+    return new DownloadCreateObserver(download_manager);
   }
 
   // Note: Cannot be used with other alternative DownloadFileFactorys
@@ -477,6 +594,70 @@ class DownloadContentTest : public ContentBrowserTest {
     return true;
   }
 
+  // Start a download and return the item.
+  DownloadItem* StartDownloadAndReturnItem(GURL url) {
+    scoped_ptr<DownloadCreateObserver> observer(
+        CreateInProgressWaiter(shell(), 1));
+    NavigateToURL(shell(), url);
+    observer->WaitForFinished();
+    std::vector<DownloadItem*> downloads;
+    DownloadManagerForShell(shell())->GetAllDownloads(&downloads);
+    EXPECT_EQ(1u, downloads.size());
+    if (1u != downloads.size())
+      return NULL;
+    return downloads[0];
+  }
+
+  // Wait for data
+  void WaitForData(DownloadItem* download, int size) {
+    DownloadUpdatedObserver data_observer(
+        download, base::Bind(&DataReceivedFilter, size));
+    data_observer.WaitForEvent();
+    ASSERT_EQ(size, download->GetReceivedBytes());
+    ASSERT_EQ(DownloadItem::IN_PROGRESS, download->GetState());
+  }
+
+  // Tell the test server to release a pending RST and confirm
+  // that the interrupt is received properly (for download resumption
+  // testing).
+  void ReleaseRSTAndConfirmInterruptForResume(DownloadItem* download) {
+    scoped_ptr<DownloadTestObserver> rst_observer(CreateWaiter(shell(), 1));
+    NavigateToURL(shell(), test_server()->GetURL("download-finish"));
+    rst_observer->WaitForFinished();
+    EXPECT_EQ(DownloadItem::INTERRUPTED, download->GetState());
+  }
+
+  // Confirm file status expected for the given location in a stream
+  // provided by the resume test server.
+  void ConfirmFileStatusForResume(
+      DownloadItem* download, bool file_exists,
+      int received_bytes, int total_bytes,
+      const base::FilePath& expected_filename) {
+    EXPECT_EQ(received_bytes, download->GetReceivedBytes());
+    EXPECT_EQ(total_bytes, download->GetTotalBytes());
+    EXPECT_EQ(expected_filename.value(),
+              download->GetFullPath().BaseName().value());
+    EXPECT_EQ(file_exists,
+              (!download->GetFullPath().empty() &&
+               file_util::PathExists(download->GetFullPath())));
+
+    if (file_exists) {
+      std::string file_contents;
+      EXPECT_TRUE(file_util::ReadFileToString(
+          download->GetFullPath(), &file_contents));
+
+      ASSERT_EQ(static_cast<size_t>(received_bytes), file_contents.size());
+      for (int i = 0; i < received_bytes; ++i) {
+        EXPECT_EQ(static_cast<char>((i * 2 + 15) % 256), file_contents[i])
+            << "File contents diverged at position " << i
+            << " for " << expected_filename.value();
+
+        if (static_cast<char>((i * 2 + 15) % 256) != file_contents[i])
+          return;
+      }
+    }
+  }
+
  private:
   static void EnsureNoPendingDownloadJobsOnIO(bool* result) {
     if (URLRequestSlowDownloadJob::NumberOutstandingRequests())
@@ -494,7 +675,8 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, DownloadCancelled) {
 
   // Create a download, wait until it's started, and confirm
   // we're in the expected state.
-  scoped_ptr<DownloadTestObserver> observer(CreateInProgressWaiter(shell(), 1));
+  scoped_ptr<DownloadCreateObserver> observer(
+      CreateInProgressWaiter(shell(), 1));
   NavigateToURL(shell(), GURL(URLRequestSlowDownloadJob::kUnknownSizeUrl));
   observer->WaitForFinished();
 
@@ -520,7 +702,7 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, MultiDownload) {
 
   // Create a download, wait until it's started, and confirm
   // we're in the expected state.
-  scoped_ptr<DownloadTestObserver> observer1(
+  scoped_ptr<DownloadCreateObserver> observer1(
       CreateInProgressWaiter(shell(), 1));
   NavigateToURL(shell(), GURL(URLRequestSlowDownloadJob::kUnknownSizeUrl));
   observer1->WaitForFinished();
@@ -676,7 +858,8 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, CancelAtRelease) {
 // works properly.
 IN_PROC_BROWSER_TEST_F(DownloadContentTest, ShutdownInProgress) {
   // Create a download that won't complete.
-  scoped_ptr<DownloadTestObserver> observer(CreateInProgressWaiter(shell(), 1));
+  scoped_ptr<DownloadCreateObserver> observer(
+      CreateInProgressWaiter(shell(), 1));
   NavigateToURL(shell(), GURL(URLRequestSlowDownloadJob::kUnknownSizeUrl));
   observer->WaitForFinished();
 
@@ -773,87 +956,406 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, ResumeInterruptedDownload) {
       switches::kEnableDownloadResumption);
   ASSERT_TRUE(test_server()->Start());
 
-  // Figure out the size of the first chunk to send so that it makes it
-  // through the buffering between DownloadResourceHandler and
-  // DownloadFileImpl.
-  int initial_chunk = (DownloadResourceHandler::kDownloadByteStreamSize /
-                       ByteStreamWriter::kFractionBufferBeforeSending) + 1;
-
   GURL url = test_server()->GetURL(
       StringPrintf("rangereset?size=%d&rst_boundary=%d",
-                   initial_chunk * 2, initial_chunk));
+                   GetSafeBufferChunk() * 3, GetSafeBufferChunk()));
 
-  // Download and wait for file determination.
-  scoped_ptr<DownloadTestObserver> observer(CreateInProgressWaiter(shell(), 1));
-  NavigateToURL(shell(), url);
-  observer->WaitForFinished();
+  DownloadItem* download(StartDownloadAndReturnItem(url));
+  WaitForData(download, GetSafeBufferChunk());
 
-  std::vector<DownloadItem*> downloads;
-  DownloadManagerForShell(shell())->GetAllDownloads(&downloads);
-  ASSERT_EQ(1u, downloads.size());
-  DownloadItem* download(downloads[0]);
-
-  // Wait for intermediate name, then for all expected data to arrive.
-  // TODO(rdsmith): Figure out how to handle the races needed
-  // so that we don't have to wait for the target name determination.
-  DownloadUpdatedObserver intermediate_observer(
-      download, base::Bind(&IntermediateFileRenameFilter));
-  intermediate_observer.WaitForEvent();
-
-  DownloadUpdatedObserver data_observer(
-      download, base::Bind(&DataReceivedFilter, initial_chunk));
-  data_observer.WaitForEvent();
-
-  // Shouldn't have received any extra data.
-  ASSERT_EQ(initial_chunk, download->GetReceivedBytes());
-
-  // Unleash the RST!
-  scoped_ptr<DownloadTestObserver> rst_observer(CreateWaiter(shell(), 1));
-  GURL release_url = test_server()->GetURL("download-finish");
-  NavigateToURL(shell(), release_url);
-  rst_observer->WaitForFinished();
-  EXPECT_EQ(DownloadItem::INTERRUPTED, download->GetState());
-  EXPECT_EQ(initial_chunk, download->GetReceivedBytes());
-  EXPECT_EQ(initial_chunk * 2, download->GetTotalBytes());
-  EXPECT_EQ(FILE_PATH_LITERAL("rangereset.crdownload"),
-            download->GetFullPath().BaseName().value());
+  // Confirm resumption while in progress doesn't do anything.
+  download->ResumeInterruptedDownload();
+  ASSERT_EQ(GetSafeBufferChunk(), download->GetReceivedBytes());
+  ASSERT_EQ(DownloadItem::IN_PROGRESS, download->GetState());
+
+  // Tell the server to send the RST and confirm the interrupt happens.
+  ReleaseRSTAndConfirmInterruptForResume(download);
+  ConfirmFileStatusForResume(
+      download, true, GetSafeBufferChunk(), GetSafeBufferChunk() * 3,
+      base::FilePath(FILE_PATH_LITERAL("rangereset.crdownload")));
+
+  // Resume, confirming received bytes on resumption is correct.
+  int initial_size = 0;
+  DownloadUpdatedObserver initial_size_observer(
+      download, base::Bind(&InitialSizeFilter, &initial_size));
+  download->ResumeInterruptedDownload();
+  initial_size_observer.WaitForEvent();
+  EXPECT_EQ(GetSafeBufferChunk(), initial_size);
 
-  {
-    std::string file_contents;
-    std::string expected_contents(initial_chunk, 'X');
-    ASSERT_TRUE(file_util::ReadFileToString(
-        download->GetFullPath(), &file_contents));
-    EXPECT_EQ(static_cast<size_t>(initial_chunk), file_contents.size());
-    // In conditional to avoid spamming the console with two very long strings.
-    if (expected_contents != file_contents)
-      EXPECT_TRUE(false) << "File contents do not have expected value.";
-  }
-
-  // Resume; should get entire file (note that a restart will fail
-  // here because it'll produce another RST).
-  bool flag(false);
-  DownloadUpdatedObserver complete_observer(
-      download, base::Bind(&DownloadResumptionFilter, &flag));
+  // and wait for expected data.
+  WaitForData(download, GetSafeBufferChunk() * 2);
+
+  // Tell the server to send the RST and confirm the interrupt happens.
+  ReleaseRSTAndConfirmInterruptForResume(download);
+  ConfirmFileStatusForResume(
+      download, true, GetSafeBufferChunk() * 2, GetSafeBufferChunk() * 3,
+      base::FilePath(FILE_PATH_LITERAL("rangereset.crdownload")));
+
+  // Resume and wait for completion.
+  DownloadUpdatedObserver completion_observer(
+      download, base::Bind(DownloadCompleteFilter));
   download->ResumeInterruptedDownload();
-  NavigateToURL(shell(), release_url);  // Needed to get past hold.
-  complete_observer.WaitForEvent();
-  EXPECT_EQ(DownloadItem::COMPLETE, download->GetState());
-  EXPECT_EQ(initial_chunk * 2, download->GetReceivedBytes());
-  EXPECT_EQ(initial_chunk * 2, download->GetTotalBytes());
-  EXPECT_EQ(FILE_PATH_LITERAL("rangereset"),
-            download->GetFullPath().BaseName().value())
-      << "Target path name: " << download->GetTargetFilePath().value();
+  completion_observer.WaitForEvent();
 
-  {
-    std::string file_contents;
-    std::string expected_contents(initial_chunk * 2, 'X');
-    ASSERT_TRUE(file_util::ReadFileToString(
-        download->GetFullPath(), &file_contents));
-    EXPECT_EQ(static_cast<size_t>(initial_chunk * 2), file_contents.size());
-    // In conditional to avoid spamming the console with two 800 char strings.
-    if (expected_contents != file_contents)
-      EXPECT_TRUE(false) << "File contents do not have expected value.";
-  }
+  ConfirmFileStatusForResume(
+      download, true, GetSafeBufferChunk() * 3, GetSafeBufferChunk() * 3,
+      base::FilePath(FILE_PATH_LITERAL("rangereset")));
+
+  // Confirm resumption while complete doesn't do anything.
+  download->ResumeInterruptedDownload();
+  ASSERT_EQ(GetSafeBufferChunk() * 3, download->GetReceivedBytes());
+  ASSERT_EQ(DownloadItem::COMPLETE, download->GetState());
+  RunAllPendingInMessageLoop();
+  ASSERT_EQ(GetSafeBufferChunk() * 3, download->GetReceivedBytes());
+  ASSERT_EQ(DownloadItem::COMPLETE, download->GetState());
+}
+
+// Confirm restart fallback happens if a range request is bounced.
+IN_PROC_BROWSER_TEST_F(DownloadContentTest, ResumeInterruptedDownloadNoRange) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableDownloadResumption);
+  ASSERT_TRUE(test_server()->Start());
+
+  // Auto-restart if server doesn't handle ranges.
+  GURL url = test_server()->GetURL(
+      StringPrintf(
+          // First download hits an RST, rest don't, no ranges.
+          "rangereset?size=%d&rst_boundary=%d&"
+          "token=NoRange&rst_limit=1&bounce_range",
+          GetSafeBufferChunk() * 3, GetSafeBufferChunk()));
+
+  // Start the download and wait for first data chunk.
+  DownloadItem* download(StartDownloadAndReturnItem(url));
+  WaitForData(download, GetSafeBufferChunk());
+
+  RecordingDownloadObserver recorder(download);
+
+  ReleaseRSTAndConfirmInterruptForResume(download);
+  ConfirmFileStatusForResume(
+      download, true, GetSafeBufferChunk(), GetSafeBufferChunk() * 3,
+      base::FilePath(FILE_PATH_LITERAL("rangereset.crdownload")));
+
+  DownloadUpdatedObserver completion_observer(
+      download, base::Bind(DownloadCompleteFilter));
+  download->ResumeInterruptedDownload();
+  completion_observer.WaitForEvent();
+
+  ConfirmFileStatusForResume(
+      download, true, GetSafeBufferChunk() * 3, GetSafeBufferChunk() * 3,
+      base::FilePath(FILE_PATH_LITERAL("rangereset")));
+
+  static const RecordingDownloadObserver::RecordStruct expected_record[] = {
+    // Result of RST
+    {DownloadItem::INTERRUPTED, GetSafeBufferChunk()},
+    // Starting continuation
+    {DownloadItem::IN_PROGRESS, GetSafeBufferChunk()},
+    // Notification of receiving whole file.
+    {DownloadItem::IN_PROGRESS, 0},
+    // Completion.
+    {DownloadItem::COMPLETE, GetSafeBufferChunk() * 3},
+  };
+
+  recorder.CompareToExpectedRecord(expected_record, arraysize(expected_record));
+}
+
+// Confirm restart fallback happens if a precondition is failed.
+IN_PROC_BROWSER_TEST_F(DownloadContentTest,
+                       ResumeInterruptedDownloadBadPrecondition) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableDownloadResumption);
+  ASSERT_TRUE(test_server()->Start());
+
+  GURL url = test_server()->GetURL(
+      StringPrintf(
+          // First download hits an RST, rest don't, precondition fail.
+          "rangereset?size=%d&rst_boundary=%d&"
+          "token=NoRange&rst_limit=1&fail_precondition=2",
+          GetSafeBufferChunk() * 3, GetSafeBufferChunk()));
+
+  // Start the download and wait for first data chunk.
+  DownloadItem* download(StartDownloadAndReturnItem(url));
+  WaitForData(download, GetSafeBufferChunk());
+
+  RecordingDownloadObserver recorder(download);
+
+  ReleaseRSTAndConfirmInterruptForResume(download);
+  ConfirmFileStatusForResume(
+      download, true, GetSafeBufferChunk(), GetSafeBufferChunk() * 3,
+      base::FilePath(FILE_PATH_LITERAL("rangereset.crdownload")));
+
+  DownloadUpdatedObserver completion_observer(
+      download, base::Bind(DownloadCompleteFilter));
+  download->ResumeInterruptedDownload();
+  completion_observer.WaitForEvent();
+
+  ConfirmFileStatusForResume(
+      download, true, GetSafeBufferChunk() * 3, GetSafeBufferChunk() * 3,
+      base::FilePath(FILE_PATH_LITERAL("rangereset")));
+
+  static const RecordingDownloadObserver::RecordStruct expected_record[] = {
+    // Result of RST
+    {DownloadItem::INTERRUPTED, GetSafeBufferChunk()},
+    // Starting continuation
+    {DownloadItem::IN_PROGRESS, GetSafeBufferChunk()},
+    // Server precondition fail.
+    {DownloadItem::INTERRUPTED, GetSafeBufferChunk()},
+    // Notification of successful restart.
+    {DownloadItem::IN_PROGRESS, 0},
+    // Completion.
+    {DownloadItem::COMPLETE, GetSafeBufferChunk() * 3},
+  };
+
+  recorder.CompareToExpectedRecord(expected_record, arraysize(expected_record));
+}
+
+// Confirm we don't try to resume if we don't have a verifier.
+IN_PROC_BROWSER_TEST_F(DownloadContentTest,
+                       ResumeInterruptedDownloadNoVerifiers) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableDownloadResumption);
+  ASSERT_TRUE(test_server()->Start());
+
+  GURL url = test_server()->GetURL(
+      StringPrintf(
+          // First download hits an RST, rest don't, no verifiers.
+          "rangereset?size=%d&rst_boundary=%d&"
+          "token=NoRange&rst_limit=1&no_verifiers",
+          GetSafeBufferChunk() * 3, GetSafeBufferChunk()));
+
+  // Start the download and wait for first data chunk.
+  DownloadItem* download(StartDownloadAndReturnItem(url));
+  WaitForData(download, GetSafeBufferChunk());
+
+  RecordingDownloadObserver recorder(download);
+
+  ReleaseRSTAndConfirmInterruptForResume(download);
+  ConfirmFileStatusForResume(
+      download, false, GetSafeBufferChunk(), GetSafeBufferChunk() * 3,
+      base::FilePath(FILE_PATH_LITERAL("rangereset.crdownload")));
+
+  DownloadUpdatedObserver completion_observer(
+      download, base::Bind(DownloadCompleteFilter));
+  download->ResumeInterruptedDownload();
+  completion_observer.WaitForEvent();
+
+  ConfirmFileStatusForResume(
+      download, true, GetSafeBufferChunk() * 3, GetSafeBufferChunk() * 3,
+      base::FilePath(FILE_PATH_LITERAL("rangereset")));
+
+  static const RecordingDownloadObserver::RecordStruct expected_record[] = {
+    // Result of RST
+    {DownloadItem::INTERRUPTED, GetSafeBufferChunk()},
+    // Restart for lack of verifiers
+    {DownloadItem::IN_PROGRESS, 0},
+    // Completion.
+    {DownloadItem::COMPLETE, GetSafeBufferChunk() * 3},
+  };
+
+  recorder.CompareToExpectedRecord(expected_record, arraysize(expected_record));
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadContentTest, ResumeWithDeletedFile) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableDownloadResumption);
+  ASSERT_TRUE(test_server()->Start());
+
+  GURL url = test_server()->GetURL(
+      StringPrintf(
+          // First download hits an RST, rest don't
+          "rangereset?size=%d&rst_boundary=%d&"
+          "token=NoRange&rst_limit=1",
+          GetSafeBufferChunk() * 3, GetSafeBufferChunk()));
+
+  // Start the download and wait for first data chunk.
+  DownloadItem* download(StartDownloadAndReturnItem(url));
+  WaitForData(download, GetSafeBufferChunk());
+
+  RecordingDownloadObserver recorder(download);
+
+  ReleaseRSTAndConfirmInterruptForResume(download);
+  ConfirmFileStatusForResume(
+      download, true, GetSafeBufferChunk(), GetSafeBufferChunk() * 3,
+      base::FilePath(FILE_PATH_LITERAL("rangereset.crdownload")));
+
+  // Delete the intermediate file.
+  file_util::Delete(download->GetFullPath(), false);
+
+  DownloadUpdatedObserver completion_observer(
+      download, base::Bind(DownloadCompleteFilter));
+  download->ResumeInterruptedDownload();
+  completion_observer.WaitForEvent();
+
+  ConfirmFileStatusForResume(
+      download, true, GetSafeBufferChunk() * 3, GetSafeBufferChunk() * 3,
+      base::FilePath(FILE_PATH_LITERAL("rangereset")));
+
+  static const RecordingDownloadObserver::RecordStruct expected_record[] = {
+    // Result of RST
+    {DownloadItem::INTERRUPTED, GetSafeBufferChunk()},
+    // Starting continuation
+    {DownloadItem::IN_PROGRESS, GetSafeBufferChunk()},
+    // Error because file isn't there.
+    {DownloadItem::INTERRUPTED, GetSafeBufferChunk()},
+    // Restart
+    {DownloadItem::IN_PROGRESS, 0},
+    // Completion.
+    {DownloadItem::COMPLETE, GetSafeBufferChunk() * 3},
+  };
+
+  recorder.CompareToExpectedRecord(expected_record, arraysize(expected_record));
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadContentTest, ResumeWithFileInitError) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableDownloadResumption);
+  base::FilePath file(FILE_PATH_LITERAL("download-test.lib"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
+
+  // Setup the error injector.
+  scoped_refptr<TestFileErrorInjector> injector(
+      TestFileErrorInjector::Create(DownloadManagerForShell(shell())));
+
+  TestFileErrorInjector::FileErrorInfo err = {
+    url.spec(),
+    TestFileErrorInjector::FILE_OPERATION_INITIALIZE,
+    0,
+    DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE
+  };
+  injector->AddError(err);
+  injector->InjectErrors();
+
+  // Start and watch for interrupt.
+  scoped_ptr<DownloadTestObserver> int_observer(CreateWaiter(shell(), 1));
+  DownloadItem* download(StartDownloadAndReturnItem(url));
+  int_observer->WaitForFinished();
+  ASSERT_EQ(DownloadItem::INTERRUPTED, download->GetState());
+  EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE,
+            download->GetLastReason());
+  EXPECT_EQ(0, download->GetReceivedBytes());
+  EXPECT_TRUE(download->GetFullPath().empty());
+  EXPECT_TRUE(download->GetTargetFilePath().empty());
+
+  // We need to make sure that any cross-thread downloads communication has
+  // quiesced before clearing and injecting the new errors, as the
+  // InjectErrors() routine alters the currently in use download file
+  // factory, which is a file thread object.
+  RunAllPendingInMessageLoop(BrowserThread::FILE);
+  RunAllPendingInMessageLoop();
+
+  // Clear the old errors list.
+  injector->ClearErrors();
+  injector->InjectErrors();
+
+  // Resume and watch completion.
+  DownloadUpdatedObserver completion_observer(
+      download, base::Bind(DownloadCompleteFilter));
+  download->ResumeInterruptedDownload();
+  completion_observer.WaitForEvent();
+  EXPECT_EQ(download->GetState(), DownloadItem::COMPLETE);
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadContentTest,
+                       ResumeWithFileIntermediateRenameError) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableDownloadResumption);
+  base::FilePath file(FILE_PATH_LITERAL("download-test.lib"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
+
+  // Setup the error injector.
+  scoped_refptr<TestFileErrorInjector> injector(
+      TestFileErrorInjector::Create(DownloadManagerForShell(shell())));
+
+  TestFileErrorInjector::FileErrorInfo err = {
+    url.spec(),
+    TestFileErrorInjector::FILE_OPERATION_RENAME_UNIQUIFY,
+    0,
+    DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE
+  };
+  injector->AddError(err);
+  injector->InjectErrors();
+
+  // Start and watch for interrupt.
+  scoped_ptr<DownloadTestObserver> int_observer(CreateWaiter(shell(), 1));
+  DownloadItem* download(StartDownloadAndReturnItem(url));
+  int_observer->WaitForFinished();
+  ASSERT_EQ(DownloadItem::INTERRUPTED, download->GetState());
+  EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE,
+            download->GetLastReason());
+  EXPECT_TRUE(download->GetFullPath().empty());
+  // Target path will have been set after file name determination,
+  // and reset when the intermediate rename fails, as that suggests
+  // we should re-do file name determination.
+  EXPECT_TRUE(download->GetTargetFilePath().empty());
+
+  // We need to make sure that any cross-thread downloads communication has
+  // quiesced before clearing and injecting the new errors, as the
+  // InjectErrors() routine alters the currently in use download file
+  // factory, which is a file thread object.
+  RunAllPendingInMessageLoop(BrowserThread::FILE);
+  RunAllPendingInMessageLoop();
+
+  // Clear the old errors list.
+  injector->ClearErrors();
+  injector->InjectErrors();
+
+  // Resume and watch completion.
+  DownloadUpdatedObserver completion_observer(
+      download, base::Bind(DownloadCompleteFilter));
+  download->ResumeInterruptedDownload();
+  completion_observer.WaitForEvent();
+  EXPECT_EQ(download->GetState(), DownloadItem::COMPLETE);
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadContentTest, ResumeWithFileFinalRenameError) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableDownloadResumption);
+  base::FilePath file(FILE_PATH_LITERAL("download-test.lib"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
+
+  // Setup the error injector.
+  scoped_refptr<TestFileErrorInjector> injector(
+      TestFileErrorInjector::Create(DownloadManagerForShell(shell())));
+
+  DownloadManagerForShell(shell())->RemoveAllDownloads();
+  TestFileErrorInjector::FileErrorInfo err = {
+    url.spec(),
+    TestFileErrorInjector::FILE_OPERATION_RENAME_ANNOTATE,
+    0,
+    DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE
+  };
+  injector->AddError(err);
+  injector->InjectErrors();
+
+  // Start and watch for interrupt.
+  scoped_ptr<DownloadTestObserver> int_observer(CreateWaiter(shell(), 1));
+  DownloadItem* download(StartDownloadAndReturnItem(url));
+  int_observer->WaitForFinished();
+  ASSERT_EQ(DownloadItem::INTERRUPTED, download->GetState());
+  EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE,
+            download->GetLastReason());
+  EXPECT_TRUE(download->GetFullPath().empty());
+  // Target path will have been set after file name determination,
+  // and reset when the rename fails, as that suggests
+  // we should re-do file name determination.
+  EXPECT_TRUE(download->GetTargetFilePath().empty());
+
+  // We need to make sure that any cross-thread downloads communication has
+  // quiesced before clearing and injecting the new errors, as the
+  // InjectErrors() routine alters the currently in use download file
+  // factory, which is a file thread object.
+  RunAllPendingInMessageLoop(BrowserThread::FILE);
+  RunAllPendingInMessageLoop();
+
+  // Clear the old errors list.
+  injector->ClearErrors();
+  injector->InjectErrors();
+
+  // Resume and watch completion.
+  DownloadUpdatedObserver completion_observer(
+      download, base::Bind(DownloadCompleteFilter));
+  download->ResumeInterruptedDownload();
+  completion_observer.WaitForEvent();
+  EXPECT_EQ(download->GetState(), DownloadItem::COMPLETE);
 }
 
 }  // namespace content
diff --git a/content/browser/download/download_file_impl.cc b/content/browser/download/download_file_impl.cc
index 259d571..5ee2187 100644
--- a/content/browser/download/download_file_impl.cc
+++ b/content/browser/download/download_file_impl.cc
@@ -76,6 +76,9 @@ void DownloadFileImpl::Initialize(const InitializeCallback& callback) {
 
   download_start_ = base::TimeTicks::Now();
 
+  // Primarily to make reset to zero in restart visible to owner.
+  SendUpdate();
+
   // Initial pull from the straw.
   StreamActive();
 
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index 1708a63..59cfbce 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -127,6 +127,7 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
       auto_opened_(false),
       is_temporary_(false),
       all_data_saved_(state == COMPLETE),
+      destination_error_(content::DOWNLOAD_INTERRUPT_REASON_NONE),
       opened_(opened),
       delegate_delayed_complete_(false),
       bound_net_log_(bound_net_log),
@@ -162,6 +163,8 @@ DownloadItemImpl::DownloadItemImpl(
       total_bytes_(info.total_bytes),
       received_bytes_(0),
       bytes_per_sec_(0),
+      last_modified_time_(info.last_modified),
+      etag_(info.etag),
       last_reason_(DOWNLOAD_INTERRUPT_REASON_NONE),
       start_tick_(base::TimeTicks::Now()),
       state_(IN_PROGRESS_INTERNAL),
@@ -175,6 +178,7 @@ DownloadItemImpl::DownloadItemImpl(
       auto_opened_(false),
       is_temporary_(!info.save_info->file_path.empty()),
       all_data_saved_(false),
+      destination_error_(content::DOWNLOAD_INTERRUPT_REASON_NONE),
       opened_(false),
       delegate_delayed_complete_(false),
       bound_net_log_(bound_net_log),
@@ -229,6 +233,7 @@ DownloadItemImpl::DownloadItemImpl(
       auto_opened_(false),
       is_temporary_(false),
       all_data_saved_(false),
+      destination_error_(content::DOWNLOAD_INTERRUPT_REASON_NONE),
       opened_(false),
       delegate_delayed_complete_(false),
       bound_net_log_(bound_net_log),
@@ -340,7 +345,12 @@ void DownloadItemImpl::Cancel(bool user_cancel) {
 
   RecordDownloadCount(CANCELLED_COUNT);
 
-  CancelDownloadFile();
+  // TODO(rdsmith/benjhayden): Remove condition as part of
+  // |SavePackage| integration.
+  // |download_file_| can be NULL if Interrupt() is called after the
+  // download file has been released.
+  if (!is_save_package_download_ && download_file_.get())
+    ReleaseDownloadFile(true);
 
   if (state_ != INTERRUPTED_INTERNAL) {
     // Cancel the originating URL request unless it's already been cancelled
@@ -775,7 +785,10 @@ DownloadItemImpl::ResumeMode DownloadItemImpl::GetResumeMode() const {
     return RESUME_MODE_INVALID;
 
   // We can't continue without a handle on the intermediate file.
-  const bool force_restart = current_path_.empty();
+  // We also can't continue if we don't have some verifier to make sure
+  // we're getting the same file.
+  const bool force_restart =
+      (current_path_.empty() || (etag_.empty() && last_modified_time_.empty()));
 
   // We won't auto-restart if we've used up our attempts or the
   // download has been paused by user action.
@@ -848,10 +861,9 @@ void DownloadItemImpl::ResumeInterruptedDownload() {
   if (!command_line.HasSwitch(switches::kEnableDownloadResumption))
     return;
 
-  // Handle the case of clicking 'Resume' in the download shelf.
-  DCHECK(IsInterrupted());
-
-  DVLOG(20) << __FUNCTION__ << "()" << DebugString(true);
+  // If we're not interrupted, ignore the request; our caller is drunk.
+  if (!IsInterrupted())
+    return;
 
   // If we can't get a web contents, we can't resume the download.
   // TODO(rdsmith): Find some alternative web contents to use--this
@@ -1008,9 +1020,13 @@ void DownloadItemImpl::DestinationUpdate(int64 bytes_so_far,
 }
 
 void DownloadItemImpl::DestinationError(DownloadInterruptReason reason) {
-  // The DestinationError and Interrupt routines are being kept separate
-  // to allow for a future merging of the Cancel and Interrupt routines.
-  Interrupt(reason);
+  // Postpone recognition of this error until after file name determination
+  // has completed and the intermediate file has been renamed to simplify
+  // resumption conditions.
+  if (current_path_.empty() || target_path_.empty())
+    destination_error_ = reason;
+  else
+    Interrupt(reason);
 }
 
 void DownloadItemImpl::DestinationCompleted(const std::string& final_hash) {
@@ -1104,6 +1120,9 @@ void DownloadItemImpl::OnDownloadFileInitialized(
   // If we're resuming an interrupted download, we may already know
   // the download target so we can skip target name determination.
   if (!GetTargetFilePath().empty() && !GetFullPath().empty()) {
+    // TODO(rdsmith/asanka): Check to confirm that the target path isn't
+    // present on disk; if it is, we should re-do filename determination to
+    // give the user a chance not to collide.
     MaybeCompleteDownload();
     return;
   }
@@ -1178,8 +1197,25 @@ void DownloadItemImpl::OnDownloadRenamedToIntermediateName(
     const base::FilePath& full_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   VLOG(20) << __FUNCTION__ << " download=" << DebugString(true);
+
+  // Process destination error.  If both reason and destination_error_
+  // refer to actual errors, we want to use the destination_error_ as the
+  // argument to the Interrupt() routine, as it happened first.
+  if (destination_error_ != DOWNLOAD_INTERRUPT_REASON_NONE) {
+    Interrupt(destination_error_);
+    destination_error_ = DOWNLOAD_INTERRUPT_REASON_NONE;
+  }
+
+  // Process the results of the rename.
   if (DOWNLOAD_INTERRUPT_REASON_NONE != reason) {
+    // Will be ignored if we called Interrupt() above.
     Interrupt(reason);
+
+    // All file errors result in file deletion above; no need to cleanup.
+    // Reset the target path so on resumption we re-do file name determination.
+    // A restart will be forced because we don't set the full path on this
+    // branch.
+    target_path_ = base::FilePath();
   } else {
     SetFullPath(full_path);
     MaybeCompleteDownload();
@@ -1272,6 +1308,12 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(
 
   if (DOWNLOAD_INTERRUPT_REASON_NONE != reason) {
     Interrupt(reason);
+
+    // All file errors result in file deletion above; no need to cleanup.
+    // Reset the paths so on resumption we re-do file name determination.
+    target_path_ = base::FilePath();
+    current_path_ = base::FilePath();
+    UpdateObservers();
     return;
   }
 
@@ -1285,9 +1327,7 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(
 
   // Complete the download and release the DownloadFile.
   DCHECK(download_file_.get());
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFileDetach, base::Passed(&download_file_)));
+  ReleaseDownloadFile(false);
 
   // We're not completely done with the download item yet, but at this
   // point we're committed to complete the download.  Cancels (or Interrupts,
@@ -1361,17 +1401,27 @@ void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) {
   TransitionTo(INTERRUPTED_INTERNAL);
 
   ResumeMode resume_mode = GetResumeMode();
-  if (resume_mode == RESUME_MODE_IMMEDIATE_RESTART ||
-      resume_mode == RESUME_MODE_USER_RESTART) {
-    // Remove the download file; no point in leaving data around we
-    // aren't going to use.
-    CancelDownloadFile();
-  } else {
-    // Keep the file around and maybe re-use it.
-    BrowserThread::PostTask(
-        BrowserThread::FILE, FROM_HERE,
-        base::Bind(&DownloadFileDetach, base::Passed(&download_file_)));
-  }
+  // Cancel (delete file) if we're going to restart; no point in leaving
+  // data around we aren't going to use.  Also cancel if resumption isn't
+  // enabled for the same reason.
+  const CommandLine& command_line = *CommandLine::ForCurrentProcess();
+  bool resumption_enabled =
+      command_line.HasSwitch(switches::kEnableDownloadResumption);
+  ReleaseDownloadFile(resume_mode == RESUME_MODE_IMMEDIATE_RESTART ||
+                      resume_mode == RESUME_MODE_USER_RESTART ||
+                      !resumption_enabled);
+
+  // Reset all data saved, as even if we did save all the data we're going
+  // to go through another round of downloading when we resume.
+  // There's a potential problem here in the abstract, as if we did download
+  // all the data and then run into a continuable error, on resumption we
+  // won't download any more data.  However, a) there are currently no
+  // continuable errors that can occur after we download all the data, and
+  // b) if there were, that would probably simply result in a null range
+  // request, which would generate a DestinationCompleted() notification
+  // from the DownloadFile, which would behave properly with setting
+  // all_data_saved_ to false here.
+  all_data_saved_ = false;
 
   // Cancel the originating URL request.
   request_handle_->CancelRequest();
@@ -1380,9 +1430,25 @@ void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) {
   AutoResumeIfValid();
 }
 
-void DownloadItemImpl::CancelDownloadFile() {
+void DownloadItemImpl::ReleaseDownloadFile(bool destroy_file) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  if (destroy_file) {
+    BrowserThread::PostTask(
+        BrowserThread::FILE, FROM_HERE,
+        // Will be deleted at end of task execution.
+        base::Bind(&DownloadFileCancel, base::Passed(&download_file_)));
+  } else {
+    BrowserThread::PostTask(
+        BrowserThread::FILE, FROM_HERE,
+        // Will be deleted at end of task execution.
+        base::Bind(&DownloadFileDetach, base::Passed(&download_file_)));
+  }
+  // Don't accept any more messages from the DownloadFile, and null
+  // out any previous "all data received".  This also breaks links to
+  // other entities we've given out weak pointers to.
+  weak_ptr_factory_.InvalidateWeakPtrs();
+
   // TODO(rdsmith/benjhayden): Remove condition as part of
   // |SavePackage| integration.
   // |download_file_| can be NULL if Interrupt() is called after the
diff --git a/content/browser/download/download_item_impl.h b/content/browser/download/download_item_impl.h
index f4ade25..25d1257 100644
--- a/content/browser/download/download_item_impl.h
+++ b/content/browser/download/download_item_impl.h
@@ -299,8 +299,10 @@ class CONTENT_EXPORT DownloadItemImpl
   // Indicate that an error has occurred on the download.
   void Interrupt(DownloadInterruptReason reason);
 
-  // Cancel the DownloadFile if we have it.
-  void CancelDownloadFile();
+  // Destroy the DownloadFile object.  If |destroy_file| is true, the file is
+  // destroyed with it.  Otherwise, DownloadFile::Detach() is called
+  // before object destruction to prevent file destruction.
+  void ReleaseDownloadFile(bool destroy_file);
 
   // Check if a download is ready for completion.  The callback provided
   // may be called at some point in the future if an external entity
@@ -463,6 +465,11 @@ class CONTENT_EXPORT DownloadItemImpl
   // True if we've saved all the data for the download.
   bool all_data_saved_;
 
+  // Error return from DestinationError.  Stored separately from
+  // last_reason_ so that we can avoid handling destination errors until
+  // after file name determination has occurred.
+  DownloadInterruptReason destination_error_;
+
   // Did the user open the item either directly or indirectly (such as by
   // setting always open files of this type)? The shelf also sets this field
   // when the user closes the shelf before the item has been opened but should
diff --git a/content/browser/download/download_item_impl_unittest.cc b/content/browser/download/download_item_impl_unittest.cc
index d3febf3..47e89b1 100644
--- a/content/browser/download/download_item_impl_unittest.cc
+++ b/content/browser/download/download_item_impl_unittest.cc
@@ -232,6 +232,7 @@ class DownloadItemTest : public testing::Test {
     info_->save_info = scoped_ptr<DownloadSaveInfo>(new DownloadSaveInfo());
     info_->save_info->prompt_for_save_location = false;
     info_->url_chain.push_back(GURL());
+    info_->etag = "SomethingToSatisfyResumption";
 
     DownloadItemImpl* download =
         new DownloadItemImpl(&delegate_, *(info_.get()), net::BoundNetLog());
@@ -387,7 +388,7 @@ TEST_F(DownloadItemTest, NotificationAfterDownloadedFileRemoved) {
 
 TEST_F(DownloadItemTest, NotificationAfterInterrupted) {
   DownloadItemImpl* item = CreateDownloadItem();
-  MockDownloadFile* download_file = AddDownloadFileToDownloadItem(item, NULL);
+  MockDownloadFile* download_file = DoIntermediateRename(item);
   EXPECT_CALL(*download_file, Cancel());
   MockObserver observer(item);
 
@@ -419,6 +420,9 @@ TEST_F(DownloadItemTest, NotificationAfterDestroyed) {
 }
 
 TEST_F(DownloadItemTest, ContinueAfterInterrupted) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableDownloadResumption);
+
   DownloadItemImpl* item = CreateDownloadItem();
   MockObserver observer(item);
   DownloadItemImplDelegate::DownloadTargetCallback callback;
@@ -441,6 +445,9 @@ TEST_F(DownloadItemTest, ContinueAfterInterrupted) {
 
 // Same as above, but with a non-continuable interrupt.
 TEST_F(DownloadItemTest, RestartAfterInterrupted) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableDownloadResumption);
+
   DownloadItemImpl* item = CreateDownloadItem();
   MockObserver observer(item);
   DownloadItemImplDelegate::DownloadTargetCallback callback;
@@ -471,9 +478,10 @@ TEST_F(DownloadItemTest, LimitRestartsAfterInterrupted) {
   scoped_ptr<DownloadFile> download_file;
   MockRequestHandle* mock_request_handle(NULL);
   scoped_ptr<DownloadRequestHandleInterface> request_handle;
+  DownloadItemImplDelegate::DownloadTargetCallback callback;
 
   EXPECT_CALL(*mock_delegate(), DetermineDownloadTarget(item, _))
-      .WillRepeatedly(Return());
+      .WillRepeatedly(SaveArg<1>(&callback));
   for (int i = 0; i < (DownloadItemImpl::kMaxAutoResumeAttempts + 1); ++i) {
     DVLOG(20) << "Loop iteration " << i;
 
@@ -490,8 +498,22 @@ TEST_F(DownloadItemTest, LimitRestartsAfterInterrupted) {
           .WillOnce(Return(static_cast<WebContents*>(NULL)));
     }
 
+    // Copied key parts of DoIntermediateRename & AddDownloadFileToDownloadItem
+    // to allow for holding onto the request handle.
     item->Start(download_file.Pass(), request_handle.Pass());
-
+    RunAllPendingInMessageLoops();
+    if (i == 0) {
+      // Target determination is only done the first time through.
+      base::FilePath target_path(kDummyPath);
+      base::FilePath intermediate_path(
+          target_path.InsertBeforeExtensionASCII("x"));
+      EXPECT_CALL(*mock_download_file, RenameAndUniquify(intermediate_path, _))
+          .WillOnce(ScheduleRenameCallback(DOWNLOAD_INTERRUPT_REASON_NONE,
+                                           intermediate_path));
+      callback.Run(target_path, DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+                   DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, intermediate_path);
+      RunAllPendingInMessageLoops();
+    }
     ASSERT_EQ(i, observer.GetResumeCount());
 
     // Use a continuable interrupt.
@@ -499,6 +521,7 @@ TEST_F(DownloadItemTest, LimitRestartsAfterInterrupted) {
         DOWNLOAD_INTERRUPT_REASON_FILE_TRANSIENT_ERROR);
 
     ASSERT_EQ(i + 1, observer.GetInterruptCount());
+    ::testing::Mock::VerifyAndClearExpectations(mock_download_file);
   }
 
   CleanupItem(item, mock_download_file, DownloadItem::INTERRUPTED);
@@ -641,6 +664,7 @@ TEST_F(DownloadItemTest, Start) {
       new NiceMock<MockRequestHandle>);
   EXPECT_CALL(*mock_delegate(), DetermineDownloadTarget(item, _));
   item->Start(download_file.Pass(), request_handle.Pass());
+  RunAllPendingInMessageLoops();
 
   CleanupItem(item, mock_download_file, DownloadItem::IN_PROGRESS);
 }
@@ -705,7 +729,7 @@ TEST_F(DownloadItemTest, CallbackAfterInterruptedRename) {
 
 TEST_F(DownloadItemTest, Interrupted) {
   DownloadItemImpl* item = CreateDownloadItem();
-  MockDownloadFile* download_file = AddDownloadFileToDownloadItem(item, NULL);
+  MockDownloadFile* download_file = DoIntermediateRename(item);
 
   const DownloadInterruptReason reason(
       DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED);
@@ -772,7 +796,7 @@ TEST_F(DownloadItemTest, DestinationUpdate) {
 
 TEST_F(DownloadItemTest, DestinationError) {
   DownloadItemImpl* item = CreateDownloadItem();
-  MockDownloadFile* download_file = AddDownloadFileToDownloadItem(item, NULL);
+  MockDownloadFile* download_file = DoIntermediateRename(item);
   base::WeakPtr<DownloadDestinationObserver> as_observer(
       item->DestinationObserverAsWeakPtr());
   MockObserver observer(item);
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index c7e8e31..b24bd0a 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -83,20 +83,26 @@ void BeginDownload(scoped_ptr<DownloadUrlParameters> params,
 
   // If we're not at the beginning of the file, retrieve only the remaining
   // portion.
+  bool has_last_modified = !params->last_modified().empty();
+  bool has_etag = !params->etag().empty();
+
+  // If we've asked for a range, we want to make sure that we only
+  // get that range if our current copy of the information is good.
+  // We shouldn't be asked to continue if we don't have a verifier.
+  DCHECK(params->offset() == 0 || has_etag || has_last_modified);
+
   if (params->offset() > 0) {
     request->SetExtraRequestHeaderByName(
         "Range",
         StringPrintf("bytes=%" PRId64 "-", params->offset()),
         true);
 
-    // If we've asked for a range, we want to make sure that we only
-    // get that range if our current copy of the information is good.
-    if (!params->last_modified().empty()) {
+    if (has_last_modified) {
       request->SetExtraRequestHeaderByName("If-Unmodified-Since",
                                            params->last_modified(),
                                            true);
     }
-    if (!params->etag().empty()) {
+    if (has_etag) {
       request->SetExtraRequestHeaderByName("If-Match", params->etag(), true);
     }
   }