Test file errors in downloads.

content/test/test_file_error_injector.cc

Index: content/test/test_file_error_injector.cc
diff --git a/content/test/test_file_error_injector.cc b/content/test/test_file_error_injector.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d348c209dbfcea44ed16331bb6a9df70f0fcd736
--- /dev/null
+++ b/content/test/test_file_error_injector.cc
@@ -0,0 +1,432 @@
+// 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_create_info.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"
+#include "googleurl/src/gurl.h"
+
+namespace {
+
+class DownloadFileWithErrors;
+class DownloadFileWithErrorsFactory;
+
+typedef std::map<std::string, 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(GURL url, content::DownloadId id)>

[cbentzel, 2012/03/01 15:44:20]

It's a bit uncommon to have these on the TestFileErrorInjectorImpl instead of
the Factory/File. As you see, the TestFileErrorInjectorImpl does not directly
depend on these typedefs, but the File/Factory do.

[ahendrickson, 2012/03/01 20:19:31]

I'd prefer to have TestFileErrorInjectorImpl declared before File/Factory, which
means that the typedefs have to be declared before File/Factory.

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

Why do you prefer that order?  I agree with Chris that the interface is a little
less clean having the typedef here.

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

Still looking for a response to this question.

[ahendrickson, 2012/03/08 22:00:08]

Rearranged.

+      ConstructionCallback;
+  typedef base::Callback<void(GURL url)> DestructionCallback;
+
+  struct InjectedFileInfo {
+    InjectedFileInfo() : id(content::DownloadId::Invalid()) {}
+
+    GURL url;
+    content::DownloadId id;
+  };
+
+  TestFileErrorInjectorImpl();
+
+  // content::TestFileErrorInjector interface.
+  virtual bool AddError(const FileErrorInfo& error_info) OVERRIDE;
+  virtual void ClearErrors() OVERRIDE;
+  virtual bool InjectErrors() OVERRIDE;
+  virtual size_t CurrentFileCount() const OVERRIDE;
+  virtual size_t TotalFileCount() const OVERRIDE;
+  virtual bool HadFile(const GURL& url) const OVERRIDE;
+  virtual const content::DownloadId GetId(const GURL& url) const OVERRIDE;
+  virtual void ClearFoundFiles() OVERRIDE;
+
+ private:
+  typedef std::map<GURL, InjectedFileInfo> FileMap;

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

Why is this a map to InjectedFileInfo instead of just id?  You never use the
rest of the structure, as best I can tell, and it's redundant with the key
anyway, which you always have access to (as best I can tell).

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

Fixed in a later patch.  The reason it didn't have a DownloadId as the value was
that the latter didn't have a default constructor.  I added one after consulting
with Ben.

+
+  virtual ~TestFileErrorInjectorImpl();
+
+  void RecordDownloadFileConstruction(GURL url, content::DownloadId id);
+  void RecordDownloadFileDestruction(GURL url);
+
+  // Callbacks from the download file, to record lifetimes.
+  void DownloadFileCreated(GURL url, content::DownloadId id);
+  void DestroyingDownloadFile(GURL url);
+
+  // Our injected error list, mapped by file index.  One per file.
+  ErrorMap injected_errors_;
+
+  // Keep track of active DownloadFiles.

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

How are you dealing with the threading issues involved here?  (This class
accessed on the UI thread by the callbacks actually happening on the file
thread.)

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

In my test code, I'm setting up all the injected errors prior to accessing any
of them (within the download loop body), so there is no threading conflict.

I have now set up a FILE-thread version of InjectErrors(), that takes a copy of
the injected errors.

The callbacks post to the UI thread for updating the records.

+  FileMap files_;
+
+  // Keep track of found DownloadFiles.
+  FileMap found_files_;
+
+  // We have created a factory.  Used for validation.
+  bool created_factory_;
+
+  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::ConstructionCallback& ctor_callback,
+      const TestFileErrorInjectorImpl::DestructionCallback& dtor_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);
+
+  // Source URL for the file being downloaded.
+  GURL source_url_;
+
+  // 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 destruction.
+  TestFileErrorInjectorImpl::DestructionCallback destruction_callback_;
+};
+
+// A factory for constructing DownloadFiles that inject errors.
+class DownloadFileWithErrorsFactory
+    : public DownloadFileManager::DownloadFileFactory {
+ public:
+
+  DownloadFileWithErrorsFactory(
+      const TestFileErrorInjectorImpl::ConstructionCallback& ctor_callback,
+      const TestFileErrorInjectorImpl::DestructionCallback& dtor_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);
+
+  void ClearErrors();
+
+ private:
+  // Our injected error list, mapped by file index.  One per file.
+  ErrorMap injected_errors_;
+
+  // Callback for creation and destruction.
+  TestFileErrorInjectorImpl::ConstructionCallback construction_callback_;
+  TestFileErrorInjectorImpl::DestructionCallback destruction_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::ConstructionCallback& ctor_callback,
+    const TestFileErrorInjectorImpl::DestructionCallback& dtor_callback)
+        : DownloadFileImpl(info,
+                           request_handle,
+                           download_manager,
+                           calculate_hash,
+                           bound_net_log),
+          source_url_(info->url()),
+          error_info_(error_info),
+          destruction_callback_(dtor_callback) {
+  // Record creation.

[cbentzel, 2012/03/01 15:44:20]

Comments are a bit inaccurate here - the DownloadFileWithErrors doesn't actually
know about what's going on inside the callbacks. The code is also pretty
self-explanatory.

[ahendrickson, 2012/03/01 20:19:31]

Removed.

+  ctor_callback.Run(source_url_, info->download_id);
+}
+
+DownloadFileWithErrors::~DownloadFileWithErrors() {
+  // Record destruction.
+  destruction_callback_.Run(source_url_);
+}
+
+
+net::Error DownloadFileWithErrors::Initialize() {
+  return ShouldReturnError(
+      content::TestFileErrorInjector::FILE_OPERATION_INITIALIZE,
+      DownloadFileImpl::Initialize());

[cbentzel, 2012/03/01 15:44:20]

Question: Right now this always runs the underlying DownloadFileImpl:: methods
even if we'll override with an injected error. Is that the behavior you want, or
do you want to short-circuit that?

[ahendrickson, 2012/03/01 20:19:31]

It is the intended behavior.

+}
+
+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];
+  ++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__ << "()"
+          << " url = '" << source_url_.spec() << "'"
+          << " code = " << content::TestFileErrorInjector::DebugString(code)
+          << " (" << code << ")"
+          << " counter = " << counter
+          << " original_error = " << net::ErrorToString(original_net_error)
+          << " (" << original_net_error << ")"
+          << " new error = " << net::ErrorToString(error_info_.net_error)
+          << " (" << error_info_.net_error << ")";
+
+  return error_info_.net_error;
+}
+
+DownloadFileWithErrorsFactory::DownloadFileWithErrorsFactory(
+    const TestFileErrorInjectorImpl::ConstructionCallback& ctor_callback,
+    const TestFileErrorInjectorImpl::DestructionCallback& dtor_callback)
+        : construction_callback_(ctor_callback),
+          destruction_callback_(dtor_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 injected_errors_ entry if it doesn't exist.
+  return new DownloadFileWithErrors(info,
+                                    new DownloadRequestHandle(request_handle),
+                                    download_manager,
+                                    calculate_hash,
+                                    bound_net_log,
+                                    injected_errors_[info->url().spec()],

[cbentzel, 2012/03/01 15:44:20]

This is a bug/flakyness issue if info->url().spec() is not in the map - which
this seems to allow.

FileErrorInfo is not a POD struct (it has std::string) so the default
constructor generated by the compiler provides no guarantees about the values of
primitive members such as code, operation_instance, and error.

[ahendrickson, 2012/03/01 20:19:31]

Fixed.

+                                    construction_callback_,
+                                    destruction_callback_);
+}
+
+bool DownloadFileWithErrorsFactory::AddError(
+    const content::TestFileErrorInjector::FileErrorInfo& error_info) {
+  DCHECK_LE(0, error_info.operation_instance);

[cbentzel, 2012/03/01 15:44:20]

This is redundant with the check in TestFileErrorInjectorImpl::AddError which is
the public interface.

[ahendrickson, 2012/03/01 20:19:31]

Done.

+
+  // Creates an empty entry if necessary.  Duplicate entries overwrite.
+  injected_errors_[error_info.url] = error_info;
+
+  return true;
+}
+
+void DownloadFileWithErrorsFactory::ClearErrors() {
+  injected_errors_.clear();
+}
+
+TestFileErrorInjectorImpl::TestFileErrorInjectorImpl()
+    : created_factory_(false) {
+}
+
+TestFileErrorInjectorImpl::~TestFileErrorInjectorImpl() {
+}
+
+bool TestFileErrorInjectorImpl::AddError(const FileErrorInfo& error_info) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  DCHECK_LE(0, error_info.operation_instance);
+  DCHECK(injected_errors_.find(error_info.url) == injected_errors_.end());
+
+  // Creates an empty entry if necessary.
+  injected_errors_[error_info.url] = error_info;

[cbentzel, 2012/03/01 15:44:20]

You could also create the factory early in the injector and just populate the
factory, rather than having a local map. 

[ahendrickson, 2012/03/01 20:19:31]

In an earlier comment, you asked that the factory be created at injection time.

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

Moved it to the constructor.

+
+  return true;
+}
+
+void TestFileErrorInjectorImpl::ClearErrors() {
+  injected_errors_.clear();
+}
+
+bool TestFileErrorInjectorImpl::InjectErrors() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  DownloadFileManager* download_file_manager = GetDownloadFileManager();
+  DCHECK(download_file_manager);
+
+  if (!created_factory_) {

[cbentzel, 2012/03/01 15:44:20]

Is it valid for InjectErrors to be called more than once? If it is, should you
also clear out files_ and found_files_?

[ahendrickson, 2012/03/01 20:19:31]

It is.  At the moment, the test checks that files_ is empty between downloads,
and clears found_files_ itself.

I moved the clearing of found_files_ here.

+    scoped_ptr<DownloadFileWithErrorsFactory> download_file_factory(
+        new DownloadFileWithErrorsFactory(
+            base::Bind(&TestFileErrorInjectorImpl::
+                           RecordDownloadFileConstruction,
+                       this),
+            base::Bind(&TestFileErrorInjectorImpl::
+                           RecordDownloadFileDestruction,
+                       this)));
+
+    download_file_manager->SetFileFactoryForTesting(
+        download_file_factory.Pass());
+
+    created_factory_ = true;
+  }
+
+  DownloadFileWithErrorsFactory* file_factory =
+      static_cast<DownloadFileWithErrorsFactory*>(
+          download_file_manager->GetFileFactoryForTesting());
+
+  file_factory->ClearErrors();
+
+  for (ErrorMap::const_iterator it = injected_errors_.begin();
+       it != injected_errors_.end();
+       ++it) {
+    file_factory->AddError(it->second);
+  }
+
+  return true;
+}
+
+size_t TestFileErrorInjectorImpl::CurrentFileCount() const {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  return files_.size();
+}
+
+size_t TestFileErrorInjectorImpl::TotalFileCount() const {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  return found_files_.size();
+}
+
+
+bool TestFileErrorInjectorImpl::HadFile(const GURL& url) const {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  return (found_files_.find(url) != found_files_.end());
+}
+
+const content::DownloadId TestFileErrorInjectorImpl::GetId(
+    const GURL& url) const {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  FileMap::const_iterator it = found_files_.find(url);
+  if (it == found_files_.end())
+    return content::DownloadId::Invalid();
+
+  return it->second.id;
+}
+
+void TestFileErrorInjectorImpl::ClearFoundFiles() {
+  found_files_.clear();
+}
+
+void TestFileErrorInjectorImpl::DownloadFileCreated(GURL url,
+                                                    content::DownloadId id) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  DCHECK(files_.find(url) == files_.end());
+
+  InjectedFileInfo info;
+  info.url = url;
+  info.id = id;
+
+  files_[url] = info;

[cbentzel, 2012/03/01 15:44:20]

Why not just maintain a map from url to id, instead of this InjectedFileInfo
struct? Seems like it would be sufficient.

[ahendrickson, 2012/03/01 20:19:31]

DownloadId doesn't have a default constructor, so that won't work.

Added a default constructor.

Done.

+  found_files_[url] = info;
+}
+
+void TestFileErrorInjectorImpl::DestroyingDownloadFile(GURL url) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

[cbentzel, 2012/03/01 15:44:20]

Should you DCHECK that |url| is in files_ here? 

[ahendrickson, 2012/03/01 20:19:31]

Done.

+
+  files_.erase(url);
+}
+
+void TestFileErrorInjectorImpl::RecordDownloadFileConstruction(
+    GURL url, content::DownloadId id) {
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI,
+      FROM_HERE,
+      base::Bind(&TestFileErrorInjectorImpl::DownloadFileCreated,
+                 this,
+                 url,
+                 id));
+}
+
+void TestFileErrorInjectorImpl::RecordDownloadFileDestruction(GURL url) {
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI,
+      FROM_HERE,
+      base::Bind(&TestFileErrorInjectorImpl::DestroyingDownloadFile,
+                 this,
+                 url));
+}
+
+}  // namespace
+
+namespace content {
+
+// static
+scoped_refptr<TestFileErrorInjector> TestFileErrorInjector::Get() {
+  scoped_refptr<TestFileErrorInjector> single_injector_;

[cbentzel, 2012/03/01 15:44:20]

What's going on here? This is always going to be NULL. Was this intended to be a
static?

[ahendrickson, 2012/03/01 20:19:31]

It was.  Fixed.

Sadly this means that the injector will hang around until the program ends.

[cbentzel, 2012/03/01 22:32:00]

I'll chat with Randy. I don't understand why this should be all singleton'y.
It's only called once from the test driver. And then you have a static non-POD
class around, which is totally frowned upon by the style guide. 

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

Now using a DCHECK to prevent multiple calls, and not keeping a static
scoped_refptr<>.

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

Perfectly happy with non-singleton semantics so long as the interface is at
least fool-resistant (i.e. that reasonable things happen if multiple instances
of the class are created).  I'll scan for that in my review.

+  if (single_injector_.get() == NULL)
+    single_injector_ = new TestFileErrorInjectorImpl;
+  return single_injector_;
+}
+
+std::string TestFileErrorInjector::DebugString(FileOperationCode code) {
+
+#define TO_STRING(code)  \
+    case 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