[Extensions] Immediately remove external extension errors after they are shown
Previously, we would wait for the extension to be unloaded before removing the
external install error. However, if the uninstallation fails (or happens
asynchronously), this could result in the error persisting, which will cause
a crash if it tries to show again. Remove the error once it's shown once (it
will be shown again on subsequent runs), and add UMA to see how often
uninstallation fails to determine if we need to address that separately.
BUG=575129
Review URL: https://codereview.chromium.org/1572563003
Cr-Commit-Position: refs/heads/master@{#369516}
diff --git a/chrome/browser/extensions/external_install_error.cc b/chrome/browser/extensions/external_install_error.cc
index 02b6e5b..0b35015 100644
--- a/chrome/browser/extensions/external_install_error.cc
+++ b/chrome/browser/extensions/external_install_error.cc
@@ -9,6 +9,8 @@
#include "base/bind.h"
#include "base/macros.h"
+#include "base/message_loop/message_loop.h"
+#include "base/metrics/histogram_macros.h"
#include "base/strings/utf_string_conversions.h"
#include "chrome/app/chrome_command_ids.h"
#include "chrome/browser/extensions/extension_install_error_menu_item_id_provider.h"
@@ -307,34 +309,39 @@
void ExternalInstallError::OnInstallPromptDone(
ExtensionInstallPrompt::Result result) {
const Extension* extension = GetExtension();
- bool did_remove_error = false;
+
+ // If the error isn't removed and deleted as part of handling the user's
+ // response (which can happen, e.g., if an uninstall fails), be sure to remove
+ // the error directly in order to ensure it's not called twice.
+ base::MessageLoop::current()->PostTask(
+ FROM_HERE,
+ base::Bind(&ExternalInstallError::RemoveError,
+ weak_factory_.GetWeakPtr()));
+
switch (result) {
case ExtensionInstallPrompt::Result::ACCEPTED:
if (extension) {
ExtensionSystem::Get(browser_context_)
->extension_service()
->GrantPermissionsAndEnableExtension(extension);
- // Since the manager listens for the extension to be loaded, this will
- // remove the error.
- did_remove_error = true;
}
break;
case ExtensionInstallPrompt::Result::USER_CANCELED:
if (extension) {
- ExtensionSystem::Get(browser_context_)
+ bool uninstallation_result = ExtensionSystem::Get(browser_context_)
->extension_service()
->UninstallExtension(extension_id_,
extensions::UNINSTALL_REASON_INSTALL_CANCELED,
base::Bind(&base::DoNothing),
nullptr); // Ignore error.
- did_remove_error = true;
+ UMA_HISTOGRAM_BOOLEAN("Extensions.ExternalWarningUninstallationResult",
+ uninstallation_result);
}
break;
case ExtensionInstallPrompt::Result::ABORTED:
break;
}
- if (!did_remove_error)
- manager_->RemoveExternalInstallError(extension_id_);
+ // NOTE: We may be deleted here!
}
void ExternalInstallError::ShowDialog(Browser* browser) {
@@ -428,4 +435,8 @@
}
}
+void ExternalInstallError::RemoveError() {
+ manager_->RemoveExternalInstallError(extension_id_);
+}
+
} // namespace extensions
