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 5541a9b5d269281f871dbd99743aa75d7fe5de60..5c127df5a7cfa834ff482fc6dfda141d57a289e9 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/logging.h"
 #include "base/memory/scoped_handle.h"
@@ -17,6 +19,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"
@@ -87,7 +90,28 @@ class CRXValidator {
   std::vector<uint8> public_key_;
 };
 
-}  // namespace.
+}  // namespace
+
+namespace component_updater {
+
+ComponentUnpacker::ComponentUnpacker(
+    const std::vector<uint8>& pk_hash,
+    const base::FilePath& path,
+    const std::string& fingerprint,
+    ComponentPatcher* patcher,
+    ComponentInstaller* installer,
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
+    : pk_hash_(pk_hash),
+      path_(path),
+      delta_(false),
+      fingerprint_(fingerprint),
+      patcher_(patcher),
+      installer_(installer),
+      error_(component_updater::kNone),
+      extended_error_(0),
+      ptr_factory_(this),
+      task_runner_(task_runner) {
+}
 
 // TODO(cpu): add a specific attribute check to a component json that the
 // extension unpacker will reject, so that a component cannot be installed
@@ -109,30 +133,36 @@ scoped_ptr<base::DictionaryValue> ReadManifest(
       static_cast<base::DictionaryValue*>(root.release())).Pass();
 }
 
-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::Unpack(
+    const base::Callback<void(component_updater::Error, int)>& callback) {
+  callback_ = callback;
+  if (!Unzip()) {
+    Finish();
     return;
   }
+  if (!BeginPatching())
+    Finish();
+  // Else, DonePatching will be called asynchronously.

[Sorin Jianu, 2013/11/21 19:48:37]

Consider rewriting so the comment does not look like code. It is sufficient to
make this a function comment imho.

[waffles, 2013/11/26 00:46:55]

Done.

+}
+
+bool ComponentUnpacker::Unzip() {
+  if (pk_hash_.empty() || path_.empty()) {
+    error_ = component_updater::kInvalidParams;

[Sorin Jianu, 2013/11/21 19:48:37]

do we want to handle pk_hash_.empty() here or let the code at 176 handle it?

[waffles, 2013/11/26 00:46:55]

I preferred this, since pk_hash_ is learned from our parameters (e.g. Chrome
code), and therefore is "InvalidParams", while at 176 it could be that the hash
in the CRX file is wrong.

This is splitting error codes very finely, I know.

+    return false;
+  }
   // 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;
-    return;
+    error_ = component_updater::kInvalidFile;
+    return false;
   }
   CRXValidator validator(file.get());
   if (!validator.valid()) {
-    error_ = kInvalidFile;
-    return;
+    error_ = component_updater::kInvalidFile;
+    return false;
   }
-  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
@@ -142,69 +172,107 @@ 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;
-    return;
+  if (!std::equal(pk_hash_.begin(), pk_hash_.end(), hash)) {
+    error_ = component_updater::kInvalidId;
+    return false;
   }
+  base::FilePath* destination;

[Sorin Jianu, 2013/11/21 19:48:37]

not initialized.

[waffles, 2013/11/26 00:46:55]

Done.

+  if (delta_)

[Sorin Jianu, 2013/11/21 19:48:37]

can use ?:

[waffles, 2013/11/26 00:46:55]

Done.

+    destination = &unpack_diff_path_;
+  else
+    destination = &unpack_path_;

[Sorin Jianu, 2013/11/21 19:48:37]

Better use a reference and pass the address of that reference to the function,
to avoid a naked pointer in the code.

[waffles, 2013/11/26 00:46:55]

Done.

   if (!file_util::CreateNewTempDirectory(FILE_PATH_LITERAL(""),
-                                         &unpack_path_)) {
-    error_ = kUnzipPathError;
-    return;
+                                         destination)) {
+    error_ = component_updater::kUnzipPathError;
+    return false;
   }
-  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.

[Sorin Jianu, 2013/11/21 19:48:37]

TODO is not clear

[waffles, 2013/11/26 00:46:55]

Removed it. It's not clear that there can be a solution here. TODO reasoning:
Unzip() can fail on some platforms if the file is open, so we must close it. But
if we close it, it may be replaced before we Unzip it.

+  if (!zip::Unzip(path_, *destination))
+    error_ = component_updater::kUnzipFailed;
+  return true;
+}
+
+
+bool ComponentUnpacker::BeginPatching() {
+  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) {
-      return;
+                                           &unpack_path_)) {
+      error_ = component_updater::kUnzipPathError;
+      return false;
     }
+    task_runner_->PostTask(
+        FROM_HERE, base::Bind(&DifferentialUpdatePatch,
+                              unpack_diff_path_,
+                              unpack_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;
-    }
+    task_runner_->PostTask(
+        FROM_HERE, base::Bind(&ComponentUnpacker::DonePatching,
+                              GetWeakPtr(),
+                              component_updater::kNone,
+                              0));
   }
-  scoped_ptr<base::DictionaryValue> manifest(ReadManifest(unpack_path_));
-  if (!manifest.get()) {
-    error_ = kBadManifest;
+  return true;
+}
+
+void ComponentUnpacker::DonePatching(component_updater::Error error,
+                                     int extended_error) {
+  error_ = error;
+  extended_error_ = extended_error;
+  if (error_ != component_updater::kNone) {
+    Finish();
     return;
   }
+  // Optimization: clean up patch files early, in case we're too low on disk to
+  // install otherwise.
+  if (!unpack_diff_path_.empty()) {
+    base::DeleteFile(unpack_diff_path_, true);
+    unpack_diff_path_.clear();
+  }
+  Install();
+  Finish();
+}
+
+void ComponentUnpacker::Install() {
   // 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;
+          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(unpack_path_));
+    if (!manifest.get()) {
+      error_ = component_updater::kBadManifest;
+      return;
+    }
+    if (!installer_->Install(*manifest, unpack_path_)) {
+      error_ = component_updater::kInstallerError;
+      return;
+    }
   }
-  // Installation successful. The directory is not our concern now.
-  unpack_path_.clear();
 }
 
-ComponentUnpacker::~ComponentUnpacker() {
+void ComponentUnpacker::Finish() {
+  if (!unpack_diff_path_.empty())
+    base::DeleteFile(unpack_diff_path_, true);
   if (!unpack_path_.empty())
     base::DeleteFile(unpack_path_, true);
+  callback_.Run(error_, extended_error_);
+}
+
+base::WeakPtr<ComponentUnpacker> ComponentUnpacker::GetWeakPtr() {
+  return ptr_factory_.GetWeakPtr();

[Sorin Jianu, 2013/11/21 19:48:37]

why do we call this a ptr_factory_ ?

[waffles, 2013/11/26 00:46:55]

Because it is a base::WeakPtrFactory<ComponentUnpacker>.

 }
+
+ComponentUnpacker::~ComponentUnpacker() {
+}
+
+}  // namespace component_updater