[Backport] Security bug 1306507

Cherry-pick of patch originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/3538001:
history: don't handle db error during destruction

Handling of db errors is delayed using a posttask. ~HistoryBackend
closes all the dbs. If closing the db results in an error, then
a PostTask() is scheduled with a HistoryBackend that is part way
through deletion. When the PostTask() runs, we get a uaf.

This patch resets the error callback in ~HistoryBackend to ensure
this doesn't happen. This means a db error is effectively ignored
during shutdown. Presumably if the error is fatal, it'll be handled
when the HistoryBackend is created again.

BUG=1306507
TEST=none

Change-Id: Ic158589a43e7bc2fd1f602fb2798ab500dc8d6d7
Reviewed-by: Victor Costan <pwnall@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/main@{#983478}
Reviewed-by: Michal Klocek <michal.klocek@qt.io>

2 files changed, 10 insertions, 0 deletions

diff --git a/chromium/components/history/core/browser/history_backend.cc b/chromium/components/history/core/browser/history_backend.cc
index ab9df06777a..6470d31cb1c 100644
--- a/chromium/components/history/core/browser/history_backend.cc
+++ b/chromium/components/history/core/browser/history_backend.cc
@@ -276,6 +276,15 @@ HistoryBackend::~HistoryBackend() {
   // Release stashed embedder object before cleaning up the databases.
   supports_user_data_helper_.reset();
 
+  // Clear the error callback. The error callback that is installed does not
+  // process an error immediately, rather it uses a PostTask() with `this`. As
+  // `this` is being deleted, scheduling a PostTask() with `this` would be
+  // fatal (use-after-free). Additionally, as we're in shutdown, there isn't
+  // much point in trying to handle the error. If the error is really fatal,
+  // we'll cleanup the next time the backend is created.
+  if (db_)
+    db_->reset_error_callback();
+
   // First close the databases before optionally running the "destroy" task.
   CloseAllDatabases();
 
diff --git a/chromium/components/history/core/browser/history_database.h b/chromium/components/history/core/browser/history_database.h
index 841da9e21aa..fe4ccb0975d 100644
--- a/chromium/components/history/core/browser/history_database.h
+++ b/chromium/components/history/core/browser/history_database.h
@@ -81,6 +81,7 @@ class HistoryDatabase : public DownloadDatabase,
   void set_error_callback(const sql::Database::ErrorCallback& error_callback) {
     db_.set_error_callback(error_callback);
   }
+  void reset_error_callback() { db_.reset_error_callback(); }
 
   // Must call this function to complete initialization. Will return
   // sql::INIT_OK on success. Otherwise, no other function should be called. You