Fix the crash caused on shutdown by OnLogChange method

When the browser is shutdown, we start destroying the objects. In
ExtensionInstallEventLogManger, we used to destroy |logger_| after the
|extension_log_upload_| pointer. This could cause to a crash because
we try add a LOGOUT event before the shutdown. And while adding the
LOGOUT event, ExtensionInstallEventLogManager::Add method is invoked
which posts a task to invoke
InstallEventLogManagerBase::LogUpload::OnLogChange. Thus we should
destroy the |extension_log_upload_| pointer after destroying the
|logger_| pointer so that we add the LOGOUT event and prevent the
crash. Similarly, this should be fixed for ARC++ reporting.

Bug: 1114191
Change-Id: I6e88f5747b6a5b7a36e1dcee3083204833c9aeff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2346306Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Oleg Davydov <burunduk@chromium.org>
Reviewed-by: Anqing Zhao <anqing@chromium.org>
Commit-Queue: Swapnil Gupta <swapnilgupta@google.com>
Cr-Commit-Position: refs/heads/master@{#797388}

chrome/browser/chromeos/policy/arc_app_install_event_log_manager.cc

@@ -37,8 +37,8 @@ ArcAppInstallEventLogManager::ArcAppInstallEventLogManager(
     : InstallEventLogManagerBase(log_task_runner_wrapper, profile),
       uploader_(uploader) {
   uploader_->SetDelegate(this);
-  app_log_upload_ = std::make_unique<AppLogUpload>(this);
   log_ = std::make_unique<ArcLog>();
+  app_log_upload_ = std::make_unique<AppLogUpload>(this);
   base::PostTaskAndReplyWithResult(
       log_task_runner_.get(), FROM_HERE,
       base::BindOnce(&ArcLog::Init, base::Unretained(log_.get()),
@@ -49,8 +49,11 @@ ArcAppInstallEventLogManager::ArcAppInstallEventLogManager(
 }
 
 ArcAppInstallEventLogManager::~ArcAppInstallEventLogManager() {
-  app_log_upload_.reset();
   logger_.reset();
+  // Destroy |app_log_upload_| after |logger_| otherwise it is possible
+  // that when we add new logs created through |logger_| cause the crash as
+  // |app_log_upload_| will be destroyed already.
+  app_log_upload_.reset();
   uploader_->SetDelegate(nullptr);
   log_task_runner_->DeleteSoon(FROM_HERE, std::move(log_));
 }
@@ -76,7 +79,7 @@ void ArcAppInstallEventLogManager::Add(
       base::BindOnce(&ArcLog::Add, base::Unretained(log_.get()), packages,
                      event),
       base::BindOnce(&ArcAppInstallEventLogManager::AppLogUpload::OnLogChange,
-                     base::Unretained(app_log_upload_.get())));
+                     app_log_upload_->log_weak_factory_.GetWeakPtr()));
 }
 
 void ArcAppInstallEventLogManager::GetAndroidId(

chrome/browser/chromeos/policy/arc_app_install_event_log_manager.h

@@ -102,11 +102,11 @@ class ArcAppInstallEventLogManager
   // to disk in its destructor.
   std::unique_ptr<ArcLog> log_;
 
-  // Collects log events and passes them to |this|.
-  std::unique_ptr<AppInstallEventLogger> logger_;
-
   // Handles storing the logs and preparing them for upload.
   std::unique_ptr<AppLogUpload> app_log_upload_;
+
+  // Collects log events and passes them to |this|.
+  std::unique_ptr<AppInstallEventLogger> logger_;
 };
 
 }  // namespace policy

chrome/browser/chromeos/policy/extension_install_event_log_manager.cc

@@ -37,8 +37,8 @@ ExtensionInstallEventLogManager::ExtensionInstallEventLogManager(
     : InstallEventLogManagerBase(log_task_runner_wrapper, profile),
       uploader_(uploader) {
   uploader_->SetDelegate(this);
-  extension_log_upload_ = std::make_unique<ExtensionLogUpload>(this);
   log_ = std::make_unique<ExtensionLog>();
+  extension_log_upload_ = std::make_unique<ExtensionLogUpload>(this);
   base::PostTaskAndReplyWithResult(
       log_task_runner_.get(), FROM_HERE,
       base::BindOnce(&ExtensionLog::Init, base::Unretained(log_.get()),
@@ -51,8 +51,11 @@ ExtensionInstallEventLogManager::ExtensionInstallEventLogManager(
 }
 
 ExtensionInstallEventLogManager::~ExtensionInstallEventLogManager() {
-  extension_log_upload_.reset();
   logger_.reset();
+  // Destroy |extension_log_upload_| after |logger_| otherwise it is possible
+  // that when we add new logs created through |logger_| cause the crash as
+  // |extension_log_upload_| will be destroyed already.
+  extension_log_upload_.reset();
   uploader_->SetDelegate(nullptr);
   log_task_runner_->DeleteSoon(FROM_HERE, std::move(log_));
 }
@@ -71,13 +74,17 @@ void ExtensionInstallEventLogManager::Add(
     const em::ExtensionInstallReportLogEvent& event) {
   if (extensions.empty())
     return;
+  // After |logger_| is destroyed, we do not add any new events. And |log_| is
+  // destroyed by creating a non nestable task at the end of
+  // ExtensionInstallEventLogManager destructor, it is safe to use
+  // base::Unretained(log_.get()) here.
   base::PostTaskAndReplyWithResult(
       log_task_runner_.get(), FROM_HERE,
       base::BindOnce(&ExtensionLog::Add, base::Unretained(log_.get()),
                      std::move(extensions), event),
       base::BindOnce(
           &ExtensionInstallEventLogManager::ExtensionLogUpload::OnLogChange,
-          base::Unretained(extension_log_upload_.get())));
+          extension_log_upload_->log_weak_factory_.GetWeakPtr()));
 }
 
 void ExtensionInstallEventLogManager::SerializeExtensionLogForUpload(

chrome/browser/chromeos/policy/extension_install_event_log_manager.h

@@ -103,11 +103,11 @@ class ExtensionInstallEventLogManager
   // to disk in its destructor.
   std::unique_ptr<ExtensionLog> log_;
 
-  // Collects log events and passes them to |this|.
-  std::unique_ptr<ExtensionInstallEventLogger> logger_;
-
   // Handles storing the logs and preparing them for upload.
   std::unique_ptr<ExtensionLogUpload> extension_log_upload_;
+
+  // Collects log events and passes them to |this|.
+  std::unique_ptr<ExtensionInstallEventLogger> logger_;
 };
 
 }  // namespace policy

chrome/browser/policy/extension_policy_browsertest.cc

@@ -1622,6 +1622,27 @@ IN_PROC_BROWSER_TEST_F(ExtensionPolicyTest,
   EXPECT_TRUE(registry->enabled_extensions().GetByID(kGoodCrxId));
 }
 
+// Verifies that the browser doesn't crash on shutdown. If the extensions are
+// being installed, and the browser is shutdown, it should not lead to a crash
+// as in (crbug/1114191).
+IN_PROC_BROWSER_TEST_F(ExtensionPolicyTest,
+                       ExtensionInstallForcelistShutdownBeforeInstall) {
+  ExtensionRequestInterceptor interceptor;
+
+  extensions::ExtensionRegistry* registry = extension_registry();
+  ASSERT_FALSE(registry->GetExtensionById(
+      kGoodCrxId, extensions::ExtensionRegistry::EVERYTHING));
+  ASSERT_TRUE(embedded_test_server()->Start());
+  GURL url =
+      embedded_test_server()->GetURL("/extensions/good_v1_update_manifest.xml");
+
+  PolicyMap policies;
+  AddExtensionToForceList(&policies, kGoodCrxId, url);
+  UpdateProviderPolicy(policies);
+  // The extension is not yet installed, shutdown the browser now and there
+  // should be no crash.
+}
+
 IN_PROC_BROWSER_TEST_F(ExtensionPolicyTest,
                        ExtensionRecommendedInstallationMode) {
   // Verifies that extensions that are recommended-installed by policies are