Support asynchronous patching operations in the component updater.

chrome/browser/component_updater/component_unpacker.cc

Index: chrome/browser/component_updater/component_unpacker.cc
diff --git a/chrome/browser/component_updater/component_unpacker.cc b/chrome/browser/component_updater/component_unpacker.cc
index d47bce829a048ff323c66ac0f61170f67720468a..5cbfc23587e606e257034520912927ea61b8c8fa 100644
--- a/chrome/browser/component_updater/component_unpacker.cc
+++ b/chrome/browser/component_updater/component_unpacker.cc
@@ -7,7 +7,9 @@
 #include <string>
 #include <vector>
 
+#include "base/bind.h"
 #include "base/file_util.h"
+#include "base/files/file_path.h"
 #include "base/json/json_file_value_serializer.h"
 #include "base/memory/scoped_handle.h"
 #include "base/strings/string_number_conversions.h"
@@ -15,6 +17,7 @@
 #include "chrome/browser/component_updater/component_patcher.h"
 #include "chrome/browser/component_updater/component_updater_service.h"
 #include "chrome/common/extensions/extension_constants.h"
+#include "content/public/browser/browser_thread.h"
 #include "crypto/secure_hash.h"
 #include "crypto/signature_verifier.h"
 #include "extensions/common/crx_file.h"
@@ -104,32 +107,182 @@ base::DictionaryValue* ReadManifest(const base::FilePath& unpack_path) {
   return static_cast<base::DictionaryValue*>(root.release());
 }
 
-}  // namespace.
+// In charge of unpacking the component CRX package and verifying that it is
+// well formed and the cryptographic signature is correct. If there is no
+// error the component specific installer will be invoked to proceed with
+// the component installation or update.
+//
+// This class should be used only by the component updater. It is inspired
+// and overlaps with code in the extension's SandboxedUnpacker.
+// The main differences are:
+// - The public key hash is full SHA256.
+// - Does not use a sandboxed unpacker. A valid component is fully trusted.
+// - The manifest can have different attributes and resources are not
+//   transcoded.

[cpu_(ooo_6.6-7.5), 2013/10/25 23:59:26]

can we get a bit of an ascii flow chart here? to me the flow got harder to
understand with all the new helpers

[waffles, 2013/10/28 22:13:23]

Done.

+class ComponentUnpacker {
+ public:
+  // Constructs an unpacker for a specific component unpacking operation.
+  // |pk_hash| is the expected
+  // public key SHA256 hash. |path| is the current location of
+  // the CRX. When done, runs |callback| with the error and extra error code.
+  ComponentUnpacker(const std::vector<uint8>& pk_hash,
+                    const base::FilePath& path,
+                    const std::string& fingerprint,
+                    ComponentPatcher* patcher,
+                    ComponentInstaller* installer);
+
+  virtual ~ComponentUnpacker();
+
+  // This helper posts a task to call |callback| with |error| and
+  // |extended_error|. It is a method in order to take advantage of
+  // base::Bind()'s ownership features. The task is posted to the FILE thread.
+  void CallbackHelper(
+      const base::Callback<void(component_updater::Error, int)>& callback,
+      component_updater::Error error,
+      int extended_error);
+
+  // Begin the actual unpacking of the files. May invoke a patcher if the
+  // package is a differential update.
+  void Start(
+      const base::Callback<void(component_updater::Error, int)>& callback);
+
+ private:
+  // The first step of unpacking is to unzip.
+  void Unzip();
+
+  // The second step is to optionally patch files - this is a no-op for
+  // full (non-differential) updates. This step is asynchronous.
+  void BeginPatching();
+
+  // When patching is complete, DonePatching is called before moving on to step
+  // three.
+  void DonePatching(component_updater::Error error, int extended_error);
+
+  // The third step is to install the component.
+  void Install();
+
+  // The final step is to do clean-up for things that can't be tidied as we go.
+  // If there is an error at any step, the remaining steps are skipped and
+  // and Finish is called.
+  // Finish is responsible for calling the callback provided in Start().
+  void Finish();
+
+  // Returns a weak pointer to this object.
+  base::WeakPtr<ComponentUnpacker> GetWeakPtr();
+
+  void UnzipHelper();
+
+  void BeginPatchingHelper();
+
+  void InstallHelper();
+
+  std::vector<uint8> pk_hash_;
+  base::FilePath path_;
+  base::FilePath unpack_path_;
+  base::FilePath unpack_diff_path_;
+  bool delta_;
+  std::string fingerprint_;
+  ComponentPatcher* patcher_;
+  ComponentInstaller* installer_;
+  base::Callback<void(component_updater::Error, int)> callback_;
+  component_updater::Error error_;
+  int extended_error_;
+  base::WeakPtrFactory<ComponentUnpacker> ptr_factory_;
+};
+
+ComponentUnpacker::ComponentUnpacker(
+    const std::vector<uint8>& pk_hash,
+    const base::FilePath& path,
+    const std::string& fingerprint,
+    ComponentPatcher* patcher,
+    ComponentInstaller* installer)
+    : pk_hash_(pk_hash),
+      path_(path),
+      delta_(false),
+      fingerprint_(fingerprint),
+      patcher_(patcher),
+      installer_(installer),
+      error_(component_updater::kNone),
+      extended_error_(0),
+      ptr_factory_(this) {
+}
+
+void ComponentUnpacker::CallbackHelper(
+    const base::Callback<void(component_updater::Error, int)>& callback,
+    component_updater::Error error,
+    int extended_error) {
+  content::BrowserThread::PostTask(
+      content::BrowserThread::FILE,
+      FROM_HERE,
+      base::Bind(callback, error, extended_error));
+}
+
+void ComponentUnpacker::Start(
+    const base::Callback<void(component_updater::Error, int)>& callback) {
+  callback_ = callback;
+  Unzip();
+}
+
+void ComponentUnpacker::Unzip() {
+  UnzipHelper();
+  if (error_ != component_updater::kNone)
+    Finish();
+  else
+    BeginPatching();
+}
+
+void ComponentUnpacker::BeginPatching() {
+  BeginPatchingHelper();
+  if (error_ != component_updater::kNone)
+    Finish();
+  // Else, if the helper succeeds, DonePatching will be called asynchronously.
+}
+
+void ComponentUnpacker::DonePatching(component_updater::Error error,
+                                     int extended_error) {
+  error_ = error;
+  extended_error_ = extended_error;
+  if (delta_) {
+    base::DeleteFile(unpack_path_, true);
+    unpack_path_.clear();
+  }
+  if (error_ != component_updater::kNone)
+    Finish();
+  else
+    Install();
+}
+
+void ComponentUnpacker::Install() {
+  InstallHelper();
+  Finish();
+}
+
+void ComponentUnpacker::Finish() {
+  if (unpack_diff_path_.empty())
+    base::DeleteFile(unpack_diff_path_, true);

[cpu_(ooo_6.6-7.5), 2013/10/25 23:59:26]

don't get the logic of lines 261-262, deleting with empty pack?

[waffles, 2013/10/28 22:13:23]

Bug; thanks.

+  if (unpack_path_.empty())
+    base::DeleteFile(unpack_path_, true);

[cpu_(ooo_6.6-7.5), 2013/10/25 23:59:26]

same

[waffles, 2013/10/28 22:13:23]

Done.

+  callback_.Run(error_, extended_error_);
+}
 
-ComponentUnpacker::ComponentUnpacker(const std::vector<uint8>& pk_hash,
-                                     const base::FilePath& path,
-                                     const std::string& fingerprint,
-                                     ComponentPatcher* patcher,
-                                     ComponentInstaller* installer)
-    : error_(kNone),
-      extended_error_(0) {
-  if (pk_hash.empty() || path.empty()) {
-    error_ = kInvalidParams;
+void ComponentUnpacker::UnzipHelper() {
+  if (pk_hash_.empty() || path_.empty()) {
+    error_ = component_updater::kInvalidParams;
     return;
   }
   // First, validate the CRX header and signature. As of today
   // this is SHA1 with RSA 1024.
-  ScopedStdioHandle file(file_util::OpenFile(path, "rb"));
+  ScopedStdioHandle file(file_util::OpenFile(path_, "rb"));
   if (!file.get()) {
-    error_ = kInvalidFile;
+    error_ = component_updater::kInvalidFile;
     return;
   }
   CRXValidator validator(file.get());
   if (!validator.valid()) {
-    error_ = kInvalidFile;
+    error_ = component_updater::kInvalidFile;
     return;
   }
-  file.Close();
+  delta_ = validator.delta();
 
   // File is valid and the digital signature matches. Now make sure
   // the public key hash matches the expected hash. If they do we fully
@@ -139,69 +292,100 @@ ComponentUnpacker::ComponentUnpacker(const std::vector<uint8>& pk_hash,
   sha256->Update(&(validator.public_key()[0]), validator.public_key().size());
   sha256->Finish(hash, arraysize(hash));
 
-  if (!std::equal(pk_hash.begin(), pk_hash.end(), hash)) {
-    error_ = kInvalidId;
+  if (!std::equal(pk_hash_.begin(), pk_hash_.end(), hash)) {
+    error_ = component_updater::kInvalidId;
     return;
   }
   if (!file_util::CreateNewTempDirectory(FILE_PATH_LITERAL(""),
                                          &unpack_path_)) {
-    error_ = kUnzipPathError;
+    error_ = component_updater::kUnzipPathError;
     return;
   }
-  if (validator.delta()) {  // Package is a diff package.
-    // We want a different temp directory for the delta files; we'll put the
-    // patch output into unpack_path_.
-    base::FilePath unpack_diff_path;
+  file.Close(); // TODO(waffles): Is it safe to close the file? It could change.
+  if (!zip::Unzip(path_, unpack_path_))
+    error_ = component_updater::kUnzipFailed;
+}
+
+
+void ComponentUnpacker::BeginPatchingHelper() {
+  if (delta_) {  // Package is a diff package.
+    // We want a different temp directory to put the patch output files into.
     if (!file_util::CreateNewTempDirectory(FILE_PATH_LITERAL(""),
-                                           &unpack_diff_path)) {
-      error_ = kUnzipPathError;
-      return;
-    }
-    if (!zip::Unzip(path, unpack_diff_path)) {
-      error_ = kUnzipFailed;
-      return;
-    }
-    ComponentUnpacker::Error result = DifferentialUpdatePatch(unpack_diff_path,
-                                                              unpack_path_,
-                                                              patcher,
-                                                              installer,
-                                                              &extended_error_);
-    base::DeleteFile(unpack_diff_path, true);
-    unpack_diff_path.clear();
-    error_ = result;
-    if (error_ != kNone) {
+                                           &unpack_diff_path_)) {
+      error_ = component_updater::kUnzipPathError;
       return;
     }

[cpu_(ooo_6.6-7.5), 2013/10/25 23:59:26]

we are in the file thread, aren't we?

[waffles, 2013/10/28 22:13:23]

This is something I'm struggling with in this code.
I want to guarantee that Finish() is called exactly once. This is my fear about
calling DifferentialUpdatePatch directly:

DifferentialUpdatePatch gets an error and calls the provided callback directly,
supplying the error. DonePatching detects the error, calls Finish. Eventually,
Finish returns, DonePatching returns, DifferentialUpdatePatch returns, and then
BeginPatching returns; since we had an error, we detect it, and call Finish().

I don't like that BeginPatching returns /after/ DonePatching; so I was using a
posted task (to the same thread) to guarantee that it doesn't (see
SequentialTaskRunner).

But, since this won't really work if we move to the blocking thread pool, maybe
I should just give up and make Finish idempotent? That seems hard to guarantee
without using a mutex. Another alternative is to track the failure location in
stack frames; this is what I have done with the next patch set.

+    content::BrowserThread::PostTask(
+        content::BrowserThread::FILE,
+        FROM_HERE,
+        base::Bind(&DifferentialUpdatePatch,
+                   unpack_path_,
+                   unpack_diff_path_,
+                   patcher_,
+                   installer_,
+                   base::Bind(&ComponentUnpacker::DonePatching,
+                              GetWeakPtr())));
   } else {
-    // Package is a normal update/install; unzip it into unpack_path_ directly.
-    if (!zip::Unzip(path, unpack_path_)) {
-      error_ = kUnzipFailed;
-      return;
-    }
-  }
-  scoped_ptr<base::DictionaryValue> manifest(ReadManifest(unpack_path_));
-  if (!manifest.get()) {
-    error_ = kBadManifest;
-    return;
+    // Post a task instead of doing a direct call; otherwise we can call
+    // Finish() twice.
+    content::BrowserThread::PostTask(
+        content::BrowserThread::FILE,
+        FROM_HERE,
+        base::Bind(&ComponentUnpacker::DonePatching,
+                   GetWeakPtr(),
+                   component_updater::kNone,
+                   0));
   }
+}
+
+void ComponentUnpacker::InstallHelper() {
+  base::FilePath inst_path;
+  if (delta_)
+    inst_path = unpack_diff_path_;
+  else
+    inst_path = unpack_path_;
   // Write the fingerprint to disk.
-  if (static_cast<int>(fingerprint.size()) !=
+  if (static_cast<int>(fingerprint_.size()) !=
       file_util::WriteFile(
-          unpack_path_.Append(FILE_PATH_LITERAL("manifest.fingerprint")),
-          fingerprint.c_str(),
-          fingerprint.size())) {
-    error_ = kFingerprintWriteFailed;
+          inst_path.Append(FILE_PATH_LITERAL("manifest.fingerprint")),
+          fingerprint_.c_str(),
+          fingerprint_.size())) {
+    error_ = component_updater::kFingerprintWriteFailed;
     return;
   }
-  if (!installer->Install(*manifest, unpack_path_)) {
-    error_ = kInstallerError;
-    return;
+  if (error_ == component_updater::kNone) {
+    scoped_ptr<base::DictionaryValue> manifest(ReadManifest(inst_path));
+    if (!manifest.get()) {
+      error_ = component_updater::kBadManifest;
+      return;
+    }
+    if (!installer_->Install(*manifest, inst_path)) {
+      error_ = component_updater::kInstallerError;
+      return;
+    }
   }
-  // Installation successful. The directory is not our concern now.
-  unpack_path_.clear();
+}
+
+base::WeakPtr<ComponentUnpacker> ComponentUnpacker::GetWeakPtr() {
+  return ptr_factory_.GetWeakPtr();
 }
 
 ComponentUnpacker::~ComponentUnpacker() {
-  if (!unpack_path_.empty())
-    base::DeleteFile(unpack_path_, true);
+}
+
+}  // namespace.
+
+void component_updater::Unpack(
+    const std::vector<uint8>& pk_hash,
+    const base::FilePath& path,
+    const std::string& fingerprint,
+    ComponentPatcher* patcher,
+    ComponentInstaller* installer,
+    const base::Callback<void(component_updater::Error, int)>& callback) {
+  // A callback wrapper will own the unpacker object.
+  ComponentUnpacker* unpacker = new ComponentUnpacker(
+      pk_hash, path, fingerprint, patcher, installer);
+  unpacker->Start(base::Bind(&ComponentUnpacker::CallbackHelper,
+                             base::Owned(unpacker),
+                             callback));
 }