Fix shutdown hang due to queued extension installs
Upon signing in to a profile (or login to Chrome OS) sync starts pulling down
extensions and installing them. This creates long tasks on the file thread.
If one of these tasks completes during browser shutdown, it initiates another
installation and more file I/O. This leads to Chrome's shutdown watchdog being
triggered, which on Chrome OS leads to a SIGABRT after 10 seconds.
This is one of our top-crashes on Chrome OS and can be easily triggered by
signing into an account (triggering sync of a bunch of apps) and immediately
signing out.
Fix it by preventing extension installation and updates after the browser has
started shutting down. We make a best-effort attempt to clean up, and on next
run the extension garbage collector cleans things up and sync continues.
BUG=155994
TEST=added to ExtensionServiceTest, existing browser_tests and unit_tests
Review URL: https://chromiumcodereview.appspot.com/11233015
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@163407 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/chrome/browser/extensions/crx_installer.cc b/chrome/browser/extensions/crx_installer.cc
index 71d94e8..94e84c9 100644
--- a/chrome/browser/extensions/crx_installer.cc
+++ b/chrome/browser/extensions/crx_installer.cc
@@ -406,6 +406,8 @@
void CrxInstaller::CheckRequirements() {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+ if (!frontend_weak_.get() || frontend_weak_->browser_terminating())
+ return;
AddRef(); // Balanced in OnRequirementsChecked().
requirements_checker_->Check(extension_,
base::Bind(&CrxInstaller::OnRequirementsChecked,
@@ -430,7 +432,7 @@
void CrxInstaller::ConfirmInstall() {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
- if (!frontend_weak_.get())
+ if (!frontend_weak_.get() || frontend_weak_->browser_terminating())
return;
string16 error;
@@ -617,7 +619,7 @@
void CrxInstaller::ReportSuccessFromUIThread() {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
- if (!frontend_weak_.get())
+ if (!frontend_weak_.get() || frontend_weak_->browser_terminating())
return;
// If there is a client, tell the client about installation.
diff --git a/chrome/browser/extensions/crx_installer.h b/chrome/browser/extensions/crx_installer.h
index d7f15e5..6f84a491 100644
--- a/chrome/browser/extensions/crx_installer.h
+++ b/chrome/browser/extensions/crx_installer.h
@@ -50,6 +50,11 @@
// installer->set_foo();
// installer->set_bar();
// installer->InstallCrx(...);
+//
+// Installation is aborted if the extension service learns that Chrome is
+// terminating during the install. We can't listen for the app termination
+// notification here in this class because it can be destroyed on any thread
+// and won't safely be able to clean up UI thread notification listeners.
class CrxInstaller
: public SandboxedUnpackerClient,
public ExtensionInstallPrompt::Delegate {
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 38e5a80..199c57d 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -351,6 +351,7 @@
new extensions::AppNotificationManager(profile)),
event_routers_initialized_(false),
update_once_all_providers_are_ready_(false),
+ browser_terminating_(false),
app_sync_bundle_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
extension_sync_bundle_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
extension_warnings_(profile),
@@ -363,6 +364,8 @@
extensions_enabled_ = false;
}
+ registrar_.Add(this, chrome::NOTIFICATION_APP_TERMINATING,
+ content::NotificationService::AllBrowserContextsAndSources());
registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_PROCESS_TERMINATED,
content::NotificationService::AllBrowserContextsAndSources());
registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_CREATED,
@@ -605,6 +608,13 @@
const GURL& download_url,
CrxInstaller** out_crx_installer) {
CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+ if (browser_terminating_) {
+ LOG(WARNING) << "Skipping UpdateExtension due to browser shutdown";
+ // Leak the temp file at extension_path. We don't want to add to the disk
+ // I/O burden at shutdown, we can't rely on the I/O completing anyway, and
+ // the file is in the OS temp directory which should be cleaned up for us.
+ return false;
+ }
const extensions::PendingExtensionInfo* pending_extension_info =
pending_extension_manager()->GetById(id);
@@ -2493,6 +2503,12 @@
const content::NotificationSource& source,
const content::NotificationDetails& details) {
switch (type) {
+ case chrome::NOTIFICATION_APP_TERMINATING:
+ // Shutdown has started. Don't start any more extension installs.
+ // (We cannot use ExtensionService::Shutdown() for this because it
+ // happens too late in browser teardown.)
+ browser_terminating_ = true;
+ break;
case chrome::NOTIFICATION_EXTENSION_PROCESS_TERMINATED: {
if (profile_ !=
content::Source<Profile>(source).ptr()->GetOriginalProfile()) {
diff --git a/chrome/browser/extensions/extension_service.h b/chrome/browser/extensions/extension_service.h
index fa08503..f18d5e01 100644
--- a/chrome/browser/extensions/extension_service.h
+++ b/chrome/browser/extensions/extension_service.h
@@ -652,6 +652,13 @@
// Specialization of syncer::SyncableService::AsWeakPtr.
base::WeakPtr<ExtensionService> AsWeakPtr() { return base::AsWeakPtr(this); }
+ bool browser_terminating() const { return browser_terminating_; }
+
+ // For testing.
+ void set_browser_terminating_for_test(bool value) {
+ browser_terminating_ = value;
+ }
+
private:
// Contains Extension data that can change during the life of the process,
// but does not persist across restarts.
@@ -885,6 +892,11 @@
// install pending extensions.
bool update_once_all_providers_are_ready_;
+ // Set when the browser is terminating. Prevents us from installing or
+ // updating additional extensions and allows in-progress installations to
+ // decide to abort.
+ bool browser_terminating_;
+
NaClModuleInfoList nacl_module_list_;
extensions::AppSyncBundle app_sync_bundle_;
diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
index 97198540..80fc68129 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -1497,6 +1497,23 @@
ExtensionErrorReporter::GetInstance()->ClearErrors();
}
+// Extensions don't install during shutdown.
+TEST_F(ExtensionServiceTest, InstallExtensionDuringShutdown) {
+ InitializeEmptyExtensionService();
+
+ // Simulate shutdown.
+ service_->set_browser_terminating_for_test(true);
+
+ FilePath path = data_dir_.AppendASCII("good.crx");
+ scoped_refptr<CrxInstaller> installer(CrxInstaller::Create(service_, NULL));
+ installer->set_allow_silent_install(true);
+ installer->InstallCrx(path);
+ loop_.RunAllPending();
+
+ EXPECT_FALSE(installed_) << "Extension installed during shutdown.";
+ ASSERT_EQ(0u, loaded_.size()) << "Extension loaded during shutdown.";
+}
+
// This tests that the granted permissions preferences are correctly set when
// installing an extension.
TEST_F(ExtensionServiceTest, GrantedPermissions) {
@@ -2364,6 +2381,27 @@
version()->GetString());
}
+// Extensions should not be updated during browser shutdown.
+TEST_F(ExtensionServiceTest, UpdateExtensionDuringShutdown) {
+ InitializeEmptyExtensionService();
+
+ // Install an extension.
+ FilePath path = data_dir_.AppendASCII("good.crx");
+ const Extension* good = InstallCRX(path, INSTALL_NEW);
+ ASSERT_EQ(good_crx, good->id());
+
+ // Simulate shutdown.
+ service_->set_browser_terminating_for_test(true);
+
+ // Update should fail and extension should not be updated.
+ path = data_dir_.AppendASCII("good2.crx");
+ bool updated = service_->UpdateExtension(good_crx, path, GURL(), NULL);
+ ASSERT_FALSE(updated);
+ ASSERT_EQ("1.0.0.0",
+ service_->GetExtensionById(good_crx, false)->
+ version()->GetString());
+}
+
// Test updating a not-already-installed extension - this should fail
TEST_F(ExtensionServiceTest, UpdateNotInstalledExtension) {
InitializeEmptyExtensionService();
