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/public/browser/browser_thread.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"
+
+namespace {
+
+class DownloadFileWithErrors;
+class DownloadFileWithErrorsFactory;
+
+typedef std::map<int, content::TestFileErrorInjector::FileErrorInfo> ErrorMap;
+
+DownloadFileManager* GetDownloadFileManager() {
+  ResourceDispatcherHost* rdh = ResourceDispatcherHost::Get();
+  DCHECK(rdh != NULL);
+  return rdh->download_file_manager();
+}
+
+class TestFileErrorInjectorImpl : public content::TestFileErrorInjector {
+ public:
+  typedef base::Callback<void(int index, bool creating)> DownloadFileCallback;

[cbentzel, 2012/02/28 14:51:41]

Design nit: You typically want this on the DownloadFileWithErrors, not on the
TestFileErrorInjectorImpl. The DownloadFileWithErrors class is the one that gets
the callback, as well as operates on it. TestFileErrorInjectorImpl simply binds
one of it's functions to be used with it.

[ahendrickson, 2012/03/01 09:17:32]

I wanted this class to be the first one declared.

My choices are:
- rearrange the classes and move the typedef,
- make the typedef external to the classes, or
- leave the typedef here.

+
+  TestFileErrorInjectorImpl();
+
+  // content::TestFileErrorInjector interface.
+  virtual bool AddError(const FileErrorInfo& error_info) OVERRIDE;
+  virtual bool InjectErrors() 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.

[Randy Smith (Not in Mondays), 2012/02/28 22:06:13]

nit: Aren't these callbacks from the factory, not the file?

[ahendrickson, 2012/03/01 09:17:32]

No, the factory passes the callback objects to the files, which then use them.

[Randy Smith (Not in Mondays), 2012/03/01 20:28:24]

Right, got it--sorry for the confusion.

+  virtual void DownloadFileCreated(int file_index);

[cbentzel, 2012/02/28 14:51:41]

These don't need to be virtual. Could also be private.

[ahendrickson, 2012/03/01 09:17:32]

Done.

+  virtual void DestroyingDownloadFile(int file_index);
+
+ private:
+  virtual ~TestFileErrorInjectorImpl();
+
+  void RecordDownloadFileConstructionDestruction(int index, bool creating);
+
+  typedef std::set<int> FileSet;
+
+  // Our injected error list, mapped by file index.  One per file.

[Randy Smith (Not in Mondays), 2012/02/28 22:06:13]

This is the first time I noticed that we could only have one injected error per
file.  Is this documented in the interface?

[ahendrickson, 2012/03/01 09:17:32]

I added a comment to AddError().

+  ErrorMap injected_errors_;
+
+  // Keep track of active DownloadFiles.
+  FileSet files_;
+
+  // We have created a factory.  Used for validation.
+  bool created_factory_;
+
+  // The number of files we've recorded.
+  int file_counter_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestFileErrorInjectorImpl);
+};
+
+// A class that performs file operations and injects errors.
+class DownloadFileWithErrors: public DownloadFileImpl {
+ public:
+  DownloadFileWithErrors(
+      const DownloadCreateInfo* info,
+      DownloadRequestHandleInterface* request_handle,
+      content::DownloadManager* download_manager,
+      bool calculate_hash,
+      const net::BoundNetLog& bound_net_log,
+      const content::TestFileErrorInjector::FileErrorInfo& error_info,
+      const TestFileErrorInjectorImpl::DownloadFileCallback& callback);
+
+  ~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;
+
+ private:
+  // Error generating helper.
+  net::Error ShouldReturnError(
+      content::TestFileErrorInjector::FileOperationCode code,
+      net::Error original_net_error);
+
+  // Our injected error.  Only one per file.
+  content::TestFileErrorInjector::FileErrorInfo error_info_;
+
+  // Count per operation.  0-based.
+  std::map<content::TestFileErrorInjector::FileOperationCode, int>
+      operation_counter_;
+
+  // Callback for creation and destruction.
+  TestFileErrorInjectorImpl::DownloadFileCallback callback_;
+};
+
+// A factory for constructing DownloadFiles that inject errors.
+class DownloadFileWithErrorsFactory
+    : public DownloadFileManager::DownloadFileFactory {
+ public:
+
+  DownloadFileWithErrorsFactory(
+      const TestFileErrorInjectorImpl::DownloadFileCallback& callback);
+  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 AddError(
+      const content::TestFileErrorInjector::FileErrorInfo& error_info);
+
+  size_t files_created() const { return file_counter_; }
+
+ private:
+  // Number of files we've created.
+  size_t file_counter_;
+
+  // Our injected error list, mapped by file index.  One per file.
+  ErrorMap injected_errors_;
+
+  // Callback for creation and destruction.
+  TestFileErrorInjectorImpl::DownloadFileCallback callback_;
+};
+
+// Implementations.
+
+DownloadFileWithErrors::DownloadFileWithErrors(
+    const DownloadCreateInfo* info,
+    DownloadRequestHandleInterface* request_handle,
+    content::DownloadManager* download_manager,
+    bool calculate_hash,
+    const net::BoundNetLog& bound_net_log,
+    const content::TestFileErrorInjector::FileErrorInfo& error_info,
+    const TestFileErrorInjectorImpl::DownloadFileCallback& callback)
+        : DownloadFileImpl(info,
+                           request_handle,
+                           download_manager,
+                           calculate_hash,
+                           bound_net_log),
+          error_info_(error_info),
+          callback_(callback) {
+  // Record creation.
+  callback_.Run(error_info_.file_index, true);
+}
+
+DownloadFileWithErrors::~DownloadFileWithErrors() {
+  // Record destruction.
+  callback_.Run(error_info_.file_index, false);
+}
+
+
+net::Error DownloadFileWithErrors::Initialize() {
+  return ShouldReturnError(
+      content::TestFileErrorInjector::FILE_OPERATION_INITIALIZE,
+      DownloadFileImpl::Initialize());
+}
+
+net::Error DownloadFileWithErrors::AppendDataToFile(const char* data,
+                                                    size_t data_len) {
+  return ShouldReturnError(
+      content::TestFileErrorInjector::FILE_OPERATION_WRITE,
+      DownloadFileImpl::AppendDataToFile(data, data_len));
+}
+
+net::Error DownloadFileWithErrors::Rename(const FilePath& full_path) {
+  return ShouldReturnError(
+      content::TestFileErrorInjector::FILE_OPERATION_RENAME,
+      DownloadFileImpl::Rename(full_path));
+}
+
+net::Error DownloadFileWithErrors::ShouldReturnError(
+    content::TestFileErrorInjector::FileOperationCode code,
+    net::Error original_net_error) {
+  int counter = operation_counter_[code];

[cbentzel, 2012/02/28 14:51:41]

I'd get rid of the operation_counter_ map here - not needed since we only care
about operation_instance of 0. 

[ahendrickson, 2012/03/02 21:58:47]

We're now using operation instances other than 0.

+  ++operation_counter_[code];
+
+  if (code != error_info_.code)
+    return original_net_error;
+
+  if (counter != error_info_.operation_instance)
+    return original_net_error;
+
+  VLOG(1) << " " << __FUNCTION__ << "()"
+          << " code = " << content::TestFileErrorInjector::DebugString(code)
+          << " (" << code << ")"
+          << " counter = " << counter
+          << " original_error = " << original_net_error
+          << " new error = " << error_info_.net_error;
+
+  return error_info_.net_error;
+}
+
+DownloadFileWithErrorsFactory::DownloadFileWithErrorsFactory(
+    const TestFileErrorInjectorImpl::DownloadFileCallback& callback)
+        : file_counter_(0), callback_(callback) {
+}
+
+DownloadFileWithErrorsFactory::~DownloadFileWithErrorsFactory() {
+}
+
+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_++],

[Randy Smith (Not in Mondays), 2012/02/28 22:06:13]

What happens if this accesses an uninitialized entry in the array?

[ahendrickson, 2012/03/01 09:17:32]

No error is injected.

[Randy Smith (Not in Mondays), 2012/03/01 20:28:24]

I think Chris did the analysis that I was worried about here and trying to point
towards: If we let the system default create an instance of the array, we don't
know what's going to go into it.

[ahendrickson, 2012/03/02 21:58:47]

Fixed.

+                                    callback_);
+}
+
+bool DownloadFileWithErrorsFactory::AddError(
+    const content::TestFileErrorInjector::FileErrorInfo& error_info) {
+  DCHECK_LE(0, error_info.file_index);
+  DCHECK_LE(0, error_info.operation_instance);
+
+  // Creates an empty entry if necessary.
+  injected_errors_[error_info.file_index] = error_info;

[Randy Smith (Not in Mondays), 2012/02/28 22:06:13]

Should this interface allow us to overwrite already existing entries in the
array?

[ahendrickson, 2012/03/01 09:17:32]

No, although it just replaces the existing value.

Fixed.

+
+  return true;
+}
+
+TestFileErrorInjectorImpl::TestFileErrorInjectorImpl()
+    : created_factory_(false), file_counter_(0) {
+}
+
+TestFileErrorInjectorImpl::~TestFileErrorInjectorImpl() {
+}
+
+bool TestFileErrorInjectorImpl::AddError(const FileErrorInfo& error_info) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  DCHECK(!created_factory_);
+  DCHECK_LE(0, error_info.file_index);
+  DCHECK_LE(0, error_info.operation_instance);
+
+  // Creates an empty entry if necessary.
+  injected_errors_[error_info.file_index] = error_info;

[cbentzel, 2012/02/28 14:51:41]

Why are you worried about adding support for a map of files at this point when
a) you aren't using it and b) haven't exercised it?

Adding extra complexity that isn't needed is a bad idea. If you have some tests
that will take advantage of this, let me know what they are.

[ahendrickson, 2012/03/01 09:17:32]

The new tests run multiple downloads, so we need this.

+
+  return true;
+}
+
+bool TestFileErrorInjectorImpl::InjectErrors() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  DCHECK(!created_factory_);
+
+  DownloadFileWithErrorsFactory* download_file_factory =
+      new DownloadFileWithErrorsFactory(
+        base::Bind(
+            &TestFileErrorInjectorImpl::
+                RecordDownloadFileConstructionDestruction,
+            this));
+
+  for (ErrorMap::const_iterator it = injected_errors_.begin();
+       it != injected_errors_.end();
+       ++it) {
+    download_file_factory->AddError(it->second);

[Randy Smith (Not in Mondays), 2012/02/28 22:06:13]

nit, suggestion: You could create download_file_factory on object construction,
and accumulate the errors directly in that object to avoid having to keep
TestFileErrorInjectorImpl::injected_errors_ around.

[ahendrickson, 2012/03/01 09:17:32]

I did that in an earlier pass, but Chris asked me to change it:

On 2012/02/27 02:18:54, cbentzel wrote:

[Randy Smith (Not in Mondays), 2012/03/01 20:28:24]

Given that I think Chris just suggested the same idea I did, you might want to
circle back with him and make sure you understood his earlier concern.

+  }
+
+  DownloadFileManager* download_file_manager = GetDownloadFileManager();
+  DCHECK(download_file_manager);
+  // Transfers ownership to |DownloadFileManager|.

[cbentzel, 2012/02/28 14:51:41]

Use scoped_ptr.Pass here instead.

[ahendrickson, 2012/03/01 09:17:32]

Done.

+  download_file_manager->SetFileFactoryForTesting(download_file_factory);
+
+  created_factory_ = true;
+
+  return true;
+}
+
+size_t TestFileErrorInjectorImpl::CurrentFileCount() const {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  return files_.size();
+}
+
+size_t TestFileErrorInjectorImpl::FileCreationCount() const {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  return file_counter_;
+}
+
+
+bool TestFileErrorInjectorImpl::HasFile(int file_index) const {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  return (files_.find(file_index) != files_.end());
+}
+
+void TestFileErrorInjectorImpl::DownloadFileCreated(int file_index) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  DCHECK_EQ(file_index, file_counter_);
+
+  file_counter_++;
+  files_.insert(file_index);
+}
+
+void TestFileErrorInjectorImpl::DestroyingDownloadFile(int file_index) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  files_.erase(file_index);
+}
+
+void TestFileErrorInjectorImpl::RecordDownloadFileConstructionDestruction(
+    int index, bool constructing) {
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI,
+      FROM_HERE,
+      base::Bind(constructing ?
+                     &TestFileErrorInjectorImpl::DownloadFileCreated :
+                     &TestFileErrorInjectorImpl::DestroyingDownloadFile,
+                 this,
+                 index));
+}
+
+}  // namespace
+
+namespace content {
+
+// static
+scoped_refptr<TestFileErrorInjector> TestFileErrorInjector::Create() {
+  return new TestFileErrorInjectorImpl;
+}
+
+std::string TestFileErrorInjector::DebugString(FileOperationCode code) {
+
+#define TO_STRING(code)  \

[cbentzel, 2012/02/28 14:51:41]

Although this macro is localized, it provides very little advantage over just
writing directly. Also note that you don't need to clarify the content namespace
here.

Also - given that this is only used in this translation unit, you may just want
to make it a non-static method and only display it here. I'm not even sure that
the VLOG is needed, but given that this is test-only code won't push too hard
against it.

[ahendrickson, 2012/03/01 09:17:32]

Removed the namespace reference.

It is now used in the browser tests.

[Randy Smith (Not in Mondays), 2012/03/06 23:05:19]

FWIW, I agree with Chris; I think writing it directly is better, though I won't
block this version.

+    case content::TestFileErrorInjector::FILE_OPERATION_##code: return #code
+
+  switch (code) {
+    TO_STRING(INITIALIZE);
+    TO_STRING(WRITE);
+    TO_STRING(RENAME);
+    default:
+      break;
+  }
+
+#undef TO_STRING
+
+  return "Unknown";
+}
+
+}  // namespace content