[TaskScheduler]: Migrate off of AssertBlockingAllowedDeprecated in /sql
base::AssertBlockingAllowedDeprecated is deprecated in favor of
ScopedBlockingCall, which serves as a precise annotation of
the scope that may/will block.
Please make sure of the following:
- ScopedBlockingCall is instantiated in a scope with minimal CPU usage.
If this is not the case, ScopedBlockingCall should be instantiated
closer to the blocking call. See scoped_blocking_call.h for more
info. Please let me know when/where the blocking call happens if this needs
to be changed.
- Parameter |blocking_type| matches expectation:
MAY_BLOCK: The call might block (e.g. file I/O that might hit in memory cache).
WILL_BLOCK: The call will definitely block (e.g. cache already checked and now pinging
server synchronously).
See BlockingType for more info. While I assumed MAY_BLOCK by default, that might
not be the best fit if we know that this callsite is guaranteed to block.
- The ScopedBlockingCall's scope covers the entirety of the blocking operation
previously asserted against by the AssertBlockingAllowed().
- Calls to blocking //base APIs don't need to be annotated
with ScopedBlockingCall. All blocking //base APIs (e.g. base::ReadFileToString, base::File::Read,
base::SysInfo::AmountOfFreeDiskSpace, base::WaitableEvent::Wait, etc.) have their
own internal annotations.
Refer to the top-level CL if necessary :
https://chromium-review.googlesource.com/c/chromium/src/+/1338391
Please CQ if LGTY!
This CL was uploaded by git cl split.
R=cmumford@chromium.org
Bug: 903957
Change-Id: Ia7d71b3bc2536dad0e6adc669aebe36addfd7fd0
Reviewed-on: https://chromium-review.googlesource.com/c/1365805
Commit-Queue: Victor Costan <pwnall@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Chris Mumford <cmumford@google.com>
Cr-Commit-Position: refs/heads/master@{#629836}diff --git a/sql/database.cc b/sql/database.cc
index e820382..3586bef 100644
--- a/sql/database.cc
+++ b/sql/database.cc
@@ -179,8 +179,6 @@
}
void Database::ReportDiagnosticInfo(int extended_error, Statement* stmt) {
- AssertIOAllowed();
-
std::string debug_info = GetDiagnosticInfo(extended_error, stmt);
if (!debug_info.empty() && RegisterIntentToUpload()) {
DEBUG_ALIAS_FOR_CSTR(debug_buf, debug_info.c_str(), 2000);
@@ -231,14 +229,15 @@
void Database::StatementRef::Close(bool forced) {
if (stmt_) {
- // Call to AssertIOAllowed() cannot go at the beginning of the function
- // because Close() is called unconditionally from destructor to clean
- // database_. 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.
+ // Call to InitScopedBlockingCall() cannot go at the beginning of the
+ // function because Close() is called unconditionally from destructor to
+ // clean database_. 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();
+ base::Optional<base::ScopedBlockingCall> scoped_blocking_call;
+ InitScopedBlockingCall(&scoped_blocking_call);
sqlite3_finalize(stmt_);
stmt_ = nullptr;
}
@@ -385,13 +384,14 @@
open_statements_.clear();
if (db_) {
- // Call to AssertIOAllowed() cannot go at the beginning of the function
- // because Close() must be called from destructor to clean
+ // Call to InitScopedBlockingCall() 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();
+ base::Optional<base::ScopedBlockingCall> scoped_blocking_call;
+ InitScopedBlockingCall(&scoped_blocking_call);
// Reseting acquires a lock to ensure no dump is happening on the database
// at the same time. Unregister takes ownership of provider and it is safe
@@ -426,7 +426,8 @@
}
void Database::Preload() {
- AssertIOAllowed();
+ base::Optional<base::ScopedBlockingCall> scoped_blocking_call;
+ InitScopedBlockingCall(&scoped_blocking_call);
if (!db_) {
DCHECK(poisoned_) << "Cannot preload null db";
@@ -557,8 +558,6 @@
static const char* kDiagnosticDumpsKey = "DiagnosticDumps";
static int kVersion = 1;
- AssertIOAllowed();
-
if (histogram_tag_.empty())
return false;
@@ -740,8 +739,6 @@
// TODO(shess): Since this is only called in an error situation, it might be
// prudent to rewrite in terms of SQLite API calls, and mark the function const.
std::string Database::CollectCorruptionInfo() {
- AssertIOAllowed();
-
// If the file cannot be accessed it is unlikely that an integrity check will
// turn up actionable information.
const base::FilePath db_path = DbPath();
@@ -825,7 +822,8 @@
}
size_t Database::GetAppropriateMmapSize() {
- AssertIOAllowed();
+ base::Optional<base::ScopedBlockingCall> scoped_blocking_call;
+ InitScopedBlockingCall(&scoped_blocking_call);
// How much to map if no errors are found. 50MB encompasses the 99th
// percentile of Chrome databases in the wild, so this should be good.
@@ -968,7 +966,8 @@
// Create an in-memory database with the existing database's page
// size, then backup that database over the existing database.
bool Database::Raze() {
- AssertIOAllowed();
+ base::Optional<base::ScopedBlockingCall> scoped_blocking_call;
+ InitScopedBlockingCall(&scoped_blocking_call);
if (!db_) {
DCHECK(poisoned_) << "Cannot raze null db";
@@ -1269,7 +1268,9 @@
// caller wishes to execute multiple statements, that should be explicit, and
// perhaps tucked into an explicit transaction with rollback in case of error.
int Database::ExecuteAndReturnErrorCode(const char* sql) {
- AssertIOAllowed();
+ base::Optional<base::ScopedBlockingCall> scoped_blocking_call;
+ InitScopedBlockingCall(&scoped_blocking_call);
+
if (!db_) {
DCHECK(poisoned_) << "Illegal use of Database without a db";
return SQLITE_ERROR;
@@ -1396,7 +1397,8 @@
scoped_refptr<Database::StatementRef> Database::GetStatementImpl(
sql::Database* tracking_db,
const char* sql) const {
- AssertIOAllowed();
+ base::Optional<base::ScopedBlockingCall> scoped_blocking_call;
+ InitScopedBlockingCall(&scoped_blocking_call);
DCHECK(sql);
DCHECK(!tracking_db || tracking_db == this);
@@ -1446,7 +1448,8 @@
}
bool Database::IsSQLValid(const char* sql) {
- AssertIOAllowed();
+ base::Optional<base::ScopedBlockingCall> scoped_blocking_call;
+ InitScopedBlockingCall(&scoped_blocking_call);
if (!db_) {
DCHECK(poisoned_) << "Illegal use of Database without a db";
return false;
@@ -1542,7 +1545,8 @@
bool Database::OpenInternal(const std::string& file_name,
Database::Retry retry_flag) {
- AssertIOAllowed();
+ base::Optional<base::ScopedBlockingCall> scoped_blocking_call;
+ InitScopedBlockingCall(&scoped_blocking_call);
if (db_) {
DLOG(DCHECK) << "sql::Database is already open.";
diff --git a/sql/database.h b/sql/database.h
index f4b6b29..4ea5d77 100644
--- a/sql/database.h
+++ b/sql/database.h
@@ -20,9 +20,9 @@
#include "base/gtest_prod_util.h"
#include "base/macros.h"
#include "base/memory/ref_counted.h"
+#include "base/optional.h"
#include "base/sequence_checker.h"
#include "base/threading/scoped_blocking_call.h"
-#include "base/threading/thread_restrictions.h"
#include "base/time/tick_clock.h"
#include "sql/internal_api_token.h"
#include "sql/statement_id.h"
@@ -554,12 +554,12 @@
// |forced| indicates that orderly-shutdown checks should not apply.
void CloseInternal(bool forced);
- // 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() const {
+ // Construct a ScopedBlockingCall to annotate IO calls, but only if
+ // database wasn't open in memory.
+ void InitScopedBlockingCall(
+ base::Optional<base::ScopedBlockingCall>* scoped_blocking_call) const {
if (!in_memory_)
- base::AssertBlockingAllowedDeprecated();
+ scoped_blocking_call->emplace(base::BlockingType::MAY_BLOCK);
}
// Internal helper for Does*Exist() functions.
@@ -622,11 +622,12 @@
// orderly-shutdown checks should apply (see Database::RazeAndClose()).
void Close(bool forced);
- // Check whether the current thread is allowed to make IO calls, but only
- // if database wasn't open in memory.
- void AssertIOAllowed() const {
+ // Construct a ScopedBlockingCall to annotate IO calls, but only if
+ // database wasn't open in memory.
+ void InitScopedBlockingCall(
+ base::Optional<base::ScopedBlockingCall>* scoped_blocking_call) const {
if (database_)
- database_->AssertIOAllowed();
+ database_->InitScopedBlockingCall(scoped_blocking_call);
}
private:
diff --git a/sql/statement.cc b/sql/statement.cc
index e1f8509..1ae42a1 100644
--- a/sql/statement.cc
+++ b/sql/statement.cc
@@ -54,7 +54,8 @@
}
int Statement::StepInternal(bool timer_flag) {
- ref_->AssertIOAllowed();
+ base::Optional<base::ScopedBlockingCall> scoped_blocking_call;
+ ref_->InitScopedBlockingCall(&scoped_blocking_call);
if (!CheckValid())
return SQLITE_ERROR;
@@ -98,7 +99,8 @@
}
void Statement::Reset(bool clear_bound_vars) {
- ref_->AssertIOAllowed();
+ base::Optional<base::ScopedBlockingCall> scoped_blocking_call;
+ ref_->InitScopedBlockingCall(&scoped_blocking_call);
if (is_valid()) {
if (clear_bound_vars)
sqlite3_clear_bindings(ref_->stmt());
