app/sql/connection hygiene enforcement.

For awhile we had an error where an invalid page_size was being passed in, and SQLite silently ignored it.  No more!  Fix cache_size logging line.  Pull the exclusive locking up to fail more obviously in case someone else has the exclusive lock.

Also, I don't think our code is likely handling the SQLITE_BUSY case correctly.  Added some initial code to execute some sql with a timeout.

BUG=56559
TEST=unit tests

Review URL: http://codereview.chromium.org/3413028

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

2 files changed, 67 insertions, 9 deletions

diff --git a/app/sql/connection.cc b/app/sql/connection.cc
index d227468..b52655a 100644
--- a/app/sql/connection.cc
+++ b/app/sql/connection.cc
@@ -13,6 +13,34 @@
 #include "base/utf_string_conversions.h"
 #include "third_party/sqlite/sqlite3.h"
 
+namespace {
+
+// Spin for up to a second waiting for the lock to clear when setting
+// up the database.
+// TODO(shess): Better story on this.  http://crbug.com/56559
+const base::TimeDelta kBusyTimeout = base::TimeDelta::FromSeconds(1);
+
+class ScopedBusyTimeout {
+ public:
+  explicit ScopedBusyTimeout(sqlite3* db)
+      : db_(db) {
+  }
+  ~ScopedBusyTimeout() {
+    sqlite3_busy_timeout(db_, 0);
+  }
+
+  int SetTimeout(base::TimeDelta timeout) {
+    DCHECK_LT(timeout.InMilliseconds(), INT_MAX);
+    return sqlite3_busy_timeout(db_,
+                                static_cast<int>(timeout.InMilliseconds()));
+  }
+
+ private:
+  sqlite3* db_;
+};
+
+}  // namespace
+
 namespace sql {
 
 bool StatementID::operator<(const StatementID& other) const {
@@ -166,6 +194,15 @@ bool Connection::Execute(const char* sql) {
   return sqlite3_exec(db_, sql, NULL, NULL, NULL) == SQLITE_OK;
 }
 
+bool Connection::ExecuteWithTimeout(const char* sql, base::TimeDelta timeout) {
+  if (!db_)
+    return false;
+
+  ScopedBusyTimeout busy_timeout(db_);
+  busy_timeout.SetTimeout(timeout);
+  return sqlite3_exec(db_, sql, NULL, NULL, NULL) == SQLITE_OK;
+}
+
 bool Connection::HasCachedStatement(const StatementID& id) const {
   return statement_cache_.find(id) != statement_cache_.end();
 }
@@ -296,19 +333,36 @@ bool Connection::OpenInternal(const std::string& file_name) {
   err = sqlite3_extended_result_codes(db_, 1);
   DCHECK_EQ(err, SQLITE_OK) << "Could not enable extended result codes";
 
-  if (page_size_ != 0) {
-    if (!Execute(StringPrintf("PRAGMA page_size=%d", page_size_).c_str()))
-      NOTREACHED() << "Could not set page size";
+  // If indicated, lock up the database before doing anything else, so
+  // that the following code doesn't have to deal with locking.
+  // TODO(shess): This code is brittle.  Find the cases where code
+  // doesn't request |exclusive_locking_| and audit that it does the
+  // right thing with SQLITE_BUSY, and that it doesn't make
+  // assumptions about who might change things in the database.
+  // http://crbug.com/56559
+  if (exclusive_locking_) {
+    // TODO(shess): This should probably be a full CHECK().  Code
+    // which requests exclusive locking but doesn't get it is almost
+    // certain to be ill-tested.
+    if (!Execute("PRAGMA locking_mode=EXCLUSIVE"))
+      NOTREACHED() << "Could not set locking mode: " << GetErrorMessage();
   }
 
-  if (cache_size_ != 0) {
-    if (!Execute(StringPrintf("PRAGMA cache_size=%d", cache_size_).c_str()))
-      NOTREACHED() << "Could not set page size";
+  if (page_size_ != 0) {
+    // Enforce SQLite restrictions on |page_size_|.
+    DCHECK(!(page_size_ & (page_size_ - 1)))
+        << " page_size_ " << page_size_ << " is not a power of two.";
+    static const int kSqliteMaxPageSize = 32768;  // from sqliteLimit.h
+    DCHECK_LE(page_size_, kSqliteMaxPageSize);
+    const std::string sql = StringPrintf("PRAGMA page_size=%d", page_size_);
+    if (!ExecuteWithTimeout(sql.c_str(), kBusyTimeout))
+      NOTREACHED() << "Could not set page size: " << GetErrorMessage();
   }
 
-  if (exclusive_locking_) {
-    if (!Execute("PRAGMA locking_mode=EXCLUSIVE"))
-      NOTREACHED() << "Could not set locking mode.";
+  if (cache_size_ != 0) {
+    const std::string sql = StringPrintf("PRAGMA cache_size=%d", cache_size_);
+    if (!ExecuteWithTimeout(sql.c_str(), kBusyTimeout))
+      NOTREACHED() << "Could not set cache size: " << GetErrorMessage();
   }
 
   return true;
diff --git a/app/sql/connection.h b/app/sql/connection.h
index 0bd28ca..680cf1d 100644
--- a/app/sql/connection.h
+++ b/app/sql/connection.h
@@ -12,6 +12,7 @@
 
 #include "base/basictypes.h"
 #include "base/ref_counted.h"
+#include "base/time.h"
 
 class FilePath;
 struct sqlite3;
@@ -339,6 +340,9 @@ class Connection {
   // The return value is the error code reflected back to client code.
   int OnSqliteError(int err, Statement* stmt);
 
+  // Like |Execute()|, but retries if the database is locked.
+  bool ExecuteWithTimeout(const char* sql, base::TimeDelta ms_timeout);
+
   // The actual sqlite database. Will be NULL before Init has been called or if
   // Init resulted in an error.
   sqlite3* db_;