Fix some lifetime issues in TrustTokenDatabaseOwner

crbug.com/1115398 concerns some crashes in
TableManager::ExecuteDbTaskOnDbSequence, which seem like they could be a
consequence of some incorrect lifetime management in
TrustTokenDatabaseOwner.

In particular, KeyValueData::FlushDataToDisk schedules a task using
TableManager by providing a callback with a bound raw pointer to the a
KeyValueTable owned by TrustTokenDatabaseOwner, but currently
TrustTokenDatabaseOwner's KeyValueTable members are destroyed before its
(Proto)TableManager member. This could mean that
TableManager::ExecuteDbTaskOnDbSequence tries to call into a
KeyValueTable that has already been freed, in cases where TableManager
tasks are executed concurrent with the TrustTokenDatabaseOwner's
destruction.

This initial fix for crbug.com/1115398 updates KeyValueTable<T> to
inherit from SupportsWeakPtr and swaps the base::Unretaineds in
KeyValueData to bind KeyValueTable weak pointers instead. This is
intended to be a simple, mergeable, shorter-term fix to stop the
crashes. A cleaner but more invasive follow-up fix could be to refactor
KeyValueTable<T> to be a stateless collection of helper functions,
eliminating the class's lifetime concerns.

Bug: 1115398
Change-Id: I4947b7678e99449b44fd48904fa92294db84a894
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2354289
Commit-Queue: David Van Cleve <davidvc@chromium.org>
Reviewed-by: Alex Ilin <alexilin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#797882}

components/sqlite_proto/key_value_data.h

@@ -131,9 +131,10 @@ template <typename T, typename Compare>
 void KeyValueData<T, Compare>::InitializeOnDBSequence() {
   DCHECK(manager_->GetTaskRunner()->RunsTasksInCurrentSequence());
   auto data_map = std::make_unique<std::map<std::string, T>>();
+
   manager_->ExecuteDBTaskOnDBSequence(
-      base::BindOnce(&KeyValueTable<T>::GetAllData,
-                     base::Unretained(backend_table_), data_map.get()));
+      base::BindOnce(&KeyValueTable<T>::GetAllData, backend_table_->AsWeakPtr(),
+                     data_map.get()));
 
   // To ensure invariant that data_cache_.size() <= max_num_entries_.
   std::vector<std::string> keys_to_delete;
@@ -145,7 +146,7 @@ void KeyValueData<T, Compare>::InitializeOnDBSequence() {
   }
   if (!keys_to_delete.empty()) {
     manager_->ExecuteDBTaskOnDBSequence(base::BindOnce(
-        &KeyValueTable<T>::DeleteData, base::Unretained(backend_table_),
+        &KeyValueTable<T>::DeleteData, backend_table_->AsWeakPtr(),
         std::vector<std::string>(keys_to_delete)));
   }
 
@@ -219,7 +220,7 @@ void KeyValueData<T, Compare>::DeleteAllData() {
   // by user.
   manager_->ScheduleDBTask(FROM_HERE,
                            base::BindOnce(&KeyValueTable<T>::DeleteAllData,
-                                          base::Unretained(backend_table_)));
+                                          backend_table_->AsWeakPtr()));
 }
 
 template <typename T, typename Compare>
@@ -236,9 +237,9 @@ void KeyValueData<T, Compare>::FlushDataToDisk() {
         auto it = data_cache_->find(key);
         if (it != data_cache_->end()) {
           manager_->ScheduleDBTask(
-              FROM_HERE, base::BindOnce(&KeyValueTable<T>::UpdateData,
-                                        base::Unretained(backend_table_), key,
-                                        it->second));
+              FROM_HERE,
+              base::BindOnce(&KeyValueTable<T>::UpdateData,
+                             backend_table_->AsWeakPtr(), key, it->second));
         }
         break;
       }
@@ -249,9 +250,8 @@ void KeyValueData<T, Compare>::FlushDataToDisk() {
 
   if (!keys_to_delete.empty()) {
     manager_->ScheduleDBTask(
-        FROM_HERE,
-        base::BindOnce(&KeyValueTable<T>::DeleteData,
-                       base::Unretained(backend_table_), keys_to_delete));
+        FROM_HERE, base::BindOnce(&KeyValueTable<T>::DeleteData,
+                                  backend_table_->AsWeakPtr(), keys_to_delete));
   }
 
   deferred_updates_.clear();

components/sqlite_proto/key_value_table.h

@@ -45,9 +45,14 @@ std::string GetDeleteAllSql(const std::string& table_name);
 // manager_->ScheduleDBTask(
 //     FROM_HERE,
 //     base::BindOnce(&KeyValueTable<PrefetchData>::UpdateData,
-//                    base::Unretained(table_), key, data));
+//                    table_->AsWeakPtr(), key, data));
+//
+// TODO(crbug.com/1115398): Supporting weak pointers is a temporary measure
+// mitigating a crash caused by complex lifetime requirements for KeyValueTable
+// relative to the related classes. Making KeyValueTable<T> stateless instead
+// could be a better way to resolve these lifetime issues in the long run.
 template <typename T>
-class KeyValueTable {
+class KeyValueTable : public base::SupportsWeakPtr<KeyValueTable<T>> {
  public:
   explicit KeyValueTable(const std::string& table_name);
   // Virtual for testing.