WebApp: Fix heap-use-after-free in WebAppDatabase.

WebAppMigrationManager shouldn't destruct owned database_ object
immediately in callbacks from database_. This would be a violation of
the caller/callee contract.

We should use PostTask instead so |this| stays valid in
WebAppDatabase even after callback call.

This CL fixes
./out/debug/sync_integration_tests --gtest_filter=All/SingleClientBookmarksSyncTest.PersistProgressMarkerOnRestart/0

test if BMO mode is on by default.

TBR=alancutter@chromium.org

Bug: 1054112
Change-Id: I170424768e7c684ed3c14e398836755201dd4f33
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089559
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748500}

chrome/browser/web_applications/web_app_migration_manager.cc

@@ -10,6 +10,7 @@
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
 #include "chrome/browser/web_applications/web_app.h"
 #include "chrome/browser/web_applications/web_app_database.h"
@@ -61,7 +62,7 @@ void WebAppMigrationManager::OnWebAppDatabaseOpened(
     // called once. All the migrated entities are already listed in sync
     // metadata. Any additional apps from this point in time should be installed
     // via WebAppSyncBridge::CommitUpdate() "as if" a user installs them.
-    CloseDatabaseAndCallCallback(/*success=*/true);
+    ScheduleDestructDatabaseAndCallCallback(/*success=*/true);
     return;
   }
 
@@ -207,7 +208,7 @@ void WebAppMigrationManager::MigrateBookmarkAppsRegistry() {
   DCHECK(database_);
 
   if (bookmark_app_ids_.empty()) {
-    CloseDatabaseAndCallCallback(/*success=*/true);
+    ScheduleDestructDatabaseAndCallCallback(/*success=*/true);
     return;
   }
 
@@ -232,17 +233,25 @@ void WebAppMigrationManager::OnWebAppRegistryWritten(bool success) {
   if (!success)
     DLOG(ERROR) << "Web app registry commit failed.";
 
-  CloseDatabaseAndCallCallback(success);
+  ScheduleDestructDatabaseAndCallCallback(success);
 }
 
 void WebAppMigrationManager::ReportDatabaseError(
     const syncer::ModelError& error) {
   DLOG(ERROR) << "Web app database error. " << error.ToString();
 
-  CloseDatabaseAndCallCallback(/*success=*/false);
+  ScheduleDestructDatabaseAndCallCallback(/*success=*/false);
 }
 
-void WebAppMigrationManager::CloseDatabaseAndCallCallback(bool success) {
+void WebAppMigrationManager::ScheduleDestructDatabaseAndCallCallback(
+    bool success) {
+  base::SequencedTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&WebAppMigrationManager::DestructDatabaseAndCallCallback,
+                     weak_ptr_factory_.GetWeakPtr(), success));
+}
+
+void WebAppMigrationManager::DestructDatabaseAndCallCallback(bool success) {
   // Close the database.
   database_ = nullptr;
 

chrome/browser/web_applications/web_app_migration_manager.h

@@ -63,7 +63,12 @@ class WebAppMigrationManager {
   void OnWebAppRegistryWritten(bool success);
 
   void ReportDatabaseError(const syncer::ModelError& error);
-  void CloseDatabaseAndCallCallback(bool success);
+
+  // We don't want to destruct database_ object immediately in callbacks from
+  // WebAppDatabase. This would be a violation of the caller/callee contract.
+  // We should use PostTask instead.
+  void ScheduleDestructDatabaseAndCallCallback(bool success);
+  void DestructDatabaseAndCallCallback(bool success);
 
   extensions::BookmarkAppRegistrar bookmark_app_registrar_;
   extensions::BookmarkAppRegistryController bookmark_app_registry_controller_;