Fixes memory leak in 149313003.

The leak was caused by the modular testing not forcing the run-and-clear of
clean-up callbacks.  This left a callback owned by the object which increment
the refcount of the object.  These callbacks should be removed and a different
method of ensuring cleanup should be employed.

Ideally we can find a way to get rid of the ref-counting on the object to avoid
this sort of issue in the future, but currently the operation manager lives on
the IO thread and the Operation must work on the FILE thread.  A sequenced task
queue or a worker may be useful.

Previous CL: https://codereview.chromium.org/149313003/

BUG=292956
BUG=337883
BUG=335404

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@251984 0039d316-1c4b-4281-b951-d872f2087c98

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

@@ -18,18 +18,11 @@ DestroyPartitionsOperation::DestroyPartitionsOperation(
     base::WeakPtr<OperationManager> manager,
     const ExtensionId& extension_id,
     const std::string& storage_unit_id)
-    : Operation(manager, extension_id, storage_unit_id) {
-  verify_write_ = false;
-}
+    : Operation(manager, extension_id, storage_unit_id) {}
 
 DestroyPartitionsOperation::~DestroyPartitionsOperation() {}
 
-void DestroyPartitionsOperation::Start() {
-  if (!temp_dir_.CreateUniqueTempDir()) {
-    Error(error::kTempDirError);
-    return;
-  }
-
+void DestroyPartitionsOperation::StartImpl() {
   if (!base::CreateTemporaryFileInDir(temp_dir_.path(), &image_path_)) {
     Error(error::kTempFileError);
     return;
@@ -44,7 +37,7 @@ void DestroyPartitionsOperation::Start() {
     return;
   }
 
-  WriteStart();
+  Write(base::Bind(&DestroyPartitionsOperation::Finish, this));
 }
 
 }  // namespace image_writer

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

@@ -20,11 +20,10 @@ class DestroyPartitionsOperation : public Operation {
   DestroyPartitionsOperation(base::WeakPtr<OperationManager> manager,
                              const ExtensionId& extension_id,
                              const std::string& storage_unit_id);
-  virtual void Start() OVERRIDE;
+  virtual void StartImpl() OVERRIDE;
 
  private:
   virtual ~DestroyPartitionsOperation();
-  base::ScopedTempDir temp_dir_;
 };
 
 }  // namespace image_writer

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

@@ -12,6 +12,7 @@ namespace image_writer {
 
 using testing::_;
 using testing::AnyNumber;
+using testing::AtLeast;
 
 namespace {
 
@@ -21,7 +22,7 @@ class ImageWriterDestroyPartitionsOperationTest
 
 // Tests that the DestroyPartitionsOperation can successfully zero the first
 // kPartitionTableSize bytes of an image.
-TEST_F(ImageWriterDestroyPartitionsOperationTest, DestroyPartitions) {
+TEST_F(ImageWriterDestroyPartitionsOperationTest, DestroyPartitionsEndToEnd) {
   MockOperationManager manager;
   base::RunLoop loop;
 
@@ -31,10 +32,15 @@ TEST_F(ImageWriterDestroyPartitionsOperationTest, DestroyPartitions) {
                                      test_device_path_.AsUTF8Unsafe()));
 
 #if defined(OS_LINUX) || defined(OS_CHROMEOS)
-  EXPECT_CALL(manager, OnProgress(kDummyExtensionId, _, _)).Times(0);
   EXPECT_CALL(manager, OnProgress(kDummyExtensionId,
                                   image_writer_api::STAGE_WRITE,
                                   _)).Times(AnyNumber());
+  EXPECT_CALL(manager,
+              OnProgress(kDummyExtensionId, image_writer_api::STAGE_WRITE, 0))
+      .Times(AtLeast(1));
+  EXPECT_CALL(manager,
+              OnProgress(kDummyExtensionId, image_writer_api::STAGE_WRITE, 100))
+      .Times(AtLeast(1));
   EXPECT_CALL(manager, OnComplete(kDummyExtensionId)).Times(1);
   EXPECT_CALL(manager, OnError(kDummyExtensionId, _, _, _)).Times(0);
 #else

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

@@ -31,17 +31,6 @@ bool ImageWriterPrivateWriteFromUrlFunction::RunImpl() {
     return false;
   }
 
-#if defined(OS_CHROMEOS)
-  // The Chrome OS temporary partition is too small for Chrome OS images, thus
-  // we must always use the downloads folder.
-  bool save_image_as_download = true;
-#else
-  bool save_image_as_download = false;
-  if (params->options.get() && params->options->save_as_download.get()) {
-    save_image_as_download = true;
-  }
-#endif
-
   std::string hash;
   if (params->options.get() && params->options->image_hash.get()) {
     hash = *params->options->image_hash;
@@ -50,9 +39,7 @@ bool ImageWriterPrivateWriteFromUrlFunction::RunImpl() {
   image_writer::OperationManager::Get(GetProfile())->StartWriteFromUrl(
       extension_id(),
       url,
-      render_view_host(),
       hash,
-      save_image_as_download,
       params->storage_unit_id,
       base::Bind(&ImageWriterPrivateWriteFromUrlFunction::OnWriteStarted,
                  this));
@@ -89,11 +76,11 @@ bool ImageWriterPrivateWriteFromFileFunction::RunImpl() {
   base::FilePath path;
 
   if (!extensions::app_file_handler_util::ValidateFileEntryAndGetPath(
-      filesystem_name,
-      filesystem_path,
-      render_view_host_,
-      &path,
-      &error_))
+           filesystem_name,
+           filesystem_path,
+           render_view_host(),
+           &path,
+           &error_))
     return false;
 
   image_writer::OperationManager::Get(GetProfile())->StartWriteFromFile(

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

-// Copyright 2014 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 "base/logging.h"
-#include "chrome/browser/extensions/api/image_writer_private/image_writer_utils.h"
-
-namespace extensions {
-namespace image_writer_utils
-{
-
-const int kFsyncRatio = 1024;
-
-ImageWriter::ImageWriter() : file_(base::kInvalidPlatformFileValue),
-                             writes_count_(0) {
-}
-
-ImageWriter::~ImageWriter() {
-  if (file_ != base::kInvalidPlatformFileValue)
-    base::ClosePlatformFile(file_);
-}
-
-bool ImageWriter::Open(const base::FilePath& path) {
-  if (file_ != base::kInvalidPlatformFileValue)
-    Close();
-  DCHECK_EQ(base::kInvalidPlatformFileValue, file_);
-  base::PlatformFileError error;
-  file_ = base::CreatePlatformFile(
-    path,
-    base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_WRITE,
-    NULL,
-    &error);
-  if (error != base::PLATFORM_FILE_OK) {
-    LOG(ERROR) << "Couldn't open target path "
-               << path.value() << ": " << error;
-    return false;
-  }
-  return true;
-}
-
-bool ImageWriter::Close() {
-  if (file_ != base::kInvalidPlatformFileValue &&
-      base::ClosePlatformFile(file_)) {
-    file_ = base::kInvalidPlatformFileValue;
-    return true;
-  } else {
-    LOG(ERROR) << "Could not close target file";
-    return false;
-  }
-}
-
-int ImageWriter::Write(const char* data_block, int data_size) {
-  int written = base::WritePlatformFileAtCurrentPos(file_, data_block,
-                                                    data_size);
-  if (written != data_size) {
-    LOG(ERROR) << "Error writing to target file";
-    return -1;
-  }
-
-  writes_count_++;
-  if (writes_count_ == kFsyncRatio) {
-    if (!base::FlushPlatformFile(file_)) {
-      LOG(ERROR) << "Error syncing target file";
-      return -1;
-    }
-    writes_count_ = 0;
-  }
-
-  return written;
-}
-
-ImageReader::ImageReader() : file_(base::kInvalidPlatformFileValue) {
-}
-
-ImageReader::~ImageReader() {
-  if (file_ != base::kInvalidPlatformFileValue)
-    base::ClosePlatformFile(file_);
-}
-
-bool ImageReader::Open(const base::FilePath& path) {
-  if (file_ != base::kInvalidPlatformFileValue)
-    Close();
-  DCHECK_EQ(base::kInvalidPlatformFileValue, file_);
-  base::PlatformFileError error;
-  file_ = base::CreatePlatformFile(
-    path,
-    base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ,
-    NULL,
-    &error);
-  if (error != base::PLATFORM_FILE_OK) {
-    LOG(ERROR) << "Couldn't open target path "
-               << path.value() << ": " << error;
-    return false;
-  }
-  return true;
-}
-
-bool ImageReader::Close() {
-  if (file_ != base::kInvalidPlatformFileValue &&
-      base::ClosePlatformFile(file_)) {
-    file_ = base::kInvalidPlatformFileValue;
-    return true;
-  } else {
-    LOG(ERROR) << "Could not close target file";
-    return false;
-  }
-  return true;
-}
-
-int ImageReader::Read(char* data_block, int data_size) {
-  int read = base::ReadPlatformFileAtCurrentPos(file_, data_block, data_size);
-  if (read < 0)
-    LOG(ERROR) << "Error reading from source file";
-  return read;
-}
-
-int64 ImageReader::GetSize() {
-  base::PlatformFileInfo info;
-  if (base::GetPlatformFileInfo(file_, &info)) {
-    return info.size;
-  } else {
-    return 0;
-  }
-}
-
-} // namespace image_writer_utils
-} // namespace extensions

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

-// Copyright 2014 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_IMAGE_WRITER_UTILS_H_
-#define CHROME_BROWSER_EXTENSIONS_API_IMAGE_WRITER_PRIVATE_IMAGE_WRITER_UTILS_H_
-
-#include "base/files/file_path.h"
-#include "base/platform_file.h"
-
-namespace extensions {
-namespace image_writer_utils {
-
-// Utility class for writing data to a removable disk.
-class ImageWriter {
- public:
-  ImageWriter();
-  virtual ~ImageWriter();
-
-  // Note: If there is already a device open, it will be closed.
-  virtual bool Open(const base::FilePath& path);
-  virtual bool Close();
-
-  // Writes from data_block to the device, and returns the amount written or -1
-  // on error.
-  virtual int Write(const char* data_block, int data_size);
-
- private:
-   base::PlatformFile file_;
-   int writes_count_;
-};
-
-// Utility class for reading a large file, such as a Chrome OS image.
-class ImageReader {
- public:
-  ImageReader();
-  virtual ~ImageReader();
-
-  // Note: If there is already a file open, it will be closed.
-  virtual bool Open(const base::FilePath& path);
-  virtual bool Close();
-  // Read from the file into data_block.
-  virtual int Read(char* data_block, int data_size);
-  virtual int64 GetSize();
-
- private:
-  base::PlatformFile file_;
-};
-
-} // namespace image_writer_utils
-} // namespace extensions
-
-#endif  // CHROME_BROWSER_EXTENSIONS_API_IMAGE_WRITER_PRIVATE_IMAGE_WRITER_UTILS_H_

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

@@ -6,12 +6,12 @@
 #define CHROME_BROWSER_EXTENSIONS_API_IMAGE_WRITER_PRIVATE_OPERATION_H_
 
 #include "base/callback.h"
+#include "base/files/scoped_temp_dir.h"
 #include "base/md5.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/memory/weak_ptr.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "base/timer/timer.h"
-#include "chrome/browser/extensions/api/image_writer_private/image_writer_utils.h"
 #include "chrome/common/extensions/api/image_writer_private.h"
 #include "third_party/zlib/google/zip_reader.h"
 
@@ -38,6 +38,13 @@ class OperationManager;
 // Run, Complete.  Start and Complete run on the UI thread and are responsible
 // for advancing to the next stage and other UI interaction.  The Run phase does
 // the work on the FILE thread and calls SendProgress or Error as appropriate.
+//
+// TODO(haven): This class is current refcounted because it is owned by the
+// OperationManager on the UI thread but needs to do work on the FILE thread.
+// There is probably a better way to organize this so that it can be represented
+// by a WeakPtr, but those are not thread-safe.  Additionally, if destruction is
+// done on the UI thread then that causes problems if any of the fields were
+// allocated/accessed on the FILE thread.  http://crbug.com/344713
 class Operation : public base::RefCountedThreadSafe<Operation> {
  public:
   typedef base::Callback<void(bool, const std::string&)> StartWriteCallback;
@@ -46,10 +53,10 @@ class Operation : public base::RefCountedThreadSafe<Operation> {
 
   Operation(base::WeakPtr<OperationManager> manager,
             const ExtensionId& extension_id,
-            const std::string& storage_unit_id);
+            const std::string& device_path);
 
   // Starts the operation.
-  virtual void Start() = 0;
+  void Start();
 
   // Cancel the operation. This must be called to clean up internal state and
   // cause the the operation to actually stop.  It will not be destroyed until
@@ -66,6 +73,23 @@ class Operation : public base::RefCountedThreadSafe<Operation> {
  protected:
   virtual ~Operation();
 
+  // This function should be overriden by subclasses to set up the work of the
+  // 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);
+
+  // Writes the current file to device_path.
+  void Write(const base::Closure& continuation);
+
+  // Verifies that the current file and device_path contents match.
+  void VerifyWrite(const base::Closure& continuation);
+
+  // Completes the operation.
+  void Finish();
+
   // Generates an error.
   // |error_message| is used to create an OnWriteError event which is
   // sent to the extension
@@ -84,12 +108,7 @@ class Operation : public base::RefCountedThreadSafe<Operation> {
   // Adds a callback that will be called during clean-up, whether the operation
   // is aborted, encounters and error, or finishes successfully.  These
   // functions will be run on the FILE thread.
-  void AddCleanUpFunction(base::Closure);
-
-  void UnzipStart(scoped_ptr<base::FilePath> zip_file);
-  void WriteStart();
-  void VerifyWriteStart();
-  void Finish();
+  void AddCleanUpFunction(const base::Closure& callback);
 
   // If |file_size| is non-zero, only |file_size| bytes will be read from file,
   // otherwise the entire file will be read.
@@ -98,43 +117,45 @@ class Operation : public base::RefCountedThreadSafe<Operation> {
   // sum.  |progress_offset| is an percentage that will be added to the progress
   // of the MD5 sum before updating |progress_| but after scaling.
   void GetMD5SumOfFile(
-      scoped_ptr<base::FilePath> file,
+      const base::FilePath& file,
       int64 file_size,
       int progress_offset,
       int progress_scale,
-      const base::Callback<void(scoped_ptr<std::string>)>& callback);
+      const base::Callback<void(const std::string&)>& callback);
 
   base::WeakPtr<OperationManager> manager_;
   const ExtensionId extension_id_;
 
   base::FilePath image_path_;
-  const std::string storage_unit_id_;
+  base::FilePath device_path_;
 
-  // Whether or not to run the final verification step.
-  bool verify_write_;
+  // Temporary directory to store files as we go.
+  base::ScopedTempDir temp_dir_;
 
  private:
   friend class base::RefCountedThreadSafe<Operation>;
 
   // TODO(haven): Clean up these switches. http://crbug.com/292956
 #if defined(OS_LINUX) && !defined(CHROMEOS)
-  void WriteRun();
-  void WriteChunk(scoped_ptr<image_writer_utils::ImageReader> reader,
-                  scoped_ptr<image_writer_utils::ImageWriter> writer,
-                  int64 bytes_written);
-  bool WriteCleanUp(scoped_ptr<image_writer_utils::ImageReader> reader,
-                    scoped_ptr<image_writer_utils::ImageWriter> writer);
-  void WriteComplete();
-
-  void VerifyWriteStage2(scoped_ptr<std::string> image_hash);
-  void VerifyWriteCompare(scoped_ptr<std::string> image_hash,
-                          scoped_ptr<std::string> device_hash);
+  void WriteChunk(const int64& bytes_written,
+                  const int64& total_size,
+                  const base::Closure& continuation);
+  void WriteComplete(const base::Closure& continuation);
+
+  void VerifyWriteChunk(const int64& bytes_written,
+                        const int64& total_size,
+                        const base::Closure& continuation);
+  void VerifyWriteComplete(const base::Closure& continuation);
+
+  base::PlatformFile image_file_;
+  base::PlatformFile device_file_;
 #endif
 
 #if defined(OS_CHROMEOS)
-  void StartWriteOnUIThread();
+  void StartWriteOnUIThread(const base::Closure& continuation);
 
-  void OnBurnFinished(const std::string& target_path,
+  void OnBurnFinished(const base::Closure& continuation,
+                      const std::string& target_path,
                       bool success,
                       const std::string& error);
   void OnBurnProgress(const std::string& target_path,
@@ -144,15 +165,15 @@ class Operation : public base::RefCountedThreadSafe<Operation> {
 #endif
 
   // Incrementally calculates the MD5 sum of a file.
-  void MD5Chunk(scoped_ptr<image_writer_utils::ImageReader> reader,
+  void MD5Chunk(const base::PlatformFile& file,
                 int64 bytes_processed,
                 int64 bytes_total,
                 int progress_offset,
                 int progress_scale,
-                const base::Callback<void(scoped_ptr<std::string>)>& callback);
+                const base::Callback<void(const std::string&)>& callback);
 
   // Callbacks for zip::ZipReader.
-  void OnUnzipSuccess();
+  void OnUnzipSuccess(const base::Closure& continuation);
   void OnUnzipFailure();
   void OnUnzipProgress(int64 total_bytes, int64 progress_bytes);
 

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

@@ -31,49 +31,56 @@ void ClearImageBurner() {
 
 }  // namespace
 
-void Operation::WriteStart() {
+void Operation::Write(const base::Closure& continuation) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   SetStage(image_writer_api::STAGE_WRITE);
 
-  BrowserThread::PostTask(BrowserThread::UI,
-                          FROM_HERE,
-                          base::Bind(&Operation::StartWriteOnUIThread, this));
+  BrowserThread::PostTask(
+      BrowserThread::UI,
+      FROM_HERE,
+      base::Bind(&Operation::StartWriteOnUIThread, this, continuation));
 
   AddCleanUpFunction(base::Bind(&ClearImageBurner));
 }
 
-void Operation::StartWriteOnUIThread() {
+void Operation::VerifyWrite(const base::Closure& continuation) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+  // No verification is available in Chrome OS currently.
+  continuation.Run();
+}
+
+void Operation::StartWriteOnUIThread(const base::Closure& continuation) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DVLOG(1) << "Starting burn.";
 
   ImageBurnerClient* burner =
       chromeos::DBusThreadManager::Get()->GetImageBurnerClient();
 
   burner->SetEventHandlers(
-      base::Bind(&Operation::OnBurnFinished, this),
+      base::Bind(&Operation::OnBurnFinished, this, continuation),
       base::Bind(&Operation::OnBurnProgress, this));
 
   burner->BurnImage(image_path_.value(),
-                    storage_unit_id_,
+                    device_path_.value(),
                     base::Bind(&Operation::OnBurnError, this));
 }
 
-void Operation::OnBurnFinished(const std::string& target_path,
-                    bool success,
-                    const std::string& error) {
-  DVLOG(1) << "Burn finished: " << success;
-
+void Operation::OnBurnFinished(const base::Closure& continuation,
+                               const std::string& target_path,
+                               bool success,
+                               const std::string& error) {
   if (success) {
     SetProgress(kProgressComplete);
-    Finish();
+    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, continuation);
   } else {
-    Error(error);
+    DLOG(ERROR) << "Error encountered while burning: " << error;
+    Error(error::kChromeOSImageBurnerError);
   }
 }
 
 void Operation::OnBurnProgress(const std::string& target_path,
-                    int64 num_bytes_burnt,
-                    int64 total_size) {
+                               int64 num_bytes_burnt,
+                               int64 total_size) {
   int progress = kProgressComplete * num_bytes_burnt / total_size;
   SetProgress(progress);
 }

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

@@ -8,11 +8,11 @@
 namespace extensions {
 namespace image_writer {
 
-void Operation::WriteStart() {
+void Operation::Write(const base::Closure& continuation) {
   Error(error::kUnsupportedOperation);
 }
 
-void Operation::VerifyWriteStart() {
+void Operation::VerifyWrite(const base::Closure& continuation) {
   Error(error::kUnsupportedOperation);
 }
 

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

@@ -58,10 +58,8 @@ void OperationManager::Shutdown() {
 void OperationManager::StartWriteFromUrl(
     const ExtensionId& extension_id,
     GURL url,
-    content::RenderViewHost* rvh,
     const std::string& hash,
-    bool saveImageAsDownload,
-    const std::string& storage_unit_id,
+    const std::string& device_path,
     const Operation::StartWriteCallback& callback) {
   OperationMap::iterator existing_operation = operations_.find(extension_id);
 
@@ -72,11 +70,10 @@ void OperationManager::StartWriteFromUrl(
   scoped_refptr<Operation> operation(
       new WriteFromUrlOperation(weak_factory_.GetWeakPtr(),
                                 extension_id,
-                                rvh,
+                                profile_->GetRequestContext(),
                                 url,
                                 hash,
-                                saveImageAsDownload,
-                                storage_unit_id));
+                                device_path));
   operations_[extension_id] = operation;
   BrowserThread::PostTask(BrowserThread::FILE,
                           FROM_HERE,
@@ -87,7 +84,7 @@ void OperationManager::StartWriteFromUrl(
 void OperationManager::StartWriteFromFile(
     const ExtensionId& extension_id,
     const base::FilePath& path,
-    const std::string& storage_unit_id,
+    const std::string& device_path,
     const Operation::StartWriteCallback& callback) {
   OperationMap::iterator existing_operation = operations_.find(extension_id);
 
@@ -95,11 +92,8 @@ void OperationManager::StartWriteFromFile(
     return callback.Run(false, error::kOperationAlreadyInProgress);
   }
 
-  scoped_refptr<Operation> operation(
-      new WriteFromFileOperation(weak_factory_.GetWeakPtr(),
-                                 extension_id,
-                                 path,
-                                 storage_unit_id));
+  scoped_refptr<Operation> operation(new WriteFromFileOperation(
+      weak_factory_.GetWeakPtr(), extension_id, path, device_path));
   operations_[extension_id] = operation;
   BrowserThread::PostTask(BrowserThread::FILE,
                           FROM_HERE,
@@ -125,7 +119,7 @@ void OperationManager::CancelWrite(
 
 void OperationManager::DestroyPartitions(
     const ExtensionId& extension_id,
-    const std::string& storage_unit_id,
+    const std::string& device_path,
     const Operation::StartWriteCallback& callback) {
   OperationMap::iterator existing_operation = operations_.find(extension_id);
 
@@ -133,10 +127,8 @@ void OperationManager::DestroyPartitions(
     return callback.Run(false, error::kOperationAlreadyInProgress);
   }
 
-  scoped_refptr<Operation> operation(
-      new DestroyPartitionsOperation(weak_factory_.GetWeakPtr(),
-                                     extension_id,
-                                     storage_unit_id));
+  scoped_refptr<Operation> operation(new DestroyPartitionsOperation(
+      weak_factory_.GetWeakPtr(), extension_id, device_path));
   operations_[extension_id] = operation;
   BrowserThread::PostTask(BrowserThread::FILE,
                           FROM_HERE,
@@ -148,7 +140,6 @@ void OperationManager::OnProgress(const ExtensionId& extension_id,
                                   image_writer_api::Stage stage,
                                   int progress) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DVLOG(2) << "progress - " << stage << " at " << progress << "%";
 
   image_writer_api::ProgressInfo info;
   info.stage = stage;

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

@@ -45,16 +45,14 @@ class OperationManager
   // Starts a WriteFromUrl operation.
   void StartWriteFromUrl(const ExtensionId& extension_id,
                          GURL url,
-                         content::RenderViewHost* rvh,
                          const std::string& hash,
-                         bool saveImageAsDownload,
-                         const std::string& storage_unit_id,
+                         const std::string& device_path,
                          const Operation::StartWriteCallback& callback);
 
   // Starts a WriteFromFile operation.
   void StartWriteFromFile(const ExtensionId& extension_id,
                           const base::FilePath& path,
-                          const std::string& storage_unit_id,
+                          const std::string& device_path,
                           const Operation::StartWriteCallback& callback);
 
   // Cancels the extensions current operation if any.
@@ -63,7 +61,7 @@ class OperationManager
 
   // Starts a write that removes the partition table.
   void DestroyPartitions(const ExtensionId& extension_id,
-                         const std::string& storage_unit_id,
+                         const std::string& device_path,
                          const Operation::StartWriteCallback& callback);
 
   // Callback for progress events.

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

@@ -8,11 +8,11 @@
 namespace extensions {
 namespace image_writer {
 
-void Operation::WriteStart() {
+void Operation::Write(const base::Closure& continuation) {
   Error(error::kUnsupportedOperation);
 }
 
-void Operation::VerifyWriteStart() {
+void Operation::VerifyWrite(const base::Closure& continuation) {
   Error(error::kUnsupportedOperation);
 }
 

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

@@ -45,16 +45,18 @@ class ImageWriterFakeImageBurnerClient
  private:
   BurnFinishedHandler burn_finished_handler_;
   BurnProgressUpdateHandler burn_progress_update_handler_;
-
 };
 
 } // namespace
 #endif
 
 MockOperationManager::MockOperationManager() : OperationManager(NULL) {}
+MockOperationManager::MockOperationManager(Profile* profile)
+    : OperationManager(profile) {}
 MockOperationManager::~MockOperationManager() {}
 
-ImageWriterUnitTestBase::ImageWriterUnitTestBase() {}
+ImageWriterUnitTestBase::ImageWriterUnitTestBase()
+    : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP) {}
 ImageWriterUnitTestBase::~ImageWriterUnitTestBase() {}
 
 void ImageWriterUnitTestBase::SetUp() {
@@ -87,31 +89,6 @@ void ImageWriterUnitTestBase::TearDown() {
 #endif
 }
 
-bool ImageWriterUnitTestBase::CompareImageAndDevice() {
-  scoped_ptr<char[]> image_buffer(new char[kTestFileSize]);
-  scoped_ptr<char[]> device_buffer(new char[kTestFileSize]);
-
-  while (true) {
-    int image_bytes_read = ReadFile(test_image_path_,
-                                    image_buffer.get(),
-                                    kTestFileSize);
-    int device_bytes_read = ReadFile(test_device_path_,
-                                     device_buffer.get(),
-                                     kTestFileSize);
-
-    if (image_bytes_read != device_bytes_read)
-      return false;
-
-    if (image_bytes_read == 0)
-      return true;
-
-    if (memcmp(image_buffer.get(), device_buffer.get(), image_bytes_read) != 0)
-      return false;
-  }
-
-  return false;
-}
-
 bool ImageWriterUnitTestBase::FillFile(const base::FilePath& file,
                                        const int pattern,
                                        const int length) {

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

@@ -31,6 +31,7 @@ const int kDevicePattern = 0xAAAAAAAA; // 10101010
 class MockOperationManager : public OperationManager {
  public:
   MockOperationManager();
+  explicit MockOperationManager(Profile* profile);
   virtual ~MockOperationManager();
 
   MOCK_METHOD3(OnProgress, void(const ExtensionId& extension_id,
@@ -57,21 +58,17 @@ class ImageWriterUnitTestBase : public testing::Test {
 
   virtual void TearDown() OVERRIDE;
 
-  // Compare the image and device files, returning true if they are the same,
-  // false if different.
-  bool CompareImageAndDevice();
-
-  base::ScopedTempDir temp_dir_;
-  base::FilePath test_image_path_;
-  base::FilePath test_device_path_;
-
- private:
   // Fills |file| with |length| bytes of |pattern|, overwriting any existing
   // data.
   bool FillFile(const base::FilePath& file,
                 const int pattern,
                 const int length);
 
+  base::ScopedTempDir temp_dir_;
+  base::FilePath test_image_path_;
+  base::FilePath test_device_path_;
+
+ private:
   content::TestBrowserThreadBundle thread_bundle_;
 };
 

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

@@ -15,25 +15,27 @@ using content::BrowserThread;
 WriteFromFileOperation::WriteFromFileOperation(
     base::WeakPtr<OperationManager> manager,
     const ExtensionId& extension_id,
-    const base::FilePath& path,
-    const std::string& storage_unit_id)
-  : Operation(manager, extension_id, storage_unit_id) {
-    image_path_ = path;
+    const base::FilePath& user_file_path,
+    const std::string& device_path)
+    : Operation(manager, extension_id, device_path) {
+  image_path_ = user_file_path;
 }
 
-WriteFromFileOperation::~WriteFromFileOperation() {
-}
-
-void WriteFromFileOperation::Start() {
-  DVLOG(1) << "Starting file-to-usb write of " << image_path_.value()
-           << " to " << storage_unit_id_;
+WriteFromFileOperation::~WriteFromFileOperation() {}
 
+void WriteFromFileOperation::StartImpl() {
   if (!base::PathExists(image_path_) || base::DirectoryExists(image_path_)) {
+    DLOG(ERROR) << "Source must exist and not be a directory.";
     Error(error::kImageInvalid);
     return;
   }
 
-  WriteStart();
+  Unzip(base::Bind(
+      &WriteFromFileOperation::Write,
+      this,
+      base::Bind(&WriteFromFileOperation::VerifyWrite,
+                 this,
+                 base::Bind(&WriteFromFileOperation::Finish, this))));
 }
 
 }  // namespace image_writer

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

@@ -15,9 +15,9 @@ class WriteFromFileOperation : public Operation {
  public:
   WriteFromFileOperation(base::WeakPtr<OperationManager> manager,
                          const ExtensionId& extension_id,
-                         const base::FilePath& path,
+                         const base::FilePath& user_file_path,
                          const std::string& storage_unit_id);
-  virtual void Start() OVERRIDE;
+  virtual void StartImpl() OVERRIDE;
 
  private:
   virtual ~WriteFromFileOperation();

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

@@ -40,5 +40,47 @@ TEST_F(ImageWriterFromFileTest, InvalidFile) {
   base::RunLoop().RunUntilIdle();
 }
 
+#if defined(OS_LINUX) && !defined(OS_CHROMEOS)
+TEST_F(ImageWriterFromFileTest, WriteFromFileEndToEnd) {
+  MockOperationManager manager;
+
+  scoped_refptr<WriteFromFileOperation> op =
+      new WriteFromFileOperation(manager.AsWeakPtr(),
+                                 kDummyExtensionId,
+                                 test_image_path_,
+                                 test_device_path_.AsUTF8Unsafe());
+
+  EXPECT_CALL(manager,
+              OnProgress(kDummyExtensionId, image_writer_api::STAGE_WRITE, _))
+      .Times(AnyNumber());
+  EXPECT_CALL(manager,
+              OnProgress(kDummyExtensionId, image_writer_api::STAGE_WRITE, 0))
+      .Times(AtLeast(1));
+  EXPECT_CALL(manager,
+              OnProgress(kDummyExtensionId, image_writer_api::STAGE_WRITE, 100))
+      .Times(AtLeast(1));
+  EXPECT_CALL(
+      manager,
+      OnProgress(kDummyExtensionId, image_writer_api::STAGE_VERIFYWRITE, _))
+      .Times(AnyNumber());
+  EXPECT_CALL(
+      manager,
+      OnProgress(kDummyExtensionId, image_writer_api::STAGE_VERIFYWRITE, 0))
+      .Times(AtLeast(1));
+  EXPECT_CALL(
+      manager,
+      OnProgress(kDummyExtensionId, image_writer_api::STAGE_VERIFYWRITE, 100))
+      .Times(AtLeast(1));
+  EXPECT_CALL(manager, OnComplete(kDummyExtensionId)).Times(1);
+  EXPECT_CALL(manager, OnError(kDummyExtensionId, _, _, _)).Times(0);
+
+  op->Start();
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(base::ContentsEqual(test_image_path_, test_device_path_));
+}
+#endif
+
 }  // namespace image_writer
 }  // namespace extensions

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

@@ -5,17 +5,13 @@
 #ifndef CHROME_BROWSER_EXTENSIONS_API_IMAGE_WRITER_PRIVATE_WRITE_FROM_URL_OPERATION_H_
 #define CHROME_BROWSER_EXTENSIONS_API_IMAGE_WRITER_PRIVATE_WRITE_FROM_URL_OPERATION_H_
 
-#include "base/scoped_observer.h"
 #include "chrome/browser/extensions/api/image_writer_private/operation.h"
-#include "content/public/browser/download_interrupt_reasons.h"
-#include "content/public/browser/download_item.h"
+#include "net/url_request/url_fetcher_delegate.h"
 #include "url/gurl.h"
 
-namespace content {
-
-class RenderViewHost;
-
-}  // namespace content
+namespace net {
+class URLFetcher;
+}  // namespace net
 
 namespace extensions {
 namespace image_writer {
@@ -23,44 +19,57 @@ namespace image_writer {
 class OperationManager;
 
 // Encapsulates a write of an image accessed via URL.
-class WriteFromUrlOperation : public Operation,
-                              public content::DownloadItem::Observer {
+class WriteFromUrlOperation : public Operation, public net::URLFetcherDelegate {
  public:
   WriteFromUrlOperation(base::WeakPtr<OperationManager> manager,
                         const ExtensionId& extension_id,
-                        content::RenderViewHost* rvh,
+                        net::URLRequestContextGetter* request_context,
                         GURL url,
                         const std::string& hash,
-                        bool saveImageAsDownload,
                         const std::string& storage_unit_id);
-  virtual void Start() OVERRIDE;
- private:
+  virtual void StartImpl() OVERRIDE;
+
+ protected:
   virtual ~WriteFromUrlOperation();
-  void CreateTempFile();
 
-  void DownloadStart();
-  void OnDownloadStarted(content::DownloadItem*,
-                         content::DownloadInterruptReason interrupt_reason);
-  virtual void OnDownloadUpdated(content::DownloadItem* download) OVERRIDE;
-  void DownloadComplete();
-  void DownloadCleanUp();
+  // Sets the image_path to the correct location to download to.
+  void GetDownloadTarget(const base::Closure& continuation);
+
+  // Downloads the |url| to the currently configured |image_path|.  Should not
+  // be called without calling |GetDownloadTarget| first.
+  void Download(const base::Closure& continuation);
+
+  // Verifies the download matches |hash|.  If the hash is empty, this stage is
+  // skipped.
+  void VerifyDownload(const base::Closure& continuation);
+
+ private:
+  // Destroys the URLFetcher.  The URLFetcher needs to be destroyed on the same
+  // thread it was created on.  The Operation may be deleted on the UI thread
+  // and so we must first delete the URLFetcher on the FILE thread.
+  void DestroyUrlFetcher();
+
+  // URLFetcherDelegate implementation.
+  virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE;
+  virtual void OnURLFetchDownloadProgress(const net::URLFetcher* source,
+                                          int64 current,
+                                          int64 total) OVERRIDE;
+  virtual void OnURLFetchUploadProgress(const net::URLFetcher* source,
+                                        int64 current,
+                                        int64 total) OVERRIDE;
 
-  void VerifyDownloadStart();
-  void VerifyDownloadRun();
-  void VerifyDownloadCompare(scoped_ptr<std::string> download_hash);
-  void VerifyDownloadComplete();
+  void VerifyDownloadCompare(const base::Closure& continuation,
+                             const std::string& download_hash);
+  void VerifyDownloadComplete(const base::Closure& continuation);
 
   // Arguments
-  content::RenderViewHost* rvh_;
+  net::URLRequestContextGetter* request_context_;
   GURL url_;
   const std::string hash_;
-  const bool saveImageAsDownload_;
 
   // Local state
-  scoped_ptr<base::FilePath> tmp_file_;
-  bool download_stopped_;
-  content::DownloadItem* download_;
-  base::FilePath download_path_;
+  scoped_ptr<net::URLFetcher> url_fetcher_;
+  base::Closure download_continuation_;
 };
 
 } // namespace image_writer

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

+// Copyright 2014 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/error_messages.h"
+#include "chrome/browser/extensions/api/image_writer_private/test_utils.h"
+#include "chrome/browser/extensions/api/image_writer_private/write_from_url_operation.h"
+#include "chrome/test/base/testing_profile.h"
+#include "content/test/net/url_request_prepackaged_interceptor.h"
+#include "net/url_request/url_fetcher.h"
+
+namespace extensions {
+namespace image_writer {
+
+namespace {
+
+using testing::_;
+using testing::AnyNumber;
+using testing::AtLeast;
+using testing::Gt;
+using testing::Lt;
+
+const char kTestImageUrl[] = "http://localhost/test/image.zip";
+
+typedef content::URLLocalHostRequestPrepackagedInterceptor GetInterceptor;
+
+// This class gives us a generic Operation with the ability to set or inspect
+// the current path to the image file.
+class OperationForTest : public WriteFromUrlOperation {
+ public:
+  OperationForTest(base::WeakPtr<OperationManager> manager_,
+                   const ExtensionId& extension_id,
+                   net::URLRequestContextGetter* request_context,
+                   GURL url,
+                   const std::string& hash,
+                   const std::string& storage_unit_id)
+      : WriteFromUrlOperation(manager_,
+                              extension_id,
+                              request_context,
+                              url,
+                              hash,
+                              storage_unit_id) {}
+
+  virtual void StartImpl() OVERRIDE {}
+
+  // Expose stages for testing.
+  void GetDownloadTarget(const base::Closure& continuation) {
+    WriteFromUrlOperation::GetDownloadTarget(continuation);
+  }
+
+  void Download(const base::Closure& continuation) {
+    WriteFromUrlOperation::Download(continuation);
+  }
+
+  void VerifyDownload(const base::Closure& continuation) {
+    WriteFromUrlOperation::VerifyDownload(continuation);
+  }
+
+  // Helpers to set-up state for intermediate stages.
+  void SetImagePath(const base::FilePath image_path) {
+    image_path_ = image_path;
+  }
+
+  base::FilePath GetImagePath() { return image_path_; }
+
+ private:
+  virtual ~OperationForTest() {};
+};
+
+class ImageWriterWriteFromUrlOperationTest : public ImageWriterUnitTestBase {
+ protected:
+  ImageWriterWriteFromUrlOperationTest() : manager_(&test_profile_) {}
+
+  virtual void SetUp() OVERRIDE {
+    ImageWriterUnitTestBase::SetUp();
+
+    // Turn on interception and set up our dummy file.
+    net::URLFetcher::SetEnableInterceptionForTests(true);
+    get_interceptor_.reset(new GetInterceptor());
+    get_interceptor_->SetResponse(GURL(kTestImageUrl), test_image_path_);
+  }
+
+  virtual void TearDown() OVERRIDE {
+    ImageWriterUnitTestBase::TearDown();
+
+    // Remember to turn off global interception.
+    net::URLFetcher::SetEnableInterceptionForTests(false);
+  }
+
+  scoped_refptr<OperationForTest> CreateOperation(const GURL& url,
+                                                  const std::string& hash) {
+    scoped_refptr<OperationForTest> operation(
+        new OperationForTest(manager_.AsWeakPtr(),
+                             kDummyExtensionId,
+                             test_profile_.GetRequestContext(),
+                             url,
+                             hash,
+                             test_device_path_.AsUTF8Unsafe()));
+    operation->Start();
+    return operation;
+  }
+
+  TestingProfile test_profile_;
+  scoped_ptr<GetInterceptor> get_interceptor_;
+
+  MockOperationManager manager_;
+};
+
+TEST_F(ImageWriterWriteFromUrlOperationTest, SelectTargetWithoutExtension) {
+  scoped_refptr<OperationForTest> operation =
+      CreateOperation(GURL("http://localhost/foo/bar"), "");
+
+  operation->GetDownloadTarget(base::Bind(&base::DoNothing));
+
+  EXPECT_EQ(FILE_PATH_LITERAL("bar"),
+            operation->GetImagePath().BaseName().value());
+
+  operation->Cancel();
+}
+
+TEST_F(ImageWriterWriteFromUrlOperationTest, SelectTargetWithExtension) {
+  scoped_refptr<OperationForTest> operation =
+      CreateOperation(GURL("http://localhost/foo/bar.zip"), "");
+
+  operation->GetDownloadTarget(base::Bind(&base::DoNothing));
+
+  EXPECT_EQ(FILE_PATH_LITERAL("bar.zip"),
+            operation->GetImagePath().BaseName().value());
+
+  operation->Cancel();
+}
+
+TEST_F(ImageWriterWriteFromUrlOperationTest, DownloadFile) {
+  // This test actually triggers the URL fetch code, which will drain the
+  // message queues while waiting for IO, thus we have to run until the
+  // operation completes.
+  base::RunLoop runloop;
+  base::Closure quit_closure = runloop.QuitClosure();
+  base::FilePath download_target_path;
+  scoped_refptr<OperationForTest> operation =
+      CreateOperation(GURL(kTestImageUrl), "");
+
+  EXPECT_TRUE(
+      base::CreateTemporaryFileInDir(temp_dir_.path(), &download_target_path));
+  operation->SetImagePath(download_target_path);
+
+  EXPECT_CALL(
+      manager_,
+      OnProgress(kDummyExtensionId, image_writer_api::STAGE_DOWNLOAD, _))
+      .Times(AtLeast(1));
+  EXPECT_CALL(
+      manager_,
+      OnProgress(kDummyExtensionId, image_writer_api::STAGE_DOWNLOAD, 0))
+      .Times(AnyNumber());
+  EXPECT_CALL(
+      manager_,
+      OnProgress(kDummyExtensionId, image_writer_api::STAGE_DOWNLOAD, 100))
+      .Times(AnyNumber());
+
+  content::BrowserThread::PostTask(
+      content::BrowserThread::FILE,
+      FROM_HERE,
+      base::Bind(&OperationForTest::Download, operation, quit_closure));
+
+  runloop.Run();
+
+  EXPECT_TRUE(base::ContentsEqual(test_image_path_, operation->GetImagePath()));
+
+  EXPECT_EQ(1, get_interceptor_->GetHitCount());
+
+  operation->Cancel();
+}
+
+TEST_F(ImageWriterWriteFromUrlOperationTest, VerifyFile) {
+  scoped_ptr<char[]> data_buffer(new char[kTestFileSize]);
+  base::ReadFile(test_image_path_, data_buffer.get(), kTestFileSize);
+  base::MD5Digest expected_digest;
+  base::MD5Sum(data_buffer.get(), kTestFileSize, &expected_digest);
+  std::string expected_hash = base::MD5DigestToBase16(expected_digest);
+
+  scoped_refptr<OperationForTest> operation =
+      CreateOperation(GURL(""), expected_hash);
+
+  EXPECT_CALL(
+      manager_,
+      OnProgress(kDummyExtensionId, image_writer_api::STAGE_VERIFYDOWNLOAD, _))
+      .Times(AtLeast(1));
+  EXPECT_CALL(
+      manager_,
+      OnProgress(kDummyExtensionId, image_writer_api::STAGE_VERIFYDOWNLOAD, 0))
+      .Times(AtLeast(1));
+  EXPECT_CALL(manager_,
+              OnProgress(kDummyExtensionId,
+                         image_writer_api::STAGE_VERIFYDOWNLOAD,
+                         100)).Times(AtLeast(1));
+
+  operation->SetImagePath(test_image_path_);
+  content::BrowserThread::PostTask(content::BrowserThread::FILE,
+                                   FROM_HERE,
+                                   base::Bind(&OperationForTest::VerifyDownload,
+                                              operation,
+                                              base::Bind(&base::DoNothing)));
+
+  base::RunLoop().RunUntilIdle();
+
+  operation->Cancel();
+}
+
+}  // namespace
+
+}  // namespace image_writer
+}  // namespace extensions

chrome/chrome_browser_extensions.gypi

@@ -337,8 +337,6 @@
         'browser/extensions/api/image_writer_private/operation_win.cc',
         'browser/extensions/api/image_writer_private/image_writer_private_api.cc',
         'browser/extensions/api/image_writer_private/image_writer_private_api.h',
-        'browser/extensions/api/image_writer_private/image_writer_utils.cc',
-        'browser/extensions/api/image_writer_private/image_writer_utils.h',
         'browser/extensions/api/image_writer_private/removable_storage_provider.h',
         'browser/extensions/api/image_writer_private/removable_storage_provider_linux.cc',
         'browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc',

chrome/chrome_tests_unit.gypi

@@ -869,6 +869,7 @@
         'browser/extensions/api/image_writer_private/operation_unittest.cc',
         'browser/extensions/api/image_writer_private/test_utils.cc',
         'browser/extensions/api/image_writer_private/write_from_file_operation_unittest.cc',
+        'browser/extensions/api/image_writer_private/write_from_url_operation_unittest.cc',
         'browser/extensions/api/log_private/syslog_parser_unittest.cc',
         'browser/extensions/api/mdns/dns_sd_registry_unittest.cc',
         'browser/extensions/api/messaging/native_message_process_host_unittest.cc',