For downloads requiring a user gesture, also require...

chrome/browser/download/download_manager.cc

 // Copyright (c) 2011 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.
 
 #include "chrome/browser/download/download_manager.h"
 
 #include "base/callback.h"
 #include "base/file_util.h"
 #include "base/i18n/case_conversion.h"
 #include "base/logging.h"
@@ skipping 251 matching lines @@
     return;
 
   // Create a client to verify download URL with safebrowsing.
   // It deletes itself after the callback.
   scoped_refptr<DownloadSBClient> sb_client = new DownloadSBClient(
       download_id, download->url_chain(), download->referrer_url());
   sb_client->CheckDownloadUrl(
       NewCallback(this, &DownloadManager::CheckDownloadUrlDone));
 }
 
+void DownloadManager::CheckVisitedBeforeDone(int32 download_id,
+                                             bool visited_before) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  DownloadItem* download = GetActiveDownloadItem(download_id);
+  if (download) {
+    download->SetVisitedBefore(visited_before);
+    HistoryAndSBChecksMaybeDone(download);
+  }
+}
+
 void DownloadManager::CheckDownloadUrlDone(int32 download_id,
                                            bool is_dangerous_url) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DownloadItem* download = GetActiveDownloadItem(download_id);
-  if (!download)
+  if (download) {

[Randy Smith (Not in Mondays), 2011/05/24 18:28:25]

It's a personal style preference (which probably means you can quote me chapter
and versus in the style guide against it :-}) but I like to have "the main path"
outside of conditionals, all other things being equal.  So my preference would
be to retain the "if (!download) return;" pattern, though if you feel strongly I
won't stop the change.

+    download->SetUrlDangerous(is_dangerous_url);
+    HistoryAndSBChecksMaybeDone(download);
+  }
+}
+
+void DownloadManager::HistoryAndSBChecksMaybeDone(DownloadItem* download) {

[Randy Smith (Not in Mondays), 2011/05/24 18:28:25]

One of the biggest things we've done in the past couple of months is to de-race
the download system down to the absolute minimum of races--that was a big source
of the flakiness that had the tests disabled for so long.  For that reason, I
want to keep to an absolute minium the number of places where we do things in
parallel and "join back up" later (as this function does).  My evaluation was
that the only really really important things to do in parallel were the data
download and the user interaction, since either of those could take an arbitrary
time.  So we've made just about everything linear along the user interaction
path and made the data download go in parallel with that.

So unless you have some reason to think that there's a major advantage to doing
the fork/join around the history and safe browsing checks, could we keep the
flow linear?  

(I'm hopeful that that also means you can ditch the TriState enum, since you'll
know by what code you're in whether or not you've done the check.)

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(download);
+
+  // Only proceed once both the history and safe browsing checks have completed.
+  DownloadStateInfo state = download->state_info();
+  if ((state.visited_referrer_before == DownloadStateInfo::UNKNOWN) ||
+      (state.is_dangerous_url == DownloadStateInfo::UNKNOWN))
     return;
 
-  if (is_dangerous_url)
-    download->MarkUrlDangerous();
-
-  DownloadStateInfo state = download->state_info();
-
   // Check whether this download is for an extension install or not.
   // Allow extensions to be explicitly saved.
   if (!state.prompt_user_for_save_location) {
     if (UserScript::IsURLUserScript(download->GetURL(),
         download->mime_type()) ||
         (download->mime_type() == Extension::kMimeType)) {
       state.is_extension_install = true;
     }
   }
 
@@ skipping 41 matching lines @@
 
   // We need to move over to the download thread because we don't want to stat
   // the suggested path on the UI thread.
   // We can only access preferences on the UI thread, so check the download path
   // now and pass the value to the FILE thread.
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       NewRunnableMethod(
           this,
           &DownloadManager::CheckIfSuggestedPathExists,
-          download_id,
+          download->id(),
           state,
           download_prefs()->download_path()));
 }
 
 void DownloadManager::CheckIfSuggestedPathExists(int32 download_id,
                                                  DownloadStateInfo state,
                                                  const FilePath& default_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   // Make sure the default download directory exists.
@@ skipping 133 matching lines @@
 void DownloadManager::CreateDownloadItem(DownloadCreateInfo* info) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DownloadItem* download = new DownloadItem(this, *info,
                                             profile_->IsOffTheRecord());
   int32 download_id = info->download_id;
   DCHECK(!ContainsKey(in_progress_, download_id));
   DCHECK(!ContainsKey(active_downloads_, download_id));
   downloads_.insert(download);
   active_downloads_[download_id] = download;
+
+  download_history_->CheckVisitedReferrerBefore(download_id, info->referrer_url,
+      NewCallback(this, &DownloadManager::CheckVisitedBeforeDone));
 }
 
 void DownloadManager::ContinueDownloadWithPath(DownloadItem* download,
                                                const FilePath& chosen_file) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(download);
 
   int32 download_id = download->id();
 
   // NOTE(ahendrickson) Eventually |active_downloads_| will replace
@@ skipping 531 matching lines @@
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   bool auto_open = ShouldOpenFileBasedOnExtension(state.suggested_path);
   download_util::DownloadDangerLevel danger_level =
       download_util::GetFileDangerLevel(state.suggested_path.BaseName());
 
   if (danger_level == download_util::Dangerous)
     return !(auto_open && state.has_user_gesture);
 
   if (danger_level == download_util::AllowOnUserGesture &&
-      !state.has_user_gesture)
+      (!state.has_user_gesture ||
+       state.visited_referrer_before != DownloadStateInfo::YES))
     return true;
 
   if (state.is_extension_install) {
     // Extensions that are not from the gallery are considered dangerous.
     ExtensionService* service = profile()->GetExtensionService();
     if (!service || !service->IsDownloadFromGallery(download.GetURL(),
                                                     download.referrer_url()))
       return true;
   }
   return false;
@@ skipping 199 matching lines @@
     observed_download_manager_->RemoveObserver(this);
 }
 
 void DownloadManager::OtherDownloadManagerObserver::ModelChanged() {
   observing_download_manager_->NotifyModelChanged();
 }
 
 void DownloadManager::OtherDownloadManagerObserver::ManagerGoingDown() {
   observed_download_manager_ = NULL;
 }