Support asynchronous patching operations in the component updater.

chrome/browser/component_updater/component_unpacker.cc

 // 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.
 
 #include "chrome/browser/component_updater/component_unpacker.h"
 
 #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"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
 #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"
 #include "third_party/zlib/google/zip.h"
 
 using crypto::SecureHash;
 
 namespace {
 
 // This class makes sure that the CRX digital signature is valid
@@ skipping 50 matching lines @@
   bool delta() const { return delta_; }
 
   const std::vector<uint8>& public_key() const { return public_key_; }
 
  private:
   bool valid_;
   bool delta_;
   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
 // as an extension.
 scoped_ptr<base::DictionaryValue> ReadManifest(
     const base::FilePath& unpack_path) {
   base::FilePath manifest =
       unpack_path.Append(FILE_PATH_LITERAL("manifest.json"));
   if (!base::PathExists(manifest))
     return scoped_ptr<base::DictionaryValue>();
   JSONFileValueSerializer serializer(manifest);
   std::string error;
   scoped_ptr<base::Value> root(serializer.Deserialize(NULL, &error));
   if (!root.get())
     return scoped_ptr<base::DictionaryValue>();
   if (!root->IsType(base::Value::TYPE_DICTIONARY))
     return scoped_ptr<base::DictionaryValue>();
   return scoped_ptr<base::DictionaryValue>(
       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
   // trust this CRX.
   uint8 hash[32];

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

not initialized.

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

Done.

   scoped_ptr<SecureHash> sha256(SecureHash::Create(SecureHash::SHA256));
   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 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(""),
+                                         destination)) {
+    error_ = component_updater::kUnzipPathError;
+    return false;
+  }
+  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_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 {
+    task_runner_->PostTask(
+        FROM_HERE, base::Bind(&ComponentUnpacker::DonePatching,
+                              GetWeakPtr(),
+                              component_updater::kNone,
+                              0));
+  }
+  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;
   }
-  if (!file_util::CreateNewTempDirectory(FILE_PATH_LITERAL(""),
-                                         &unpack_path_)) {
-    error_ = kUnzipPathError;
+  // 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()) !=
+      file_util::WriteFile(
+          unpack_path_.Append(FILE_PATH_LITERAL("manifest.fingerprint")),
+          fingerprint_.c_str(),
+          fingerprint_.size())) {
+    error_ = component_updater::kFingerprintWriteFailed;
     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;
-    if (!file_util::CreateNewTempDirectory(FILE_PATH_LITERAL(""),
-                                           &unpack_diff_path)) {
-      error_ = kUnzipPathError;
+  if (error_ == component_updater::kNone) {
+    scoped_ptr<base::DictionaryValue> manifest(ReadManifest(unpack_path_));
+    if (!manifest.get()) {
+      error_ = component_updater::kBadManifest;
       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;
-    }
-  } else {
-    // Package is a normal update/install; unzip it into unpack_path_ directly.
-    if (!zip::Unzip(path, unpack_path_)) {
-      error_ = kUnzipFailed;
+    if (!installer_->Install(*manifest, unpack_path_)) {
+      error_ = component_updater::kInstallerError;
       return;
     }
   }
-  scoped_ptr<base::DictionaryValue> manifest(ReadManifest(unpack_path_));
-  if (!manifest.get()) {
-    error_ = kBadManifest;
-    return;
-  }
-  // Write the fingerprint to disk.
-  if (static_cast<int>(fingerprint.size()) !=
-      file_util::WriteFile(
-          unpack_path_.Append(FILE_PATH_LITERAL("manifest.fingerprint")),
-          fingerprint.c_str(),
-          fingerprint.size())) {
-    error_ = kFingerprintWriteFailed;
-    return;
-  }
-  if (!installer->Install(*manifest, unpack_path_)) {
-    error_ = kInstallerError;
-    return;
-  }
-  // Installation successful. The directory is not our concern now.
-  unpack_path_.clear();
+}
+
+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() {
-  if (!unpack_path_.empty())
-    base::DeleteFile(unpack_path_, true);
 }
+
+}  // namespace component_updater