Chrome Cleaner: download the Chrome Cleaner executable in an empty directory.

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc

Index: chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc
diff --git a/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc b/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc
index f6138161612642579bd6a734765ddae3fcbad669..7d052383087620ed5900873d147d7895d60ba7f5 100644
--- a/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc
+++ b/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc
@@ -7,12 +7,21 @@
 #include <memory>
 #include <utility>
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted.h"
+#include "base/task_runner_util.h"
+#include "base/task_scheduler/post_task.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
-#include "components/variations/net/variations_http_headers.h"
 #include "content/public/browser/browser_thread.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_request_headers.h"
@@ -26,9 +35,10 @@ namespace safe_browsing {
 
 namespace {
 
-// Class that will attempt to download the Chrome Cleaner executable and
-// call a given callback when done. Instances of ChromeCleanerFetcher own
-// themselves and will self-delete on completion of the network request.
+// Class that will attempt to download the Chrome Cleaner executable and call a
+// given callback when done. Instances of ChromeCleanerFetcher own themselves
+// and will self-delete if they encounter an error or when the network request
+// has completed.
 class ChromeCleanerFetcher : public net::URLFetcherDelegate {
  public:
   explicit ChromeCleanerFetcher(ChromeCleanerFetchedCallback fetched_callback);
@@ -37,14 +47,31 @@ class ChromeCleanerFetcher : public net::URLFetcherDelegate {
   ~ChromeCleanerFetcher() override;
 
  private:
+  // Must be called on a sequence where IO is allowed.
+  bool CreateTemporaryFile();
+  // Will be called back on the same sequence as this object was created on.

[csharp, 2017/06/06 19:01:53]

nit: sequence -> task runner sequence? or maybe SequencedTaskRunner (I just
haven't seen sequence in a comment like this before, not sure how clear it is.)

Same idea for line 70

[alito, 2017/06/06 19:32:44]

Sequence seems to be fairly standard lingo now. Taken from the comments in
post_task.h:

"Posts |task| to the TaskScheduler and posts |reply| on the caller's execution
context (i.e. same sequence or thread and same TaskTraits if applicable) when
|task| completes."

+  void OnTemporaryFileCreated(bool success);
+  void PostCallbackAndDeleteSelf(base::FilePath path,
+                                 ChromeCleanerFetchStatus fetch_status);
+
   // net::URLFetcherDelegate overrides.
   void OnURLFetchComplete(const net::URLFetcher* source) override;
 
   ChromeCleanerFetchedCallback fetched_callback_;
+
   // The underlying URL fetcher. The instance is alive from construction through
   // OnURLFetchComplete.
   std::unique_ptr<net::URLFetcher> url_fetcher_;
 
+  // Used for file operations such as creating a new temporary directory.
+  scoped_refptr<base::SequencedTaskRunner> blocking_task_runner_;
+
+  // We will take ownership of the scoped temp directory once we know that the
+  // fetch has succeeded. Must be deleted on a sequence where IO is allowed.
+  std::unique_ptr<base::ScopedTempDir, content::BrowserThread::DeleteOnIOThread>
+      scoped_temp_dir_;
+  base::FilePath temp_file_;
+
   DISALLOW_COPY_AND_ASSIGN(ChromeCleanerFetcher);
 };
 
@@ -54,42 +81,91 @@ ChromeCleanerFetcher::ChromeCleanerFetcher(
       url_fetcher_(net::URLFetcher::Create(0,
                                            GURL(GetSRTDownloadURL()),
                                            net::URLFetcher::GET,
-                                           this)) {
+                                           this)),
+      blocking_task_runner_(base::CreateSequencedTaskRunnerWithTraits(
+          {base::MayBlock(), base::TaskPriority::BACKGROUND,
+           base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})),
+      scoped_temp_dir_(new base::ScopedTempDir()) {
+  base::PostTaskAndReplyWithResult(
+      blocking_task_runner_.get(), FROM_HERE,
+      base::Bind(&ChromeCleanerFetcher::CreateTemporaryFile,
+                 base::Unretained(this)),
+      base::Bind(&ChromeCleanerFetcher::OnTemporaryFileCreated,
+                 base::Unretained(this)));
+}
+
+ChromeCleanerFetcher::~ChromeCleanerFetcher() = default;
+
+bool ChromeCleanerFetcher::CreateTemporaryFile() {
+  return scoped_temp_dir_->CreateUniqueTempDir() &&

[csharp, 2017/06/06 19:01:53]

AS discussed offline, you should create the path name manually using
GetTempDirPrefix so you can add a common string to all the temp dirs, so they
can be easily grabbed

[alito, 2017/06/06 19:32:44]

Done.

+         base::CreateTemporaryFileInDir(scoped_temp_dir_->GetPath(),
+                                        &temp_file_);

[csharp, 2017/06/06 19:01:54]

Does a file need to be created here, or is this just for the temp file name? If
just for the name, why not just create it yourself? Something like
ASCIIToUTF16(base::GenerateGUID())

[alito, 2017/06/06 19:32:44]

Done.

+}
+
+void ChromeCleanerFetcher::OnTemporaryFileCreated(bool success) {
+  if (!success) {
+    PostCallbackAndDeleteSelf(
+        base::FilePath(),
+        ChromeCleanerFetchStatus::kFailedToCreateTemporaryFile);
+    return;
+  }
+
+  DCHECK(!temp_file_.empty());
+
   data_use_measurement::DataUseUserData::AttachToFetcher(
       url_fetcher_.get(), data_use_measurement::DataUseUserData::SAFE_BROWSING);
   url_fetcher_->SetLoadFlags(net::LOAD_DISABLE_CACHE);
   url_fetcher_->SetMaxRetriesOn5xx(3);
-  url_fetcher_->SaveResponseToTemporaryFile(
-      content::BrowserThread::GetTaskRunnerForThread(
-          content::BrowserThread::FILE));
+  url_fetcher_->SaveResponseToFileAtPath(temp_file_, blocking_task_runner_);
   url_fetcher_->SetRequestContext(g_browser_process->system_request_context());
   url_fetcher_->Start();
 }
 
-ChromeCleanerFetcher::~ChromeCleanerFetcher() = default;
+void ChromeCleanerFetcher::PostCallbackAndDeleteSelf(
+    base::FilePath path,
+    ChromeCleanerFetchStatus fetch_status) {
+  DCHECK(fetched_callback_);
+
+  std::move(fetched_callback_).Run(std::move(path), fetch_status);
+
+  // Since url_fetcher_ is passed a pointer to this object during construction,
+  // explicitly destroy the url_fetcher_ to avoid potential destruction races.
+  url_fetcher_.reset();
+
+  // At this point, the url_fetcher_ is gone and this ChromeCleanerFetcher
+  // instance is no longer needed.
+  delete this;
+}
 
 void ChromeCleanerFetcher::OnURLFetchComplete(const net::URLFetcher* source) {
   // Take ownership of the fetcher in this scope (source == url_fetcher_).
   DCHECK_EQ(url_fetcher_.get(), source);
+  DCHECK(!source->GetStatus().is_io_pending());
+  DCHECK(fetched_callback_);
+
+  if (source->GetResponseCode() == net::HTTP_NOT_FOUND) {
+    PostCallbackAndDeleteSelf(base::FilePath(),
+                              ChromeCleanerFetchStatus::kNotFoundOnServer);
+    return;
+  }
 
   base::FilePath download_path;
-  if (source->GetStatus().is_success() &&
-      source->GetResponseCode() == net::HTTP_OK &&
-      source->GetResponseAsFilePath(true, &download_path)) {
-    DCHECK(!download_path.empty());
+  if (!source->GetStatus().is_success() ||
+      source->GetResponseCode() != net::HTTP_OK ||
+      !source->GetResponseAsFilePath(/*take_ownership=*/true, &download_path)) {
+    PostCallbackAndDeleteSelf(base::FilePath(),
+                              ChromeCleanerFetchStatus::kOtherFailure);
+    return;
   }
 
-  DCHECK(fetched_callback_);
-  std::move(fetched_callback_)
-      .Run(std::move(download_path), source->GetResponseCode());
+  DCHECK(!download_path.empty());
+  DCHECK_EQ(temp_file_.value(), download_path.value());
 
-  // Since url_fetcher_ is passed a pointer to this object during construction,
-  // explicitly destroy the url_fetcher_ to avoid potential destruction races.
-  url_fetcher_.reset();
+  // Take ownership of the scoped temp directory so it is not deleted.
+  scoped_temp_dir_->Take();
 
-  // At this point, the url_fetcher_ is gone and this ChromeCleanerFetcher
-  // instance is no longer needed.
-  delete this;
+  PostCallbackAndDeleteSelf(std::move(download_path),
+                            ChromeCleanerFetchStatus::kSuccess);
 }
 
 }  // namespace