[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
diff --git a/chrome/browser/extensions/external_install_error.h b/chrome/browser/extensions/external_install_error.h
index 6d6c2a6..abb06974 100644
--- a/chrome/browser/extensions/external_install_error.h
+++ b/chrome/browser/extensions/external_install_error.h
@@ -78,6 +78,9 @@
const ExtensionInstallPrompt::DoneCallback& done_callback,
scoped_ptr<ExtensionInstallPrompt::Prompt> prompt);
+ // Removes the error.
+ void RemoveError();
+
// The associated BrowserContext.
content::BrowserContext* browser_context_;
diff --git a/chrome/browser/extensions/external_install_manager.cc b/chrome/browser/extensions/external_install_manager.cc
index 385e81bc..3908e49 100644
--- a/chrome/browser/extensions/external_install_manager.cc
+++ b/chrome/browser/extensions/external_install_manager.cc
@@ -76,8 +76,9 @@
void ExternalInstallManager::AddExternalInstallError(const Extension* extension,
bool is_new_profile) {
- // Error already exists.
- if (ContainsKey(errors_, extension->id()))
+ // Error already exists or has been previously shown.
+ if (ContainsKey(errors_, extension->id()) ||
+ shown_ids_.count(extension->id()) > 0)
return;
ExternalInstallError::AlertType alert_type =
@@ -87,13 +88,14 @@
scoped_ptr<ExternalInstallError> error(new ExternalInstallError(
browser_context_, extension->id(), alert_type, this));
+ shown_ids_.insert(extension->id());
errors_.insert(std::make_pair(extension->id(), std::move(error)));
}
void ExternalInstallManager::RemoveExternalInstallError(
const std::string& extension_id) {
- errors_.erase(extension_id);
- UpdateExternalExtensionAlert();
+ if (errors_.erase(extension_id) > 0)
+ UpdateExternalExtensionAlert();
}
void ExternalInstallManager::UpdateExternalExtensionAlert() {
@@ -106,7 +108,8 @@
const ExtensionSet& disabled_extensions =
ExtensionRegistry::Get(browser_context_)->disabled_extensions();
for (const scoped_refptr<const Extension>& extension : disabled_extensions) {
- if (ContainsKey(errors_, extension->id()))
+ if (ContainsKey(errors_, extension->id()) ||
+ shown_ids_.count(extension->id()) > 0)
continue;
if (!IsUnacknowledgedExternalExtension(extension.get()))
diff --git a/chrome/browser/extensions/external_install_manager.h b/chrome/browser/extensions/external_install_manager.h
index 5312283..afc7cb9 100644
--- a/chrome/browser/extensions/external_install_manager.h
+++ b/chrome/browser/extensions/external_install_manager.h
@@ -83,6 +83,8 @@
// The collection of ExternalInstallErrors.
std::map<std::string, scoped_ptr<ExternalInstallError>> errors_;
+ std::set<std::string> shown_ids_;
+
content::NotificationRegistrar registrar_;
ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver>
diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
index d0803377..7756cf9 100644
--- a/tools/metrics/histograms/histograms.xml
+++ b/tools/metrics/histograms/histograms.xml
@@ -12334,6 +12334,15 @@
</summary>
</histogram>
+<histogram name="Extensions.ExternalWarningUninstallationResult" enum="Boolean">
+ <owner>[email protected]</owner>
+ <summary>
+ Whether or not the uninstallation of an external extension succeeded.
+ Triggered when the external install warning is shown to the user and the
+ user selects to remove the extension.
+ </summary>
+</histogram>
+
<histogram name="Extensions.FeatureProviderStaticInitTime" units="ms">
<owner>[email protected]</owner>
<summary>
