Test file errors in downloads.

content/test/test_file_error_injector.cc

+// Copyright (c) 2011 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 "content/test/test_file_error_injector.h"
+
+#include <map>
+#include <vector>
+
+#include "base/compiler_specific.h"
+#include "base/logging.h"
+#include "content/browser/download/download_file_impl.h"
+#include "content/browser/download/download_file_manager.h"
+#include "content/browser/renderer_host/resource_dispatcher_host.h"
+
+class TestFileErrorInjectorImpl;
+
+namespace {
+
+// Structure that encapsulates the information needed to inject a file error.
+struct FileErrorInfo {

[cbentzel, 2012/02/27 02:18:54]

This is kind of duplicated in the browsertest. Perhaps expose in the .h and have
only one version?

[ahendrickson, 2012/02/28 03:28:56]

Done.

+  FileErrorInfo()
+      : file_index(-1),
+        code(content::FILE_OPERATION_INITIALIZE),
+        operation_instance(-1),
+        net_error(net::OK) {
+  }
+
+  int file_index;  // 0-based index into created DownloadFile instances.
+  content::FileOperationCode code;  // Operation to affect.
+  int operation_instance;  // 0-based count of operation calls.
+  net::Error net_error;  // Error to inject.
+};
+
+DownloadFileManager* GetDownloadFileManager() {
+  ResourceDispatcherHost* rdh = ResourceDispatcherHost::Get();
+  DCHECK(rdh != NULL);

[cbentzel, 2012/02/27 02:18:54]

What cases could it be NULL? 

[ahendrickson, 2012/02/28 03:28:56]

There are tests (perhaps just unit tests) where RDH is NULL.  I'm just
double-checking that it is available.

Should I remove the DCHECK?

+  return rdh->download_file_manager();
+}
+
+std::string DebugString(content::FileOperationCode code) {

[cbentzel, 2012/02/27 02:18:54]

Is this needed?

[ahendrickson, 2012/02/28 03:28:56]

It's used below.  I think it's useful to trace error injections in tests.

+
+#define TO_STRING(code)  case content::FILE_OPERATION_##code: return #code
+
+  switch (code) {
+    TO_STRING(INITIALIZE);
+    TO_STRING(WRITE);
+    TO_STRING(RENAME);
+    default:
+      break;
+  }
+
+#undef TO_STRING
+
+  return "Unknown";
+}
+
+}  // namespace
+
+namespace content {
+
+// A class that performs file operations and injects errors.
+class DownloadFileWithErrors: public DownloadFileImpl {

[cbentzel, 2012/02/27 02:18:54]

Could this be in an anonymous nested namespace in content?

[ahendrickson, 2012/02/28 03:28:56]

Done.

+ public:
+  DownloadFileWithErrors(const DownloadCreateInfo* info,
+                         DownloadRequestHandleInterface* request_handle,
+                         content::DownloadManager* download_manager,
+                         bool calculate_hash,
+                         const net::BoundNetLog& bound_net_log,
+                         const FileErrorInfo error_info,
+                         TestFileErrorInjectorImpl* injector);
+
+  ~DownloadFileWithErrors();
+
+  // DownloadFile interface.
+  virtual net::Error Initialize() OVERRIDE;
+  virtual net::Error AppendDataToFile(const char* data,
+                                      size_t data_len) OVERRIDE;
+  virtual net::Error Rename(const FilePath& full_path) OVERRIDE;
+
+  // For |TestFileErrorInjectorImpl| to record lifetime.
+  int index() const { return error_info_.file_index; }
+
+ private:
+  // Error generating helper.
+  net::Error ShouldReturnError(FileOperationCode code,
+                               net::Error original_net_error);
+
+  // The class that injects errors.
+  TestFileErrorInjectorImpl* injector_;  // Needed for destructor.
+
+  // Our injected error.  Only one per file.
+  FileErrorInfo error_info_;
+
+  // Count per operation.  0-based.
+  std::map<FileOperationCode, int> operation_counter_;
+};
+
+// A factory for constructing DownloadFiles that inject errors.
+class DownloadFileWithErrorsFactory
+    : public DownloadFileManager::DownloadFileFactory {
+ public:
+
+  explicit DownloadFileWithErrorsFactory(TestFileErrorInjectorImpl* injector);
+  virtual ~DownloadFileWithErrorsFactory();
+
+  // DownloadFileFactory interface.
+  virtual content::DownloadFile* CreateFile(
+      DownloadCreateInfo* info,
+      const DownloadRequestHandle& request_handle,
+      content::DownloadManager* download_manager,
+      bool calculate_hash,
+      const net::BoundNetLog& bound_net_log);
+
+  bool InjectError(int file_index,
+                   content::FileOperationCode operation,
+                   int operation_instance,
+                   net::Error net_error);
+
+  size_t files_created() const { return file_counter_; }
+
+ private:
+  // The number of files we've created.
+  int file_counter_;
+
+  // We pass this to DownloadFileWithErrors, for lifetime tracking.
+  // We own this.
+  TestFileErrorInjectorImpl* injector_;

[cbentzel, 2012/02/26 15:01:48]

scoped_ptr is much clearer declaration of ownership. 

[ahendrickson, 2012/02/28 03:28:56]

Now using RefCountedThreadSafe.

+
+  // Our injected error list, mapped by file index.  One per file.
+  std::map<int, FileErrorInfo> injected_errors_;
+};
+
+}  // namespace content
+
+class TestFileErrorInjectorImpl : public content::TestFileErrorInjector {
+ public:
+  TestFileErrorInjectorImpl();
+  virtual ~TestFileErrorInjectorImpl();
+
+  // content::TestFileErrorInjector interface.
+  virtual bool InjectError(int file_index,
+                           content::FileOperationCode operation,
+                           int operation_instance,
+                           net::Error net_error) OVERRIDE;
+  virtual size_t CurrentFileCount() const OVERRIDE;
+  virtual size_t FileCreationCount() const OVERRIDE;
+  virtual bool HasFile(int file_index) const OVERRIDE;
+
+  // Callbacks from the download file, to record lifetimes.
+  virtual void DownloadFileCreated(
+      content::DownloadFileWithErrors* download_file);
+  virtual void DestroyingDownloadFile(
+      content::DownloadFileWithErrors* download_file);
+
+ private:
+  typedef std::map<int, content::DownloadFileWithErrors*> FileMap;
+
+  // Keep track of active DownloadFiles.
+  FileMap files_;
+
+  content::DownloadFileWithErrorsFactory* download_file_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestFileErrorInjectorImpl);
+};
+
+
+// Implementations.
+namespace content {
+
+DownloadFileWithErrors::DownloadFileWithErrors(
+    const DownloadCreateInfo* info,
+    DownloadRequestHandleInterface* request_handle,
+    content::DownloadManager* download_manager,
+    bool calculate_hash,
+    const net::BoundNetLog& bound_net_log,
+    const FileErrorInfo error_info,
+    TestFileErrorInjectorImpl* injector)
+        : DownloadFileImpl(info,
+                           request_handle,
+                           download_manager,
+                           calculate_hash,
+                           bound_net_log),
+          injector_(injector),
+          error_info_(error_info) {
+  // Record creation.
+  injector_->DownloadFileCreated(this);

[cbentzel, 2012/02/27 02:18:54]

Are you worried at all about thread-safety issues on the injector_ here? What
threads are these done on?

[ahendrickson, 2012/02/28 03:28:56]

Switched to using RefCountedThreadSafe for the injector, and passing a callback
to the DownloadFileWithErrors class that posts information to the injector on
the UI thread.

DownloadFiles are always created on the FILE thread.

+}
+
+DownloadFileWithErrors::~DownloadFileWithErrors() {
+  // Record destruction.
+  injector_->DestroyingDownloadFile(this);
+}
+
+
+net::Error DownloadFileWithErrors::Initialize() {
+  return ShouldReturnError(FILE_OPERATION_INITIALIZE,
+                           DownloadFileImpl::Initialize());
+}
+
+net::Error DownloadFileWithErrors::AppendDataToFile(const char* data,
+                                                    size_t data_len) {
+  return ShouldReturnError(FILE_OPERATION_WRITE,
+                           DownloadFileImpl::AppendDataToFile(data, data_len));
+}
+
+net::Error DownloadFileWithErrors::Rename(const FilePath& full_path) {
+  return ShouldReturnError(FILE_OPERATION_RENAME,
+                           DownloadFileImpl::Rename(full_path));
+}
+
+net::Error DownloadFileWithErrors::ShouldReturnError(
+    FileOperationCode code,
+    net::Error original_net_error) {
+  int counter = operation_counter_[code];

[cbentzel, 2012/02/27 02:18:54]

Looks like the operation_instance is always 0 when used. Could remove this
logic. Also, only really needed if code == error_info_[code]

[ahendrickson, 2012/02/28 03:28:56]

Would prefer not to, as I have a use for this in mind, and it doesn't add much
complexity.

However, if you feel strongly about it, I will.

+  ++operation_counter_[code];
+
+  if (code != error_info_.code)
+    return original_net_error;  // No error for this operation.

[cbentzel, 2012/02/27 02:18:54]

Comments not really needed and a bit unclear actually. 

[ahendrickson, 2012/02/28 03:28:56]

Removed.

+
+  if (counter != error_info_.operation_instance)
+    return original_net_error;  // No error for this instance of the operation.
+
+  DVLOG(20) << " " << __FUNCTION__ << "()"
+            << " code = " << ::DebugString(code) << " (" << code << ")"
+            << " counter = " << counter
+            << " original_error = " << original_net_error
+            << " new error = " << error_info_.net_error;
+
+  return error_info_.net_error;  // Error!
+}
+
+DownloadFileWithErrorsFactory::DownloadFileWithErrorsFactory(
+    TestFileErrorInjectorImpl* injector)

[cbentzel, 2012/02/27 02:18:54]

should be passed in via scoped_ptr if the Factory owns it. Still feels weird to
own it, though.

[ahendrickson, 2012/02/28 03:28:56]

Now using RefCountedThreadSafe for the injector.

+        : file_counter_(0), injector_(injector) {
+}
+
+DownloadFileWithErrorsFactory::~DownloadFileWithErrorsFactory() {
+  delete injector_;
+  injector_ = NULL;
+}
+
+content::DownloadFile* DownloadFileWithErrorsFactory::CreateFile(
+    DownloadCreateInfo* info,
+    const DownloadRequestHandle& request_handle,
+    content::DownloadManager* download_manager,
+    bool calculate_hash,
+    const net::BoundNetLog& bound_net_log) {
+  // Creates entry if it doesn't exist.
+  injected_errors_[file_counter_].file_index = file_counter_; // Set index.
+  return new DownloadFileWithErrors(info,
+                                    new DownloadRequestHandle(request_handle),
+                                    download_manager,
+                                    calculate_hash,
+                                    bound_net_log,
+                                    injected_errors_[file_counter_++],
+                                    injector_);
+}
+
+bool DownloadFileWithErrorsFactory::InjectError(
+    int file_index,
+    content::FileOperationCode operation,
+    int operation_instance,
+    net::Error net_error) {
+  DCHECK_LE(0, file_index);
+  DCHECK_LE(0, operation_instance);
+
+  // Fill in a new entry.
+  FileErrorInfo info;
+  info.file_index = file_index;
+  info.code = operation;
+  info.operation_instance = operation_instance;
+  info.net_error = net_error;
+
+  // Creates an empty entry if necessary.
+  injected_errors_[file_index] = info;
+
+  return true;
+}
+
+}  // namespace content
+
+
+// static
+content::TestFileErrorInjector* content::TestFileErrorInjector::Create() {
+  return new TestFileErrorInjectorImpl;
+}
+
+TestFileErrorInjectorImpl::TestFileErrorInjectorImpl()
+    : download_file_factory_(new content::DownloadFileWithErrorsFactory(this)) {
+  // |DownloadFileWithErrorsFactory| now owns |TestFileErrorInjectorImpl|.
+  DownloadFileManager* download_file_manager =
+      GetDownloadFileManager();
+  DCHECK(download_file_manager);
+  // Transfers ownership to |DownloadFileManager|.
+  download_file_manager->SetFileFactory(download_file_factory_);
+}
+
+TestFileErrorInjectorImpl::~TestFileErrorInjectorImpl() {
+}
+
+bool TestFileErrorInjectorImpl::InjectError(
+    int file_index,
+    content::FileOperationCode operation,
+    int operation_instance,
+    net::Error net_error) {
+  return download_file_factory_->InjectError(file_index,
+                                             operation,
+                                             operation_instance,
+                                             net_error);
+}
+
+size_t TestFileErrorInjectorImpl::CurrentFileCount() const {
+  return files_.size();

[cbentzel, 2012/02/27 02:18:54]

What is the thread-safety here?

[ahendrickson, 2012/02/28 03:28:56]

All operations on files_ now happen on the UI thread.

+}
+
+size_t TestFileErrorInjectorImpl::FileCreationCount() const {
+  return download_file_factory_->files_created();
+}
+
+
+bool TestFileErrorInjectorImpl::HasFile(int file_index) const {

[cbentzel, 2012/02/27 02:18:54]

You aren't using this. Would also remove the need for a map of files_, just need
a counter.

[ahendrickson, 2012/02/28 03:28:56]

Now using it.

+  return (files_.find(file_index) != files_.end());
+}
+
+void TestFileErrorInjectorImpl::DownloadFileCreated(
+    content::DownloadFileWithErrors* download_file) {
+  DCHECK(download_file != NULL);
+
+  files_[download_file->index()] = download_file;
+}
+
+void TestFileErrorInjectorImpl::DestroyingDownloadFile(
+    content::DownloadFileWithErrors* download_file) {
+  DCHECK(download_file != NULL);
+
+  files_.erase(download_file->index());
+}