Annotate calls to SQLite functions - they have to be executed on a thread allowing IO access.
Also expanded scope of ScopedAllowIO in
SQLiteServerBoundCertStore::Backend::Load() to cover SQLite functions.
And added ScopedAllowIO to PasswordStoreFactory::BuildServiceInstanceFor() --
it calls LoginDatabase::Init() which should be executed on DB thread.
This is a reland of
https://src.chromium.org/viewvc/chrome?view=rev&revision=147309
which was reverted because of missing ScopedAllowIO in PasswordStoreFactory.
Patch from Pavel Ivanov <paivanof@gmail.com>
BUG=75232, 52909, 137961, 138903
TEST=no test fails with message "Function marked as IO-only was called from a thread that disallows IO!"
Review URL: https://chromiumcodereview.appspot.com/10824008
Patch from Pavel Ivanov <paivanof@gmail.com>.
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@148788 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/chrome/browser/net/sqlite_server_bound_cert_store.cc b/chrome/browser/net/sqlite_server_bound_cert_store.cc
index 0a297ee..75f9318 100644
--- a/chrome/browser/net/sqlite_server_bound_cert_store.cc
+++ b/chrome/browser/net/sqlite_server_bound_cert_store.cc
@@ -159,16 +159,16 @@
// This function should be called only once per instance.
DCHECK(!db_.get());
+ // TODO(paivanof@gmail.com): We do a lot of disk access in this function,
+ // thus we do an exception to allow IO on the UI thread. This code will be
+ // moved to the DB thread as part of http://crbug.com/89665.
+ base::ThreadRestrictions::ScopedAllowIO allow_io;
+
// Ensure the parent directory for storing certs is created before reading
- // from it. We make an exception to allow IO on the UI thread here because
- // we are going to disk anyway in db_->Open. (This code will be moved to the
- // DB thread as part of http://crbug.com/52909.)
- {
- base::ThreadRestrictions::ScopedAllowIO allow_io;
- const FilePath dir = path_.DirName();
- if (!file_util::PathExists(dir) && !file_util::CreateDirectory(dir))
- return false;
- }
+ // from it.
+ const FilePath dir = path_.DirName();
+ if (!file_util::PathExists(dir) && !file_util::CreateDirectory(dir))
+ return false;
db_.reset(new sql::Connection);
if (!db_->Open(path_)) {
diff --git a/chrome/browser/password_manager/password_store_factory.cc b/chrome/browser/password_manager/password_store_factory.cc
index bee596d..b110cb5d 100644
--- a/chrome/browser/password_manager/password_store_factory.cc
+++ b/chrome/browser/password_manager/password_store_factory.cc
@@ -99,10 +99,15 @@
FilePath login_db_file_path = profile->GetPath();
login_db_file_path = login_db_file_path.Append(chrome::kLoginDataFileName);
LoginDatabase* login_db = new LoginDatabase();
- if (!login_db->Init(login_db_file_path)) {
- LOG(ERROR) << "Could not initialize login database.";
- delete login_db;
- return NULL;
+ {
+ // TODO(paivanof@gmail.com): execution of login_db->Init() should go
+ // to DB thread. http://crbug.com/138903
+ base::ThreadRestrictions::ScopedAllowIO allow_io;
+ if (!login_db->Init(login_db_file_path)) {
+ LOG(ERROR) << "Could not initialize login database.";
+ delete login_db;
+ return NULL;
+ }
}
#if defined(OS_WIN)
ps = new PasswordStoreWin(
diff --git a/sql/connection.cc b/sql/connection.cc
index 2d7baf25..c7666f9 100644
--- a/sql/connection.cc
+++ b/sql/connection.cc
@@ -81,6 +81,14 @@
void Connection::StatementRef::Close() {
if (stmt_) {
+ // Call to AssertIOAllowed() cannot go at the beginning of the function
+ // because Close() is called unconditionally from destructor to clean
+ // connection_. And if this is inactive statement this won't cause any
+ // disk access and destructor most probably will be called on thread
+ // not allowing disk access.
+ // TODO(paivanof@gmail.com): This should move to the beginning
+ // of the function. http://crbug.com/136655.
+ AssertIOAllowed();
sqlite3_finalize(stmt_);
stmt_ = NULL;
}
@@ -93,7 +101,8 @@
cache_size_(0),
exclusive_locking_(false),
transaction_nesting_(0),
- needs_rollback_(false) {
+ needs_rollback_(false),
+ in_memory_(false) {
}
Connection::~Connection() {
@@ -109,6 +118,7 @@
}
bool Connection::OpenInMemory() {
+ in_memory_ = true;
return OpenInternal(":memory:");
}
@@ -130,6 +140,13 @@
ClearCache();
if (db_) {
+ // Call to AssertIOAllowed() cannot go at the beginning of the function
+ // because Close() must be called from destructor to clean
+ // statement_cache_, it won't cause any disk access and it most probably
+ // will happen on thread not allowing disk access.
+ // TODO(paivanof@gmail.com): This should move to the beginning
+ // of the function. http://crbug.com/136655.
+ AssertIOAllowed();
// TODO(shess): Histogram for failure.
sqlite3_close(db_);
db_ = NULL;
@@ -137,6 +154,8 @@
}
void Connection::Preload() {
+ AssertIOAllowed();
+
if (!db_) {
DLOG(FATAL) << "Cannot preload null db";
return;
@@ -161,6 +180,8 @@
// Create an in-memory database with the existing database's page
// size, then backup that database over the existing database.
bool Connection::Raze() {
+ AssertIOAllowed();
+
if (!db_) {
DLOG(FATAL) << "Cannot raze null db";
return false;
@@ -297,6 +318,7 @@
}
int Connection::ExecuteAndReturnErrorCode(const char* sql) {
+ AssertIOAllowed();
if (!db_)
return false;
return sqlite3_exec(db_, sql, NULL, NULL, NULL);
@@ -347,6 +369,8 @@
scoped_refptr<Connection::StatementRef> Connection::GetUniqueStatement(
const char* sql) {
+ AssertIOAllowed();
+
if (!db_)
return new StatementRef(); // Return inactive statement.
@@ -375,6 +399,7 @@
}
bool Connection::IsSQLValid(const char* sql) {
+ AssertIOAllowed();
sqlite3_stmt* stmt = NULL;
if (sqlite3_prepare_v2(db_, sql, -1, &stmt, NULL) != SQLITE_OK)
return false;
@@ -455,6 +480,8 @@
}
bool Connection::OpenInternal(const std::string& file_name) {
+ AssertIOAllowed();
+
if (db_) {
DLOG(FATAL) << "sql::Connection is already open.";
return false;
diff --git a/sql/connection.h b/sql/connection.h
index e5c9469..65020a04 100644
--- a/sql/connection.h
+++ b/sql/connection.h
@@ -12,6 +12,7 @@
#include "base/basictypes.h"
#include "base/compiler_specific.h"
#include "base/memory/ref_counted.h"
+#include "base/threading/thread_restrictions.h"
#include "base/time.h"
#include "sql/sql_export.h"
@@ -320,6 +321,14 @@
// sqlite3_open. The string can also be sqlite's special ":memory:" string.
bool OpenInternal(const std::string& file_name);
+ // Check whether the current thread is allowed to make IO calls, but only
+ // if database wasn't open in memory. Function is inlined to be a no-op in
+ // official build.
+ void AssertIOAllowed() {
+ if (!in_memory_)
+ base::ThreadRestrictions::AssertIOAllowed();
+ }
+
// Internal helper for DoesTableExist and DoesIndexExist.
bool DoesTableOrIndexExist(const char* name, const char* type) const;
@@ -356,6 +365,10 @@
// no longer be active.
void Close();
+ // Check whether the current thread is allowed to make IO calls, but only
+ // if database wasn't open in memory.
+ void AssertIOAllowed() { if (connection_) connection_->AssertIOAllowed(); }
+
private:
friend class base::RefCounted<StatementRef>;
@@ -426,6 +439,10 @@
// a rollback instead of a commit.
bool needs_rollback_;
+ // True if database is open with OpenInMemory(), False if database is open
+ // with Open().
+ bool in_memory_;
+
// This object handles errors resulting from all forms of executing sqlite
// commands or statements. It can be null which means default handling.
scoped_refptr<ErrorDelegate> error_delegate_;
diff --git a/sql/statement.cc b/sql/statement.cc
index 3616dcc..84dfd2eb 100644
--- a/sql/statement.cc
+++ b/sql/statement.cc
@@ -48,6 +48,7 @@
}
bool Statement::Run() {
+ ref_->AssertIOAllowed();
if (!CheckValid())
return false;
@@ -55,6 +56,7 @@
}
bool Statement::Step() {
+ ref_->AssertIOAllowed();
if (!CheckValid())
return false;
@@ -62,6 +64,7 @@
}
void Statement::Reset(bool clear_bound_vars) {
+ ref_->AssertIOAllowed();
if (is_valid()) {
// We don't call CheckError() here because sqlite3_reset() returns
// the last error that Step() caused thereby generating a second
