Offer user to send feedback from profile error dialog

components/history/core/browser/history_backend.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 "components/history/core/browser/history_backend.h"
 
 #include <algorithm>
 #include <functional>
 #include <list>
 #include <map>
@@ skipping 50 matching lines @@
       DownloadDatabase (stores a list of downloads)
       VisitDatabase (stores a list of visits for the URLs)
       VisitSegmentDatabase (stores groups of URLs for the most visited view).
 
     ExpireHistoryBackend (manages deleting things older than 3 months)
 */
 
 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.
 const int kCommitIntervalSeconds = 10;
 
 // The amount of time before we re-fetch the favicon.
 const int kFaviconRefetchDays = 7;
 
 // The maximum number of items we'll allow in the redirect list before
 // deleting some.
@@ skipping 112 matching lines @@
 }
 
 // HistoryBackend --------------------------------------------------------------
 
 HistoryBackend::HistoryBackend(
     Delegate* delegate,
     std::unique_ptr<HistoryBackendClient> backend_client,
     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),
       segment_queried_(false),
       backend_client_(std::move(backend_client)),
       task_runner_(task_runner) {}
 
 HistoryBackend::~HistoryBackend() {
   DCHECK(!scheduled_commit_) << "Deleting without cleanup";
   STLDeleteContainerPointers(queued_history_db_tasks_.begin(),
@@ skipping 422 matching lines @@
   base::FilePath thumbnail_name = GetFaviconsFileName();
 
   // Delete the old index database files which are no longer used.
   DeleteFTSIndexDatabases();
 
   // History database.
   db_.reset(new HistoryDatabase(
       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: {
       // A null db_ will cause all calls on this object to notice this error
       // and to not continue. If the error callback scheduled killing the
       // database, the task it posted has not executed yet. Try killing the
       // database now before we close it.
       bool kill_db = scheduled_kill_db_;
       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;
     }
     default:
       NOTREACHED();
   }
 
   // Fill the in-memory database and send it back to the history service on the
   // main thread.
   {
@@ skipping 1720 matching lines @@
     // that we can delete all associated icons in the case of deleting an
     // unvisited bookmarked URL.
     if (visits.empty())
       expirer_.DeleteURL(*i);  // There are no more visits; nuke the URL.
   }
 }
 
 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
     // RazeAndClose().  Then it can be cleared immediately.
     task_runner_->PostTask(
         FROM_HERE, base::Bind(&HistoryBackend::KillHistoryDatabase, this));
   }
 }
 
 void HistoryBackend::KillHistoryDatabase() {
@@ skipping 200 matching lines @@
   // transaction is currently open.
   db_->CommitTransaction();
   db_->Vacuum();
   db_->BeginTransaction();
   db_->GetStartDate(&first_recorded_time_);
 
   return true;
 }
 
 }  // namespace history