sql: Remove DCHECKs on SQL compilation errors.

SQL compilation errors can be caused by database schema corruption,
which can occur in normal operation -- data written to disk is always
subject to corruption.

Bug: 839186
Change-Id: I639214dee6db6d6df9c4611b5dae3e4f70cca207
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3015299
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#899985}

sql/database.cc

@@ -1172,12 +1172,20 @@ bool Database::Execute(const char* sql) {
   if (error != SQLITE_OK)
     error = OnSqliteError(error, nullptr, sql);
 
-  // This needs to be a FATAL log because the error case of arriving here is
-  // that there's a malformed SQL statement. This can arise in development if
-  // a change alters the schema but not all queries adjust.  This can happen
-  // in production if the schema is corrupted.
-  DCHECK_NE(error, SQLITE_ERROR)
-      << "SQL Error in " << sql << ", " << GetErrorMessage();
+#if DCHECK_IS_ON()
+  // Report SQL compilation errors. On developer machines, the errors are most
+  // likely caused by invalid SQL in an under-development feature. In
+  // production, SQL compilation errors are caused by database schema
+  // corruption.
+  //
+  // DCHECK would not be appropriate here, because on-disk data is always
+  // subject to corruption, so Chrome cannot assume that the database schema
+  // will remain intact.
+  if (error == SQLITE_ERROR) {
+    DLOG(ERROR) << "SQL compilation error: " << GetErrorMessage()
+                << ". Statement: " << sql;
+  }
+#endif  // DCHECK_IS_ON()
   return error == SQLITE_OK;
 }
 
@@ -1244,11 +1252,23 @@ scoped_refptr<Database::StatementRef> Database::GetStatementImpl(
   int rc = sqlite3_prepare_v3(db_, sql, /* nByte= */ -1, /* prepFlags= */ 0,
                               &sqlite_statement, /* pzTail= */ nullptr);
   if (rc != SQLITE_OK) {
-    // This is evidence of a syntax error in the incoming SQL.
-    DCHECK_NE(rc, SQLITE_ERROR) << "SQL compile error " << GetErrorMessage();
-
-    // It could also be database corruption.
     OnSqliteError(rc, nullptr, sql);
+
+#if DCHECK_IS_ON()
+    // Report SQL compilation errors. On developer machines, the errors are most
+    // likely caused by invalid SQL in an under-development feature. In
+    // production, SQL compilation errors are caused by database schema
+    // corruption.
+    //
+    // DCHECK would not be appropriate here, because on-disk data is always
+    // subject to corruption, so Chrome cannot assume that the database schema
+    // will remain intact.
+    if (rc == SQLITE_ERROR) {
+      DLOG(ERROR) << "SQL compilation error: " << GetErrorMessage()
+                  << ". Statement: " << sql;
+    }
+#endif  // DCHECK_IS_ON()
+
     return base::MakeRefCounted<StatementRef>(nullptr, nullptr, false);
   }
   return base::MakeRefCounted<StatementRef>(tracking_db, sqlite_statement,
@@ -1628,35 +1648,39 @@ void Database::set_histogram_tag(const std::string& tag) {
   histogram_tag_ = tag;
 }
 
-int Database::OnSqliteError(int err,
-                            sql::Statement* stmt,
+int Database::OnSqliteError(int sqlite_error_code,
+                            sql::Statement* statement,
                             const char* sql) const {
   TRACE_EVENT0("sql", "Database::OnSqliteError");
 
-  // Always log the error.
-  if (!sql && stmt)
-    sql = stmt->GetSQLStatement();
-  if (!sql)
-    sql = "-- unknown";
+  bool is_expected_error = IsExpectedSqliteError(sqlite_error_code);
+  if (!is_expected_error) {
+    // Log unexpected errors.
+    if (!sql && statement)
+      sql = statement->GetSQLStatement();
+    if (!sql)
+      sql = "(SQL unknown)";
 
-  std::string id = histogram_tag_;
-  if (id.empty())
-    id = DbPath().BaseName().AsUTF8Unsafe();
-  LOG(ERROR) << id << " sqlite error " << err << ", errno " << GetLastErrno()
-             << ": " << GetErrorMessage() << ", sql: " << sql;
+    std::string id = histogram_tag_;
+    if (id.empty())
+      id = DbPath().BaseName().AsUTF8Unsafe();
+    LOG(ERROR) << id << " SQLite error: code " << sqlite_error_code << " errno "
+               << GetLastErrno() << ": " << GetErrorMessage()
+               << " sql: " << sql;
+  }
 
   if (!error_callback_.is_null()) {
     // Fire from a copy of the callback in case of reentry into
     // re/set_error_callback().
     // TODO(shess): <http://crbug.com/254584>
-    ErrorCallback(error_callback_).Run(err, stmt);
-    return err;
+    ErrorCallback(error_callback_).Run(sqlite_error_code, statement);
+    return sqlite_error_code;
   }
 
   // The default handling is to assert on debug and to ignore on release.
-  if (!IsExpectedSqliteError(err))
+  if (!is_expected_error)
     DLOG(DCHECK) << GetErrorMessage();
-  return err;
+  return sqlite_error_code;
 }
 
 bool Database::FullIntegrityCheck(std::vector<std::string>* messages) {

sql/database_unittest.cc

@@ -12,6 +12,7 @@
 #include "base/files/scoped_temp_dir.h"
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/test/bind.h"
 #include "base/test/gtest_util.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/trace_event/process_memory_dump.h"
@@ -433,6 +434,79 @@ TEST_P(SQLDatabaseTest, ErrorCallback) {
   }
 }
 
+TEST_P(SQLDatabaseTest, Execute_CompilationError) {
+  bool error_callback_called = false;
+  db_->set_error_callback(base::BindLambdaForTesting([&](int error,
+                                                         sql::Statement*
+                                                             statement) {
+    EXPECT_EQ(SQLITE_ERROR, error);
+    EXPECT_EQ(nullptr, statement);
+    EXPECT_FALSE(error_callback_called)
+        << "SQL compilation errors should call the error callback exactly once";
+    error_callback_called = true;
+  }));
+
+  {
+    sql::test::ScopedErrorExpecter expecter;
+    expecter.ExpectError(SQLITE_ERROR);
+    EXPECT_FALSE(db_->Execute("SELECT missing_column FROM missing_table"));
+    EXPECT_TRUE(expecter.SawExpectedErrors());
+  }
+
+  EXPECT_TRUE(error_callback_called)
+      << "SQL compilation errors should call the error callback";
+}
+
+TEST_P(SQLDatabaseTest, GetUniqueStatement_CompilationError) {
+  bool error_callback_called = false;
+  db_->set_error_callback(base::BindLambdaForTesting([&](int error,
+                                                         sql::Statement*
+                                                             statement) {
+    EXPECT_EQ(SQLITE_ERROR, error);
+    EXPECT_EQ(nullptr, statement);
+    EXPECT_FALSE(error_callback_called)
+        << "SQL compilation errors should call the error callback exactly once";
+    error_callback_called = true;
+  }));
+
+  {
+    sql::test::ScopedErrorExpecter expecter;
+    expecter.ExpectError(SQLITE_ERROR);
+    sql::Statement statement(
+        db_->GetUniqueStatement("SELECT missing_column FROM missing_table"));
+    EXPECT_FALSE(statement.is_valid());
+    EXPECT_TRUE(expecter.SawExpectedErrors());
+  }
+
+  EXPECT_TRUE(error_callback_called)
+      << "SQL compilation errors should call the error callback";
+}
+
+TEST_P(SQLDatabaseTest, GetCachedStatement_CompilationError) {
+  bool error_callback_called = false;
+  db_->set_error_callback(base::BindLambdaForTesting([&](int error,
+                                                         sql::Statement*
+                                                             statement) {
+    EXPECT_EQ(SQLITE_ERROR, error);
+    EXPECT_EQ(nullptr, statement);
+    EXPECT_FALSE(error_callback_called)
+        << "SQL compilation errors should call the error callback exactly once";
+    error_callback_called = true;
+  }));
+
+  {
+    sql::test::ScopedErrorExpecter expecter;
+    expecter.ExpectError(SQLITE_ERROR);
+    sql::Statement statement(db_->GetCachedStatement(
+        SQL_FROM_HERE, "SELECT missing_column FROM missing_table"));
+    EXPECT_FALSE(statement.is_valid());
+    EXPECT_TRUE(expecter.SawExpectedErrors());
+  }
+
+  EXPECT_TRUE(error_callback_called)
+      << "SQL compilation errors should call the error callback";
+}
+
 // Test that Database::Raze() results in a database without the
 // tables from the original database.
 TEST_P(SQLDatabaseTest, Raze) {
@@ -1407,20 +1481,6 @@ TEST_P(SQLDatabaseTest, CorruptSizeInHeaderTest) {
   }
 }
 
-// To prevent invalid SQL from accidentally shipping to production, prepared
-// statements which fail to compile with SQLITE_ERROR call DLOG(DCHECK).  This
-// case cannot be suppressed with an error callback.
-TEST_P(SQLDatabaseTest, CompileError) {
-// DEATH tests not supported on Android, iOS, or Fuchsia.
-#if !defined(OS_ANDROID) && !defined(OS_IOS) && !defined(OS_FUCHSIA)
-  if (DLOG_IS_ON(FATAL)) {
-    db_->set_error_callback(base::BindRepeating(&IgnoreErrorCallback));
-    ASSERT_DEATH({ db_->GetUniqueStatement("SELECT x"); },
-                 "SQL compile error no such column: x");
-  }
-#endif  // !defined(OS_ANDROID) && !defined(OS_IOS) && !defined(OS_FUCHSIA)
-}
-
 // WAL mode is currently not supported on Fuchsia.
 #if !defined(OS_FUCHSIA)
 INSTANTIATE_TEST_SUITE_P(JournalMode, SQLDatabaseTest, testing::Bool());