Unload all apps / extensions when deleting a profile.

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, 374683
TEST=Manual testing as described on http://crbug.com/368684#c1 and http://crbug.com/374683#c7

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269343

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270890

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272090

[not at google - send to devlin, 2014-05-06 22:25:28]

lgtm

https://codereview.chromium.org/266343002/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/266343002/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:1102: // Disable apps & extensions
now to stop them from running after a profile
I have a weak preference for this logic to be somewhere in extension service,
like extension_service->DeleteProfile(), but even better would be for this step
to be a notification with ExtensionService listening to it (TODO other code in
here, do this too).

I'll defer to the profile/ owners for what they prefer, though.

[scheib, 2014-05-07 21:29:35]

kalman, Thanks - refactored as requested.

sky, ptal as owner:
  chrome/browser/background/background_contents_service.cc
  chrome/browser/chrome_notification_types.h
  chrome/browser/profiles/profile_manager.cc
  chrome/browser/ui/browser.cc

https://codereview.chromium.org/266343002/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/266343002/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:1102: // Disable apps & extensions
now to stop them from running after a profile
On 2014/05/06 22:25:29, kalman wrote:
> I have a weak preference for this logic to be somewhere in extension service,
> like extension_service->DeleteProfile(), but even better would be for this
step
> to be a notification with ExtensionService listening to it (TODO other code in
> here, do this too).
> 
> I'll defer to the profile/ owners for what they prefer, though.

Done.

[sky, 2014-05-07 22:17:32]

https://codereview.chromium.org/266343002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/266343002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:306:
content::NotificationService::AllBrowserContextsAndSources());
nit: seems like you could add for the source you care about and remove
conditional around 2159.

https://codereview.chromium.org/266343002/diff/100001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/266343002/diff/100001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:450: registrar_.RemoveAll();
Why is this explicitly needed? Assuming it is neeeed, added a comment as to why.

[scheib, 2014-05-08 15:17:51]

Thanks,

https://codereview.chromium.org/266343002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/266343002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:306:
content::NotificationService::AllBrowserContextsAndSources());
On 2014/05/07 22:17:32, sky wrote:
> nit: seems like you could add for the source you care about and remove
> conditional around 2159.

Done.

https://codereview.chromium.org/266343002/diff/100001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/266343002/diff/100001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:450: registrar_.RemoveAll();
On 2014/05/07 22:17:32, sky wrote:
> Why is this explicitly needed? Assuming it is neeeed, added a comment as to
why.

Done.

[not at google - send to devlin, 2014-05-08 15:57:32]

lgtm

https://codereview.chromium.org/266343002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/266343002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_unittest.cc:6938: EXPECT_EQ(0u,
registry_->blacklisted_extensions().size());
is there any chance you can test that the disable reason was profile shutdown
(i.e. capture and check the enum)?

[sky, 2014-05-08 15:58:44]

LGTM

[scheib, 2014-05-08 16:42:15]

thx

https://codereview.chromium.org/266343002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/266343002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_unittest.cc:6938: EXPECT_EQ(0u,
registry_->blacklisted_extensions().size());
On 2014/05/08 15:57:33, kalman wrote:
> is there any chance you can test that the disable reason was profile shutdown
> (i.e. capture and check the enum)?

Done.

[scheib, 2014-05-08 16:42:26]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2014-05-08 16:44:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scheib@chromium.org/266343002/120002

[commit-bot: I haz the power, 2014-05-09 17:03:07]

Change committed as 269343

[scheib, 2014-05-14 14:49:14]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2014-05-14 14:49:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scheib@chromium.org/266343002/150001

[commit-bot: I haz the power, 2014-05-14 16:33:48]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-05-14 16:37:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-14 16:37:12]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[scheib, 2014-05-14 16:45:32]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2014-05-14 16:45:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scheib@chromium.org/266343002/150001

[commit-bot: I haz the power, 2014-05-14 16:51:07]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-05-14 17:04:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-14 17:04:50]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[scheib, 2014-05-14 21:22:22]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2014-05-14 21:23:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scheib@chromium.org/266343002/150001

[commit-bot: I haz the power, 2014-05-14 21:45:41]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-05-14 21:59:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-14 21:59:47]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[scheib, 2014-05-16 00:23:17]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2014-05-16 00:24:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scheib@chromium.org/266343002/150001

[commit-bot: I haz the power, 2014-05-16 01:26:34]

Change committed as 270890

[scheib, 2014-05-21 16:59:24]

A revert of this CL has been created in
https://codereview.chromium.org/289283013/ by scheib@chromium.org.

The reason for reverting is: Caused crash in official builds when deleting
profiles while the chrome:chrome-signin tab is open:
https://code.google.com/p/chromium/issues/detail?id=374683.

[scheib, 2014-05-21 19:18:15]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2014-05-21 19:56:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scheib@chromium.org/266343002/200001

[commit-bot: I haz the power, 2014-05-22 04:16:13]

Change committed as 272090