Convert utility process Safe Browsing ZIP/DMG Analyzer IPC to mojo

Add SafeArchiveAnalyzer mojom service used to inspect ZIP / DMG
file archives for safe browsing download protection. Expose the
mojo to the browser via the utility process policy file. Update
clients to call the service using a mojo utility client, remove
all dependencies on UtilityProcessHostClient. Add native traits
for the file analyze results.

Align the ZIP/DMG clients: make their API's match, remove class
base::File fields (curry the file(s) to the analyzer step), and
remove ZIP client temp file clean-up code (not needed since the
utility process now owns the files).

Covered by existing tests:

SandboxedDMGAnalyzerTest.AnalyzeDMG, SandboxedZipAnalyzerTest.*

both of which use a content::InProcessUtilityThreadHelper viz.,
SafeArchiveAnalyzer mojo is called by these tests.

BUG=597124

Review-Url: https://codereview.chromium.org/2737763002
Cr-Commit-Position: refs/heads/master@{#456255}

chrome/browser/chrome_content_utility_manifest_overlay.json

@@ -7,6 +7,7 @@
           "chrome::mojom::FilePatcher",
           "chrome::mojom::ProfileImport",
           "chrome::mojom::ResourceUsageReporter",
+          "chrome::mojom::SafeArchiveAnalyzer",
           "chrome::mojom::ShellHandler",
           "chrome::mojom::ZipFileCreator",
           "extensions::mojom::MediaParser",

chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac.cc

@@ -8,96 +8,79 @@
 
 #include "base/bind.h"
 #include "base/task_scheduler/post_task.h"
-#include "chrome/common/chrome_utility_messages.h"
 #include "chrome/common/safe_browsing/zip_analyzer_results.h"
 #include "chrome/grit/generated_resources.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/child_process_data.h"
-#include "content/public/browser/render_process_host.h"
-#include "ipc/ipc_message_macros.h"
-#include "ipc/ipc_platform_file.h"
 #include "ui/base/l10n/l10n_util.h"
 
-using content::BrowserThread;
-
 namespace safe_browsing {
 
 SandboxedDMGAnalyzer::SandboxedDMGAnalyzer(const base::FilePath& dmg_file,
-                                           const ResultsCallback& callback)
-    : file_path_(dmg_file), callback_(callback), callback_called_(false) {
+                                           const ResultCallback& callback)
+    : file_path_(dmg_file), callback_(callback) {
+  DCHECK(callback);
 }
 
-SandboxedDMGAnalyzer::~SandboxedDMGAnalyzer() {}
-
 void SandboxedDMGAnalyzer::Start() {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   base::PostTaskWithTraits(
-      FROM_HERE, base::TaskTraits()
-                     .MayBlock()
-                     .WithPriority(base::TaskPriority::BACKGROUND)
-                     .WithShutdownBehavior(
-                         base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN),
-      base::Bind(&SandboxedDMGAnalyzer::OpenDMGFile, this));
+      FROM_HERE,
+      base::TaskTraits()
+          .MayBlock()
+          .WithPriority(base::TaskPriority::BACKGROUND)
+          .WithShutdownBehavior(
+              base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN),
+      base::Bind(&SandboxedDMGAnalyzer::PrepareFileToAnalyze, this));
 }
 
-void SandboxedDMGAnalyzer::OpenDMGFile() {
-  file_.Initialize(file_path_, base::File::FLAG_OPEN | base::File::FLAG_READ);
-  if (!file_.IsValid()) {
-    DLOG(ERROR) << "Could not open DMG file at path " << file_path_.value();
-    BrowserThread::PostTask(
-        BrowserThread::IO, FROM_HERE,
-        base::Bind(&SandboxedDMGAnalyzer::OnAnalysisFinished, this,
-                   zip_analyzer::Results()));
+SandboxedDMGAnalyzer::~SandboxedDMGAnalyzer() = default;
+
+void SandboxedDMGAnalyzer::PrepareFileToAnalyze() {
+  base::File file(file_path_, base::File::FLAG_OPEN | base::File::FLAG_READ);
+
+  if (!file.IsValid()) {
+    DLOG(ERROR) << "Could not open file: " << file_path_.value();
+    ReportFileFailure();
     return;
   }
 
-  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
-                          base::Bind(&SandboxedDMGAnalyzer::StartAnalysis,
-                                     this));
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&SandboxedDMGAnalyzer::AnalyzeFile, this,
+                 base::Passed(&file)));
 }
 
-void SandboxedDMGAnalyzer::StartAnalysis() {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+void SandboxedDMGAnalyzer::ReportFileFailure() {
+  DCHECK(!utility_process_mojo_client_);
 
-  content::UtilityProcessHost* utility_process_host =
-      content::UtilityProcessHost::Create(
-          this, BrowserThread::GetTaskRunnerForThread(BrowserThread::IO));
-
-  utility_process_host->SetName(l10n_util::GetStringUTF16(
-      IDS_UTILITY_PROCESS_SAFE_BROWSING_ZIP_FILE_ANALYZER_NAME));
-  utility_process_host->Send(
-      new ChromeUtilityMsg_AnalyzeDmgFileForDownloadProtection(
-          IPC::TakePlatformFileForTransit(std::move(file_))));
+  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+                                   base::Bind(callback_, Results()));
 }
 
-void SandboxedDMGAnalyzer::OnProcessCrashed(int exit_code) {
-  OnAnalysisFinished(zip_analyzer::Results());
-}
+void SandboxedDMGAnalyzer::AnalyzeFile(base::File file) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(!utility_process_mojo_client_);
 
-void SandboxedDMGAnalyzer::OnProcessLaunchFailed(int error_code) {
-  OnAnalysisFinished(zip_analyzer::Results());
-}
+  utility_process_mojo_client_ = base::MakeUnique<
+      content::UtilityProcessMojoClient<chrome::mojom::SafeArchiveAnalyzer>>(
+      l10n_util::GetStringUTF16(
+          IDS_UTILITY_PROCESS_SAFE_BROWSING_ZIP_FILE_ANALYZER_NAME));
+  utility_process_mojo_client_->set_error_callback(
+      base::Bind(&SandboxedDMGAnalyzer::AnalyzeFileDone, this, Results()));
 
-bool SandboxedDMGAnalyzer::OnMessageReceived(const IPC::Message& message) {
-  bool handled = true;
-  IPC_BEGIN_MESSAGE_MAP(SandboxedDMGAnalyzer, message)
-    IPC_MESSAGE_HANDLER(
-        ChromeUtilityHostMsg_AnalyzeDmgFileForDownloadProtection_Finished,
-        OnAnalysisFinished)
-    IPC_MESSAGE_UNHANDLED(handled = false)
-  IPC_END_MESSAGE_MAP()
-  return handled;
+  utility_process_mojo_client_->Start();
+
+  utility_process_mojo_client_->service()->AnalyzeDmgFile(
+      std::move(file),
+      base::Bind(&SandboxedDMGAnalyzer::AnalyzeFileDone, this));
 }
 
-void SandboxedDMGAnalyzer::OnAnalysisFinished(
-    const zip_analyzer::Results& results) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  if (callback_called_)
-    return;
+void SandboxedDMGAnalyzer::AnalyzeFileDone(const Results& results) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  callback_called_ = true;
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                          base::Bind(callback_, results));
+  utility_process_mojo_client_.reset();
+  callback_.Run(results);
 }
 
 }  // namespace safe_browsing

chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac.h

@@ -9,54 +9,55 @@
 #include "base/files/file.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
-#include "base/memory/weak_ptr.h"
-#include "content/public/browser/utility_process_host.h"
-#include "content/public/browser/utility_process_host_client.h"
+#include "base/memory/ref_counted.h"
+#include "chrome/common/safe_archive_analyzer.mojom.h"
+#include "content/public/browser/utility_process_mojo_client.h"
 
 namespace safe_browsing {
 
-namespace zip_analyzer {
-struct Results;
-}
-
-// This class is used to analyze DMG files in a sandboxed utility process for
-// file download protection. This class must be created on the UI thread, and
-// which is where the result callback will be invoked.
-class SandboxedDMGAnalyzer : public content::UtilityProcessHostClient {
+// This class is used to analyze DMG files in a sandboxed utility process
+// for file download protection. This class lives on the UI thread, which
+// is where the result callback will be invoked.
+class SandboxedDMGAnalyzer
+    : public base::RefCountedThreadSafe<SandboxedDMGAnalyzer> {
  public:
-  // Callback that is invoked when the analysis results are ready.
-  using ResultsCallback = base::Callback<void(const zip_analyzer::Results&)>;
+  using Results = zip_analyzer::Results;
+
+  using ResultCallback = base::Callback<void(const Results&)>;
 
   SandboxedDMGAnalyzer(const base::FilePath& dmg_file,
-                       const ResultsCallback& callback);
+                       const ResultCallback& callback);
 
-  // Begins the analysis. This must be called on the UI thread.
+  // Starts the analysis. Must be called on the UI thread.
   void Start();
 
  private:
-  ~SandboxedDMGAnalyzer() override;
+  friend class base::RefCountedThreadSafe<SandboxedDMGAnalyzer>;
+
+  ~SandboxedDMGAnalyzer();
+
+  // Prepare the file for analysis.
+  void PrepareFileToAnalyze();
 
-  // Called on the blocking pool, this opens the DMG file, in preparation for
-  // sending the FD to the utility process.
-  void OpenDMGFile();
+  // If file preparation failed, analysis has failed: report failure.
+  void ReportFileFailure();
 
-  // Called on the IO thread, this starts the utility process.
-  void StartAnalysis();
+  // Starts the utility process and sends it a file analyze request.
+  void AnalyzeFile(base::File file);
 
-  // content::UtilityProcessHostClient:
-  void OnProcessCrashed(int exit_code) override;
-  void OnProcessLaunchFailed(int error_code) override;
-  bool OnMessageReceived(const IPC::Message& message) override;
+  // The response containing the file analyze results.
+  void AnalyzeFileDone(const Results& results);
 
-  // Message handler to receive the results of the analysis. Invokes the
-  // |callback_|.
-  void OnAnalysisFinished(const zip_analyzer::Results& results);
+  // The file path of the file to analyze.
+  const base::FilePath file_path_;
 
-  const base::FilePath file_path_;  // The path of the DMG file.
-  base::File file_;  // The opened file handle for |file_path_|.
+  // Utility client used to send analyze tasks to the utility process.
+  std::unique_ptr<
+      content::UtilityProcessMojoClient<chrome::mojom::SafeArchiveAnalyzer>>
+      utility_process_mojo_client_;
 
-  const ResultsCallback callback_;  // Result callback.
-  bool callback_called_;  // Whether |callback_| has already been invoked.
+  // Callback invoked on the UI thread with the file analyze results.
+  const ResultCallback callback_;
 
   DISALLOW_COPY_AND_ASSIGN(SandboxedDMGAnalyzer);
 };

chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac_unittest.cc

@@ -47,15 +47,15 @@ class SandboxedDMGAnalyzerTest : public testing::Test {
   }
 
  private:
-  // A helper class to store the results from the ResultsCallback and run
-  // another closure.
+  // A helper that provides a SandboxedDMGAnalyzer::ResultCallback that will
+  // store a copy of an analyzer's results and then run a closure.
   class ResultsGetter {
    public:
     ResultsGetter(const base::Closure& next_closure,
                   zip_analyzer::Results* results)
         : next_closure_(next_closure), results_(results) {}
 
-    SandboxedDMGAnalyzer::ResultsCallback GetCallback() {
+    SandboxedDMGAnalyzer::ResultCallback GetCallback() {
       return base::Bind(&ResultsGetter::ResultsCallback,
                         base::Unretained(this));
     }

chrome/browser/safe_browsing/sandboxed_zip_analyzer.cc

@@ -7,146 +7,96 @@
 #include <utility>
 
 #include "base/bind.h"
-#include "base/command_line.h"
 #include "base/files/file_util.h"
-#include "base/location.h"
-#include "base/logging.h"
 #include "base/task_scheduler/post_task.h"
-#include "base/threading/sequenced_worker_pool.h"
-#include "chrome/common/chrome_utility_messages.h"
 #include "chrome/common/safe_browsing/zip_analyzer_results.h"
 #include "chrome/grit/generated_resources.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/child_process_data.h"
-#include "content/public/browser/render_process_host.h"
-#include "content/public/common/content_switches.h"
-#include "ipc/ipc_message_macros.h"
-#include "ipc/ipc_platform_file.h"
 #include "ui/base/l10n/l10n_util.h"
 
-using content::BrowserThread;
-
 namespace safe_browsing {
 
-SandboxedZipAnalyzer::SandboxedZipAnalyzer(
-    const base::FilePath& zip_file,
-    const ResultCallback& result_callback)
-    : zip_file_name_(zip_file),
-      callback_(result_callback),
-      callback_called_(false) {
+SandboxedZipAnalyzer::SandboxedZipAnalyzer(const base::FilePath& zip_file,
+                                           const ResultCallback& callback)
+    : file_path_(zip_file), callback_(callback) {
+  DCHECK(callback);
 }
 
 void SandboxedZipAnalyzer::Start() {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  // Starting the analyzer will block on opening the zip file, so run this
-  // on a worker thread.  The task does not need to block shutdown.
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   base::PostTaskWithTraits(
-      FROM_HERE, base::TaskTraits()
-                     .MayBlock()
-                     .WithPriority(base::TaskPriority::BACKGROUND)
-                     .WithShutdownBehavior(
-                         base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN),
-      base::Bind(&SandboxedZipAnalyzer::AnalyzeInSandbox, this));
+      FROM_HERE,
+      base::TaskTraits()
+          .MayBlock()
+          .WithPriority(base::TaskPriority::BACKGROUND)
+          .WithShutdownBehavior(
+              base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN),
+      base::Bind(&SandboxedZipAnalyzer::PrepareFileToAnalyze, this));
 }
 
-SandboxedZipAnalyzer::~SandboxedZipAnalyzer() {
-  // If we're using UtilityProcessHost, we may not be destroyed on
-  // the UI or IO thread.
-  CloseTemporaryFile();
-}
+SandboxedZipAnalyzer::~SandboxedZipAnalyzer() = default;
 
-void SandboxedZipAnalyzer::CloseTemporaryFile() {
-  if (!temp_file_.IsValid())
-    return;
-  // Close the temporary file in the blocking pool since doing so will delete
-  // the file.
-  base::PostTaskWithTraits(
-      FROM_HERE, base::TaskTraits()
-                     .MayBlock()
-                     .WithPriority(base::TaskPriority::BACKGROUND)
-                     .WithShutdownBehavior(
-                         base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN),
-      base::Bind(&base::File::Close,
-                 base::Owned(new base::File(std::move(temp_file_)))));
-}
+void SandboxedZipAnalyzer::PrepareFileToAnalyze() {
+  base::File file(file_path_, base::File::FLAG_OPEN | base::File::FLAG_READ);
 
-void SandboxedZipAnalyzer::AnalyzeInSandbox() {
-  // This zip file will be closed on the IO thread once it has been handed
-  // off to the child process.
-  zip_file_.Initialize(zip_file_name_,
-                       base::File::FLAG_OPEN | base::File::FLAG_READ);
-  if (!zip_file_.IsValid()) {
-    DVLOG(1) << "Could not open zip file: " << zip_file_name_.value();
-    if (!BrowserThread::PostTask(
-            BrowserThread::IO, FROM_HERE,
-            base::Bind(&SandboxedZipAnalyzer::OnAnalyzeZipFileFinished, this,
-                       zip_analyzer::Results()))) {
-      NOTREACHED();
-    }
+  if (!file.IsValid()) {
+    DLOG(ERROR) << "Could not open file: " << file_path_.value();
+    ReportFileFailure();
     return;
   }
 
-  // This temp file will be closed in the blocking pool when results from the
-  // analyzer return.
   base::FilePath temp_path;
+  base::File temp_file;
   if (base::CreateTemporaryFile(&temp_path)) {
-    temp_file_.Initialize(temp_path, (base::File::FLAG_CREATE_ALWAYS |
-                                      base::File::FLAG_READ |
-                                      base::File::FLAG_WRITE |
-                                      base::File::FLAG_TEMPORARY |
-                                      base::File::FLAG_DELETE_ON_CLOSE));
+    temp_file.Initialize(
+        temp_path, (base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_READ |
+                    base::File::FLAG_WRITE | base::File::FLAG_TEMPORARY |
+                    base::File::FLAG_DELETE_ON_CLOSE));
   }
-  DVLOG_IF(1, !temp_file_.IsValid())
-      << "Could not open temporary output file: " << temp_path.value();
 
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(&SandboxedZipAnalyzer::StartProcessOnIOThread, this));
-}
+  if (!temp_file.IsValid()) {
+    DLOG(ERROR) << "Could not open temp file: " << temp_path.value();
+    ReportFileFailure();
+    return;
+  }
 
-void SandboxedZipAnalyzer::OnProcessCrashed(int exit_code) {
-  OnAnalyzeZipFileFinished(zip_analyzer::Results());
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&SandboxedZipAnalyzer::AnalyzeFile, this, base::Passed(&file),
+                 base::Passed(&temp_file)));
 }
 
-void SandboxedZipAnalyzer::OnProcessLaunchFailed(int error_code) {
-  OnAnalyzeZipFileFinished(zip_analyzer::Results());
-}
+void SandboxedZipAnalyzer::ReportFileFailure() {
+  DCHECK(!utility_process_mojo_client_);
 
-bool SandboxedZipAnalyzer::OnMessageReceived(const IPC::Message& message) {
-  bool handled = true;
-  IPC_BEGIN_MESSAGE_MAP(SandboxedZipAnalyzer, message)
-    IPC_MESSAGE_HANDLER(
-        ChromeUtilityHostMsg_AnalyzeZipFileForDownloadProtection_Finished,
-        OnAnalyzeZipFileFinished)
-    IPC_MESSAGE_UNHANDLED(handled = false)
-  IPC_END_MESSAGE_MAP()
-  return handled;
+  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+                                   base::Bind(callback_, Results()));
 }
 
-void SandboxedZipAnalyzer::StartProcessOnIOThread() {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  utility_process_host_ =
-      content::UtilityProcessHost::Create(
-          this, BrowserThread::GetTaskRunnerForThread(BrowserThread::IO).get())
-          ->AsWeakPtr();
-  utility_process_host_->SetName(l10n_util::GetStringUTF16(
-      IDS_UTILITY_PROCESS_SAFE_BROWSING_ZIP_FILE_ANALYZER_NAME));
-  utility_process_host_->Send(
-      new ChromeUtilityMsg_AnalyzeZipFileForDownloadProtection(
-          IPC::TakePlatformFileForTransit(std::move(zip_file_)),
-          IPC::GetPlatformFileForTransit(temp_file_.GetPlatformFile(),
-                                         false /* !close_source_handle */)));
+void SandboxedZipAnalyzer::AnalyzeFile(base::File file, base::File temp_file) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(!utility_process_mojo_client_);
+
+  utility_process_mojo_client_ = base::MakeUnique<
+      content::UtilityProcessMojoClient<chrome::mojom::SafeArchiveAnalyzer>>(
+      l10n_util::GetStringUTF16(
+          IDS_UTILITY_PROCESS_SAFE_BROWSING_ZIP_FILE_ANALYZER_NAME));
+  utility_process_mojo_client_->set_error_callback(
+      base::Bind(&SandboxedZipAnalyzer::AnalyzeFileDone, this, Results()));
+
+  utility_process_mojo_client_->Start();
+
+  utility_process_mojo_client_->service()->AnalyzeZipFile(
+      std::move(file), std::move(temp_file),
+      base::Bind(&SandboxedZipAnalyzer::AnalyzeFileDone, this));
 }
 
-void SandboxedZipAnalyzer::OnAnalyzeZipFileFinished(
-    const zip_analyzer::Results& results) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  if (callback_called_)
-    return;
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                          base::Bind(callback_, results));
-  callback_called_ = true;
-  CloseTemporaryFile();
+void SandboxedZipAnalyzer::AnalyzeFileDone(const Results& results) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  utility_process_mojo_client_.reset();
+  callback_.Run(results);
 }
 
 }  // namespace safe_browsing

chrome/browser/safe_browsing/sandboxed_zip_analyzer.h

 // 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.
-//
-// Browser-side interface to analyze zip files for SafeBrowsing download
-// protection.  The actual zip decoding is performed in a sandboxed utility
-// process.
-//
-// This class lives on the UI thread.
 
 #ifndef CHROME_BROWSER_SAFE_BROWSING_SANDBOXED_ZIP_ANALYZER_H_
 #define CHROME_BROWSER_SAFE_BROWSING_SANDBOXED_ZIP_ANALYZER_H_
@@ -15,66 +9,55 @@
 #include "base/files/file.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
-#include "base/memory/weak_ptr.h"
-#include "content/public/browser/utility_process_host.h"
-#include "content/public/browser/utility_process_host_client.h"
-
-namespace IPC {
-class Message;
-}
+#include "base/memory/ref_counted.h"
+#include "chrome/common/safe_archive_analyzer.mojom.h"
+#include "content/public/browser/utility_process_mojo_client.h"
 
 namespace safe_browsing {
-namespace zip_analyzer {
-struct Results;
-}
 
-class SandboxedZipAnalyzer : public content::UtilityProcessHostClient {
+// This class is used to analyze zip files in a sandboxed utility process
+// for file download protection. This class lives on the UI thread, which
+// is where the result callback will be invoked.
+class SandboxedZipAnalyzer
+    : public base::RefCountedThreadSafe<SandboxedZipAnalyzer> {
  public:
-  // Callback that is invoked when the analysis results are ready.
-  typedef base::Callback<void(const zip_analyzer::Results&)> ResultCallback;
+  using Results = zip_analyzer::Results;
+
+  using ResultCallback = base::Callback<void(const Results&)>;
 
   SandboxedZipAnalyzer(const base::FilePath& zip_file,
-                       const ResultCallback& result_callback);
+                       const ResultCallback& callback);
 
-  // Posts a task to start the zip analysis in the utility process.
+  // Starts the analysis. Must be called on the UI thread.
   void Start();
 
  private:
-  ~SandboxedZipAnalyzer() override;
-
-  // Posts a fire-and-forget task to close the temporary file in the blocking
-  // pool.
-  void CloseTemporaryFile();
-
-  // Creates the sandboxed utility process and tells it to start analysis.
-  // Runs on a worker thread.
-  void AnalyzeInSandbox();
-
-  // content::UtilityProcessHostClient implementation.
-  // These notifications run on the IO thread.
-  void OnProcessCrashed(int exit_code) override;
-  void OnProcessLaunchFailed(int error_code) override;
-  bool OnMessageReceived(const IPC::Message& message) override;
-
-  // Launches the utility process.  Must run on the IO thread.
-  void StartProcessOnIOThread();
-
-  // Notification from the utility process that the zip file has been analyzed,
-  // with the given results.  Runs on the IO thread.
-  void OnAnalyzeZipFileFinished(const zip_analyzer::Results& results);
-
-  const base::FilePath zip_file_name_;
-  // Once we have opened the file, we store the handle so that we can use it
-  // once the utility process has launched.
-  base::File zip_file_;
-
-  // A temporary file to be used by the utility process for extracting files
-  // from the archive.
-  base::File temp_file_;
-  base::WeakPtr<content::UtilityProcessHost> utility_process_host_;
+  friend class base::RefCountedThreadSafe<SandboxedZipAnalyzer>;
+
+  ~SandboxedZipAnalyzer();
+
+  // Prepare the file for analysis.
+  void PrepareFileToAnalyze();
+
+  // If file preparation failed, analysis has failed: report failure.
+  void ReportFileFailure();
+
+  // Starts the utility process and sends it a file analyze request.
+  void AnalyzeFile(base::File file, base::File temp);
+
+  // The response containing the file analyze results.
+  void AnalyzeFileDone(const Results& results);
+
+  // The file path of the file to analyze.
+  const base::FilePath file_path_;
+
+  // Utility client used to send analyze tasks to the utility process.
+  std::unique_ptr<
+      content::UtilityProcessMojoClient<chrome::mojom::SafeArchiveAnalyzer>>
+      utility_process_mojo_client_;
+
+  // Callback invoked on the UI thread with the file analyze results.
   const ResultCallback callback_;
-  // Initialized on the UI thread, but only accessed on the IO thread.
-  bool callback_called_;
 
   DISALLOW_COPY_AND_ASSIGN(SandboxedZipAnalyzer);
 };

chrome/common/BUILD.gn

@@ -517,6 +517,10 @@ static_library("common") {
         "safe_browsing/zip_analyzer_results.cc",
         "safe_browsing/zip_analyzer_results.h",
       ]
+
+      # safe_archive_analyzer.mojom has a [Native] trait that depends on
+      # the protobuf headers in the sources list above.
+      allow_circular_includes_from = [ ":mojo_bindings" ]
     }
   }
 
@@ -702,6 +706,10 @@ mojom("mojo_bindings") {
     sources += [ "zip_file_creator.mojom" ]
   }
 
+  if (safe_browsing_mode == 1) {
+    sources += [ "safe_archive_analyzer.mojom" ]
+  }
+
   public_deps = [
     "//components/content_settings/core/common:mojo_bindings",
     "//mojo/common:common_custom_types",

chrome/common/chrome_utility_messages.h

@@ -17,9 +17,6 @@
 #include "base/values.h"
 #include "build/build_config.h"
 #include "ipc/ipc_message_macros.h"
-#include "ipc/ipc_platform_file.h"
-#include "ui/gfx/ipc/gfx_param_traits.h"
-#include "ui/gfx/ipc/skia/gfx_skia_param_traits.h"
 
 #if defined(FULL_SAFE_BROWSING)
 #include "chrome/common/safe_browsing/ipc_protobuf_message_macros.h"
@@ -140,22 +137,6 @@ IPC_STRUCT_END()
 // Utility process messages:
 // These are messages from the browser to the utility process.
 
-#if defined(FULL_SAFE_BROWSING)
-// Tells the utility process to analyze a zip file for malicious download
-// protection, providing a file that can be used temporarily to analyze binaries
-// contained therein.
-IPC_MESSAGE_CONTROL2(ChromeUtilityMsg_AnalyzeZipFileForDownloadProtection,
-                     IPC::PlatformFileForTransit /* zip_file */,
-                     IPC::PlatformFileForTransit /* temp_file */)
-
-#if defined(OS_MACOSX)
-// Tells the utility process to analyze a DMG file for malicious download
-// protection.
-IPC_MESSAGE_CONTROL1(ChromeUtilityMsg_AnalyzeDmgFileForDownloadProtection,
-                     IPC::PlatformFileForTransit /* dmg_file */)
-#endif  // defined(OS_MACOSX)
-#endif  // defined(FULL_SAFE_BROWSING)
-
 #if defined(OS_WIN)
 // Instructs the utility process to invoke GetOpenFileName. |owner| is the
 // parent of the modal dialog, |flags| are OFN_* flags. |filter| constrains the
@@ -185,20 +166,6 @@ IPC_MESSAGE_CONTROL1(ChromeUtilityMsg_GetSaveFileName,
 IPC_MESSAGE_CONTROL1(ChromeUtilityHostMsg_UnpackWebResource_Failed,
                      std::string /* error_message, if any */)
 
-#if defined(FULL_SAFE_BROWSING)
-// Reply when a zip file has been analyzed for malicious download protection.
-IPC_MESSAGE_CONTROL1(
-    ChromeUtilityHostMsg_AnalyzeZipFileForDownloadProtection_Finished,
-    safe_browsing::zip_analyzer::Results)
-
-#if defined(OS_MACOSX)
-// Reply when a DMG file has been analyzed for malicious download protection.
-IPC_MESSAGE_CONTROL1(
-    ChromeUtilityHostMsg_AnalyzeDmgFileForDownloadProtection_Finished,
-    safe_browsing::zip_analyzer::Results)
-#endif  // defined(OS_MACOSX)
-#endif  // defined(FULL_SAFE_BROWSING)
-
 #if defined(OS_WIN)
 IPC_MESSAGE_CONTROL0(ChromeUtilityHostMsg_GetOpenFileName_Failed)
 IPC_MESSAGE_CONTROL2(ChromeUtilityHostMsg_GetOpenFileName_Result,

chrome/common/safe_archive_analyzer.mojom

+// Copyright 2017 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.
+
+// Safe archive file analyzer provided by the utility process and exposed
+// by mojo policy control to the chrome browser process when build flag
+// FULL_SAFE_BROWSING is enabled.
+
+module chrome.mojom;
+
+import "mojo/common/file.mojom";
+
+interface SafeArchiveAnalyzer {
+  // Build flag FULL_SAFE_BROWSING: Analyze the |zip_file| for malicious
+  // download protection, given a |temporary_file| used to extract files
+  // from the |zip_file| archive.
+  AnalyzeZipFile(mojo.common.mojom.File zip_file,
+                 mojo.common.mojom.File temporary_file)
+      => (SafeArchiveAnalyzerResults results);
+
+  // Build flag FULL_SAFE_BROWSING, on OS_MACOSX: Analyze the |dmg_file|
+  // for malicious download protection.
+  AnalyzeDmgFile(mojo.common.mojom.File dmg_file)
+      => (SafeArchiveAnalyzerResults results);
+};
+
+[Native]
+struct SafeArchiveAnalyzerResults;

chrome/common/safe_archive_analyzer.typemap

+# Copyright 2017 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.
+
+mojom = "//chrome/common/safe_archive_analyzer.mojom"
+
+public_headers = [ "//chrome/common/safe_browsing/zip_analyzer.h" ]
+
+traits_headers = [ "//chrome/common/chrome_utility_messages.h" ]
+
+deps = [
+  "//chrome/common/safe_browsing:proto",
+  "//components/safe_browsing:csd_proto",
+]
+
+type_mappings = [ "chrome.mojom.SafeArchiveAnalyzerResults=safe_browsing::zip_analyzer::Results" ]

chrome/typemaps.gni

@@ -2,4 +2,7 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-typemaps = [ "//chrome/common/instant.typemap" ]
+typemaps = [
+  "//chrome/common/safe_archive_analyzer.typemap",
+  "//chrome/common/instant.typemap",
+]

chrome/utility/chrome_content_utility_client.cc

@@ -13,11 +13,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/time/time.h"
-#include "build/build_config.h"
-#include "chrome/common/chrome_utility_messages.h"
 #include "chrome/common/file_patcher.mojom.h"
-#include "chrome/common/safe_browsing/zip_analyzer.h"
-#include "chrome/common/safe_browsing/zip_analyzer_results.h"
 #include "chrome/utility/utility_message_handler.h"
 #include "components/safe_json/utility/safe_json_parser_mojo_impl.h"
 #include "content/public/child/image_decoder_utils.h"
@@ -27,14 +23,12 @@
 #include "courgette/courgette.h"
 #include "courgette/third_party/bsdiff/bsdiff.h"
 #include "extensions/features/features.h"
-#include "ipc/ipc_channel.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
 #include "printing/features/features.h"
 #include "services/image_decoder/image_decoder_service.h"
 #include "services/image_decoder/public/interfaces/constants.mojom.h"
 #include "services/service_manager/public/cpp/interface_registry.h"
 #include "third_party/zlib/google/zip.h"
-#include "ui/gfx/geometry/size.h"
 
 #if !defined(OS_ANDROID)
 #include "chrome/common/resource_usage_reporter.mojom.h"
@@ -62,9 +56,14 @@
 #include "chrome/utility/printing_handler.h"
 #endif
 
-#if defined(OS_MACOSX) && defined(FULL_SAFE_BROWSING)
+#if defined(FULL_SAFE_BROWSING)
+#include "chrome/common/safe_archive_analyzer.mojom.h"
+#include "chrome/common/safe_browsing/zip_analyzer.h"
+#include "chrome/common/safe_browsing/zip_analyzer_results.h"
+#if defined(OS_MACOSX)
 #include "chrome/utility/safe_browsing/mac/dmg_analyzer.h"
 #endif
+#endif
 
 namespace {
 
@@ -135,6 +134,47 @@ class ZipFileCreatorImpl : public chrome::mojom::ZipFileCreator {
 };
 #endif  // defined(OS_CHROMEOS)
 
+#if defined(FULL_SAFE_BROWSING)
+class SafeArchiveAnalyzerImpl : public chrome::mojom::SafeArchiveAnalyzer {
+ public:
+  SafeArchiveAnalyzerImpl() = default;
+  ~SafeArchiveAnalyzerImpl() override = default;
+
+  static void Create(chrome::mojom::SafeArchiveAnalyzerRequest request) {
+    mojo::MakeStrongBinding(base::MakeUnique<SafeArchiveAnalyzerImpl>(),
+                            std::move(request));
+  }
+
+ private:
+  // chrome::mojom::SafeArchiveAnalyzer:
+  void AnalyzeZipFile(base::File zip_file,
+                      base::File temporary_file,
+                      const AnalyzeZipFileCallback& callback) override {
+    DCHECK(temporary_file.IsValid());
+    DCHECK(zip_file.IsValid());
+
+    safe_browsing::zip_analyzer::Results results;
+    safe_browsing::zip_analyzer::AnalyzeZipFile(
+        std::move(zip_file), std::move(temporary_file), &results);
+    callback.Run(results);
+  }
+
+  void AnalyzeDmgFile(base::File dmg_file,
+                      const AnalyzeDmgFileCallback& callback) override {
+#if defined(OS_MACOSX)
+    DCHECK(dmg_file.IsValid());
+    safe_browsing::zip_analyzer::Results results;
+    safe_browsing::dmg::AnalyzeDMGFile(std::move(dmg_file), &results);
+    callback.Run(results);
+#else
+    NOTREACHED();
+#endif
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(SafeArchiveAnalyzerImpl);
+};
+#endif  // defined(FULL_SAFE_BROWSING)
+
 #if !defined(OS_ANDROID)
 void CreateProxyResolverFactory(
     net::interfaces::ProxyResolverFactoryRequest request) {
@@ -210,22 +250,6 @@ bool ChromeContentUtilityClient::OnMessageReceived(
   if (utility_process_running_elevated_)
     return false;
 
-  bool handled = true;
-  IPC_BEGIN_MESSAGE_MAP(ChromeContentUtilityClient, message)
-#if defined(FULL_SAFE_BROWSING)
-    IPC_MESSAGE_HANDLER(ChromeUtilityMsg_AnalyzeZipFileForDownloadProtection,
-                        OnAnalyzeZipFileForDownloadProtection)
-#if defined(OS_MACOSX)
-    IPC_MESSAGE_HANDLER(ChromeUtilityMsg_AnalyzeDmgFileForDownloadProtection,
-                        OnAnalyzeDmgFileForDownloadProtection)
-#endif  // defined(OS_MACOSX)
-#endif  // defined(FULL_SAFE_BROWSING)
-    IPC_MESSAGE_UNHANDLED(handled = false)
-  IPC_END_MESSAGE_MAP()
-
-  if (handled)
-    return true;
-
   for (auto* handler : handlers_) {
     if (handler->OnMessageReceived(message))
       return true;
@@ -260,6 +284,9 @@ void ChromeContentUtilityClient::ExposeInterfacesToBrowser(
 #if defined(OS_CHROMEOS)
   registry->AddInterface(base::Bind(&ZipFileCreatorImpl::Create));
 #endif
+#if defined(FULL_SAFE_BROWSING)
+  registry->AddInterface(base::Bind(&SafeArchiveAnalyzerImpl::Create));
+#endif
 }
 
 void ChromeContentUtilityClient::RegisterServices(StaticServiceMap* services) {
@@ -275,32 +302,3 @@ void ChromeContentUtilityClient::PreSandboxStartup() {
   extensions::ExtensionsHandler::PreSandboxStartup();
 #endif
 }
-
-#if defined(FULL_SAFE_BROWSING)
-void ChromeContentUtilityClient::OnAnalyzeZipFileForDownloadProtection(
-    const IPC::PlatformFileForTransit& zip_file,
-    const IPC::PlatformFileForTransit& temp_file) {
-  safe_browsing::zip_analyzer::Results results;
-  safe_browsing::zip_analyzer::AnalyzeZipFile(
-      IPC::PlatformFileForTransitToFile(zip_file),
-      IPC::PlatformFileForTransitToFile(temp_file), &results);
-  content::UtilityThread::Get()->Send(
-      new ChromeUtilityHostMsg_AnalyzeZipFileForDownloadProtection_Finished(
-          results));
-  content::UtilityThread::Get()->ReleaseProcessIfNeeded();
-}
-
-#if defined(OS_MACOSX)
-void ChromeContentUtilityClient::OnAnalyzeDmgFileForDownloadProtection(
-    const IPC::PlatformFileForTransit& dmg_file) {
-  safe_browsing::zip_analyzer::Results results;
-  safe_browsing::dmg::AnalyzeDMGFile(
-      IPC::PlatformFileForTransitToFile(dmg_file), &results);
-  content::UtilityThread::Get()->Send(
-      new ChromeUtilityHostMsg_AnalyzeDmgFileForDownloadProtection_Finished(
-          results));
-  content::UtilityThread::Get()->ReleaseProcessIfNeeded();
-}
-#endif  // defined(OS_MACOSX)
-
-#endif  // defined(FULL_SAFE_BROWSING)

chrome/utility/chrome_content_utility_client.h

@@ -5,12 +5,9 @@
 #ifndef CHROME_UTILITY_CHROME_CONTENT_UTILITY_CLIENT_H_
 #define CHROME_UTILITY_CHROME_CONTENT_UTILITY_CLIENT_H_
 
-#include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/memory/scoped_vector.h"
-#include "build/build_config.h"
 #include "content/public/utility/content_utility_client.h"
-#include "ipc/ipc_platform_file.h"
 
 class UtilityMessageHandler;
 
@@ -30,20 +27,10 @@ class ChromeContentUtilityClient : public content::ContentUtilityClient {
 
  private:
   // IPC message handlers.
-  void OnStartupPing();
-#if defined(FULL_SAFE_BROWSING)
-  void OnAnalyzeZipFileForDownloadProtection(
-      const IPC::PlatformFileForTransit& zip_file,
-      const IPC::PlatformFileForTransit& temp_file);
-#if defined(OS_MACOSX)
-  void OnAnalyzeDmgFileForDownloadProtection(
-      const IPC::PlatformFileForTransit& dmg_file);
-#endif  // defined(OS_MACOSX)
-#endif  // defined(FULL_SAFE_BROWSING)
-
   typedef ScopedVector<UtilityMessageHandler> Handlers;
   Handlers handlers_;
 
+  // True if the utility process runs with elevated privileges.
   bool utility_process_running_elevated_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeContentUtilityClient);