imageWriterPrivate: Add ExtractionProperties

In order to support other compression formats in imageWriterPrivate,
UnzipHelper is renamed to ZipExtractor and its arguments are split into
ExtractionProperties, so that other extractor classes can easily take
the same set of arguments.

Also this CL simplifies the lifecycle of the class by managing it on
its own.

This refactoring is described in "Adding multiple compression format
supports" section of the design doc.

Design doc: http://go/tar-xz-cros-recovery

TEST=manually verified that zip file flashing and normal file flashing
are working in Recovery Utility
TEST=ImageWriterOperationTest
BUG=b:153027505

Change-Id: I505bd426b163500926d0acae5947f6c3f0531758
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2573963
Commit-Queue: Tetsui Ohkubo <tetsui@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843494}

chrome/browser/extensions/BUILD.gn

@@ -205,6 +205,8 @@ static_library("extensions") {
     "api/image_writer_private/destroy_partitions_operation.h",
     "api/image_writer_private/error_messages.cc",
     "api/image_writer_private/error_messages.h",
+    "api/image_writer_private/extraction_properties.cc",
+    "api/image_writer_private/extraction_properties.h",
     "api/image_writer_private/image_writer_private_api.cc",
     "api/image_writer_private/image_writer_private_api.h",
     "api/image_writer_private/image_writer_utility_client.cc",
@@ -215,12 +217,12 @@ static_library("extensions") {
     "api/image_writer_private/operation_manager.h",
     "api/image_writer_private/removable_storage_provider.cc",
     "api/image_writer_private/removable_storage_provider.h",
-    "api/image_writer_private/unzip_helper.cc",
-    "api/image_writer_private/unzip_helper.h",
     "api/image_writer_private/write_from_file_operation.cc",
     "api/image_writer_private/write_from_file_operation.h",
     "api/image_writer_private/write_from_url_operation.cc",
     "api/image_writer_private/write_from_url_operation.h",
+    "api/image_writer_private/zip_extractor.cc",
+    "api/image_writer_private/zip_extractor.h",
     "api/instance_id/instance_id_api.cc",
     "api/instance_id/instance_id_api.h",
     "api/language_settings_private/language_settings_private_api.cc",

chrome/browser/extensions/api/image_writer_private/extraction_properties.cc

+// Copyright 2020 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/extensions/api/image_writer_private/extraction_properties.h"
+
+namespace extensions {
+namespace image_writer {
+
+ExtractionProperties::ExtractionProperties() = default;
+ExtractionProperties::ExtractionProperties(ExtractionProperties&&) = default;
+ExtractionProperties::~ExtractionProperties() = default;
+
+}  // namespace image_writer
+}  // namespace extensions

chrome/browser/extensions/api/image_writer_private/extraction_properties.h

+// Copyright 2020 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.
+
+#ifndef CHROME_BROWSER_EXTENSIONS_API_IMAGE_WRITER_PRIVATE_EXTRACTION_PROPERTIES_H_
+#define CHROME_BROWSER_EXTENSIONS_API_IMAGE_WRITER_PRIVATE_EXTRACTION_PROPERTIES_H_
+
+#include "base/callback.h"
+#include "base/files/file_path.h"
+
+namespace extensions {
+namespace image_writer {
+
+struct ExtractionProperties {
+  ExtractionProperties();
+  ExtractionProperties(ExtractionProperties&&);
+  ~ExtractionProperties();
+
+  base::FilePath image_path;
+  base::FilePath temp_dir_path;
+
+  base::OnceCallback<void(const base::FilePath&)> open_callback;
+  base::OnceClosure complete_callback;
+  base::OnceCallback<void(const std::string&)> failure_callback;
+  base::RepeatingCallback<void(int64_t, int64_t)> progress_callback;
+};
+
+}  // namespace image_writer
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_EXTENSIONS_API_IMAGE_WRITER_PRIVATE_EXTRACTION_PROPERTIES_H_

chrome/browser/extensions/api/image_writer_private/operation.cc

@@ -12,8 +12,9 @@
 #include "build/build_config.h"
 #include "build/chromeos_buildflags.h"
 #include "chrome/browser/extensions/api/image_writer_private/error_messages.h"
+#include "chrome/browser/extensions/api/image_writer_private/extraction_properties.h"
 #include "chrome/browser/extensions/api/image_writer_private/operation_manager.h"
-#include "chrome/browser/extensions/api/image_writer_private/unzip_helper.h"
+#include "chrome/browser/extensions/api/image_writer_private/zip_extractor.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
 
@@ -24,6 +25,21 @@ namespace {
 
 const int kMD5BufferSize = 1024;
 
+// Returns true if the file at |image_path| is an archived image.
+bool IsArchive(const base::FilePath& image_path) {
+  // TODO(tetsui): Support .tar and .tar.xz file formats.
+  return ZipExtractor::IsZipFile(image_path);
+}
+
+// Extracts the archive at |image_path| using to |temp_dir_path| using a proper
+// extractor.
+void ExtractArchive(ExtractionProperties properties) {
+  if (ZipExtractor::IsZipFile(properties.image_path)) {
+    ZipExtractor::Extract(std::move(properties));
+  }
+  // TODO(tetsui): Support .tar and .tar.xz file formats.
+}
+
 }  // namespace
 
 Operation::Operation(base::WeakPtr<OperationManager> manager,
@@ -96,30 +112,36 @@ void Operation::Start() {
   StartImpl();
 }
 
-void Operation::OnUnzipOpenComplete(const base::FilePath& image_path) {
+void Operation::OnExtractOpenComplete(const base::FilePath& image_path) {
   DCHECK(IsRunningInCorrectSequence());
   image_path_ = image_path;
 }
 
-void Operation::Unzip(const base::Closure& continuation) {
+void Operation::Extract(const base::Closure& continuation) {
   DCHECK(IsRunningInCorrectSequence());
   if (IsCancelled()) {
     return;
   }
 
-  if (image_path_.Extension() != FILE_PATH_LITERAL(".zip")) {
+  if (IsArchive(image_path_)) {
+    SetStage(image_writer_api::STAGE_UNZIP);
+
+    ExtractionProperties properties;
+    properties.image_path = image_path_;
+    properties.temp_dir_path = temp_dir_->GetPath();
+    properties.open_callback =
+        base::BindOnce(&Operation::OnExtractOpenComplete, this);
+    properties.complete_callback =
+        base::BindOnce(&Operation::CompleteAndContinue, this, continuation);
+    properties.failure_callback =
+        base::BindOnce(&Operation::OnExtractFailure, this);
+    properties.progress_callback =
+        base::BindRepeating(&Operation::OnExtractProgress, this);
+
+    ExtractArchive(std::move(properties));
+  } else {
     PostTask(continuation);
-    return;
   }
-
-  SetStage(image_writer_api::STAGE_UNZIP);
-
-  auto unzip_helper = base::MakeRefCounted<UnzipHelper>(
-      base::Bind(&Operation::OnUnzipOpenComplete, this),
-      base::Bind(&Operation::CompleteAndContinue, this, continuation),
-      base::Bind(&Operation::OnUnzipFailure, this),
-      base::Bind(&Operation::OnUnzipProgress, this));
-  unzip_helper->Unzip(image_path_, temp_dir_->GetPath());
 }
 
 void Operation::Finish() {
@@ -301,12 +323,12 @@ void Operation::MD5Chunk(
   }
 }
 
-void Operation::OnUnzipFailure(const std::string& error) {
+void Operation::OnExtractFailure(const std::string& error) {
   DCHECK(IsRunningInCorrectSequence());
   Error(error);
 }
 
-void Operation::OnUnzipProgress(int64_t total_bytes, int64_t progress_bytes) {
+void Operation::OnExtractProgress(int64_t total_bytes, int64_t progress_bytes) {
   DCHECK(IsRunningInCorrectSequence());
 
   int progress_percent = kProgressComplete * progress_bytes / total_bytes;

chrome/browser/extensions/api/image_writer_private/operation.h

@@ -97,9 +97,9 @@ class Operation : public base::RefCountedThreadSafe<Operation> {
   // operation.  It will be called from Start().
   virtual void StartImpl() = 0;
 
-  // Unzips the current file if it ends in ".zip".  The current_file will be set
-  // to the unzipped file.
-  void Unzip(const base::Closure& continuation);
+  // Extracts the current file if it's an archive.  The current_file will be set
+  // to the extracted file.
+  void Extract(const base::Closure& continuation);
 
   // Writes the current file to device_path.
   void Write(const base::Closure& continuation);
@@ -207,10 +207,10 @@ class Operation : public base::RefCountedThreadSafe<Operation> {
                 int progress_scale,
                 const base::OnceCallback<void(const std::string&)> callback);
 
-  // Callbacks for UnzipHelper.
-  void OnUnzipOpenComplete(const base::FilePath& image_path);
-  void OnUnzipProgress(int64_t total_bytes, int64_t progress_bytes);
-  void OnUnzipFailure(const std::string& error);
+  // Callbacks for Extractor.
+  void OnExtractOpenComplete(const base::FilePath& image_path);
+  void OnExtractProgress(int64_t total_bytes, int64_t progress_bytes);
+  void OnExtractFailure(const std::string& error);
 
   // Runs all cleanup functions.
   void CleanUp();

chrome/browser/extensions/api/image_writer_private/operation_unittest.cc

@@ -59,8 +59,8 @@ class OperationForTest : public Operation {
 
   // Expose internal stages for testing.
   // Also wraps Operation's methods to run on correct sequence.
-  void Unzip(const base::Closure& continuation) {
-    PostTask(base::BindOnce(&Operation::Unzip, this, continuation));
+  void Extract(const base::Closure& continuation) {
+    PostTask(base::BindOnce(&Operation::Extract, this, continuation));
   }
 
   void Write(const base::Closure& continuation) {
@@ -132,7 +132,7 @@ class ImageWriterOperationTest : public ImageWriterUnitTestBase {
 };
 
 // Unizpping a non-zip should do nothing.
-TEST_F(ImageWriterOperationTest, UnzipNonZipFile) {
+TEST_F(ImageWriterOperationTest, ExtractNonZipFile) {
   EXPECT_CALL(manager_, OnProgress(kDummyExtensionId, _, _)).Times(0);
 
   EXPECT_CALL(manager_, OnError(kDummyExtensionId, _, _, _)).Times(0);
@@ -141,11 +141,11 @@ TEST_F(ImageWriterOperationTest, UnzipNonZipFile) {
 
   operation_->Start();
   base::RunLoop run_loop;
-  operation_->Unzip(run_loop.QuitClosure());
+  operation_->Extract(run_loop.QuitClosure());
   run_loop.Run();
 }
 
-TEST_F(ImageWriterOperationTest, UnzipZipFile) {
+TEST_F(ImageWriterOperationTest, ExtractZipFile) {
   EXPECT_CALL(manager_, OnError(kDummyExtensionId, _, _, _)).Times(0);
   EXPECT_CALL(manager_,
               OnProgress(kDummyExtensionId, image_writer_api::STAGE_UNZIP, _))
@@ -161,7 +161,7 @@ TEST_F(ImageWriterOperationTest, UnzipZipFile) {
 
   operation_->Start();
   base::RunLoop run_loop;
-  operation_->Unzip(run_loop.QuitClosure());
+  operation_->Extract(run_loop.QuitClosure());
   run_loop.Run();
 
   EXPECT_TRUE(base::ContentsEqual(image_path_, operation_->GetImagePath()));

chrome/browser/extensions/api/image_writer_private/unzip_helper.cc

-// Copyright 2017 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/extensions/api/image_writer_private/unzip_helper.h"
-
-#include <memory>
-
-#include "base/bind.h"
-#include "base/files/file_util.h"
-#include "base/task/post_task.h"
-#include "base/task/thread_pool.h"
-#include "chrome/browser/extensions/api/image_writer_private/error_messages.h"
-#include "third_party/zlib/google/zip_reader.h"
-
-namespace extensions {
-namespace image_writer {
-
-UnzipHelper::UnzipHelper(
-    const base::Callback<void(const base::FilePath&)>& open_callback,
-    const base::Closure& complete_callback,
-    const base::Callback<void(const std::string&)>& failure_callback,
-    const base::Callback<void(int64_t, int64_t)>& progress_callback)
-    : open_callback_(open_callback),
-      complete_callback_(complete_callback),
-      failure_callback_(failure_callback),
-      progress_callback_(progress_callback),
-      zip_reader_(std::make_unique<zip::ZipReader>()) {}
-
-UnzipHelper::~UnzipHelper() {}
-
-void UnzipHelper::Unzip(const base::FilePath& image_path,
-                        const base::FilePath& temp_dir_path) {
-  if (!zip_reader_->Open(image_path) || !zip_reader_->AdvanceToNextEntry() ||
-      !zip_reader_->OpenCurrentEntryInZip()) {
-    OnError(error::kUnzipGenericError);
-    return;
-  }
-
-  if (zip_reader_->HasMore()) {
-    OnError(error::kUnzipInvalidArchive);
-    return;
-  }
-
-  // Create a new target to unzip to.  The original file is opened by
-  // |zip_reader_|.
-  zip::ZipReader::EntryInfo* entry_info = zip_reader_->current_entry_info();
-
-  if (!entry_info) {
-    OnError(error::kTempDirError);
-    return;
-  }
-
-  base::FilePath out_image_path =
-      temp_dir_path.Append(entry_info->file_path().BaseName());
-  OnOpenSuccess(out_image_path);
-
-  zip_reader_->ExtractCurrentEntryToFilePathAsync(
-      out_image_path, base::BindOnce(&UnzipHelper::OnComplete, this),
-      base::BindOnce(&UnzipHelper::OnError, this, error::kUnzipGenericError),
-      base::Bind(&UnzipHelper::OnProgress, this, entry_info->original_size()));
-}
-
-void UnzipHelper::OnError(const std::string& error) {
-  failure_callback_.Run(error);
-}
-
-void UnzipHelper::OnOpenSuccess(const base::FilePath& image_path) {
-  open_callback_.Run(image_path);
-}
-
-void UnzipHelper::OnComplete() {
-  complete_callback_.Run();
-}
-
-void UnzipHelper::OnProgress(int64_t total_bytes, int64_t curr_bytes) {
-  progress_callback_.Run(total_bytes, curr_bytes);
-}
-
-}  // namespace image_writer
-}  // namespace extensions

chrome/browser/extensions/api/image_writer_private/unzip_helper.h

-// Copyright 2017 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.
-
-#ifndef CHROME_BROWSER_EXTENSIONS_API_IMAGE_WRITER_PRIVATE_UNZIP_HELPER_H_
-#define CHROME_BROWSER_EXTENSIONS_API_IMAGE_WRITER_PRIVATE_UNZIP_HELPER_H_
-
-#include "base/callback.h"
-#include "base/memory/ref_counted_memory.h"
-#include "build/build_config.h"
-
-namespace base {
-class FilePath;
-}
-
-namespace zip {
-class ZipReader;
-}
-
-namespace extensions {
-namespace image_writer {
-
-// A helper to provide Unzip operation.
-class UnzipHelper : public base::RefCountedThreadSafe<UnzipHelper> {
- public:
-  explicit UnzipHelper(
-      const base::Callback<void(const base::FilePath&)>& open_callback,
-      const base::Closure& complete_callback,
-      const base::Callback<void(const std::string&)>& failure_callback,
-      const base::Callback<void(int64_t, int64_t)>& progress_callback);
-
-  void Unzip(const base::FilePath& image_path,
-             const base::FilePath& temp_dir_path);
-
- private:
-  friend class base::RefCountedThreadSafe<UnzipHelper>;
-  ~UnzipHelper();
-
-  void OnError(const std::string& error);
-  void OnOpenSuccess(const base::FilePath& image_path);
-  void OnComplete();
-  void OnProgress(int64_t total_bytes, int64_t curr_bytes);
-
-  base::Callback<void(const base::FilePath&)> open_callback_;
-  base::Closure complete_callback_;
-  base::Callback<void(const std::string&)> failure_callback_;
-  base::Callback<void(int64_t, int64_t)> progress_callback_;
-
-  // Zip reader for unzip operations. The reason for using a pointer is that we
-  // don't want to include zip_reader.h here which can mangle definitions in
-  // jni.h when included in the same file. See crbug.com/554199.
-  std::unique_ptr<zip::ZipReader> zip_reader_;
-
-  DISALLOW_COPY_AND_ASSIGN(UnzipHelper);
-};
-
-}  // namespace image_writer
-}  // namespace extensions
-
-#endif  // CHROME_BROWSER_EXTENSIONS_API_IMAGE_WRITER_PRIVATE_UNZIP_HELPER_H_

chrome/browser/extensions/api/image_writer_private/write_from_file_operation.cc

@@ -35,7 +35,7 @@ void WriteFromFileOperation::StartImpl() {
   }
 
   PostTask(base::BindOnce(
-      &WriteFromFileOperation::Unzip, this,
+      &WriteFromFileOperation::Extract, this,
       base::Bind(
           &WriteFromFileOperation::Write, this,
           base::Bind(&WriteFromFileOperation::VerifyWrite, this,

chrome/browser/extensions/api/image_writer_private/write_from_url_operation.cc

@@ -43,7 +43,7 @@ void WriteFromUrlOperation::StartImpl() {
       base::BindOnce(
           &WriteFromUrlOperation::VerifyDownload, this,
           base::BindOnce(
-              &WriteFromUrlOperation::Unzip, this,
+              &WriteFromUrlOperation::Extract, this,
               base::Bind(&WriteFromUrlOperation::Write, this,
                          base::Bind(&WriteFromUrlOperation::VerifyWrite, this,
                                     base::Bind(&WriteFromUrlOperation::Finish,

chrome/browser/extensions/api/image_writer_private/zip_extractor.cc

+// Copyright 2020 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/extensions/api/image_writer_private/zip_extractor.h"
+
+#include <memory>
+
+#include "base/bind.h"
+#include "base/files/file_util.h"
+#include "base/task/post_task.h"
+#include "base/task/thread_pool.h"
+#include "chrome/browser/extensions/api/image_writer_private/error_messages.h"
+
+namespace extensions {
+namespace image_writer {
+
+// static
+bool ZipExtractor::IsZipFile(const base::FilePath& image_path) {
+  // TODO(tetsui): Check the file header instead of the extension.
+  return image_path.Extension() == FILE_PATH_LITERAL(".zip");
+}
+
+// static
+void ZipExtractor::Extract(ExtractionProperties properties) {
+  // ZipExtractor manages its own lifetime, and will delete itself when it
+  // completes.
+  ZipExtractor* extractor = new ZipExtractor(std::move(properties));
+  extractor->ExtractImpl();
+}
+
+ZipExtractor::ZipExtractor(ExtractionProperties properties)
+    : properties_(std::move(properties)) {}
+
+ZipExtractor::~ZipExtractor() = default;
+
+void ZipExtractor::ExtractImpl() {
+  if (!zip_reader_.Open(properties_.image_path) ||
+      !zip_reader_.AdvanceToNextEntry() ||
+      !zip_reader_.OpenCurrentEntryInZip()) {
+    // |this| will be deleted inside.
+    OnError(error::kUnzipGenericError);
+    return;
+  }
+
+  if (zip_reader_.HasMore()) {
+    // |this| will be deleted inside.
+    OnError(error::kUnzipInvalidArchive);
+    return;
+  }
+
+  // Create a new target to unzip to.  The original file is opened by
+  // |zip_reader_|.
+  zip::ZipReader::EntryInfo* entry_info = zip_reader_.current_entry_info();
+
+  if (!entry_info) {
+    // |this| will be deleted inside.
+    OnError(error::kTempDirError);
+    return;
+  }
+
+  base::FilePath out_image_path =
+      properties_.temp_dir_path.Append(entry_info->file_path().BaseName());
+  std::move(properties_.open_callback).Run(out_image_path);
+
+  // |this| will be deleted when OnComplete or OnError is called.
+  zip_reader_.ExtractCurrentEntryToFilePathAsync(
+      out_image_path,
+      base::BindOnce(&ZipExtractor::OnComplete, weak_ptr_factory_.GetWeakPtr()),
+      base::BindOnce(&ZipExtractor::OnError, weak_ptr_factory_.GetWeakPtr(),
+                     error::kUnzipGenericError),
+      base::BindRepeating(properties_.progress_callback,
+                          entry_info->original_size()));
+}
+
+void ZipExtractor::OnError(const std::string& error) {
+  std::move(properties_.failure_callback).Run(error);
+  delete this;
+}
+
+void ZipExtractor::OnComplete() {
+  std::move(properties_.complete_callback).Run();
+  delete this;
+}
+
+}  // namespace image_writer
+}  // namespace extensions

chrome/browser/extensions/api/image_writer_private/zip_extractor.h

+// Copyright 2020 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.
+
+#ifndef CHROME_BROWSER_EXTENSIONS_API_IMAGE_WRITER_PRIVATE_ZIP_EXTRACTOR_H_
+#define CHROME_BROWSER_EXTENSIONS_API_IMAGE_WRITER_PRIVATE_ZIP_EXTRACTOR_H_
+
+#include "base/callback.h"
+#include "build/build_config.h"
+#include "chrome/browser/extensions/api/image_writer_private/extraction_properties.h"
+#include "third_party/zlib/google/zip_reader.h"
+
+namespace base {
+class FilePath;
+}
+
+namespace extensions {
+namespace image_writer {
+
+// An extractor that supports extraction of zip-archived OS images.
+class ZipExtractor {
+ public:
+  static bool IsZipFile(const base::FilePath& image_path);
+
+  // Start extracting the archive at |image_path| to |temp_dir_path| in
+  // |properties|.
+  static void Extract(ExtractionProperties properties);
+
+  ZipExtractor(const ZipExtractor&) = delete;
+  ZipExtractor& operator=(const ZipExtractor&) = delete;
+
+ private:
+  // This class manages its own lifetime.
+  explicit ZipExtractor(ExtractionProperties properties);
+  ~ZipExtractor();
+
+  void ExtractImpl();
+
+  void OnError(const std::string& error);
+  void OnComplete();
+
+  zip::ZipReader zip_reader_;
+
+  ExtractionProperties properties_;
+
+  base::WeakPtrFactory<ZipExtractor> weak_ptr_factory_{this};
+};
+
+}  // namespace image_writer
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_EXTENSIONS_API_IMAGE_WRITER_PRIVATE_ZIP_EXTRACTOR_H_