Use UtilityProcessHost to patch files.

chrome/browser/component_updater/component_patcher_operation.cc

 // Copyright 2013 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/component_updater/component_patcher_operation.h"
 
 #include <string>
 #include <vector>
 
+#include "base/bind.h"
 #include "base/file_util.h"
 #include "base/files/memory_mapped_file.h"
 #include "base/json/json_file_value_serializer.h"
 #include "base/memory/scoped_handle.h"
 #include "base/path_service.h"
 #include "base/strings/string_number_conversions.h"
 #include "chrome/browser/component_updater/component_patcher.h"
 #include "chrome/browser/component_updater/component_updater_service.h"
+#include "chrome/common/chrome_utility_messages.h"
 #include "chrome/common/extensions/extension_constants.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/utility_process_host.h"
+#include "courgette/courgette.h"
+#include "courgette/third_party/bsdiff.h"
 #include "crypto/secure_hash.h"
 #include "crypto/sha2.h"
 #include "crypto/signature_verifier.h"
 #include "extensions/common/crx_file.h"
+#include "ipc/ipc_message_macros.h"
 #include "third_party/zlib/google/zip.h"
 
 using crypto::SecureHash;
 
 namespace component_updater {
 
 namespace {
 
 const char kInput[] = "input";
 const char kOp[] = "op";
 const char kOutput[] = "output";
 const char kPatch[] = "patch";
 const char kSha256[] = "sha256";
 
+// These error ranges are overlapping, so we use an offset to disambiguate

[Sorin Jianu, 2014/02/03 20:57:57]

The integer offset disambiguates between overlapping error ranges.

[waffles, 2014/02/07 01:00:59]

Done.

+// them.
+const int kCourgetteErrorOffset = 300;
+const int kBsdiffErrorOffset = 600;
+
 }  // namespace
 
 DeltaUpdateOp* CreateDeltaUpdateOp(base::DictionaryValue* command) {

[Sorin Jianu, 2014/02/03 20:57:57]

can the argument be const?

[waffles, 2014/02/07 01:00:59]

Done.

   std::string operation;
   if (!command->GetString(kOp, &operation))
     return NULL;
   if (operation == "copy")
     return new DeltaUpdateOpCopy();
   else if (operation == "create")
     return new DeltaUpdateOpCreate();
   else if (operation == "bsdiff")
-    return new DeltaUpdateOpPatchBsdiff();
+    return new DeltaUpdateOpPatch(component_updater::kPatchTypeBsdiff);
   else if (operation == "courgette")
-    return new DeltaUpdateOpPatchCourgette();
+    return new DeltaUpdateOpPatch(component_updater::kPatchTypeCourgette);
   return NULL;
 }
 
-DeltaUpdateOp::DeltaUpdateOp() {}
+DeltaUpdateOp::DeltaUpdateOp() : in_process_(false), ptr_factory_(this) {}
 
 DeltaUpdateOp::~DeltaUpdateOp() {}
 
-ComponentUnpacker::Error DeltaUpdateOp::Run(base::DictionaryValue* command_args,
-                                            const base::FilePath& input_dir,
-                                            const base::FilePath& unpack_dir,
-                                            ComponentPatcher* patcher,
-                                            ComponentInstaller* installer,
-                                            int* error) {
+void DeltaUpdateOp::Run(
+    base::DictionaryValue* command_args,
+    const base::FilePath& input_dir,
+    const base::FilePath& unpack_dir,
+    ComponentInstaller* installer,
+    bool in_process,
+    const base::Callback<void(ComponentUnpacker::Error, int)>& callback,
+    scoped_refptr<base::SequencedTaskRunner> task_runner) {
+  callback_ = callback;
+  in_process_ = in_process;
+  task_runner_ = task_runner;
   std::string output_rel_path;
   if (!command_args->GetString(kOutput, &output_rel_path) ||
-      !command_args->GetString(kSha256, &output_sha256_))
-    return ComponentUnpacker::kDeltaBadCommands;
+      !command_args->GetString(kSha256, &output_sha256_)) {
+    DoneRunning(ComponentUnpacker::kDeltaBadCommands, 0);
+    return;
+  }
 
   output_abs_path_ = unpack_dir.Append(
       base::FilePath::FromUTF8Unsafe(output_rel_path));
   ComponentUnpacker::Error parse_result = DoParseArguments(
       command_args, input_dir, installer);
-  if (parse_result != ComponentUnpacker::kNone)
-    return parse_result;
+  if (parse_result != ComponentUnpacker::kNone) {
+    DoneRunning(parse_result, 0);
+    return;
+  }
 
   const base::FilePath parent = output_abs_path_.DirName();
   if (!base::DirectoryExists(parent)) {
-    if (!base::CreateDirectory(parent))
-      return ComponentUnpacker::kIoError;
+    if (!base::CreateDirectory(parent)) {
+      DoneRunning(ComponentUnpacker::kIoError, 0);
+      return;
+    }
   }
 
-  ComponentUnpacker::Error run_result = DoRun(patcher, error);
-  if (run_result != ComponentUnpacker::kNone)
-    return run_result;
+  DoRun(base::Bind(&DeltaUpdateOp::DoneRunning, GetWeakPtr()));
+}
 
-  return CheckHash();
+void DeltaUpdateOp::DoneRunning(
+    ComponentUnpacker::Error error, int extended_error) {

[Sorin Jianu, 2014/02/03 20:57:57]

does this line fit on the line above?

[Sorin Jianu, 2014/02/03 20:57:57]

Coding style nit:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

The style guide is:
void DeltaUpdateOp::DoneRunning(ComponentUnpacker::Error error, 
                                int extended_error) {

[waffles, 2014/02/07 01:00:59]

Not the whole line, no.

[waffles, 2014/02/07 01:00:59]

Done.

+  if (error == ComponentUnpacker::kNone)
+    error = CheckHash();
+  task_runner_->PostTask(
+      FROM_HERE,

[Sorin Jianu, 2014/02/03 20:57:57]

params vertical align as above?

[waffles, 2014/02/07 01:00:59]

Done.

+      base::Bind(callback_, error, extended_error));
 }
 
 // Uses the hash as a checksum to confirm that the file now residing in the
 // output directory probably has the contents it should.
 ComponentUnpacker::Error DeltaUpdateOp::CheckHash() {
   std::vector<uint8> expected_hash;
   if (!base::HexStringToBytes(output_sha256_, &expected_hash) ||
       expected_hash.size() != crypto::kSHA256Length)
     return ComponentUnpacker::kDeltaVerificationFailure;
 
   base::MemoryMappedFile output_file_mmapped;
   if (!output_file_mmapped.Initialize(output_abs_path_))
     return ComponentUnpacker::kDeltaVerificationFailure;
 
   uint8 actual_hash[crypto::kSHA256Length] = {0};
   const scoped_ptr<SecureHash> hasher(SecureHash::Create(SecureHash::SHA256));
   hasher->Update(output_file_mmapped.data(), output_file_mmapped.length());
   hasher->Finish(actual_hash, sizeof(actual_hash));
   if (memcmp(actual_hash, &expected_hash[0], sizeof(actual_hash)))
     return ComponentUnpacker::kDeltaVerificationFailure;
 
   return ComponentUnpacker::kNone;
 }
 
+base::WeakPtr<DeltaUpdateOp> DeltaUpdateOp::GetWeakPtr() {
+  return ptr_factory_.GetWeakPtr();
+}
+
 DeltaUpdateOpCopy::DeltaUpdateOpCopy() {}
 
 ComponentUnpacker::Error DeltaUpdateOpCopy::DoParseArguments(
     base::DictionaryValue* command_args,
     const base::FilePath& input_dir,
     ComponentInstaller* installer) {
   std::string input_rel_path;
   if (!command_args->GetString(kInput, &input_rel_path))
     return ComponentUnpacker::kDeltaBadCommands;
 
   if (!installer->GetInstalledFile(input_rel_path, &input_abs_path_))
     return ComponentUnpacker::kDeltaMissingExistingFile;
 
   return ComponentUnpacker::kNone;
 }
 
-ComponentUnpacker::Error DeltaUpdateOpCopy::DoRun(ComponentPatcher*,
-                                                  int* error) {
-  *error = 0;
+void DeltaUpdateOpCopy::DoRun(
+    const base::Callback<void(ComponentUnpacker::Error, int)>& callback) {
   if (!base::CopyFile(input_abs_path_, output_abs_path_))
-    return ComponentUnpacker::kDeltaOperationFailure;
-
-  return ComponentUnpacker::kNone;
+    callback.Run(ComponentUnpacker::kDeltaOperationFailure, 0);
+  else
+    callback.Run(ComponentUnpacker::kNone, 0);
 }
 
 DeltaUpdateOpCreate::DeltaUpdateOpCreate() {}
 
 ComponentUnpacker::Error DeltaUpdateOpCreate::DoParseArguments(
     base::DictionaryValue* command_args,
     const base::FilePath& input_dir,
     ComponentInstaller* installer) {
   std::string patch_rel_path;
   if (!command_args->GetString(kPatch, &patch_rel_path))
     return ComponentUnpacker::kDeltaBadCommands;
 
   patch_abs_path_ = input_dir.Append(
       base::FilePath::FromUTF8Unsafe(patch_rel_path));
 
   return ComponentUnpacker::kNone;
 }
 
-ComponentUnpacker::Error DeltaUpdateOpCreate::DoRun(ComponentPatcher*,
-                                                    int* error) {
-  *error = 0;
+void DeltaUpdateOpCreate::DoRun(
+    const base::Callback<void(ComponentUnpacker::Error, int)>& callback) {
   if (!base::Move(patch_abs_path_, output_abs_path_))
-    return ComponentUnpacker::kDeltaOperationFailure;
-
-  return ComponentUnpacker::kNone;
+    callback.Run(ComponentUnpacker::kDeltaOperationFailure, 0);
+  else
+    callback.Run(ComponentUnpacker::kNone, 0);
 }
 
-DeltaUpdateOpPatchBsdiff::DeltaUpdateOpPatchBsdiff() {}
 
-ComponentUnpacker::Error DeltaUpdateOpPatchBsdiff::DoParseArguments(
+DeltaUpdateOpPatch::DeltaUpdateOpPatch(
+    component_updater::PatchType patch_type) : patch_type_(patch_type) {

[Sorin Jianu, 2014/02/03 20:57:57]

Formatting of function params and initializers.

[waffles, 2014/02/07 01:00:59]

Done.

+}
+
+DeltaUpdateOpPatch::~DeltaUpdateOpPatch() {
+}
+
+ComponentUnpacker::Error DeltaUpdateOpPatch::DoParseArguments(
     base::DictionaryValue* command_args,
     const base::FilePath& input_dir,
     ComponentInstaller* installer) {
   std::string patch_rel_path;
   std::string input_rel_path;
   if (!command_args->GetString(kPatch, &patch_rel_path) ||
       !command_args->GetString(kInput, &input_rel_path))
     return ComponentUnpacker::kDeltaBadCommands;
 
   if (!installer->GetInstalledFile(input_rel_path, &input_abs_path_))
     return ComponentUnpacker::kDeltaMissingExistingFile;
 
   patch_abs_path_ = input_dir.Append(
       base::FilePath::FromUTF8Unsafe(patch_rel_path));
 
   return ComponentUnpacker::kNone;
 }
 
-ComponentUnpacker::Error DeltaUpdateOpPatchBsdiff::DoRun(
-    ComponentPatcher* patcher,
-    int* error) {
-  *error = 0;
-  return patcher->Patch(ComponentPatcher::kPatchTypeBsdiff,
-                        input_abs_path_,
-                        patch_abs_path_,
-                        output_abs_path_,
-                        error);
+void DeltaUpdateOpPatch::DoRun(
+    const base::Callback<void(ComponentUnpacker::Error, int)>& callback) {
+  callback_ = callback;
+  if (!in_process_) {
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(&DeltaUpdateOpPatch::StartProcess, base::Unretained(this)));

[Sorin Jianu, 2014/02/03 20:57:57]

Can we have a race here due to ownership issues, not sure, just asking.

[waffles, 2014/02/07 01:00:59]

Yes - this should be resolved now that this class is refcounted.

+    return;
+  }
+  switch (patch_type_) {
+    case component_updater::kPatchTypeCourgette: {
+      const int result = courgette::ApplyEnsemblePatch(
+          input_abs_path_.value().c_str(),
+          patch_abs_path_.value().c_str(),
+          output_abs_path_.value().c_str());
+      if (result == courgette::C_OK)
+        callback_.Run(ComponentUnpacker::kNone, 0);
+      else
+        DonePatching(false, result);

[Sorin Jianu, 2014/02/03 20:57:57]

I've noticed that is most cases DonePatching is called in case of errors and the
callback is called directly in the success cases. DonePatching also supports the
success case, and that is called from the bridge. Is is worth unifying these
cases and call DonePatching as a completion function in all cases. 

Throughout the CL, being consistent when we call a callback directly or a
function would help ease the readability and the maintenance cost.

[waffles, 2014/02/07 01:00:59]

This should all be much more organized now.

+      break;
+    }
+    case component_updater::kPatchTypeBsdiff: {
+      const int result = courgette::ApplyBinaryPatch(
+          input_abs_path_, patch_abs_path_, output_abs_path_);
+      if (result == courgette::OK)
+        callback_.Run(ComponentUnpacker::kNone, 0);
+      else
+        DonePatching(false, result);
+      break;
+    }
+    default: {
+      NOTREACHED() << "Invalid patch type.";
+      callback_.Run(ComponentUnpacker::kDeltaUnsupportedCommand, 0);
+    }
+  }
 }
 
-DeltaUpdateOpPatchCourgette::DeltaUpdateOpPatchCourgette() {}
-
-ComponentUnpacker::Error DeltaUpdateOpPatchCourgette::DoParseArguments(
-    base::DictionaryValue* command_args,
-    const base::FilePath& input_dir,
-    ComponentInstaller* installer) {
-  std::string patch_rel_path;
-  std::string input_rel_path;
-  if (!command_args->GetString(kPatch, &patch_rel_path) ||
-      !command_args->GetString(kInput, &input_rel_path))
-    return ComponentUnpacker::kDeltaBadCommands;
-
-  if (!installer->GetInstalledFile(input_rel_path, &input_abs_path_))
-    return ComponentUnpacker::kDeltaMissingExistingFile;
-
-  patch_abs_path_ = input_dir.Append(
-      base::FilePath::FromUTF8Unsafe(patch_rel_path));
-
-  return ComponentUnpacker::kNone;
+void DeltaUpdateOpPatch::StartProcess() {
+  content::UtilityProcessHost* host = content::UtilityProcessHost::Create(
+      new PatcherBridge(this), base::MessageLoopProxy::current().get());
+  host->DisableSandbox();
+  switch (patch_type_) {
+    case component_updater::kPatchTypeCourgette: {
+      host->Send(new ChromeUtilityMsg_PatchFileCourgette(
+          input_abs_path_, patch_abs_path_, output_abs_path_));
+      break;
+    }
+    case component_updater::kPatchTypeBsdiff: {
+      host->Send(new ChromeUtilityMsg_PatchFileBsdiff(
+          input_abs_path_, patch_abs_path_, output_abs_path_));
+      break;
+    }
+    default: {
+      NOTREACHED() << "Invalid patch type.";
+      callback_.Run(ComponentUnpacker::kDeltaUnsupportedCommand, 0);
+    }
+  }
 }
 
-ComponentUnpacker::Error DeltaUpdateOpPatchCourgette::DoRun(
-    ComponentPatcher* patcher,
-    int* error) {
-  *error = 0;
-  return patcher->Patch(ComponentPatcher::kPatchTypeCourgette,
-                        input_abs_path_,
-                        patch_abs_path_,
-                        output_abs_path_,
-                        error);
+void DeltaUpdateOpPatch::DonePatching(bool success, int error_code) {
+  if (success) {
+    callback_.Run(ComponentUnpacker::kNone, error_code);
+  } else {
+    int offset = patch_type_ == component_updater::kPatchTypeCourgette

[Sorin Jianu, 2014/02/03 20:57:57]

we have the switch on the patch_type_ in three different functions, would it be
worth introducing polymorphism to handle this?

[waffles, 2014/02/07 01:00:59]

I've tried out a using a traits class - let me know what you think.

+       ? kCourgetteErrorOffset : kBsdiffErrorOffset;
+    callback_.Run(ComponentUnpacker::kDeltaOperationFailure,
+                  error_code + offset);
+  }
+}
+
+DeltaUpdateOpPatch::PatcherBridge::PatcherBridge(DeltaUpdateOpPatch* op)
+    : op_(op) {
+}
+
+DeltaUpdateOpPatch::PatcherBridge::~PatcherBridge() {
+}
+
+bool DeltaUpdateOpPatch::PatcherBridge::OnMessageReceived(
+    const IPC::Message& message) {
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(PatcherBridge, message)
+    IPC_MESSAGE_HANDLER(ChromeUtilityHostMsg_PatchFile_Succeeded,
+                        OnPatchSucceeded)
+    IPC_MESSAGE_HANDLER(ChromeUtilityHostMsg_PatchFile_Failed,
+                        OnPatchFailed)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+  return handled;
+}
+
+void DeltaUpdateOpPatch::PatcherBridge::OnProcessCrashed(int exit_code) {
+  if (op_)
+    op_->callback_.Run(ComponentUnpacker::kDeltaPatchProcessFailure, exit_code);
+}
+
+void DeltaUpdateOpPatch::PatcherBridge::OnPatchSucceeded() {
+  if (op_)
+    op_->DonePatching(true, 0);
+}
+
+void DeltaUpdateOpPatch::PatcherBridge::OnPatchFailed(int error_code) {
+  if (op_)
+    op_->DonePatching(false, error_code);
 }
 
 }  // namespace component_updater