Recreate the WebData database on a catastrophic SQL error

components/webdata/common/web_database_backend.cc

Index: components/webdata/common/web_database_backend.cc
diff --git a/components/webdata/common/web_database_backend.cc b/components/webdata/common/web_database_backend.cc
index 1408bd2b9d8613ab61a2fbae136ff58bd7375a7e..b61198bbd06a6b3bb03c40f33fea35bda9dc68ee 100644
--- a/components/webdata/common/web_database_backend.cc
+++ b/components/webdata/common/web_database_backend.cc
@@ -41,32 +41,6 @@ void WebDatabaseBackend::InitDatabase() {
   }
 }
 
-sql::InitStatus WebDatabaseBackend::LoadDatabaseIfNecessary() {
-  if (init_complete_ || db_path_.empty()) {
-    return init_status_;
-  }
-  init_complete_ = true;
-  db_.reset(new WebDatabase());
-
-  for (const auto& table : tables_)
-    db_->AddTable(table);
-
-  // Unretained to avoid a ref loop since we own |db_|.
-  db_->set_error_callback(base::Bind(&WebDatabaseBackend::DatabaseErrorCallback,
-                                     base::Unretained(this)));
-  diagnostics_.clear();
-  init_status_ = db_->Init(db_path_);
-  if (init_status_ != sql::INIT_OK) {
-    LOG(ERROR) << "Cannot initialize the web database: " << init_status_;
-    diagnostics_ += sql::GetCorruptFileDiagnosticsInfo(db_path_);
-    db_.reset();
-    return init_status_;
-  }
-
-  db_->BeginTransaction();
-  return init_status_;
-}
-
 void WebDatabaseBackend::ShutdownDatabase() {
   if (db_ && init_status_ == sql::INIT_OK)
     db_->CommitTransaction();
@@ -118,12 +92,47 @@ WebDatabaseBackend::~WebDatabaseBackend() {
   ShutdownDatabase();
 }
 
+void WebDatabaseBackend::LoadDatabaseIfNecessary() {
+  if (init_complete_ || db_path_.empty())
+    return;
+
+  init_complete_ = true;
+  db_.reset(new WebDatabase());
+
+  for (const auto& table : tables_)
+    db_->AddTable(table);
+
+  // Unretained to avoid a ref loop since we own |db_|.
+  db_->set_error_callback(base::Bind(&WebDatabaseBackend::DatabaseErrorCallback,
+                                     base::Unretained(this)));
+  diagnostics_.clear();

[Scott Hess - ex-Googler, 2016/08/18 21:51:38]

Also clear catastrophic_error_occurred_, since it's a new open?  Or is the above
just to save memory if everything has gone poorly?

[afakhry, 2016/08/19 18:01:57]

Done.

+  init_status_ = db_->Init(db_path_);
+
+  if (init_status_ == sql::INIT_OK) {
+    // A catastrophic error might have happened and recovered.
+    if (catastrophic_error_occurred_) {
+      init_status_ = sql::INIT_OK_WITH_DATA_LOSS;
+      LOG(WARNING)
+          << "Webdata recovered from a catastrophic error. Data loss possible.";
+      diagnostics_ += sql::GetCorruptFileDiagnosticsInfo(db_path_);
+    }
+    db_->BeginTransaction();
+    return;
+  }
+
+  LOG(ERROR) << "Cannot initialize the web database: " << init_status_;
+  diagnostics_ += sql::GetCorruptFileDiagnosticsInfo(db_path_);
+  db_.reset();

[Scott Hess - ex-Googler, 2016/08/18 21:51:38]

It might make more sense to put this in an init_status != sql::INIT_OK block,
with early return, and put the common/expected case down here.  I think that's
closer to common Chromium style.

[afakhry, 2016/08/19 18:01:57]

Done.

+}
+
 void WebDatabaseBackend::DatabaseErrorCallback(int error,
                                                sql::Statement* statement) {
   // We ignore any further error callbacks after the first catastrophic error.
   if (!catastrophic_error_occurred_ && sql::IsErrorCatastrophic(error)) {
     catastrophic_error_occurred_ = true;
     diagnostics_ = db_->GetDiagnosticInfo(error, statement);
+
+    db_->GetSQLConnection()->RazeAndClose();

[Scott Hess - ex-Googler, 2016/08/18 21:51:38]

Makes sense to me.

Note that this callback could be called after LoadDatabaseIfNecessary() has run
successfully to completion.  This can happen if there is corruption in the
database data, but not in the header data.  The levels I can think of are:
 - header corruption, which should be touched off in sql::Connection::Open().
 - schema corruption, which should be touched off by table creation and
checking.
 - data corruption, which is only noticed when the corrupt data is queried.
I _think_ the second bit is covered by db_->Init(), assuming that does queries
like "CREATE TABLE IF NOT EXISTS" or sql::Connection:DoesTableExist() type
calls.

Anyhow, I think what will happen is that data corruption will result in a raze,
but the analysis data will never be surfaced to the user or sent up.  This
_does_ happen, but I can't really say how often.  It may be small enough not to
worry about.  At one point I thought I had determined that ~25% of corruptions
were detect in scope of sql::Connection::Open(), which is why I introduced the
retry logic, but it's hard to make general statements based on research I did a
few years ago ...

[afakhry, 2016/08/19 18:01:57]

It seems that WebDatabase::Init() does a lot of the things that make it most
likely that a DB corruption will be detected by the time it returns. Things like
Connection::Open(), MetaTable::Init(), and
WebDatabaseTable::CreateTablesIfNecessary(), which you already mentioned above.
This makes me less worried about the scenario you mentioned. Am I correct?

   }
 }