Keep browser process alive while there are platform apps with background pages running.

Keep browser process alive while there are platform apps with background pages running. This does not happen on Android or ChromeOS builds: Android does not support browser process keep alive, and ChromeOS does not need or like it.

This change prevents platform apps getting killed unceremoniously while they have background pages active.

This also delays the process being shutdown after closing the last platform app window, as background pages are kept alive for 15 seconds after their last activity is completed.

This change re-lands r165772 which caused problems on Google branded ChromeOS. This version does not add keep alives for ChromeOS and was tested on the ChromeOS try bots.

TBR=davemoore@chromium.org for the ash launcher test change.

BUG=155457
TEST=Make sure platform apps can be used from the launcher without chrome windows open; make sure Chrome shuts down properly in all cases.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165772

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

[benwells, 2012-10-12 05:33:38]

Try jobs are running and might show problems with this change, which I am a bit
nervous about.

Adding two big guns sky@ and aa@ for review as it feels like a heavy / scary
change.

http://codereview.chromium.org/11117011/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/11117011/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_host.cc:162: extension_host_type_ ==
chrome::VIEW_TYPE_EXTENSION_BACKGROUND_PAGE) {
I don't like putting the is_platform_app check here. This is the only difference
so far for platform apps in the background page behavior.

Also, the extension_host_type check is strictly unnecessary but it feels less
fragile / scary with it in.

[benwells, 2012-10-12 07:52:00]

From the try jobs it looks like I'll need to use the command line switches to
override the default 10/5 second timeouts in tests.

[sky, 2012-10-12 17:06:34]

I don't have a good feel for how much this is going to come up and whether we
really want it. Aaron will have a better idea here.

[Aaron Boodman, 2012-10-12 17:58:30]

http://codereview.chromium.org/11117011/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/11117011/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_host.cc:162: extension_host_type_ ==
chrome::VIEW_TYPE_EXTENSION_BACKGROUND_PAGE) {
On 2012/10/12 05:33:38, benwells wrote:
> I don't like putting the is_platform_app check here. This is the only
difference
> so far for platform apps in the background page behavior.
> 
> Also, the extension_host_type check is strictly unnecessary but it feels less
> fragile / scary with it in.

What about if ExtensionHost supported this feature as a property:

extension_host->set_keeps_browser_alive(true)

Would there be a natural app-specific place to call this?

[benwells, 2012-10-25 06:47:54]

So ... only one test to go (I think), but I thought before sinking more time
into it I would get some comments on some things.

1) is the need to wait for the platform app host to die in all these tests a
good thing or a bad thing? I guess it tests our keep alive counts (on background
pages) well, but it has been fiddly. Maybe there is a simpler / less robust way
(like nuking all platform apps).

2) Yes, platform app tests are messs. The tests in platform_app_browsertests
should be moved into different files and maybe PlatformAppApiTest can be
removed. I'd like to do that later.

3) The last bluetooth test has me a bit stumped. It is leaking a background page
keep alive. What happens is that it calls a function twice, concurrently. The
first one creates a browser process function which doesn't get freed (this is
the leak). The second one doesn't as it causes an exception to be thrown.

Is this a problem in general or just in the test? I'm guessing the latter, and
there is some tricky way to kick the function along, but maybe someone can save
me more trawling.

[bryeung, 2012-10-25 14:18:59]

On 2012/10/25 06:47:54, benwells wrote:
> 3) The last bluetooth test has me a bit stumped. It is leaking a background
page
> keep alive. What happens is that it calls a function twice, concurrently. The
> first one creates a browser process function which doesn't get freed (this is
> the leak). The second one doesn't as it causes an exception to be thrown.

The leak seems like it might have been introduced by your CL.  I put a LOG in
UIThreadExtensionFunction::Destruct.  Without your patch, I a LOG for the
BluetoothGetDevicesFunction.  With your patch, I no longer see the log.

I've also verified that BluetoothGetDevicesFunction is correctly calling
SendResponse (with and without your patch).  There are no AddRefs in
bluetooth_api.cc.  I'm not sure what is still holding onto the reference.

Let me know if I can provide more insight into how the test or the Bluetooth
code works.

[benwells, 2012-10-25 22:34:36]

On 2012/10/25 14:18:59, bryeung wrote:
> On 2012/10/25 06:47:54, benwells wrote:
> > 3) The last bluetooth test has me a bit stumped. It is leaking a background
> page
> > keep alive. What happens is that it calls a function twice, concurrently.
The
> > first one creates a browser process function which doesn't get freed (this
is
> > the leak). The second one doesn't as it causes an exception to be thrown.
> 
> The leak seems like it might have been introduced by your CL.  I put a LOG in
> UIThreadExtensionFunction::Destruct.  Without your patch, I a LOG for the
> BluetoothGetDevicesFunction.  With your patch, I no longer see the log.
> 
> I've also verified that BluetoothGetDevicesFunction is correctly calling
> SendResponse (with and without your patch).  There are no AddRefs in
> bluetooth_api.cc.  I'm not sure what is still holding onto the reference.
> 
> Let me know if I can provide more insight into how the test or the Bluetooth
> code works.

I am seeing the problem without the change as well, but my guess is it's not a
bluetooth api specific problem. The function is destroyed after the test
finishes. I'll keep looking.

[benwells, 2012-10-26 05:35:42]

Now ready for review.

aa: all of it
davemoore: owner for chrome/browser/ui/ash/launcher

[benwells, 2012-10-29 05:27:44]

On 2012/10/26 05:35:42, benwells wrote:
> Now ready for review.
> 
> aa: all of it
> davemoore: owner for chrome/browser/ui/ash/launcher

aa: we discussed pulling the is_platform_app check out of ExtensionHost to keep
it platform app / extension agnostic. I think this is a non trivial refactor as
extension hosts are all created in equally low level extension / app agnostic
code.

Here are some approaches I can think of:

1. Create PlatformAppBackgroundHost, and create that instead in
ExtensionProcessManager::CreateBackgroundHost

2. Have a delegate attached to the ExtensionHost where we can put behavior which
changes.

I am imagining this change taking place in a world where app code is seperate
and uses extension code, but the core extension code knows nothing about apps.

Observations:

1. Would create two near identical classes. I'm not sure if in the long term
there would be enough difference to justify this approach.

2. This feels better. It will need some way to create the delegate. To keep the
low level extension code ignorant of apps, we should have some way of
registering delegate factories with the extension process manager. When needing
a delegate the process manager would ask its registered factories for one for a
given extension. Or something like that.

Maybe you or Matt have other suggestions? If possible I'd like to land this
change soon so there is not more fiddly test breakage.

[benwells, 2012-10-29 05:29:40]

On 2012/10/29 05:27:44, benwells wrote:
> On 2012/10/26 05:35:42, benwells wrote:
> > Now ready for review.
> > 
> > aa: all of it
> > davemoore: owner for chrome/browser/ui/ash/launcher
> 
> aa: we discussed pulling the is_platform_app check out of ExtensionHost to
keep
> it platform app / extension agnostic. I think this is a non trivial refactor
as
> extension hosts are all created in equally low level extension / app agnostic
> code.
> 
> Here are some approaches I can think of:
> 
> 1. Create PlatformAppBackgroundHost, and create that instead in
> ExtensionProcessManager::CreateBackgroundHost
> 
> 2. Have a delegate attached to the ExtensionHost where we can put behavior
which
> changes.
> 
> I am imagining this change taking place in a world where app code is seperate
> and uses extension code, but the core extension code knows nothing about apps.
> 
> Observations:
> 
> 1. Would create two near identical classes. I'm not sure if in the long term
> there would be enough difference to justify this approach.
> 
> 2. This feels better. It will need some way to create the delegate. To keep
the
> low level extension code ignorant of apps, we should have some way of
> registering delegate factories with the extension process manager. When
needing
> a delegate the process manager would ask its registered factories for one for
a
> given extension. Or something like that.
> 
> Maybe you or Matt have other suggestions? If possible I'd like to land this
> change soon so there is not more fiddly test breakage.

Just thinking some more, maybe an interim approach is to separate the different
logic into a delegate but keep an 'is_platform_app' check to create the delegate
in the extension process manager. Maybe that is a good step in the right
direction. I'll put something up with that shortly....

[benwells, 2012-10-29 07:10:58]

On 2012/10/29 05:29:40, benwells wrote:
> On 2012/10/29 05:27:44, benwells wrote:
> > On 2012/10/26 05:35:42, benwells wrote:
> > > Now ready for review.
> > > 
> > > aa: all of it
> > > davemoore: owner for chrome/browser/ui/ash/launcher
> > 
> > aa: we discussed pulling the is_platform_app check out of ExtensionHost to
> keep
> > it platform app / extension agnostic. I think this is a non trivial refactor
> as
> > extension hosts are all created in equally low level extension / app
agnostic
> > code.
> > 
> > Here are some approaches I can think of:
> > 
> > 1. Create PlatformAppBackgroundHost, and create that instead in
> > ExtensionProcessManager::CreateBackgroundHost
> > 
> > 2. Have a delegate attached to the ExtensionHost where we can put behavior
> which
> > changes.
> > 
> > I am imagining this change taking place in a world where app code is
seperate
> > and uses extension code, but the core extension code knows nothing about
apps.
> > 
> > Observations:
> > 
> > 1. Would create two near identical classes. I'm not sure if in the long term
> > there would be enough difference to justify this approach.
> > 
> > 2. This feels better. It will need some way to create the delegate. To keep
> the
> > low level extension code ignorant of apps, we should have some way of
> > registering delegate factories with the extension process manager. When
> needing
> > a delegate the process manager would ask its registered factories for one
for
> a
> > given extension. Or something like that.
> > 
> > Maybe you or Matt have other suggestions? If possible I'd like to land this
> > change soon so there is not more fiddly test breakage.
> 
> Just thinking some more, maybe an interim approach is to separate the
different
> logic into a delegate but keep an 'is_platform_app' check to create the
delegate
> in the extension process manager. Maybe that is a good step in the right
> direction. I'll put something up with that shortly....

OK, delegate added. I think it gives us a place to hang app specific host logic,
but it adds some app specific logic to extension_process_manager. WDYT?

[Yoyo Zhou, 2012-10-30 01:35:21]

http://codereview.chromium.org/11117011/diff/47001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/11117011/diff/47001/chrome/browser/extensions/...
chrome/browser/extensions/extension_browsertest.cc:564:
destroyed_observer.Wait();
This looks like it's only going to wait for 1 shell window to be closed. Unless
it's waiting for the background page? Can you document this if that's the case?

If you're subsequently exiting the test (as most invocations seem to be), then
why wait? (I did find 1 example where you do need to wait.)

http://codereview.chromium.org/11117011/diff/47001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/11117011/diff/47001/chrome/browser/extensions/...
chrome/browser/extensions/extension_host.h:58: class Delegate {
As we discussed, this delegate feels like more complexity than it's worth. Also,
I'm not confident that "delegate" is really the right name for this. It seems
easier to subclass ExtensionHost, overriding KeepsBrowserProcessAlive.

This may change later if you want to have more different behaviors for platform
app hosts, but as it is, you could pass a bool instead of a Delegate to the
constructor and achieve the same effect.

I'll let Aaron comment on this also.

http://codereview.chromium.org/11117011/diff/47001/chrome/test/data/extension...
File chrome/test/data/extensions/platform_apps/geometry/page1.js (right):

http://codereview.chromium.org/11117011/diff/47001/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/geometry/page1.js:15:
chrome.test.sendMessage('Launched');
Was this intentional?

[benwells, 2012-10-30 02:55:23]

http://codereview.chromium.org/11117011/diff/47001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/11117011/diff/47001/chrome/browser/extensions/...
chrome/browser/extensions/extension_browsertest.cc:564:
destroyed_observer.Wait();
On 2012/10/30 01:35:22, Yoyo Zhou wrote:
> This looks like it's only going to wait for 1 shell window to be closed.
Unless
> it's waiting for the background page? Can you document this if that's the
case?
> 
> If you're subsequently exiting the test (as most invocations seem to be), then
> why wait? (I did find 1 example where you do need to wait.)

It's waiting for the background page to exit. I'll document it in the next patch
set.

It needs to wait as otherwise the browser process is prevented from being shut
down, making tests time out.

http://codereview.chromium.org/11117011/diff/47001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/11117011/diff/47001/chrome/browser/extensions/...
chrome/browser/extensions/extension_host.h:58: class Delegate {
On 2012/10/30 01:35:22, Yoyo Zhou wrote:
> As we discussed, this delegate feels like more complexity than it's worth.
Also,
> I'm not confident that "delegate" is really the right name for this. It seems
> easier to subclass ExtensionHost, overriding KeepsBrowserProcessAlive.
> 
> This may change later if you want to have more different behaviors for
platform
> app hosts, but as it is, you could pass a bool instead of a Delegate to the
> constructor and achieve the same effect.
> 
> I'll let Aaron comment on this also.

I agree with your comments. I don't like passing the bool in as it just moves
the platform app specific behavior down to ExtensionProcessManager, where it
belongs even less.

A better approach would be to add a PlatformAppService which listens to
EXTENSION_HOST_CREATED and EXTENSION_HOST_DESTROYED messages. That shouldn't
have any of these problems, and would be a natural place for the
ShellWindowRegistry to live.

I'll see how that feels later today, if I get to it.

http://codereview.chromium.org/11117011/diff/47001/chrome/test/data/extension...
File chrome/test/data/extensions/platform_apps/geometry/page1.js (right):

http://codereview.chromium.org/11117011/diff/47001/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/geometry/page1.js:15:
chrome.test.sendMessage('Launched');
On 2012/10/30 01:35:22, Yoyo Zhou wrote:
> Was this intentional?

Yes. I've restructured a bit - most tests that used
ExtensionHost::(Install|Load)AndLaunchPlatformApp had a listener waiting for
this message to come through. To simplify things I moved that code into
(Install|Load|AndLaunchPlatformApp.

There should be more restructuring done of the platform apps but I'd like to do
them in separate CLs.

[Aaron Boodman, 2012-10-30 05:30:08]

http://codereview.chromium.org/11117011/diff/47001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/11117011/diff/47001/chrome/browser/extensions/...
chrome/browser/extensions/extension_host.h:58: class Delegate {
On 2012/10/30 02:55:23, benwells wrote:
> I agree with your comments. I don't like passing the bool in as it just moves
> the platform app specific behavior down to ExtensionProcessManager, where it
> belongs even less.
> 
> A better approach would be to add a PlatformAppService which listens to
> EXTENSION_HOST_CREATED and EXTENSION_HOST_DESTROYED messages. That shouldn't
> have any of these problems, and would be a natural place for the
> ShellWindowRegistry to live.

This sounds like a good plan to me!

[benwells, 2012-10-31 01:46:35]

With notification, and tests passing.

This change highlights the mess that is platform app testing. I'd like to clean
it up, but in a follow up series of patches if that's deemed acceptable.

[Aaron Boodman, 2012-10-31 07:31:22]

Yay, I like this a lot. Thanks for figuring it out.

http://codereview.chromium.org/11117011/diff/51007/chrome/browser/extensions/...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/11117011/diff/51007/chrome/browser/extensions/...
chrome/browser/extensions/extension_host.cc:477: void
ExtensionHost::KeepBrowserProcessAlive() {
Nit: I think SetKeepsBrowserProcessAlive() would be a bit more understandable.

http://codereview.chromium.org/11117011/diff/51007/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_service.cc (right):

http://codereview.chromium.org/11117011/diff/51007/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_service.cc:45:
///////////////////////////////////////////////////////////////////////////////
It seems like there could be a factory template or something instead of
everybody recoding this boilerplate, but oh well.

http://codereview.chromium.org/11117011/diff/51007/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_service.h (right):

http://codereview.chromium.org/11117011/diff/51007/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_service.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
Might as well put this in c/b/apps, eh?

http://codereview.chromium.org/11117011/diff/51007/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_service.h:29: //
content::NotificationObserver:
No reason to make this protected. It can be private.

[benwells, 2012-10-31 08:08:22]

http://codereview.chromium.org/11117011/diff/51007/chrome/browser/extensions/...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/11117011/diff/51007/chrome/browser/extensions/...
chrome/browser/extensions/extension_host.cc:477: void
ExtensionHost::KeepBrowserProcessAlive() {
On 2012/10/31 07:31:22, Aaron Boodman wrote:
> Nit: I think SetKeepsBrowserProcessAlive() would be a bit more understandable.

Done.

http://codereview.chromium.org/11117011/diff/51007/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_service.cc (right):

http://codereview.chromium.org/11117011/diff/51007/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_service.cc:45:
///////////////////////////////////////////////////////////////////////////////
On 2012/10/31 07:31:22, Aaron Boodman wrote:
> It seems like there could be a factory template or something instead of
> everybody recoding this boilerplate, but oh well.

Yes, it could be a template with traits and suchlike.

http://codereview.chromium.org/11117011/diff/51007/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_service.h (right):

http://codereview.chromium.org/11117011/diff/51007/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_service.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2012/10/31 07:31:22, Aaron Boodman wrote:
> Might as well put this in c/b/apps, eh?

I was hoping to do that in another CL, to allow full and proper bikeshedding on
the name and OWNERS. For an example of bikeshedding, I am thinking it should be
c/b/platform_apps.

http://codereview.chromium.org/11117011/diff/51007/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_service.h:29: //
content::NotificationObserver:
On 2012/10/31 07:31:22, Aaron Boodman wrote:
> No reason to make this protected. It can be private.

Done.

[Aaron Boodman, 2012-10-31 08:30:37]

lgtm

[benwells, 2012-10-31 08:41:49]

davemoore: could you look at the change to
chroem_launcher_controller_browsertest.cc?
sky: just chrome/chrome_browser_extensions.gypi needed

[benwells, 2012-10-31 08:42:28]

+sky this time, just for chrome/chrome_browser_extensions.gypi

[sky, 2012-10-31 14:21:35]

LGTM - feel free to TBR me on gyp changes

[benwells, 2012-10-31 23:04:57]

On 2012/10/31 14:21:35, sky wrote:
> LGTM - feel free to TBR me on gyp changes

As the ash test change is so minor I am TBR'ing it.

[benwells, 2012-11-01 00:17:01]

Yoyo pointed out how to do the service stuff better, so some slight changes.

yoyo: could you have a look at it now?
sky: could you look at the change in c/b/profiles/profile_dependency_manager?

[Yoyo Zhou, 2012-11-01 00:23:41]

LGTM

http://codereview.chromium.org/11117011/diff/61011/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_service_factory.cc (right):

http://codereview.chromium.org/11117011/diff/61011/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_service_factory.cc:24:
ProfileDependencyManager::GetInstance()) {
If it doesn't make sense to outlive ExtensionService and friends at destruction
time, then you should declare

DependsOn(ExtensionSystemFactory::GetInstance())

here.

[sky, 2012-11-01 14:10:35]

.gypi LGTM

[sky, 2012-11-01 16:15:57]

I'm not a good person for profile_dependency_manager.cc. I added Elliot to cover
it.

[Elliot Glaysher, 2012-11-01 17:24:29]

profiles lgtm

(Also, if Yoyo is correct and your PlatformApp stuff can't outlive the extension
service, please add the proper DependsOn() calls to make sure things are
destroyed in the right order.)

[Yoyo Zhou, 2012-11-01 21:25:45]

On 2012/11/01 17:24:29, Elliot Glaysher wrote:
> (Also, if Yoyo is correct and your PlatformApp stuff can't outlive the
extension
> service, please add the proper DependsOn() calls to make sure things are
> destroyed in the right order.)

LGTM as is; after some discussion, DependingOn doesn't appear to actually be
necessary here.

[commit-bot: I haz the power, 2012-11-01 21:27:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/11117011/61011

[commit-bot: I haz the power, 2012-11-01 22:02:42]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-02 05:33:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/11117011/61011

[commit-bot: I haz the power, 2012-11-02 05:48:37]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-02 06:47:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/11117011/67006

[commit-bot: I haz the power, 2012-11-02 06:47:33]

Failed to apply patch for
chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc
  Hunk #1 FAILED at 30.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc.rej

Patch:      
chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc
Index:
chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc
diff --git
a/chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc
b/chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc
index
220f15fe7a40d43c4c0b7ac268d93f1a022f9c22..c58f474750ad3fa944bab017b0a144e80096053e
100644
--- a/chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc
+++ b/chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc
@@ -30,12 +30,12 @@ class SyncFileSystemApiTest : public ExtensionApiTest {
 IN_PROC_BROWSER_TEST_F(SyncFileSystemApiTest, GetUsageAndQuota) {
   // TODO(calvinlo): Check value against mock syncFileSystem quota value
instead
   // of comparing against the default. (http://crbug.com/153501).
-  ASSERT_TRUE(RunExtensionTest("sync_file_system/get_usage_and_quota"))
+  ASSERT_TRUE(RunPlatformAppTest("sync_file_system/get_usage_and_quota"))
       << message_;
 }
 
 IN_PROC_BROWSER_TEST_F(SyncFileSystemApiTest, RequestFileSystem) {
-  ASSERT_TRUE(RunExtensionTest("sync_file_system/request_file_system"))
+  ASSERT_TRUE(RunPlatformAppTest("sync_file_system/request_file_system"))
       << message_;
 }

[commit-bot: I haz the power, 2012-11-02 06:55:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/11117011/37041

[commit-bot: I haz the power, 2012-11-02 10:18:53]

Retried try job too often for step(s) content_browsertests

[commit-bot: I haz the power, 2012-11-02 21:10:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/11117011/37041

[commit-bot: I haz the power, 2012-11-02 22:21:02]

Change committed as 165772

[Peter Mayo, 2012-11-08 14:46:14]

On 2012/11/02 22:21:02, I haz the power (commit-bot) wrote:
> Change committed as 165772

This breaks logout on ChromeOS ... see bug crosbug.com/36058

[benwells, 2012-11-27 09:15:13]

On 2012/11/08 14:46:14, Peter Mayo wrote:
> On 2012/11/02 22:21:02, I haz the power (commit-bot) wrote:
> > Change committed as 165772
> 
> This breaks logout on ChromeOS ... see bug crosbug.com/36058

Reopened to bring back after it was reverted...

[benwells, 2012-11-27 10:18:31]

Here is the re-land / unrevert.

davidjames: please have a look at the chromeos disabling
calvinlo: please have a look at the sync test, i'm trying to pass a TODo to you.

https://codereview.chromium.org/11117011/diff/78005/chrome/browser/extensions...
File chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc
(right):

https://codereview.chromium.org/11117011/diff/78005/chrome/browser/extensions...
chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc:88:
#endif  // !defined(OS_CHROMEOS)
This is not in the local file. I'm not sure why it is shown in the patch, I
think it has confused reitveld somehow (notice the shading which usually means
it is deleted white space).

[calvinlo, 2012-11-27 10:21:51]

lgtm, I'll take a look at the test again after your patch lands.

[davidjames, 2012-11-27 14:32:59]

lgtm for Chrome OS disabling. Thanks for adding the comment explaining why.

Can you also update your CL description to mention your successful remote trybot
run on the Chrome OS tryserver?

[commit-bot: I haz the power, 2012-11-28 00:14:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/11117011/85039

[benwells, 2012-11-28 09:15:31]

+atwilson for chrome/browser/sync changes

[Andrew T Wilson (Slow), 2012-11-28 10:16:24]

LGTM with some nits you can ignore. Just a note to remind you to try setting the
idle/unload time to 0 secs, and you said there were some other tests (Sync?)
that need to have their unload time set to 0 too.

Maybe the default should be 0 for tests, and you can set it to non-zero just for
those tests that need to test BG pages? Otherwise, people may add new tests that
are unintentionally slow.

https://codereview.chromium.org/11117011/diff/76045/chrome/browser/extensions...
File chrome/browser/extensions/extension_host.h (right):

https://codereview.chromium.org/11117011/diff/76045/chrome/browser/extensions...
chrome/browser/extensions/extension_host.h:134: // added. Hosts which have had
this called will remove the keep alive when
nit: btw, I've always preferred "keep-alive" to "keep alive" as it makes it
clear that "keep-alive" is a singular noun and makes comments read more
smoothly. Feel free to leave as-is though :)

https://codereview.chromium.org/11117011/diff/76045/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_service.cc (right):

https://codereview.chromium.org/11117011/diff/76045/chrome/browser/extensions...
chrome/browser/extensions/platform_app_service.cc:18: registrar_.Add(this,
chrome::NOTIFICATION_EXTENSION_HOST_CREATED,
If we're keeping this class around on cros, consider #if !defined(OS_CHROMEOS)
this part too since there's no need to register for this notification if we're
ignoring it.

https://codereview.chromium.org/11117011/diff/76045/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_service.h (right):

https://codereview.chromium.org/11117011/diff/76045/chrome/browser/extensions...
chrome/browser/extensions/platform_app_service.h:26: virtual void Observe(int
type,
nit: this is fine as-is, but typically since we are publicly inheriting from
NotificationObserver, we might as well make the overridden methods public too,
since people could always just upcast to access them anyway.

https://codereview.chromium.org/11117011/diff/76045/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_service_factory.h (right):

https://codereview.chromium.org/11117011/diff/76045/chrome/browser/extensions...
chrome/browser/extensions/platform_app_service_factory.h:15: class
PlatformAppServiceFactory : public ProfileKeyedServiceFactory {
nit: (again, fine as-is) it's nice to document if the factory behavior differs
from the default (for example, if Incognito profiles have an instance of the
service, etc). Not a huge deal since callers can probably infer this from the
override below. But if GetForProfile() can sometimes return NULL, it's useful to
document under what circumstances it can do so (or likewise, if it never returns
NULL, that's good to document too).

[benwells, 2012-11-29 06:50:57]

https://codereview.chromium.org/11117011/diff/76045/chrome/browser/extensions...
File chrome/browser/extensions/extension_host.h (right):

https://codereview.chromium.org/11117011/diff/76045/chrome/browser/extensions...
chrome/browser/extensions/extension_host.h:134: // added. Hosts which have had
this called will remove the keep alive when
On 2012/11/28 10:16:24, Andrew T Wilson wrote:
> nit: btw, I've always preferred "keep-alive" to "keep alive" as it makes it
> clear that "keep-alive" is a singular noun and makes comments read more
> smoothly. Feel free to leave as-is though :)

Done.

https://codereview.chromium.org/11117011/diff/76045/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_service.cc (right):

https://codereview.chromium.org/11117011/diff/76045/chrome/browser/extensions...
chrome/browser/extensions/platform_app_service.cc:18: registrar_.Add(this,
chrome::NOTIFICATION_EXTENSION_HOST_CREATED,
On 2012/11/28 10:16:24, Andrew T Wilson wrote:
> If we're keeping this class around on cros, consider #if !defined(OS_CHROMEOS)
> this part too since there's no need to register for this notification if we're
> ignoring it.

Done.

https://codereview.chromium.org/11117011/diff/76045/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_service.h (right):

https://codereview.chromium.org/11117011/diff/76045/chrome/browser/extensions...
chrome/browser/extensions/platform_app_service.h:26: virtual void Observe(int
type,
On 2012/11/28 10:16:24, Andrew T Wilson wrote:
> nit: this is fine as-is, but typically since we are publicly inheriting from
> NotificationObserver, we might as well make the overridden methods public too,
> since people could always just upcast to access them anyway.

Ah. I've been asked to do the opposite before ... I think I'll leave it as is.

[commit-bot: I haz the power, 2012-11-29 06:51:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/11117011/77017

[commit-bot: I haz the power, 2012-11-29 06:51:47]

Failed to apply patch for chrome/browser/extensions/platform_app_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/platform_app_browsertest.cc
  Hunk #1 FAILED at 20.
  Hunk #2 succeeded at 146 (offset 1 line).
  Hunk #3 succeeded at 155 (offset 1 line).
  Hunk #4 succeeded at 180 (offset 1 line).
  Hunk #5 succeeded at 224 (offset 1 line).
  Hunk #6 succeeded at 235 (offset 1 line).
  Hunk #7 succeeded at 252 (offset 1 line).
  Hunk #8 succeeded at 276 (offset 1 line).
  Hunk #9 succeeded at 300 (offset 1 line).
  Hunk #10 succeeded at 325 (offset 1 line).
  Hunk #11 succeeded at 350 (offset 1 line).
  Hunk #12 succeeded at 373 (offset 1 line).
  Hunk #13 succeeded at 389 (offset 1 line).
  Hunk #14 succeeded at 460 (offset 1 line).
  Hunk #15 succeeded at 477 (offset 1 line).
  Hunk #16 succeeded at 527 (offset 1 line).
  Hunk #17 succeeded at 594 (offset 1 line).
  Hunk #18 FAILED at 671.
  Hunk #19 succeeded at 698 (offset -4 lines).
  Hunk #20 succeeded at 716 (offset -4 lines).
  Hunk #21 succeeded at 755 (offset -4 lines).
  Hunk #22 succeeded at 797 (offset -4 lines).
  Hunk #23 succeeded at 818 with fuzz 1 (offset -4 lines).
  2 out of 23 hunks FAILED -- saving rejects to file
chrome/browser/extensions/platform_app_browsertest.cc.rej

Patch:       chrome/browser/extensions/platform_app_browsertest.cc
Index: chrome/browser/extensions/platform_app_browsertest.cc
diff --git a/chrome/browser/extensions/platform_app_browsertest.cc
b/chrome/browser/extensions/platform_app_browsertest.cc
index
46b176fe4c0bde5af29c5616fad1812d1eb4f244..7312ab03dbc0e51e69bdf14fd4b7eee95b600ea9
100644
--- a/chrome/browser/extensions/platform_app_browsertest.cc
+++ b/chrome/browser/extensions/platform_app_browsertest.cc
@@ -20,9 +20,11 @@
 #include "chrome/browser/extensions/platform_app_browsertest_util.h"
 #include "chrome/browser/extensions/platform_app_launcher.h"
 #include "chrome/browser/extensions/shell_window_registry.h"
+#include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/tab_contents/render_view_context_menu.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
+#include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/constrained_window_tab_helper.h"
 #include "chrome/browser/ui/extensions/application_launch.h"
 #include "chrome/browser/ui/extensions/shell_window.h"
@@ -145,6 +147,8 @@ const char kTestFilePath[] =
"platform_apps/launch_files/test.txt";
 
 }  // namespace
 
+// TODO(benwells): Break up this file into some sensible smaller files.
+
 // Tests that CreateShellWindow doesn't crash if you close it straight away.
 // LauncherPlatformAppBrowserTest relies on this behaviour, but is only run for
 // ash, so we test that it works here.
@@ -152,6 +156,7 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest,
CreateAndCloseShellWindow) {
   const Extension* extension = LoadAndLaunchPlatformApp("minimal");
   ShellWindow* window = CreateShellWindow(extension);
   CloseShellWindow(window);
+  CloseShellWindowsAndWaitForAppToExit();
 }
 
 // Tests that platform apps can be launched in incognito mode without crashing.
@@ -176,6 +181,16 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest,
MAYBE_LaunchAppIncognito) {
           NEW_WINDOW));
 
   ASSERT_TRUE(launched_listener.WaitUntilSatisfied());
+  CloseShellWindowsAndWaitForAppToExit();
+}
+
+// Tests that the browser process is kept alive by the platform app's
background
+// page.
+IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, AppEventPageKeepsBrowserAlive) {
+  LoadAndLaunchPlatformApp("minimal");
+  browser()->window()->Close();
+  ASSERT_TRUE(browser::WillKeepAlive());
+  CloseShellWindowsAndWaitForAppToExit();
 }
 
 // Tests that platform apps received the "launch" event when launched.
@@ -210,6 +225,7 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, LaunchReply)
{
       web_contents);
 
   ASSERT_TRUE(handler.WaitUntilReply());
+  CloseShellWindowsAndWaitForAppToExit();
 }
 
 // Tests that platform apps cannot use certain disabled window properties, but
@@ -220,11 +236,8 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest,
DisabledWindowProperties) {
 }
 
 IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, EmptyContextMenu) {
-  ExtensionTestMessageListener launched_listener("Launched", false);
   LoadAndLaunchPlatformApp("minimal");
 
-  ASSERT_TRUE(launched_listener.WaitUntilSatisfied());
-
   // The empty app doesn't add any context menu items, so its menu should
   // only include the developer tools.
   WebContents* web_contents = GetFirstShellWindowWebContents();
@@ -240,16 +253,12 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest,
EmptyContextMenu) {
   ASSERT_TRUE(menu->HasCommandWithId(IDC_CONTENT_CONTEXT_RELOAD_PACKAGED_APP));
   ASSERT_FALSE(menu->HasCommandWithId(IDC_BACK));
   ASSERT_FALSE(menu->HasCommandWithId(IDC_SAVE_PAGE));
+  CloseShellWindowsAndWaitForAppToExit();
 }
 
 IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, AppWithContextMenu) {
-  ExtensionTestMessageListener launched_listener("Launched", false);
   LoadAndLaunchPlatformApp("context_menu");
 
-  // Wait for the extension to tell us it's initialized its context menus and
-  // launched a window.
-  ASSERT_TRUE(launched_listener.WaitUntilSatisfied());
-
   // The context_menu app has two context menu items. These, along with a
   // separator and the developer tools, is all that should be in the menu.
   WebContents* web_contents = GetFirstShellWindowWebContents();
@@ -268,16 +277,12 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest,
AppWithContextMenu) {
   ASSERT_FALSE(menu->HasCommandWithId(IDC_BACK));
   ASSERT_FALSE(menu->HasCommandWithId(IDC_SAVE_PAGE));
   ASSERT_FALSE(menu->HasCommandWithId(IDC_CONTENT_CONTEXT_UNDO));
+  CloseShellWindowsAndWaitForAppToExit();
 }
 
 IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, InstalledAppWithContextMenu) {
-  ExtensionTestMessageListener launched_listener("Launched", false);
   InstallAndLaunchPlatformApp("context_menu");
 
-  // Wait for the extension to tell us it's initialized its context menus and
-  // launched a window.
-  ASSERT_TRUE(launched_listener.WaitUntilSatisfied());
-
   // The context_menu app has two context menu items. For an installed app
   // these are all that should be in the menu.
   WebContents* web_contents = GetFirstShellWindowWebContents();
@@ -296,16 +301,12 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest,
InstalledAppWithContextMenu) {
   ASSERT_FALSE(menu->HasCommandWithId(IDC_BACK));
   ASSERT_FALSE(menu->HasCommandWithId(IDC_SAVE_PAGE));
   ASSERT_FALSE(menu->HasCommandWithId(IDC_CONTENT_CONTEXT_UNDO));
+  CloseShellWindowsAndWaitForAppToExit();
 }
 
 IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, AppWithContextMenuTextField) {
-  ExtensionTestMessageListener launched_listener("Launched", false);
   LoadAndLaunchPlatformApp("context_menu");
 
-  // Wait for the extension to tell us it's initialized its context menus and
-  // launched a window.
-  ASSERT_TRUE(launched_listener.WaitUntilSatisfied());
-
   // The context_menu app has one context menu item. This, along with a
   // separator and the developer tools, is all that should be in the menu.
   WebContents* web_contents = GetFirstShellWindowWebContents();
@@ -325,16 +326,12 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest,
AppWithContextMenuTextField) {
   ASSERT_TRUE(menu->HasCommandWithId(IDC_CONTENT_CONTEXT_COPY));
   ASSERT_FALSE(menu->HasCommandWithId(IDC_BACK));
   ASSERT_FALSE(menu->HasCommandWithId(IDC_SAVE_PAGE));
+  CloseShellWindowsAndWaitForAppToExit();
 }
 
 IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, AppWithContextMenuSelection) {
-  ExtensionTestMessageListener launched_listener("Launched", false);
   LoadAndLaunchPlatformApp("context_menu");
 
-  // Wait for the extension to tell us it's initialized its context menus and
-  // launched a window.
-  ASSERT_TRUE(launched_listener.WaitUntilSatisfied());
-
   // The context_menu app has one context menu item. This, along with a
   // separator and the developer tools, is all that should be in the menu.
   WebContents* web_contents = GetFirstShellWindowWebContents();
@@ -354,16 +351,12 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest,
AppWithContextMenuSelection) {
   ASSERT_TRUE(menu->HasCommandWithId(IDC_CONTENT_CONTEXT_COPY));
   ASSERT_FALSE(menu->HasCommandWithId(IDC_BACK));
   ASSERT_FALSE(menu->HasCommandWithId(IDC_SAVE_PAGE));
+  CloseShellWindowsAndWaitForAppToExit();
 }
 
 IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, AppWithContextMenuClicked) {
-  ExtensionTestMessageListener launched_listener("Launched", false);
   LoadAndLaunchPlatformApp("context_menu_click");
 
-  // Wait for the extension to tell us it's initialized its context menus and
-  // launched a window.
-  ASSERT_TRUE(launched_listener.WaitUntilSatisfied());
-
   // Test that the menu item shows up
   WebContents* web_contents = GetFirstShellWindowWebContents();
   ASSERT_TRUE(web_contents);
@@ -381,13 +374,15 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest,
AppWithContextMenuClicked) {
   menu->ExecuteCommand(IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST);
 
   ASSERT_TRUE(onclicked_listener.WaitUntilSatisfied());
+  CloseShellWindowsAndWaitForAppToExit();
 }
 
 IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, DisallowNavigation) {
   TabsAddedNotificationObserver observer(2);
 
   ASSERT_TRUE(StartTestServer());
-  ASSERT_TRUE(RunPlatformAppTest("platform_apps/navigation")) << message_;
+  ASSERT_TRUE(RunPlatformAppTestReturnImmediately("platform_apps/navigation"))
+      << message_;
 
   observer.Wait();
   ASSERT_EQ(2U, observer.tabs().size());
@@ -395,6 +390,8 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest,
DisallowNavigation) {
             observer.tabs()[0]->GetURL().spec());
   EXPECT_EQ("http://chromium.org/",
             observer.tabs()[1]->GetURL().spec());
+  browser()->window()->Close();
+  CloseShellWindowsAndWaitForAppToExit();
 }
 …
(message too large)

[commit-bot: I haz the power, 2012-11-29 08:45:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/11117011/80042

[commit-bot: I haz the power, 2012-11-29 09:46:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/11117011/75011

[commit-bot: I haz the power, 2012-11-29 10:17:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/11117011/75011

[commit-bot: I haz the power, 2012-11-29 13:23:06]

Retried try job too often on linux_chromeos for step(s) browser_tests

[commit-bot: I haz the power, 2012-11-30 00:07:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/11117011/75011

[commit-bot: I haz the power, 2012-11-30 02:01:48]

Retried try job too often on linux_chromeos for step(s) browser_tests

[commit-bot: I haz the power, 2012-11-30 21:52:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/11117011/74022

[commit-bot: I haz the power, 2012-11-30 21:53:07]

Failed to apply patch for chrome/browser/extensions/extension_apitest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_apitest.cc
  Hunk #1 FAILED at 6.
  Hunk #2 succeeded at 174 (offset 1 line).
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_apitest.cc.rej

Patch:       chrome/browser/extensions/extension_apitest.cc
Index: chrome/browser/extensions/extension_apitest.cc
diff --git a/chrome/browser/extensions/extension_apitest.cc
b/chrome/browser/extensions/extension_apitest.cc
index
f70b3b12768c29246d9d2cbd02a4ed3c31b930c9..698ae40cdcd25a338ba37903ae70566724e933eb
100644
--- a/chrome/browser/extensions/extension_apitest.cc
+++ b/chrome/browser/extensions/extension_apitest.cc
@@ -6,8 +6,8 @@
 
 #include "base/string_util.h"
 #include "base/stringprintf.h"
-#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/api/test/test_api.h"
+#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/unpacked_installer.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
@@ -173,6 +173,13 @@ bool ExtensionApiTest::RunPageTest(const std::string&
page_url,
 }
 
 bool ExtensionApiTest::RunPlatformAppTest(const char* extension_name) {
+  bool res = RunPlatformAppTestReturnImmediately(extension_name);
+  CloseShellWindowsAndWaitForAppToExit();
+  return res;
+}
+
+bool ExtensionApiTest::RunPlatformAppTestReturnImmediately(
+    const char* extension_name) {
   return RunExtensionTestImpl(extension_name, "", kFlagLaunchPlatformApp);
 }

[commit-bot: I haz the power, 2012-11-30 22:17:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/11117011/77064

[commit-bot: I haz the power, 2012-11-30 23:51:21]

Retried try job too often on linux_rel for step(s) content_unittests

[commit-bot: I haz the power, 2012-12-01 02:51:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/11117011/85051

[commit-bot: I haz the power, 2012-12-01 07:16:48]

Change committed as 170660