Using a mock LevelDBTransaction for corruption tests.

content/browser/indexed_db/indexed_db_browsertest.cc

Index: content/browser/indexed_db/indexed_db_browsertest.cc
diff --git a/content/browser/indexed_db/indexed_db_browsertest.cc b/content/browser/indexed_db/indexed_db_browsertest.cc
index 0d62a1336749cddc3cdb854f2c44e8292fe0cdc2..8210ab45fe627e0b01323dfdc887f2204d3f23b1 100644
--- a/content/browser/indexed_db/indexed_db_browsertest.cc
+++ b/content/browser/indexed_db/indexed_db_browsertest.cc
@@ -8,11 +8,14 @@
 #include "base/files/file.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
+#include "base/lazy_instance.h"
 #include "base/memory/ref_counted.h"
 #include "base/message_loop/message_loop.h"
+#include "base/strings/string_split.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/thread_test_helper.h"
 #include "content/browser/browser_main_loop.h"
+#include "content/browser/indexed_db/indexed_db_class_factory.h"
 #include "content/browser/indexed_db/indexed_db_context_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/browser_context.h"
@@ -39,11 +42,192 @@ using webkit_database::DatabaseUtil;
 
 namespace content {
 
+enum FailClass {
+  FAIL_CLASS_NOTHING,
+  FAIL_CLASS_LEVELDB_TRANSACTION,
+};
+
+enum FailMethod {
+  FAIL_METHOD_NOTHING,
+  FAIL_METHOD_COMMIT,
+  FAIL_METHOD_GET,
+};
+
+class FunctionTracer {
+ public:
+  FunctionTracer(const std::string& class_name,
+                 const std::string& method_name,
+                 int instance_num)
+      : class_name_(class_name),
+        method_name_(method_name),
+        instance_count_(instance_num),
+        current_call_num_(0) {}
+
+  void log_call() {
+    current_call_num_++;
+    VLOG(0) << class_name_ << '[' << instance_count_ << "]::" << method_name_
+            << "()[" << current_call_num_ << ']';
+  }
+
+ private:
+  std::string class_name_;
+  std::string method_name_;
+  int instance_count_;
+  int current_call_num_;
+};
+
+class LevelDBTestTansaction : public LevelDBTransaction {
+ public:
+  LevelDBTestTansaction(LevelDBDatabase* db,
+                        FailMethod fail_method,
+                        int fail_on_call_num)
+      : LevelDBTransaction(db),
+        fail_method_(fail_method),
+        fail_on_call_num_(fail_on_call_num),
+        current_call_num_(0) {
+    DCHECK(fail_method != FAIL_METHOD_NOTHING);
+    DCHECK_GT(fail_on_call_num, 0);
+  }
+
+  virtual leveldb::Status Get(const base::StringPiece& key,
+                              std::string* value,
+                              bool* found) OVERRIDE {
+    if (fail_method_ != FAIL_METHOD_GET ||
+        ++current_call_num_ != fail_on_call_num_)
+      return LevelDBTransaction::Get(key, value, found);
+
+    *found = false;
+    return leveldb::Status::Corruption("Corrupted for the test");
+  }
+
+  virtual leveldb::Status Commit() OVERRIDE {
+    if (fail_method_ != FAIL_METHOD_COMMIT ||
+        ++current_call_num_ != fail_on_call_num_)
+      return LevelDBTransaction::Commit();
+
+    return leveldb::Status::Corruption("Corrupted for the test");
+  }
+
+ private:
+  virtual ~LevelDBTestTansaction() {}
+
+  FailMethod fail_method_;
+  int fail_on_call_num_;
+  int current_call_num_;
+};
+
+class LevelDBTraceTansaction : public LevelDBTransaction {
+ public:
+  LevelDBTraceTansaction(LevelDBDatabase* db, int tx_num)
+      : LevelDBTransaction(db),
+        commit_tracer_(s_class_name, "Commit", tx_num),
+        get_tracer_(s_class_name, "Get", tx_num) {}
+
+  virtual leveldb::Status Get(const base::StringPiece& key,
+                              std::string* value,
+                              bool* found) OVERRIDE {
+    get_tracer_.log_call();
+    return LevelDBTransaction::Get(key, value, found);
+  }
+
+  virtual leveldb::Status Commit() OVERRIDE {
+    commit_tracer_.log_call();
+    return LevelDBTransaction::Commit();
+  }
+
+ private:
+  virtual ~LevelDBTraceTansaction() {}
+
+  const std::string s_class_name = "LevelDBTransaction";
+
+  FunctionTracer commit_tracer_;
+  FunctionTracer get_tracer_;
+};
+
+class IndexedDBBrowserTestClassFactory : public IndexedDBClassFactory {
+ public:
+  IndexedDBBrowserTestClassFactory()
+      : failure_class_(FAIL_CLASS_NOTHING),
+        failure_method_(FAIL_METHOD_NOTHING),
+        only_trace_calls_(false) {}
+  virtual LevelDBTransaction* CreateLevelDBTransaction(
+      LevelDBDatabase* db) OVERRIDE {
+    instance_count_[FAIL_CLASS_LEVELDB_TRANSACTION] =
+        instance_count_[FAIL_CLASS_LEVELDB_TRANSACTION] + 1;
+    if (only_trace_calls_) {
+      return new LevelDBTraceTansaction(
+          db, instance_count_[FAIL_CLASS_LEVELDB_TRANSACTION]);
+    } else {
+      if (failure_class_ == FAIL_CLASS_LEVELDB_TRANSACTION &&
+          instance_count_[FAIL_CLASS_LEVELDB_TRANSACTION] ==
+              fail_on_instance_num_[FAIL_CLASS_LEVELDB_TRANSACTION])
+        return new LevelDBTestTansaction(
+            db,
+            failure_method_,
+            fail_on_call_num_[FAIL_CLASS_LEVELDB_TRANSACTION]);
+      else
+        return IndexedDBClassFactory::CreateLevelDBTransaction(db);
+    }
+  }
+
+  void FailOperation(FailClass failure_class,
+                     FailMethod failure_method,
+                     int fail_on_instance_num,
+                     int fail_on_call_num) {
+    VLOG(0) << "FailOperation: class=" << failure_class
+            << ", method=" << failure_method
+            << ", instanceNum=" << fail_on_instance_num
+            << ", callNum=" << fail_on_call_num;
+    DCHECK(failure_class != FAIL_CLASS_NOTHING);
+    DCHECK(failure_method != FAIL_METHOD_NOTHING);
+    failure_class_ = failure_class;
+    failure_method_ = failure_method;
+    fail_on_instance_num_[failure_class_] = fail_on_instance_num;
+    fail_on_call_num_[failure_class_] = fail_on_call_num;
+    instance_count_.clear();
+  }
+
+  void Reset() {
+    failure_method_ = FAIL_METHOD_NOTHING;
+    instance_count_.clear();
+    fail_on_instance_num_.clear();
+    fail_on_call_num_.clear();
+  }
+
+ private:
+  FailClass failure_class_;
+  FailMethod failure_method_;
+  std::map<FailClass, int> instance_count_;
+  std::map<FailClass, int> fail_on_instance_num_;
+  std::map<FailClass, int> fail_on_call_num_;
+  bool only_trace_calls_;
+};
+
+static ::base::LazyInstance<IndexedDBBrowserTestClassFactory>::Leaky s_factory =
+    LAZY_INSTANCE_INITIALIZER;
+
 // This browser test is aimed towards exercising the IndexedDB bindings and
 // the actual implementation that lives in the browser side.
 class IndexedDBBrowserTest : public ContentBrowserTest {
  public:
-  IndexedDBBrowserTest() : disk_usage_(-1) {}
+  IndexedDBBrowserTest()
+      : disk_usage_(-1),
+        test_class_factory_(new IndexedDBBrowserTestClassFactory) {}

[ericu, 2014/06/17 01:12:53]

Is test_class_factory_ actually used somewhere?  If so, should it be a
scoped_ptr instead of raw?

[cmumford, 2014/06/17 16:29:24]

Embarrassingly no :-|

+
+  static IndexedDBClassFactory* GetIDBClassFactory() {
+    return s_factory.Pointer();
+  }
+
+  virtual void SetUp() OVERRIDE {
+    s_factory.Get().Reset();
+    IndexedDBClassFactory::SetIndexedDBClassFactoryGetter(GetIDBClassFactory);
+    ContentBrowserTest::SetUp();
+  }
+
+  virtual void TearDown() OVERRIDE {
+    IndexedDBClassFactory::SetIndexedDBClassFactoryGetter(NULL);

[ericu, 2014/06/17 01:12:53]

Given this cleanup, do we need s_factory to be a static?

[cmumford, 2014/06/17 16:29:24]

I was able to eliminate the use of s_factory in CorruptDBRequestHandler by
passing in a ptr to the test as a param. However, GetIDBClassFactory is the one
function which still requires a static and I was unable to eliminate that
requirement. I was able to reduce the scope of s_factory down to a single
function, but now there are two factory getters (GetIDBClassFactory &
GetTestClassFactory). Let me know if you see a better alternative.

+    ContentBrowserTest::TearDown();
+  }
 
   void SimpleTest(const GURL& test_url, bool incognito = false) {
     // The test page will perform tests on IndexedDB, then navigate to either
@@ -133,6 +317,7 @@ class IndexedDBBrowserTest : public ContentBrowserTest {
   }
 
   int64 disk_usage_;
+  IndexedDBBrowserTestClassFactory* test_class_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(IndexedDBBrowserTest);
 };
@@ -452,10 +637,13 @@ static scoped_ptr<net::test_server::HttpResponse> CorruptDBRequestHandler(
 
   // Remove the query string if present.
   std::string request_query;
+  std::vector<std::pair<std::string, std::string> > query_params;
   size_t query_pos = request_path.find('?');
   if (query_pos != std::string::npos) {
     request_query = request_path.substr(query_pos + 1);
     request_path = request_path.substr(0, query_pos);
+
+    base::SplitStringIntoKeyValuePairs(request_query, '=', '&', &query_params);
   }
 
   if (request_path == "corruptdb" && !request_query.empty()) {
@@ -472,6 +660,55 @@ static scoped_ptr<net::test_server::HttpResponse> CorruptDBRequestHandler(
         new net::test_server::BasicHttpResponse);
     http_response->set_code(net::HTTP_OK);
     return http_response.PassAs<net::test_server::HttpResponse>();
+  } else if (request_path == "fail" && !query_params.empty()) {
+    FailClass failure_class = FAIL_CLASS_NOTHING;
+    FailMethod failure_method = FAIL_METHOD_NOTHING;
+    int instance_num = 1;
+    int call_num = 1;
+    std::string fail_class;
+    std::string fail_method;
+
+    for (std::vector<std::pair<std::string, std::string> >::iterator it =
+             query_params.begin();
+         it != query_params.end();
+         it++) {
+      if (it->first == "method")
+        fail_method = it->second;
+      else if (it->first == "class")
+        fail_class = it->second;
+      else if (it->first == "num")

[ericu, 2014/06/17 01:12:53]

Should "num" be qualified, just as "callNum" is?  "instanceNum"?

[cmumford, 2014/06/17 16:29:24]

How about "instNum"?

[ericu, 2014/06/17 17:03:04]

Sure.

+        instance_num = atoi(it->second.c_str());
+      else if (it->first == "callNum")
+        call_num = atoi(it->second.c_str());
+      else {
+        VLOG(0) << "Unknown param: \"" << it->first << '"';
+        NOTREACHED();
+      }
+    }
+
+    if (fail_class == "LevelDBTransaction") {
+      failure_class = FAIL_CLASS_LEVELDB_TRANSACTION;
+      if (fail_method == "Get")
+        failure_method = FAIL_METHOD_GET;
+      else if (fail_method == "Commit")
+        failure_method = FAIL_METHOD_COMMIT;
+    }

[ericu, 2014/06/17 01:12:53]

else...?  It's undoubtedly a typo of some sort.

[cmumford, 2014/06/17 16:29:24]

Done. A DCHECK would have caught the error, but a VLOG is better.

+
+    DCHECK(instance_num >= 1);
+    DCHECK(call_num >= 1);
+
+    s_factory.Get().FailOperation(
+        failure_class, failure_method, instance_num, call_num);
+
+    scoped_ptr<net::test_server::BasicHttpResponse> http_response(
+        new net::test_server::BasicHttpResponse);
+    http_response->set_code(net::HTTP_OK);
+    return http_response.PassAs<net::test_server::HttpResponse>();
+  } else if (request_path == "nothing") {

[ericu, 2014/06/17 01:12:53]

Is this clause needed by anything, or do you have something in the works for it?

[cmumford, 2014/06/17 16:29:24]

Oops - shouldn't have been there. I was experimenting with improving the test
authoring workflow.

+    scoped_ptr<net::test_server::BasicHttpResponse> http_response(
+        new net::test_server::BasicHttpResponse);
+    http_response->set_code(net::HTTP_OK);
+    return http_response.PassAs<net::test_server::HttpResponse>();
   }
 
   // A request for a test resource