Pull unzipping out of the Unpacker class

Before this patch, the Unpacker did unzipping of a .crx file followed by
a bunch of work to parse/sanitize the extension contents. The CL pulls
out the unzipping and leaves Unpacker just doing the parsing/sanitizing.

This is incremental work towards the goal of supporting support
differential extension updates. The common updater code in
components/updater_client that we'll eventually be using will not hand
us a .crx file for the new version of the extenion, but rather an
already unzipped directory. The files in that directory can have 3
sources: (1) copied from the previous install, (2) entire new files that
were downloaded, or (3) files that were copied and then had a patch
applied.

The next step in a future CL will be similarly splitting the currently commingled crx file and unpacked directory sanitizing in the SandboxedUnpacker class.

BUG=490418

Review URL: https://codereview.chromium.org/1168533005

Cr-Commit-Position: refs/heads/master@{#333306}

4 files changed, 69 insertions, 86 deletions

diff --git a/extensions/utility/unpacker.cc b/extensions/utility/unpacker.cc
index a65aabb..2cd8f01 100644
--- a/extensions/utility/unpacker.cc
+++ b/extensions/utility/unpacker.cc
@@ -31,7 +31,6 @@
 #include "ipc/ipc_message_utils.h"
 #include "net/base/file_stream.h"
 #include "third_party/skia/include/core/SkBitmap.h"
-#include "third_party/zlib/google/zip.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/geometry/size.h"
 
@@ -97,11 +96,13 @@ struct Unpacker::InternalData {
   DecodedImages decoded_images;
 };
 
-Unpacker::Unpacker(const base::FilePath& extension_path,
+Unpacker::Unpacker(const base::FilePath& working_dir,
+                   const base::FilePath& extension_dir,
                    const std::string& extension_id,
                    Manifest::Location location,
                    int creation_flags)
-    : extension_path_(extension_path),
+    : working_dir_(working_dir),
+      extension_dir_(extension_dir),
       extension_id_(extension_id),
       location_(location),
       creation_flags_(creation_flags) {
@@ -112,7 +113,7 @@ Unpacker::~Unpacker() {
 }
 
 base::DictionaryValue* Unpacker::ReadManifest() {
-  base::FilePath manifest_path = temp_install_dir_.Append(kManifestFilename);
+  base::FilePath manifest_path = extension_dir_.Append(kManifestFilename);
   if (!base::PathExists(manifest_path)) {
     SetError(errors::kInvalidManifest);
     return NULL;
@@ -135,7 +136,7 @@ base::DictionaryValue* Unpacker::ReadManifest() {
 }
 
 bool Unpacker::ReadAllMessageCatalogs(const std::string& default_locale) {
-  base::FilePath locales_path = temp_install_dir_.Append(kLocaleFolder);
+  base::FilePath locales_path = extension_dir_.Append(kLocaleFolder);
 
   // Not all folders under _locales have to be valid locales.
   base::FileEnumerator locales(locales_path, false,
@@ -159,24 +160,6 @@ bool Unpacker::ReadAllMessageCatalogs(const std::string& default_locale) {
 }
 
 bool Unpacker::Run() {
-  DVLOG(1) << "Installing extension " << extension_path_.value();
-
-  // <profile>/Extensions/CRX_INSTALL
-  temp_install_dir_ = extension_path_.DirName().AppendASCII(kTempExtensionName);
-
-  if (!base::CreateDirectory(temp_install_dir_)) {
-    SetUTF16Error(l10n_util::GetStringFUTF16(
-        IDS_EXTENSION_PACKAGE_DIRECTORY_ERROR,
-        base::i18n::GetDisplayStringInLTRDirectionality(
-            temp_install_dir_.LossyDisplayName())));
-    return false;
-  }
-
-  if (!zip::Unzip(extension_path_, temp_install_dir_)) {
-    SetUTF16Error(l10n_util::GetStringUTF16(IDS_EXTENSION_PACKAGE_UNZIP_ERROR));
-    return false;
-  }
-
   // Parse the manifest.
   parsed_manifest_.reset(ReadManifest());
   if (!parsed_manifest_.get())
@@ -184,7 +167,7 @@ bool Unpacker::Run() {
 
   std::string error;
   scoped_refptr<Extension> extension(
-      Extension::Create(temp_install_dir_, location_, *parsed_manifest_,
+      Extension::Create(extension_dir_, location_, *parsed_manifest_,
                         creation_flags_, extension_id_, &error));
   if (!extension.get()) {
     SetError(error);
@@ -214,15 +197,14 @@ bool Unpacker::Run() {
       return false;  // Error was already reported.
   }
 
-  return true;
+  return DumpImagesToFile() && DumpMessageCatalogsToFile();
 }
 
 bool Unpacker::DumpImagesToFile() {
   IPC::Message pickle;  // We use a Message so we can use WriteParam.
   IPC::WriteParam(&pickle, internal_data_->decoded_images);
 
-  base::FilePath path =
-      extension_path_.DirName().AppendASCII(kDecodedImagesFilename);
+  base::FilePath path = working_dir_.AppendASCII(kDecodedImagesFilename);
   if (!WritePickle(pickle, path)) {
     SetError("Could not write image data to disk.");
     return false;
@@ -236,7 +218,7 @@ bool Unpacker::DumpMessageCatalogsToFile() {
   IPC::WriteParam(&pickle, *parsed_catalogs_.get());
 
   base::FilePath path =
-      extension_path_.DirName().AppendASCII(kDecodedMessageCatalogsFilename);
+      working_dir_.AppendASCII(kDecodedMessageCatalogsFilename);
   if (!WritePickle(pickle, path)) {
     SetError("Could not write message catalogs to disk.");
     return false;
@@ -255,7 +237,7 @@ bool Unpacker::AddDecodedImage(const base::FilePath& path) {
     return false;
   }
 
-  SkBitmap image_bitmap = DecodeImage(temp_install_dir_.Append(path));
+  SkBitmap image_bitmap = DecodeImage(extension_dir_.Append(path));
   if (image_bitmap.isNull()) {
     SetUTF16Error(l10n_util::GetStringFUTF16(
         IDS_EXTENSION_PACKAGE_IMAGE_ERROR,
@@ -288,7 +270,7 @@ bool Unpacker::ReadMessageCatalog(const base::FilePath& message_path) {
 
   base::FilePath relative_path;
   // message_path was created from temp_install_dir. This should never fail.
-  if (!temp_install_dir_.AppendRelativePath(message_path, &relative_path)) {
+  if (!extension_dir_.AppendRelativePath(message_path, &relative_path)) {
     NOTREACHED();
     return false;
   }
diff --git a/extensions/utility/unpacker.h b/extensions/utility/unpacker.h
index 240172e..7e5a7e4 100644
--- a/extensions/utility/unpacker.h
+++ b/extensions/utility/unpacker.h
@@ -21,21 +21,29 @@ class DictionaryValue;
 namespace extensions {
 
 // This class unpacks an extension.  It is designed to be used in a sandboxed
-// child process.  We unpack and parse various bits of the extension, then
-// report back to the browser process, who then transcodes the pre-parsed bits
-// and writes them back out to disk for later use.
+// child process.  We parse various bits of the extension, then report back to
+// the browser process, who then transcodes the pre-parsed bits and writes them
+// back out to disk for later use.
 class Unpacker {
  public:
-  Unpacker(const base::FilePath& extension_path,
+  Unpacker(const base::FilePath& working_dir,
+           const base::FilePath& extension_dir,
            const std::string& extension_id,
            Manifest::Location location,
            int creation_flags);
   ~Unpacker();
 
-  // Install the extension file at |extension_path|.  Returns true on success.
-  // Otherwise, error_message will contain a string explaining what went wrong.
+  // Runs the processing steps for the extension. On success, this returns true
+  // and the decoded images will be in a file at
+  // |working_dir|/kDecodedImagesFilename and the decoded messages will be in a
+  // file at |working_dir|/kDecodedMessageCatalogsFilename.
   bool Run();
 
+  const base::string16& error_message() { return error_message_; }
+  base::DictionaryValue* parsed_manifest() { return parsed_manifest_.get(); }
+  base::DictionaryValue* parsed_catalogs() { return parsed_catalogs_.get(); }
+
+ private:
   // Write the decoded images to kDecodedImagesFilename.  We do this instead
   // of sending them over IPC, since they are so large.  Returns true on
   // success.
@@ -46,11 +54,6 @@ class Unpacker {
   // success.
   bool DumpMessageCatalogsToFile();
 
-  const base::string16& error_message() { return error_message_; }
-  base::DictionaryValue* parsed_manifest() { return parsed_manifest_.get(); }
-  base::DictionaryValue* parsed_catalogs() { return parsed_catalogs_.get(); }
-
- private:
   // Parse the manifest.json file inside the extension (not in the header).
   // Caller takes ownership of return value.
   base::DictionaryValue* ReadManifest();
@@ -70,8 +73,11 @@ class Unpacker {
   void SetError(const std::string& error);
   void SetUTF16Error(const base::string16& error);
 
-  // The extension to unpack.
-  base::FilePath extension_path_;
+  // The directory to do work in.
+  base::FilePath working_dir_;
+
+  // The directory where the extension source lives.
+  base::FilePath extension_dir_;
 
   // The extension ID if known.
   std::string extension_id_;
@@ -82,9 +88,6 @@ class Unpacker {
   // The creation flags to use with the created extension.
   int creation_flags_;
 
-  // The place we unpacked the extension to.
-  base::FilePath temp_install_dir_;
-
   // The parsed version of the manifest JSON contained in the extension.
   scoped_ptr<base::DictionaryValue> parsed_manifest_;
 
diff --git a/extensions/utility/unpacker_unittest.cc b/extensions/utility/unpacker_unittest.cc
index 917f66e..107a0cc 100644
--- a/extensions/utility/unpacker_unittest.cc
+++ b/extensions/utility/unpacker_unittest.cc
@@ -16,6 +16,7 @@
 #include "extensions/utility/unpacker.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
+#include "third_party/zlib/google/zip.h"
 
 using base::ASCIIToUTF16;
 
@@ -32,22 +33,22 @@ class UnpackerTest : public testing::Test {
   }
 
   void SetupUnpacker(const std::string& crx_name) {
-    base::FilePath original_path;
-    ASSERT_TRUE(PathService::Get(DIR_TEST_DATA, &original_path));
-    original_path = original_path.AppendASCII("unpacker").AppendASCII(crx_name);
-    ASSERT_TRUE(base::PathExists(original_path)) << original_path.value();
+    base::FilePath crx_path;
+    ASSERT_TRUE(PathService::Get(DIR_TEST_DATA, &crx_path));
+    crx_path = crx_path.AppendASCII("unpacker").AppendASCII(crx_name);
+    ASSERT_TRUE(base::PathExists(crx_path)) << crx_path.value();
 
     // Try bots won't let us write into DIR_TEST_DATA, so we have to create
     // a temp folder to play in.
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
 
-    base::FilePath crx_path = temp_dir_.path().AppendASCII(crx_name);
-    ASSERT_TRUE(base::CopyFile(original_path, crx_path))
-        << "Original path " << original_path.value() << ", Crx path "
-        << crx_path.value();
+    base::FilePath unzipped_dir = temp_dir_.path().AppendASCII("unzipped");
+    ASSERT_TRUE(zip::Unzip(crx_path, unzipped_dir))
+        << "Failed to unzip " << crx_path.value() << " to "
+        << unzipped_dir.value();
 
-    unpacker_.reset(new Unpacker(crx_path, std::string(), Manifest::INTERNAL,
-                                 Extension::NO_FLAGS));
+    unpacker_.reset(new Unpacker(temp_dir_.path(), unzipped_dir, std::string(),
+                                 Manifest::INTERNAL, Extension::NO_FLAGS));
   }
 
  protected:
@@ -130,25 +131,6 @@ TEST_F(UnpackerTest, NoL10n) {
   EXPECT_EQ(0U, unpacker_->parsed_catalogs()->size());
 }
 
-TEST_F(UnpackerTest, UnzipDirectoryError) {
-  const char kExpected[] = "Could not create directory for unzipping: ";
-  SetupUnpacker("good_package.crx");
-  base::FilePath path = temp_dir_.path().AppendASCII(kTempExtensionName);
-  ASSERT_TRUE(base::WriteFile(path, "foo", 3));
-  EXPECT_FALSE(unpacker_->Run());
-  EXPECT_TRUE(
-      StartsWith(unpacker_->error_message(), ASCIIToUTF16(kExpected), false))
-      << "Expected prefix: \"" << kExpected << "\", actual error: \""
-      << unpacker_->error_message() << "\"";
-}
-
-TEST_F(UnpackerTest, UnzipError) {
-  const char kExpected[] = "Could not unzip extension";
-  SetupUnpacker("bad_zip.crx");
-  EXPECT_FALSE(unpacker_->Run());
-  EXPECT_EQ(ASCIIToUTF16(kExpected), unpacker_->error_message());
-}
-
 namespace {
 
 // Inserts an illegal path into the browser images returned by
diff --git a/extensions/utility/utility_handler.cc b/extensions/utility/utility_handler.cc
index 1620337..65cf509 100644
--- a/extensions/utility/utility_handler.cc
+++ b/extensions/utility/utility_handler.cc
@@ -6,17 +6,22 @@
 
 #include "base/command_line.h"
 #include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/i18n/rtl.h"
 #include "content/public/utility/utility_thread.h"
+#include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_l10n_util.h"
 #include "extensions/common/extension_utility_messages.h"
 #include "extensions/common/extensions_client.h"
 #include "extensions/common/manifest.h"
 #include "extensions/common/update_manifest.h"
+#include "extensions/strings/grit/extensions_strings.h"
 #include "extensions/utility/unpacker.h"
 #include "ipc/ipc_message.h"
 #include "ipc/ipc_message_macros.h"
 #include "third_party/zlib/google/zip.h"
+#include "ui/base/l10n/l10n_util.h"
 #include "ui/base/ui_base_switches.h"
 
 namespace extensions {
@@ -94,19 +99,30 @@ void UtilityHandler::OnUnpackExtension(
   CHECK_GT(location, Manifest::INVALID_LOCATION);
   CHECK_LT(location, Manifest::NUM_LOCATIONS);
   DCHECK(ExtensionsClient::Get());
-  Unpacker unpacker(extension_path,
-                    extension_id,
-                    static_cast<Manifest::Location>(location),
-                    creation_flags);
-  if (unpacker.Run() && unpacker.DumpImagesToFile() &&
-      unpacker.DumpMessageCatalogsToFile()) {
-    Send(new ExtensionUtilityHostMsg_UnpackExtension_Succeeded(
-        *unpacker.parsed_manifest()));
-  } else {
+  base::FilePath working_dir = extension_path.DirName();
+  base::FilePath unzipped_dir = working_dir.AppendASCII(kTempExtensionName);
+  base::string16 error;
+  if (!base::CreateDirectory(unzipped_dir)) {
+    Send(new ExtensionUtilityHostMsg_UnpackExtension_Failed(
+        l10n_util::GetStringFUTF16(
+            IDS_EXTENSION_PACKAGE_DIRECTORY_ERROR,
+            base::i18n::GetDisplayStringInLTRDirectionality(
+                unzipped_dir.LossyDisplayName()))));
+  } else if (!zip::Unzip(extension_path, unzipped_dir)) {
     Send(new ExtensionUtilityHostMsg_UnpackExtension_Failed(
-        unpacker.error_message()));
+        l10n_util::GetStringUTF16(IDS_EXTENSION_PACKAGE_UNZIP_ERROR)));
+  } else {
+    Unpacker unpacker(working_dir, unzipped_dir, extension_id,
+                      static_cast<Manifest::Location>(location),
+                        creation_flags);
+    if (unpacker.Run()) {
+      Send(new ExtensionUtilityHostMsg_UnpackExtension_Succeeded(
+          *unpacker.parsed_manifest()));
+    } else {
+      Send(new ExtensionUtilityHostMsg_UnpackExtension_Failed(
+          unpacker.error_message()));
+    }
   }
-
   ReleaseProcessIfNeeded();
 }