Recreate the WebData database on a catastrophic SQL error

sql/connection.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sql/connection.h"
 
 #include <limits.h>
 #include <stddef.h>
 #include <stdint.h>
 #include <string.h>
@@ skipping 13 matching lines @@
 #include "base/metrics/sparse_histogram.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/memory_dump_manager.h"
 #include "sql/connection_memory_dump_provider.h"
+#include "sql/error_delegate_util.h"
 #include "sql/meta_table.h"
 #include "sql/statement.h"
 #include "third_party/sqlite/sqlite3.h"
 
 #if defined(OS_IOS) && defined(USE_SYSTEM_SQLITE)
 #include "base/ios/ios_util.h"
 #include "third_party/sqlite/src/ext/icu/sqliteicu.h"
 #endif
 
 namespace {
@@ skipping 1649 matching lines @@
   err = sqlite3_extended_result_codes(db_, 1);
   DCHECK_EQ(err, SQLITE_OK) << "Could not enable extended result codes";
 
   // sqlite3_open() does not actually read the database file (unless a
   // hot journal is found).  Successfully executing this pragma on an
   // existing database requires a valid header on page 1.
   // TODO(shess): For now, just probing to see what the lay of the
   // land is.  If it's mostly SQLITE_NOTADB, then the database should
   // be razed.
   err = ExecuteAndReturnErrorCode("PRAGMA auto_vacuum");
-  if (err != SQLITE_OK)
+  if (err != SQLITE_OK) {
     UMA_HISTOGRAM_SPARSE_SLOWLY("Sqlite.OpenProbeFailure", err);
+    if (IsErrorCatastrophic(err)) {
+      OnSqliteError(err, nullptr, "PRAGMA auto_vacuum");
+      return false;

[afakhry, 2016/08/09 21:36:17]

shess@, I need your advice about this code.

The reason I added it is because I want to avoid the following crash on Debug
builds when the WebData DB file is corrupted:

[4470:4489:0809/140648:FATAL:statement.cc(52)] Cannot call mutating statements
on an invalid statement.
#0 0x2b105313d14e base::debug::StackTrace::StackTrace()
#1 0x2b1053198b1c logging::LogMessage::~LogMessage()
#2 0x2b1055b4f892 sql::Statement::CheckValid()
#3 0x2b1055b4f8e2 sql::Statement::StepInternal()
#4 0x2b1055b4fd8a sql::Statement::Step()
#5 0x2b1055b4a178 sql::MetaTable::RazeIfDeprecated()
#6 0x2b1055b0afa4 WebDatabase::Init()
#7 0x2b1055b0e785 WebDatabaseBackend::LoadDatabaseIfNecessary()
#8 0x2b1055b0e549 WebDatabaseBackend::InitDatabase()
#9 0x2b1055b13382
_ZN4base8internal13FunctorTraitsIM18WebDatabaseBackendFvvEvE6InvokeIRK13scoped_refptrIS2_EJEEEvS4_OT_DpOT0_
#10 0x2b1055b132d1
_ZN4base8internal12InvokeHelperILb0EvE8MakeItSoIRKM18WebDatabaseBackendFvvEJRK13scoped_refptrIS4_EEEEvOT_DpOT0_
#11 0x2b1055b13272
_ZN4base8internal7InvokerINS0_9BindStateIM18WebDatabaseBackendFvvEJ13scoped_refptrIS3_EEEEFvvEE7RunImplIRKS5_RKSt5tupleIJS7_EEJLm0EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE
#12 0x2b1055b12e4c
_ZN4base8internal7InvokerINS0_9BindStateIM18WebDatabaseBackendFvvEJ13scoped_refptrIS3_EEEEFvvEE3RunEPNS0_13BindStateBaseE
#13 0x2b1053120d5e base::Callback<>::Run()
#14 0x2b1053142a4e base::debug::TaskAnnotator::RunTask()
#15 0x2b10531b5551 base::MessageLoop::RunTask()
#16 0x2b10531b57d4 base::MessageLoop::DeferOrRunPendingTask()
#17 0x2b10531b5a9e base::MessageLoop::DoWork()
#18 0x2b10531ca8a4 base::MessagePumpDefault::Run()
#19 0x2b10531b4fba base::MessageLoop::RunHandler()
#20 0x2b105324d694 base::RunLoop::Run()
#21 0x2b10532e26d1 base::Thread::Run()
#22 0x2b1057e88648 content::BrowserThreadImpl::DBThreadRun()
#23 0x2b1057e88c38 content::BrowserThreadImpl::Run()
#24 0x2b10532e2cab base::Thread::ThreadMain()
#25 0x2b10532cf58a base::(anonymous namespace)::ThreadFunc()
#26 0x2b1068615184 start_thread
#27 0x2b106892537d clone


I wanted to make sure that a DB with file that's either SQLITE_NOTADB or
SQLITE_CORRUPT is not openable to return early at:
https://cs.chromium.org/chromium/src/components/webdata/common/web_database.c...

and skip the sql::MetaTable::RazeIfDeprecated()
https://cs.chromium.org/chromium/src/components/webdata/common/web_database.c...
that causes the crash.

Unfortunately a lot of tests expect that opening a DB in this state is
successful, so I'm not sure how to overcome this problem.

[Scott Hess - ex-Googler, 2016/08/10 17:16:19]

I'm not sure what your question is.  I wouldn't expect you to see enough
corruption that having this corrupt ever comes up as a problem in debug builds. 
So what's the context where the corruption is happening?  A test or something
like that?

If I read the existing code correctly, it's probably using
ExecuteAndReturnErrorCode() Specifically to avoid calling the error callback. 
Otherwise it could have just used plain Execute().

[afakhry, 2016/08/11 17:32:54]

I'm forcing a corruption in the Web Data file on a debug ChromeOS Linux build
just for testing. My question was if there's a way to avoid that crash by making
sure opening the corrupt db fails so that we can skip running
sql::MetaTable::RazeIfDeprecated() which causes the crash. But it seems that
opening a corrupt db is expected to succeed!

With the solution in the new patch, this is not needed. However I found that
razing a NOTADB database always fails at: sqlite3_backup_step(backup, -1);
[https://cs.chromium.org/chromium/src/sql/connection.cc?type=cs&q=%22sqlite3_backup_step(backup,+-1);%22&sq=package:chromium&l=102]
which returns NOTADB as well, and hence we hit this DCHECK here
https://cs.chromium.org/chromium/src/sql/connection.cc?type=cs&q=Connection::....
So maybe the assumptions need to be revised?

If debug builds are not something we need to worry about, then we can ignore
this comment.

+    }
+  }
 
 #if defined(OS_IOS) && defined(USE_SYSTEM_SQLITE)
   // The version of SQLite shipped with iOS doesn't enable ICU, which includes
   // REGEXP support. Add it in dynamically.
   err = sqlite3IcuInit(db_);
   DCHECK_EQ(err, SQLITE_OK) << "Could not enable ICU support";
 #endif  // OS_IOS && USE_SYSTEM_SQLITE
 
   // If indicated, lock up the database before doing anything else, so
   // that the following code doesn't have to deal with locking.
@@ skipping 259 matching lines @@
   ignore_result(Execute(kNoWritableSchema));
 
   return ret;
 }
 
 base::TimeTicks TimeSource::Now() {
   return base::TimeTicks::Now();
 }
 
 }  // namespace sql