Don't prompt to save malicious downloads on exit

chrome/browser/lifetime/browser_close_manager_browsertest.cc

Index: chrome/browser/lifetime/browser_close_manager_browsertest.cc
diff --git a/chrome/browser/lifetime/browser_close_manager_browsertest.cc b/chrome/browser/lifetime/browser_close_manager_browsertest.cc
index ce61b244028c4b7e4ea4a2f1fbdc7fe02d91fae3..e58fc23bb189cb89237d63845ccee62e3fefd632 100644
--- a/chrome/browser/lifetime/browser_close_manager_browsertest.cc
+++ b/chrome/browser/lifetime/browser_close_manager_browsertest.cc
@@ -8,6 +8,9 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_shutdown.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/download/chrome_download_manager_delegate.h"
+#include "chrome/browser/download/download_service.h"
+#include "chrome/browser/download/download_service_factory.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/lifetime/browser_close_manager.h"
 #include "chrome/browser/net/url_request_mock_util.h"
@@ -31,6 +34,7 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/download_test_observer.h"
 #include "content/public/test/test_navigation_observer.h"
+#include "content/test/net/url_request_mock_http_job.h"
 #include "content/test/net/url_request_slow_download_job.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 
@@ -121,6 +125,7 @@ class TestBrowserCloseManager : public BrowserCloseManager {
   enum UserChoice {
     USER_CHOICE_USER_CANCELS_CLOSE,
     USER_CHOICE_USER_ALLOWS_CLOSE,
+    NO_USER_CHOICE
   };
 
   static void AttemptClose(UserChoice user_choice) {
@@ -135,7 +140,9 @@ class TestBrowserCloseManager : public BrowserCloseManager {
   virtual void ConfirmCloseWithPendingDownloads(
       int download_count,
       const base::Callback<void(bool)>& callback) OVERRIDE {
+    EXPECT_NE(NO_USER_CHOICE, user_choice_);
     switch (user_choice_) {
+      case NO_USER_CHOICE:
       case USER_CHOICE_USER_CANCELS_CLOSE: {
         callback.Run(false);
         break;
@@ -156,6 +163,38 @@ class TestBrowserCloseManager : public BrowserCloseManager {
   DISALLOW_COPY_AND_ASSIGN(TestBrowserCloseManager);
 };
 
+class TestDownloadManagerDelegate : public ChromeDownloadManagerDelegate {
+ public:
+  explicit TestDownloadManagerDelegate(Profile* profile)
+      : ChromeDownloadManagerDelegate(profile) {

[asanka, 2013/10/19 00:42:31]

On ChromeOS, the DriveIntegrationService kicks in very early and pokes at the
DownloadManager causing ChromeDownloadManagerDelegate and the DownloadHistory
classes to be created and initialized. One bit that has changed recently is that
CDMD::SetNextId() needs to be called in order for CDMD to be able to handle new
downloads. This is done once by DownloadHistory.

By the time your test calls DownloadService::SetDownloadManagerForTesting(),
this has already happened on Chrome OS. So the new TestDownloadManagerDelegate
never gets SetNextId() called on it. This is likely why your download hangs.

You can just call "SetNextId(content::DownloadItem::kInvalidId + 1);" here in
the constructor so that this test delegate can handle downloads.

[felt, 2013/10/19 02:51:50]

Thank you for looking into this. That has fixed it!!! (The failures that remain
in the try jobs are unrelated to this change. All the relevant browsertests are
running green.)

+  }
+
+  virtual bool DetermineDownloadTarget(
+      content::DownloadItem* item,
+      const content::DownloadTargetCallback& callback) OVERRIDE {
+    content::DownloadTargetCallback dangerous_callback =
+        base::Bind(&TestDownloadManagerDelegate::SetDangerous, this, callback);
+    ChromeDownloadManagerDelegate::DetermineDownloadTarget(
+        item, dangerous_callback);

[asanka, 2013/10/19 00:42:31]

Nit: For extra resiliency:

return CDMD::DetermineDownloadTarget(...);

[felt, 2013/10/19 02:51:50]

Done.

+    return true;
+  }
+
+  void SetDangerous(
+      const content::DownloadTargetCallback& callback,
+      const base::FilePath& target_path,
+      content::DownloadItem::TargetDisposition disp,
+      content::DownloadDangerType danger_type,
+      const base::FilePath& intermediate_path) {
+    callback.Run(target_path,
+                 disp,
+                 content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL,
+                 intermediate_path);
+  }
+
+ private:
+  virtual ~TestDownloadManagerDelegate() {}
+};
+
 }  // namespace
 
 class BrowserCloseManagerBrowserTest
@@ -619,6 +658,55 @@ IN_PROC_BROWSER_TEST_P(BrowserCloseManagerBrowserTest,
   EXPECT_TRUE(chrome::BrowserIterator().done());
 }
 
+#if defined(CHROMEOS)
+// TODO(felt): figure out why this isn't working on CrOS.
+#define MAYBE_TestWithDangerousUrlDownload DISABLED_TestWithDangerousUrlDownload
+#else
+#define MAYBE_TestWithDangerousUrlDownload TestWithDangerousUrlDownload
+#endif
+// Test shutdown with a DANGEROUS_URL download undecided.
+IN_PROC_BROWSER_TEST_P(BrowserCloseManagerBrowserTest,
+    TestWithDangerousUrlDownload) {
+  ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+
+  // Set up the fake delegate that forces the download to be malicious.
+  scoped_refptr<TestDownloadManagerDelegate> test_delegate(
+      new TestDownloadManagerDelegate(browser()->profile()));
+  DownloadServiceFactory::GetForBrowserContext(browser()->profile())->
+      SetDownloadManagerDelegateForTesting(test_delegate.get());
+
+  // Run a dangerous download, but the user doesn't make a decision.
+  // This .swf normally would be categorized as DANGEROUS_HOST, but

[asanka, 2013/10/19 00:42:31]

Nit: DANGEROUS_FILE

[felt, 2013/10/19 02:51:50]

Done.

+  // TestDownloadManagerDelegate turns it into DANGEROUS_URL.
+  base::FilePath file(FILE_PATH_LITERAL("downloads/dangerous/dangerous.swf"));
+  GURL download_url(content::URLRequestMockHTTPJob::GetMockUrl(file));
+  content::DownloadTestObserverInterrupted observer(
+      content::BrowserContext::GetDownloadManager(browser()->profile()),
+      1,
+      content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_QUIT);
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(),
+      GURL(download_url),
+      NEW_BACKGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_NONE);
+  observer.WaitForFinished();
+
+  // Check that the download manager has the expected state.
+  EXPECT_EQ(1, content::BrowserContext::GetDownloadManager(
+      browser()->profile())->InProgressCount());
+  EXPECT_EQ(0, content::BrowserContext::GetDownloadManager(
+      browser()->profile())->NonMaliciousInProgressCount());
+
+  // Close the browser with no user action.
+  RepeatedNotificationObserver close_observer(
+      chrome::NOTIFICATION_BROWSER_CLOSED, 1);
+  TestBrowserCloseManager::AttemptClose(
+      TestBrowserCloseManager::NO_USER_CHOICE);
+  close_observer.Wait();
+  EXPECT_TRUE(browser_shutdown::IsTryingToQuit());
+  EXPECT_TRUE(chrome::BrowserIterator().done());
+}
+
 // Test shutdown with a download in progress.
 IN_PROC_BROWSER_TEST_P(BrowserCloseManagerBrowserTest, TestWithDownloads) {
   ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());