Fixed issues with interrupting a download at the start.

Added unit tests for direct and navigated download 404 errors.

BUG= 114020
TEST=Set up a server with a web page that has an invalid link.  Right click on the link and select Save As to download the file.



Review URL: http://codereview.chromium.org/9378035

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

8 files changed, 207 insertions, 36 deletions

diff --git a/chrome/browser/download/download_browsertest.cc b/chrome/browser/download/download_browsertest.cc
index b190208..d3e0f19 100644
--- a/chrome/browser/download/download_browsertest.cc
+++ b/chrome/browser/download/download_browsertest.cc
@@ -73,29 +73,6 @@ const FilePath kGoodCrxPath(FILE_PATH_LITERAL("extensions/good.crx"));
 const char kLargeThemeCrxId[] = "pjpgmfcmabopnnfonnhmdjglfpjjfkbf";
 const FilePath kLargeThemePath(FILE_PATH_LITERAL("extensions/theme2.crx"));
 
-// Action a test should take if a dangerous download is encountered.
-enum DangerousDownloadAction {
-  ON_DANGEROUS_DOWNLOAD_ACCEPT,  // Accept the download
-  ON_DANGEROUS_DOWNLOAD_DENY,  // Deny the download
-  ON_DANGEROUS_DOWNLOAD_FAIL  // Fail if a dangerous download is seen
-};
-
-// Fake user click on "Accept".
-void AcceptDangerousDownload(scoped_refptr<DownloadManager> download_manager,
-                             int32 download_id) {
-  DownloadItem* download = download_manager->GetDownloadItem(download_id);
-  download->DangerousDownloadValidated();
-}
-
-// Fake user click on "Deny".
-void DenyDangerousDownload(scoped_refptr<DownloadManager> download_manager,
-                           int32 download_id) {
-  DownloadItem* download = download_manager->GetDownloadItem(download_id);
-  ASSERT_TRUE(download->IsPartialDownload());
-  download->Cancel(true);
-  download->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD);
-}
-
 // Collect the information from FILE and IO threads needed for the Cancel
 // Test, specifically the number of outstanding requests on the
 // ResourceDispatcherHost and the number of pending downloads on the
@@ -315,6 +292,20 @@ class DownloadTest : public InProcessBrowserTest {
     EXPECT_SELECT_DIALOG
   };
 
+  // Choice of navigation or direct fetch.  Used by |DownloadFileCheckErrors()|.
+  enum DownloadMethod {
+    DOWNLOAD_NAVIGATE,
+    DOWNLOAD_DIRECT
+  };
+
+  // Information passed in to |DownloadFileCheckErrors()|.
+  struct DownloadInfo {
+    const char* url_name;  // URL for the download.
+    DownloadMethod download_method;  // Navigation or Direct.
+    InterruptReason reason;  // Download interrupt reason (NONE is OK).
+    bool show_download_item;  // True if the download item appears on the shelf.
+  };
+
   DownloadTest() {
     EnableDOMAutomation();
   }
@@ -689,6 +680,62 @@ class DownloadTest : public InProcessBrowserTest {
     return true;
   }
 
+  // Attempts to download a file, based on information in |download_info|.
+  void DownloadFileCheckErrors(const DownloadInfo& download_info) {
+    ASSERT_TRUE(test_server()->Start());
+    std::vector<DownloadItem*> download_items;
+    GetDownloads(browser(), &download_items);
+    ASSERT_TRUE(download_items.empty());
+
+    std::string server_path = "files/downloads/";
+    server_path += download_info.url_name;
+    GURL url = test_server()->GetURL(server_path);
+    ASSERT_TRUE(url.is_valid());
+
+    DownloadManager* download_manager = DownloadManagerForBrowser(browser());
+    scoped_ptr<DownloadTestObserver> observer(
+        new DownloadTestObserver(
+            download_manager,
+            1,
+            download_info.reason == DOWNLOAD_INTERRUPT_REASON_NONE ?
+                DownloadItem::COMPLETE :  // Really done
+                DownloadItem::INTERRUPTED,
+            true,                   // Bail on select file
+            DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL));
+
+    if (download_info.download_method == DOWNLOAD_DIRECT) {
+      // Go directly to download.
+      WebContents* web_contents = browser()->GetSelectedWebContents();
+      ASSERT_TRUE(web_contents);
+      DownloadSaveInfo save_info;
+      save_info.prompt_for_save_location = false;
+
+      DownloadManagerForBrowser(browser())->DownloadUrl(
+          url, GURL(""), "", false, -1, save_info, web_contents);
+    } else {
+      // Navigate to URL normally, wait until done.
+      ui_test_utils::NavigateToURLBlockUntilNavigationsComplete(browser(),
+                                                                url,
+                                                                1);
+    }
+
+    if (download_info.show_download_item)
+      observer->WaitForFinished();
+
+    // Validate that the correct file was downloaded.
+    download_items.clear();
+    GetDownloads(browser(), &download_items);
+    size_t item_count = download_info.show_download_item ? 1 : 0;
+    ASSERT_EQ(item_count, download_items.size());
+
+    if (download_info.show_download_item) {
+      DownloadItem* item = download_items[0];
+      ASSERT_EQ(url, item->GetOriginalUrl());
+
+      ASSERT_EQ(download_info.reason, item->GetLastReason());
+    }
+  }
+
  private:
   // Location of the test data.
   FilePath test_dir_;
@@ -1956,3 +2003,81 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, SavePageNonHTMLViaPost) {
   ASSERT_EQ(jpeg_url, download_items[0]->GetOriginalUrl());
   ASSERT_EQ(jpeg_url, download_items[1]->GetOriginalUrl());
 }
+
+IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadNavigate) {
+  DownloadInfo download_info = {
+    "a_zip_file.zip",
+    DOWNLOAD_NAVIGATE,
+    DOWNLOAD_INTERRUPT_REASON_NONE,
+    true
+  };
+
+  // Do initial setup.
+  ASSERT_TRUE(InitialSetup(false));
+  DownloadFileCheckErrors(download_info);
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadDirect) {
+  DownloadInfo download_info = {
+    "a_zip_file.zip",
+    DOWNLOAD_DIRECT,
+    DOWNLOAD_INTERRUPT_REASON_NONE,
+    true
+  };
+
+  // Do initial setup.
+  ASSERT_TRUE(InitialSetup(false));
+  DownloadFileCheckErrors(download_info);
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadError404Direct) {
+  DownloadInfo download_info = {
+    "there_IS_no_spoon.zip",
+    DOWNLOAD_DIRECT,
+    DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT,
+    true
+  };
+
+  // Do initial setup.
+  ASSERT_TRUE(InitialSetup(false));
+  DownloadFileCheckErrors(download_info);
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadError404Navigate) {
+  DownloadInfo download_info = {
+    "there_IS_no_spoon.zip",
+    DOWNLOAD_NAVIGATE,
+    DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT,
+    false
+  };
+
+  // Do initial setup.
+  ASSERT_TRUE(InitialSetup(false));
+  DownloadFileCheckErrors(download_info);
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadError400Direct) {
+  DownloadInfo download_info = {
+    "zip_file_not_found.zip",
+    DOWNLOAD_DIRECT,
+    DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED,
+    true
+  };
+
+  // Do initial setup.
+  ASSERT_TRUE(InitialSetup(false));
+  DownloadFileCheckErrors(download_info);
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadError400Navigate) {
+  DownloadInfo download_info = {
+    "zip_file_not_found.zip",
+    DOWNLOAD_NAVIGATE,
+    DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED,
+    false
+  };
+
+  // Do initial setup.
+  ASSERT_TRUE(InitialSetup(false));
+  DownloadFileCheckErrors(download_info);
+}
diff --git a/chrome/browser/download/download_test_observer.cc b/chrome/browser/download/download_test_observer.cc
index c495a9f..0cbcd0c 100644
--- a/chrome/browser/download/download_test_observer.cc
+++ b/chrome/browser/download/download_test_observer.cc
@@ -57,7 +57,7 @@ DownloadTestObserver::DownloadTestObserver(
   finished_downloads_at_construction_ = finished_downloads_.size();
   EXPECT_NE(DownloadItem::REMOVING, download_finished_state)
       << "Waiting for REMOVING is not supported.  Try COMPLETE.";
-      }
+}
 
 DownloadTestObserver::~DownloadTestObserver() {
   for (DownloadSet::iterator it = downloads_observed_.begin();
diff --git a/chrome/test/data/downloads/a_zip_file.zip.mock-http-headers b/chrome/test/data/downloads/a_zip_file.zip.mock-http-headers
new file mode 100644
index 0000000..2cbc087
--- /dev/null
+++ b/chrome/test/data/downloads/a_zip_file.zip.mock-http-headers
@@ -0,0 +1,4 @@
+HTTP/1.1 200 OK
+Cache-Control: no-cache
+Content-Type: application/octet-stream
+X-Content-Type-Options: nosniff
diff --git a/chrome/test/data/downloads/zip_file_not_found.zip.mock-http-headers b/chrome/test/data/downloads/zip_file_not_found.zip.mock-http-headers
new file mode 100644
index 0000000..8c54e51
--- /dev/null
+++ b/chrome/test/data/downloads/zip_file_not_found.zip.mock-http-headers
@@ -0,0 +1,3 @@
+HTTP/1.0 400 Bad Request
+Content-Type: application/octet-stream
+X-Content-Type-Options: nosniff
diff --git a/content/browser/download/download_file_manager.cc b/content/browser/download/download_file_manager.cc
index a0a6c46..d6168e4 100644
--- a/content/browser/download/download_file_manager.cc
+++ b/content/browser/download/download_file_manager.cc
@@ -99,20 +99,33 @@ void DownloadFileManager::CreateDownloadFile(
   // Life of |info| ends here. No more references to it after this method.
   scoped_ptr<DownloadCreateInfo> infop(info);
 
+  // Create the download file.
   scoped_ptr<DownloadFile> download_file(download_file_factory_->CreateFile(
       info, request_handle, download_manager, get_hash, bound_net_log));
-  if (net::OK != download_file->Initialize()) {
-    request_handle.CancelRequest();
-    return;
-  }
 
-  DCHECK(GetDownloadFile(info->download_id) == NULL);
-  downloads_[info->download_id] = download_file.release();
+  net::Error init_result = download_file->Initialize();
+  if (net::OK != init_result) {
+    // Error:  Handle via download manager/item.
+    BrowserThread::PostTask(
+        BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(
+            &DownloadManager::OnDownloadInterrupted,
+            download_manager,
+            info->download_id.local(),
+            0,
+            "",
+            ConvertNetErrorToInterruptReason(init_result,
+                                             DOWNLOAD_INTERRUPT_FROM_DISK)));
+  } else {
+    DCHECK(GetDownloadFile(info->download_id) == NULL);
+    downloads_[info->download_id] = download_file.release();
 
-  // The file is now ready, we can un-pause the request and start saving data.
-  request_handle.ResumeRequest();
+    // The file is now ready, we can un-pause the request and start saving data.
+    request_handle.ResumeRequest();
 
-  StartUpdateTimer();
+    StartUpdateTimer();
+  }
 
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
diff --git a/content/browser/download/download_resource_handler.cc b/content/browser/download/download_resource_handler.cc
index e366511..1227651 100644
--- a/content/browser/download/download_resource_handler.cc
+++ b/content/browser/download/download_resource_handler.cc
@@ -267,6 +267,7 @@ void DownloadResourceHandler::OnResponseCompletedInternal(
   InterruptReason reason =
       ConvertNetErrorToInterruptReason(error_code,
                                        DOWNLOAD_INTERRUPT_FROM_NETWORK);
+
   if ((status.status() == net::URLRequestStatus::CANCELED) &&
       (status.error() == net::ERR_ABORTED)) {
     // TODO(ahendrickson) -- Find a better set of codes to use here, as
@@ -274,6 +275,26 @@ void DownloadResourceHandler::OnResponseCompletedInternal(
     reason = DOWNLOAD_INTERRUPT_REASON_USER_CANCELED;  // User canceled.
   }
 
+  if (status.is_success()) {
+    int response_code = request_->GetResponseCode();
+    if (response_code >= 400) {
+      switch(response_code) {
+        case 404:  // File Not Found.
+          reason = DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT;
+          break;
+        case 416:  // Range Not Satisfiable.
+          reason = DOWNLOAD_INTERRUPT_REASON_SERVER_NO_RANGE;
+          break;
+        case 412:  // Precondition Failed.
+          reason = DOWNLOAD_INTERRUPT_REASON_SERVER_PRECONDITION;
+          break;
+        default:
+          reason = DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED;
+          break;
+      }
+    }
+  }
+
   download_stats::RecordAcceptsRanges(accept_ranges_, bytes_read_);
 
   // If the callback was already run on the UI thread, this will be a noop.
diff --git a/content/browser/download/interrupt_reasons.cc b/content/browser/download/interrupt_reasons.cc
index 6d9ccc6..8e5d7af 100644
--- a/content/browser/download/interrupt_reasons.cc
+++ b/content/browser/download/interrupt_reasons.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -28,7 +28,7 @@ InterruptReason ConvertNetErrorToInterruptReason(
     FILE_ERROR_TO_INTERRUPT_REASON(ACCESS_DENIED, ACCESS_DENIED)
 
     // There were not enough resources to complete the operation.
-     FILE_ERROR_TO_INTERRUPT_REASON(INSUFFICIENT_RESOURCES, TRANSIENT_ERROR)
+    FILE_ERROR_TO_INTERRUPT_REASON(INSUFFICIENT_RESOURCES, TRANSIENT_ERROR)
 
     // Memory allocation failed.
     FILE_ERROR_TO_INTERRUPT_REASON(OUT_OF_MEMORY, TRANSIENT_ERROR)
diff --git a/net/url_request/url_request.cc b/net/url_request/url_request.cc
index b871d07..144029f 100644
--- a/net/url_request/url_request.cc
+++ b/net/url_request/url_request.cc
@@ -534,9 +534,14 @@ void URLRequest::DoCancel(int error, const SSLInfo& ssl_info) {
 bool URLRequest::Read(IOBuffer* dest, int dest_size, int* bytes_read) {
   DCHECK(job_);
   DCHECK(bytes_read);
-  DCHECK(!job_->is_done());
   *bytes_read = 0;
 
+  // This handles a cancel that happens while paused.
+  // TODO(ahendrickson): DCHECK() that it is not done after
+  // http://crbug.com/115705 is fixed.
+  if (job_->is_done())
+    return false;
+
   if (dest_size == 0) {
     // Caller is not too bright.  I guess we've done what they asked.
     return true;