Offer user to send feedback from profile error dialog

components/history/core/browser/history_backend.cc

Index: components/history/core/browser/history_backend.cc
diff --git a/components/history/core/browser/history_backend.cc b/components/history/core/browser/history_backend.cc
index 42732d85f1fe31ec625d0eedd10747b901a47a50..31871ee97e43db6d2ec0aa107d256f94d98eeaa5 100644
--- a/components/history/core/browser/history_backend.cc
+++ b/components/history/core/browser/history_backend.cc
@@ -68,12 +68,18 @@ using base::TimeTicks;
 namespace history {
 
 namespace {
+
 void RunUnlessCanceled(
     const base::Closure& closure,
     const base::CancelableTaskTracker::IsCanceledCallback& is_canceled) {
   if (!is_canceled.Run())
     closure.Run();
 }
+
+void NullErrorHandler(int error, sql::Statement* stmt) {
+  // Does nothing. Prevents DCHECKING on SQLITE errors on Debug builds.
+}

[Scott Hess - ex-Googler, 2016/07/10 05:16:52]

This makes me suspect you have something going on and don't really know why, so
you're just arbitrarily hacking around it.  I don't know what you have going on,
as I see no questions about it in my inbox.  The DCHECKs are there for a reason,
honestly mostly because people don't write good test coverage for their code. 
ScopedErrorExpecter allows testing around those DCHECKs.

[afakhry, 2016/07/11 16:47:45]

When I try to prevent reentrant error callbacks in line 2423, if I clear the
error callback (so none exists) and hit a sqlite error, I will crash on here:
https://cs.chromium.org/chromium/src/sql/connection.cc?q=Connection::OnSqlite....

[Scott Hess - ex-Googler, 2016/07/13 01:18:46]

I have written a number of tests which test induced error conditions in the
code, so your explanation is not sufficient.  Maybe you could describe what
you're doing when you get the crash?  Like is it a test or something like that?

[afakhry, 2016/07/13 19:57:58]

I think this is not needed now. What I used to do was induce a corruption in the
db file by writing to it arbitrary values. The crash I used to get was due to
running the FullIntegrityCheck() in all cases even if the DB file was known to
be corrupt. The debug crash stack trace was as follows:

[6695:6730:0713/082407:FATAL:connection.cc(1912)] file is encrypted or is not a
database
#0 0x2ac034ebf3fe base::debug::StackTrace::StackTrace()
#1 0x2ac034f171dc logging::LogMessage::~LogMessage()
#2 0x2ac04108a824 sql::Connection::OnSqliteError()
#3 0x2ac04108ad89 sql::Connection::GetStatementImpl()
#4 0x2ac041087547 sql::Connection::GetUniqueStatement()
#5 0x2ac04108b927 sql::Connection::IntegrityCheckHelper()
#6 0x2ac041086bb7 sql::Connection::FullIntegrityCheck()
#7 0x2ac04108bc62 sql::Connection::GetDiagnosticMap()
#8 0x2ac02e75f880 history::HistoryDatabase::GetDiagnosticMap()
#9 0x2ac02e72cd3b history::HistoryBackend::DatabaseErrorCallback()

But I already changed that a couple of patches earlier, and made sure we don't
run FullIntegrityCheck() nor GetSchema() unless both:
"PRAGMA auto_vacuum" and
"SELECT COUNT(*) FROM sqlite_master"
Succeed.

+
 }  // namespace
 
 // How long we'll wait to do a commit, so that things are batched together.
@@ -206,6 +212,8 @@ HistoryBackend::HistoryBackend(
     scoped_refptr<base::SequencedTaskRunner> task_runner)
     : delegate_(delegate),
       scheduled_kill_db_(false),
+      error_callback_(base::Bind(&HistoryBackend::DatabaseErrorCallback,
+                                 base::Unretained(this))),
       expirer_(this, backend_client.get(), task_runner),
       recent_redirects_(kMaxRedirectCount),
       backend_destroy_message_loop_(nullptr),
@@ -648,16 +656,15 @@ void HistoryBackend::InitImpl(
       history_database_params.download_interrupt_reason_none,
       history_database_params.download_interrupt_reason_crash));
 
-  // Unretained to avoid a ref loop with db_.
-  db_->set_error_callback(base::Bind(&HistoryBackend::DatabaseErrorCallback,
-                                     base::Unretained(this)));
+  db_->set_error_callback(error_callback_);
 
   sql::InitStatus status = db_->Init(history_name);
   switch (status) {
     case sql::INIT_OK:
       break;
     case sql::INIT_TOO_NEW:
-      delegate_->NotifyProfileError(status);
+      db_diagnostics_["Corrupted file"] = history_name.AsUTF8Unsafe();
+      delegate_->NotifyProfileError(status, db_diagnostics_);
       db_.reset();
       return;
     case sql::INIT_FAILURE: {
@@ -669,7 +676,8 @@ void HistoryBackend::InitImpl(
       if (kill_db)
         KillHistoryDatabase();
       UMA_HISTOGRAM_BOOLEAN("History.AttemptedToFixProfileError", kill_db);
-      delegate_->NotifyProfileError(status);
+      db_diagnostics_["Corrupted file"] = history_name.AsUTF8Unsafe();
+      delegate_->NotifyProfileError(status, db_diagnostics_);
       db_.reset();
       return;
     }
@@ -2410,6 +2418,12 @@ void HistoryBackend::URLsNoLongerBookmarked(const std::set<GURL>& urls) {
 void HistoryBackend::DatabaseErrorCallback(int error, sql::Statement* stmt) {
   if (!scheduled_kill_db_ && sql::IsErrorCatastrophic(error)) {
     scheduled_kill_db_ = true;
+
+    // Gather diagnostic info, but prevent reentrant error callbacks.
+    db_->set_error_callback(base::Bind(&NullErrorHandler));
+    db_diagnostics_ = db_->GetDiagnosticMap();

[michaeln, 2016/07/12 20:05:32]

This function is similar to Connection::ReportDiagnosticInfo which internally
resets and restores the error callback. I think getting the error handler out of
the way is probably something the new method should do was well.

[Scott Hess - ex-Googler, 2016/07/13 01:18:46]

This is reasonable - note the comment in that code, it's happening because
IntegrityCheck is implemented in terms of Connection/Statement, and it needs to
prevent reentrant calls.  But it's constraining the blocking to sql::
implementation code so it doesn't accidentally mask newly-introduced errors.

[afakhry, 2016/07/13 19:57:58]

Done.

+    db_->set_error_callback(error_callback_);
+
     // Don't just do the close/delete here, as we are being called by |db| and
     // that seems dangerous.
     // TODO(shess): Consider changing KillHistoryDatabase() to use