Disable ephemeral apps after they stop running

Disable ephemeral apps after they stop running

Ephemeral apps are unloaded and disabled after they stop running to
ensure that they have no background activity while they are cached.

The event router, message service and message center no longer need
special handling for idle ephemeral apps.

BUG=339001, 358052
TEST=browser_tests
TBR=dewittj@chromium.org (for removal of code in message_center_settings_controller.cc)

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

[tmdiep, 2014-06-30 02:46:18]

This patch unloads ephemeral apps when they stop running. Rather than adding
another list to the registry, I disabled the inactive ephemeral apps and added
another disable reason.

The main changes are in extension_service and ephemeral_app_service. The rest
are tests and removal of code from places that previously had to handle
ephemeral apps. Other things we need to implement, such as releasing power keep
alive when ephemeral apps stop running, just happen naturally without special
handling.

Let me know what you think of the approach. Thanks.

[benwells, 2014-07-02 00:56:53]

On 2014/06/30 02:46:18, tmdiep wrote:
> This patch unloads ephemeral apps when they stop running. Rather than adding
> another list to the registry, I disabled the inactive ephemeral apps and added
> another disable reason.
> 
> The main changes are in extension_service and ephemeral_app_service. The rest
> are tests and removal of code from places that previously had to handle
> ephemeral apps. Other things we need to implement, such as releasing power
keep
> alive when ephemeral apps stop running, just happen naturally without special
> handling.
> 
> Let me know what you think of the approach. Thanks.

Just looking at the description - by unload do you mean stop the background
page, or remove from the list of enabled extensions?

[tmdiep, 2014-07-02 00:58:31]

On 2014/07/02 00:56:53, benwells wrote:
> On 2014/06/30 02:46:18, tmdiep wrote:
> > This patch unloads ephemeral apps when they stop running. Rather than adding
> > another list to the registry, I disabled the inactive ephemeral apps and
added
> > another disable reason.
> > 
> > The main changes are in extension_service and ephemeral_app_service. The
rest
> > are tests and removal of code from places that previously had to handle
> > ephemeral apps. Other things we need to implement, such as releasing power
> keep
> > alive when ephemeral apps stop running, just happen naturally without
special
> > handling.
> > 
> > Let me know what you think of the approach. Thanks.
> 
> Just looking at the description - by unload do you mean stop the background
> page, or remove from the list of enabled extensions?

Remove from the list of enabled extensions - which sends out an "unload"
notification.

[benwells, 2014-07-02 06:56:11]

+yoz for his opinion on the overall approach.

[tmdiep, 2014-07-02 07:13:39]

https://codereview.chromium.org/344543006/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/344543006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:893:
extension_prefs_->AddDisableReason(extension_id, disable_reason);
yoz: Are multiple disable reasons meant to be supported? I see that a lot of
code does handle multiple disable reasons, but here, we do not add additional
reasons if the extension has already been disabled.

For example, an extension could be disabled due to a permissions increase, but
also have unsupported requirements.

[tmdiep, 2014-07-10 05:17:00]

Pinging yoz for opinion about the approach. Is it reasonable to disable
ephemeral apps that are not running? Also, are multiple disable reasons
expected? Thanks.

[Yoyo Zhou, 2014-07-10 23:30:12]

Sorry, I'm not too familiar with ephemeral apps in general. I think the general
idea of disabling it when it's not running is ok (I assume the status quo is
that it stays enabled?). Can you describe the possible state transitions an
ephemeral app can take? Let me know if this is correct. I think the question
marks are the areas that I'm not sure are covered here:

(first launch) not installed -> enabled
(app stops running) enabled -> disabled
(launched again?) disabled -> enabled
(promoted) enabled/disabled -> regular install
(uninstalled?) enabled/disabled -> not installed

Launched again: say the user launches X, then X stops running, then they launch
X again. It should be re-enabled, rather than reinstalled, right? Does that
happen in this patch?

Promoted: X can be promoted while it's either enabled or disabled; I think
that's handled here.

Uninstall: When does an ephemeral app get uninstalled? Shutdown?

https://codereview.chromium.org/344543006/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/344543006/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:893:
extension_prefs_->AddDisableReason(extension_id, disable_reason);
On 2014/07/02 07:13:39, tmdiep wrote:
> yoz: Are multiple disable reasons meant to be supported? I see that a lot of
> code does handle multiple disable reasons, but here, we do not add additional
> reasons if the extension has already been disabled.
> 
> For example, an extension could be disabled due to a permissions increase, but
> also have unsupported requirements. 

Yes, they are meant to be supported. This is most likely a bug. I'm not certain
that our code to remove disable reasons, on the other hand, is always correct,
e.g. an extension could be enabled without all of its disable reasons being
cleared.

[tmdiep, 2014-07-11 00:28:08]

Thanks for taking a look at this. Good questions!

> Sorry, I'm not too familiar with ephemeral apps in general. I think the
general
> idea of disabling it when it's not running is ok (I assume the status quo is
> that it stays enabled?). 

Yes, they currently stay enabled. But it means that I have to put special
handling in event router, message service, etc. to prevent the apps from having
background activity after they are closed. I think this approach is better and
affords us stronger guarantees of no background activity after they stop
running.

> Can you describe the possible state transitions an
> ephemeral app can take? Let me know if this is correct. I think the question
> marks are the areas that I'm not sure are covered here:
> 
> (first launch) not installed -> enabled
> (app stops running) enabled -> disabled
> (launched again?) disabled -> enabled
> (promoted) enabled/disabled -> regular install
> (uninstalled?) enabled/disabled -> not installed

Add a couple of edge cases:
(updated and requested more permissions) enabled/disabled -> disabled
(clobbered by regular install due to race conditions instead of being promoted)
enabled/disabled -> regular install

> Launched again: say the user launches X, then X stops running, then they
launch
> X again. It should be re-enabled, rather than reinstalled, right? Does that
> happen in this patch?

Yes, this is handled, but a bit tricksy. Control flow hits
EphemeralAppLauncher::EnableInstalledApp(), which then calls upon the
ExtensionEnableFlow to enable the extension. If the ephemeral app was updated in
the background and requested more permissions, we'll get a "Confirm launch"
prompt. Otherwise, the app will get silently enabled. There are some test cases
for this in ephemeral_app_launcher_browsertest.cc.

> Promoted: X can be promoted while it's either enabled or disabled; I think
> that's handled here.

Yep, that's handled.

> Uninstall: When does an ephemeral app get uninstalled? Shutdown?

EphemeralAppService::GarbageCollectApps() essentially manages a LRU caches of
ephemeral apps.

[tmdiep, 2014-07-15 07:53:57]

Ben: yoz seems to be ok with the use of multiple disable reasons. Can you please
proceed with detailed review? Thanks.

[benwells, 2014-07-29 08:58:42]

Sorry, I forgot about this.

Just the one question...

https://codereview.chromium.org/344543006/diff/210001/chrome/browser/apps/eph...
File chrome/browser/apps/ephemeral_app_service.cc (right):

https://codereview.chromium.org/344543006/diff/210001/chrome/browser/apps/eph...
chrome/browser/apps/ephemeral_app_service.cc:266: if
(!prefs->IsExtensionDisabled(extension->id()) &&
Does this mean if an ephemeral app is disabled for other reasons between being
closed / the system shutting down and this happening, it won't be get
DISABLE_INACTIVE_EPHEMERAL_APP?

[tmdiep, 2014-08-04 06:22:54]

Thanks for taking a look at this.

Patch set 10 addresses the review comment and has a rename of unload -> disable
to avoid confusion.

https://codereview.chromium.org/344543006/diff/210001/chrome/browser/apps/eph...
File chrome/browser/apps/ephemeral_app_service.cc (right):

https://codereview.chromium.org/344543006/diff/210001/chrome/browser/apps/eph...
chrome/browser/apps/ephemeral_app_service.cc:266: if
(!prefs->IsExtensionDisabled(extension->id()) &&
On 2014/07/29 08:58:42, benwells wrote:
> Does this mean if an ephemeral app is disabled for other reasons between being
> closed / the system shutting down and this happening, it won't be get
> DISABLE_INACTIVE_EPHEMERAL_APP?

Good catch. If the app happened to be disabled for other reasons, it should
still have that disable reason set.  Fixed in patch set 10 with tests.

[benwells, 2014-08-05 22:34:49]

lgtm, thanks!

[tmdiep, 2014-08-05 22:59:29]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-08-05 23:01:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tmdiep@chromium.org/344543006/250001

[commit-bot: I haz the power, 2014-08-06 06:26:33]

Change committed as 287705