diff options
author | scheib@chromium.org <scheib@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-05-21 18:25:34 +0000 |
---|---|---|
committer | scheib@chromium.org <scheib@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-05-21 18:25:34 +0000 |
commit | bc118e52c97b739a72a0987a368075429e923ba6 (patch) | |
tree | 8a4643f292fd61d2d7342562a88e9df51f06522a | |
parent | 3b09d5e1d53f61291e20e2701c4ada568a2ae765 (diff) | |
download | chromium_src-bc118e52c97b739a72a0987a368075429e923ba6.zip chromium_src-bc118e52c97b739a72a0987a368075429e923ba6.tar.gz chromium_src-bc118e52c97b739a72a0987a368075429e923ba6.tar.bz2 |
Revert of Unload all apps / extensions when deleting a profile. (https://codereview.chromium.org/266343002/)
Reason for revert:
Caused crash in official builds when deleting profiles while the chrome:chrome-signin tab is open
BUG=374683
TBR=kalman, sky
Original issue's description:
> Unload all apps / extensions immediately when deleting a profile.
>
> Previously apps could remain running with references to profiles that had been deleted by users, but before the browser shut down and profiles were fully removed. Problems included E.g. opening a link in an app would open a tab in the deleted profile.
>
> Relanding patch: ShutdownStartupCycle failed in build [49353], this patch was speculatively reverted in r269383 [49355], but ShutdownStartupCycle failed again in [49362] after the revert had landed.
>
> [49353] http://build.chromium.org/p/chromium.mac/builders/Mac%2010.6%20Tests%20%28dbg%29%281%29/builds/49353
> [49355] http://build.chromium.org/p/chromium.mac/builders/Mac%2010.6%20Tests%20%28dbg%29%281%29/builds/49355
> [49362] http://build.chromium.org/p/chromium.mac/builders/Mac%2010.6%20Tests%20%28dbg%29%281%29/builds/49362
>
> BUG=368684
> TEST=Manual testing as described on http://crbug.com/368684#c1
>
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269343
>
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270890
Review URL: https://codereview.chromium.org/289283013
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@271926 0039d316-1c4b-4281-b951-d872f2087c98
-rw-r--r-- | chrome/browser/background/background_contents_service.cc | 3 | ||||
-rw-r--r-- | chrome/browser/chrome_notification_types.h | 6 | ||||
-rw-r--r-- | chrome/browser/extensions/extension_service.cc | 16 | ||||
-rw-r--r-- | chrome/browser/extensions/extension_service.h | 5 | ||||
-rw-r--r-- | chrome/browser/extensions/extension_service_unittest.cc | 40 | ||||
-rw-r--r-- | chrome/browser/profiles/profile_manager.cc | 7 | ||||
-rw-r--r-- | chrome/browser/ui/browser.cc | 5 | ||||
-rw-r--r-- | extensions/browser/process_manager.cc | 5 | ||||
-rw-r--r-- | extensions/common/extension.h | 12 |
9 files changed, 12 insertions, 87 deletions
diff --git a/chrome/browser/background/background_contents_service.cc b/chrome/browser/background/background_contents_service.cc index b83decd..0d21871 100644 --- a/chrome/browser/background/background_contents_service.cc +++ b/chrome/browser/background/background_contents_service.cc @@ -468,8 +468,7 @@ void BackgroundContentsService::Observe( case UnloadedExtensionInfo::REASON_DISABLE: // Fall through. case UnloadedExtensionInfo::REASON_TERMINATE: // Fall through. case UnloadedExtensionInfo::REASON_UNINSTALL: // Fall through. - case UnloadedExtensionInfo::REASON_BLACKLIST: // Fall through. - case UnloadedExtensionInfo::REASON_PROFILE_SHUTDOWN: + case UnloadedExtensionInfo::REASON_BLACKLIST: ShutdownAssociatedBackgroundContents(base::ASCIIToUTF16( content::Details<UnloadedExtensionInfo>(details)-> extension->id())); diff --git a/chrome/browser/chrome_notification_types.h b/chrome/browser/chrome_notification_types.h index 9303cac..6e1adf4 100644 --- a/chrome/browser/chrome_notification_types.h +++ b/chrome/browser/chrome_notification_types.h @@ -287,12 +287,6 @@ enum NotificationType { // The details are none and the source is the new profile. NOTIFICATION_PROFILE_ADDED, - // Sent early in the process of destroying a Profile, at the time a user - // initiates the deletion of a profile versus the much later time when the - // profile object is actually destroyed (use NOTIFICATION_PROFILE_DESTROYED). - // The details are none and the source is a Profile*. - NOTIFICATION_PROFILE_DESTRUCTION_STARTED, - // Sent before a Profile is destroyed. This notification is sent both for // normal and OTR profiles. // The details are none and the source is a Profile*. diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc index 04dac42..7a357fd 100644 --- a/chrome/browser/extensions/extension_service.cc +++ b/chrome/browser/extensions/extension_service.cc @@ -301,9 +301,6 @@ ExtensionService::ExtensionService(Profile* profile, content::NotificationService::AllBrowserContextsAndSources()); registrar_.Add(this, chrome::NOTIFICATION_UPGRADE_RECOMMENDED, content::NotificationService::AllBrowserContextsAndSources()); - registrar_.Add(this, - chrome::NOTIFICATION_PROFILE_DESTRUCTION_STARTED, - content::Source<Profile>(profile_)); pref_change_registrar_.Init(profile->GetPrefs()); base::Closure callback = base::Bind(&ExtensionService::OnExtensionInstallPrefChanged, @@ -2168,10 +2165,6 @@ void ExtensionService::Observe(int type, OnChromeUpdateAvailable()); break; } - case chrome::NOTIFICATION_PROFILE_DESTRUCTION_STARTED: { - OnProfileDestructionStarted(); - break; - } default: NOTREACHED() << "Unexpected notification type."; @@ -2435,12 +2428,3 @@ void ExtensionService::UnloadAllExtensionsInternal() { // EXTENSION_UNLOADED since that implies that the extension has been disabled // or uninstalled. } - -void ExtensionService::OnProfileDestructionStarted() { - ExtensionIdSet ids_to_unload = registry_->enabled_extensions().GetIDs(); - for (ExtensionIdSet::iterator it = ids_to_unload.begin(); - it != ids_to_unload.end(); - ++it) { - UnloadExtension(*it, UnloadedExtensionInfo::REASON_PROFILE_SHUTDOWN); - } -} diff --git a/chrome/browser/extensions/extension_service.h b/chrome/browser/extensions/extension_service.h index 1e5f5b5..adafa07 100644 --- a/chrome/browser/extensions/extension_service.h +++ b/chrome/browser/extensions/extension_service.h @@ -549,11 +549,6 @@ class ExtensionService // Used only by test code. void UnloadAllExtensionsInternal(); - // Disable apps & extensions now to stop them from running after a profile - // has been conceptually deleted. Don't wait for full browser shutdown and - // the actual profile objects to be destroyed. - void OnProfileDestructionStarted(); - // The normal profile associated with this ExtensionService. Profile* profile_; diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc index 0fef7ad..a5ed2ba 100644 --- a/chrome/browser/extensions/extension_service_unittest.cc +++ b/chrome/browser/extensions/extension_service_unittest.cc @@ -165,7 +165,6 @@ using extensions::FeatureSwitch; using extensions::Manifest; using extensions::PermissionSet; using extensions::TestExtensionSystem; -using extensions::UnloadedExtensionInfo; using extensions::URLPatternSet; namespace keys = extensions::manifest_keys; @@ -667,12 +666,10 @@ class ExtensionServiceTest : public ExtensionServiceTestBase, public content::NotificationObserver { public: ExtensionServiceTest() - : unloaded_reason_(UnloadedExtensionInfo::REASON_UNDEFINED), - installed_(NULL), + : installed_(NULL), was_update_(false), override_external_install_prompt_( - FeatureSwitch::prompt_for_external_extensions(), - false) { + FeatureSwitch::prompt_for_external_extensions(), false) { registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_LOADED_DEPRECATED, content::NotificationService::AllSources()); @@ -697,11 +694,10 @@ class ExtensionServiceTest } case chrome::NOTIFICATION_EXTENSION_UNLOADED_DEPRECATED: { - UnloadedExtensionInfo* unloaded_info = - content::Details<UnloadedExtensionInfo>(details).ptr(); - const Extension* e = unloaded_info->extension; + const Extension* e = + content::Details<extensions::UnloadedExtensionInfo>( + details)->extension; unloaded_id_ = e->id(); - unloaded_reason_ = unloaded_info->reason; extensions::ExtensionList::iterator i = std::find(loaded_.begin(), loaded_.end(), e); // TODO(erikkay) fix so this can be an assert. Right now the tests @@ -1254,7 +1250,6 @@ class ExtensionServiceTest protected: extensions::ExtensionList loaded_; std::string unloaded_id_; - UnloadedExtensionInfo::Reason unloaded_reason_; const Extension* installed_; bool was_update_; std::string old_name_; @@ -4202,7 +4197,6 @@ TEST_F(ExtensionServiceTest, UninstallExtension) { EXPECT_EQ(1u, registry_->enabled_extensions().size()); UninstallExtension(good_crx, false); EXPECT_EQ(0u, registry_->enabled_extensions().size()); - EXPECT_EQ(UnloadedExtensionInfo::REASON_UNINSTALL, unloaded_reason_); } TEST_F(ExtensionServiceTest, UninstallTerminatedExtension) { @@ -4210,7 +4204,6 @@ TEST_F(ExtensionServiceTest, UninstallTerminatedExtension) { InstallCRX(data_dir_.AppendASCII("good.crx"), INSTALL_NEW); TerminateExtension(good_crx); UninstallExtension(good_crx, false); - EXPECT_EQ(UnloadedExtensionInfo::REASON_TERMINATE, unloaded_reason_); } // Tests the uninstaller helper. @@ -4218,7 +4211,6 @@ TEST_F(ExtensionServiceTest, UninstallExtensionHelper) { InitializeEmptyExtensionService(); InstallCRX(data_dir_.AppendASCII("good.crx"), INSTALL_NEW); UninstallExtension(good_crx, true); - EXPECT_EQ(UnloadedExtensionInfo::REASON_UNINSTALL, unloaded_reason_); } TEST_F(ExtensionServiceTest, UninstallExtensionHelperTerminated) { @@ -4226,7 +4218,6 @@ TEST_F(ExtensionServiceTest, UninstallExtensionHelperTerminated) { InstallCRX(data_dir_.AppendASCII("good.crx"), INSTALL_NEW); TerminateExtension(good_crx); UninstallExtension(good_crx, true); - EXPECT_EQ(UnloadedExtensionInfo::REASON_TERMINATE, unloaded_reason_); } // An extension disabled because of unsupported requirements should re-enabled @@ -6962,24 +6953,3 @@ TEST_F(ExtensionServiceTest, ReconcileKnownDisabledWithSideEnable) { EXPECT_EQ(expected_disabled_extensions, registry_->disabled_extensions().GetIDs()); } - -// Tests a profile being destroyed correctly disables extensions. -TEST_F(ExtensionServiceTest, DestroyingProfileClearsExtensions) { - InitializeEmptyExtensionService(); - - InstallCRX(data_dir_.AppendASCII("good.crx"), INSTALL_NEW); - EXPECT_NE(UnloadedExtensionInfo::REASON_PROFILE_SHUTDOWN, unloaded_reason_); - EXPECT_EQ(1u, registry_->enabled_extensions().size()); - EXPECT_EQ(0u, registry_->disabled_extensions().size()); - EXPECT_EQ(0u, registry_->terminated_extensions().size()); - EXPECT_EQ(0u, registry_->blacklisted_extensions().size()); - - service_->Observe(chrome::NOTIFICATION_PROFILE_DESTRUCTION_STARTED, - content::Source<Profile>(profile_.get()), - content::NotificationService::NoDetails()); - EXPECT_EQ(UnloadedExtensionInfo::REASON_PROFILE_SHUTDOWN, unloaded_reason_); - EXPECT_EQ(0u, registry_->enabled_extensions().size()); - EXPECT_EQ(0u, registry_->disabled_extensions().size()); - EXPECT_EQ(0u, registry_->terminated_extensions().size()); - EXPECT_EQ(0u, registry_->blacklisted_extensions().size()); -} diff --git a/chrome/browser/profiles/profile_manager.cc b/chrome/browser/profiles/profile_manager.cc index 1746f90..5d67669 100644 --- a/chrome/browser/profiles/profile_manager.cc +++ b/chrome/browser/profiles/profile_manager.cc @@ -1073,13 +1073,6 @@ void ProfileManager::FinishDeletingProfile(const base::FilePath& profile_dir) { Profile* profile = GetProfileByPath(profile_dir); if (profile) { - // TODO: Migrate additional code in this block to observe this notification - // instead of being implemented here. - content::NotificationService::current()->Notify( - chrome::NOTIFICATION_PROFILE_DESTRUCTION_STARTED, - content::Source<Profile>(profile), - content::NotificationService::NoDetails()); - // By this point, all in-progress downloads for the profile being deleted // must have been canceled (crbug.com/336725). DCHECK(DownloadServiceFactory::GetForBrowserContext(profile)-> diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc index f3e71c1..0643a18 100644 --- a/chrome/browser/ui/browser.cc +++ b/chrome/browser/ui/browser.cc @@ -445,11 +445,6 @@ Browser::Browser(const CreateParams& params) } Browser::~Browser() { - // Stop observing notifications before continuing with destruction. Profile - // destruction will unload extensions and reentrant calls to Browser:: should - // be avoided while it is being torn down. - registrar_.RemoveAll(); - // The tab strip should not have any tabs at this point. DCHECK(tab_strip_model_->empty()); tab_strip_model_->RemoveObserver(this); diff --git a/extensions/browser/process_manager.cc b/extensions/browser/process_manager.cc index b110aa3..9e5f0c6 100644 --- a/extensions/browser/process_manager.cc +++ b/extensions/browser/process_manager.cc @@ -436,10 +436,7 @@ void ProcessManager::DecrementLazyKeepaliveCount(const Extension* extension) { void ProcessManager::DecrementLazyKeepaliveCount( const std::string& extension_id) { int& count = background_page_data_[extension_id].lazy_keepalive_count; - DCHECK(count > 0 || - !ExtensionRegistry::Get(GetBrowserContext()) - ->enabled_extensions() - .Contains(extension_id)); + DCHECK_GT(count, 0); // If we reach a zero keepalive count when the lazy background page is about // to be closed, incrementing close_sequence_id will cancel the close diff --git a/extensions/common/extension.h b/extensions/common/extension.h index 6679252..4000910 100644 --- a/extensions/common/extension.h +++ b/extensions/common/extension.h @@ -522,13 +522,11 @@ struct InstalledExtensionInfo { struct UnloadedExtensionInfo { // TODO(DHNishi): Move this enum to ExtensionRegistryObserver. enum Reason { - REASON_UNDEFINED, // Undefined state used to initialize variables. - REASON_DISABLE, // Extension is being disabled. - REASON_UPDATE, // Extension is being updated to a newer version. - REASON_UNINSTALL, // Extension is being uninstalled. - REASON_TERMINATE, // Extension has terminated. - REASON_BLACKLIST, // Extension has been blacklisted. - REASON_PROFILE_SHUTDOWN, // Profile is being shut down. + REASON_DISABLE, // Extension is being disabled. + REASON_UPDATE, // Extension is being updated to a newer version. + REASON_UNINSTALL, // Extension is being uninstalled. + REASON_TERMINATE, // Extension has terminated. + REASON_BLACKLIST, // Extension has been blacklisted. }; Reason reason; |