Use UtilityProcessHost to patch files.

chrome/browser/component_updater/component_patcher_operation.cc

Index: chrome/browser/component_updater/component_patcher_operation.cc
diff --git a/chrome/browser/component_updater/component_patcher_operation.cc b/chrome/browser/component_updater/component_patcher_operation.cc
index 6b86d9fc8a6dc241caa3db8eced617f6e5887f3b..616d01496c5e48fbee338e4c67d34f7866135d4d 100644
--- a/chrome/browser/component_updater/component_patcher_operation.cc
+++ b/chrome/browser/component_updater/component_patcher_operation.cc
@@ -7,6 +7,7 @@
 #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"
@@ -15,11 +16,17 @@
 #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;
@@ -34,6 +41,11 @@ 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.

@@ -45,45 +57,61 @@ DeltaUpdateOp* CreateDeltaUpdateOp(base::DictionaryValue* command) {
   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
@@ -108,6 +136,10 @@ ComponentUnpacker::Error DeltaUpdateOp::CheckHash() {
   return ComponentUnpacker::kNone;
 }
 
+base::WeakPtr<DeltaUpdateOp> DeltaUpdateOp::GetWeakPtr() {
+  return ptr_factory_.GetWeakPtr();
+}
+
 DeltaUpdateOpCopy::DeltaUpdateOpCopy() {}
 
 ComponentUnpacker::Error DeltaUpdateOpCopy::DoParseArguments(
@@ -124,13 +156,12 @@ ComponentUnpacker::Error DeltaUpdateOpCopy::DoParseArguments(
   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() {}
@@ -149,18 +180,23 @@ ComponentUnpacker::Error DeltaUpdateOpCreate::DoParseArguments(
   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;
+    callback.Run(ComponentUnpacker::kDeltaOperationFailure, 0);
+  else
+    callback.Run(ComponentUnpacker::kNone, 0);
+}
 
-  return ComponentUnpacker::kNone;
+
+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.

 }
 
-DeltaUpdateOpPatchBsdiff::DeltaUpdateOpPatchBsdiff() {}
+DeltaUpdateOpPatch::~DeltaUpdateOpPatch() {
+}
 
-ComponentUnpacker::Error DeltaUpdateOpPatchBsdiff::DoParseArguments(
+ComponentUnpacker::Error DeltaUpdateOpPatch::DoParseArguments(
     base::DictionaryValue* command_args,
     const base::FilePath& input_dir,
     ComponentInstaller* installer) {
@@ -179,47 +215,110 @@ ComponentUnpacker::Error DeltaUpdateOpPatchBsdiff::DoParseArguments(
   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() {}
+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::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;
+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);
+  }
+}
 
-  if (!installer->GetInstalledFile(input_rel_path, &input_abs_path_))
-    return ComponentUnpacker::kDeltaMissingExistingFile;
+DeltaUpdateOpPatch::PatcherBridge::PatcherBridge(DeltaUpdateOpPatch* op)
+    : op_(op) {
+}
 
-  patch_abs_path_ = input_dir.Append(
-      base::FilePath::FromUTF8Unsafe(patch_rel_path));
+DeltaUpdateOpPatch::PatcherBridge::~PatcherBridge() {
+}
 
-  return ComponentUnpacker::kNone;
+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);
 }
 
-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::PatcherBridge::OnPatchFailed(int error_code) {
+  if (op_)
+    op_->DonePatching(false, error_code);
 }
 
 }  // namespace component_updater