Ephemeral apps should not trigger Chrome background mode

Ephemeral apps should not trigger Chrome background mode

Ephemeral apps should not trigger Chrome background mode and the
"New background app added" notification bubble should not be
displayed when they are launched as they do not have background
activity after they are closed.

BUG=339001
TEST=unit_tests

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

[tmdiep, 2014-07-11 07:32:34]

Firstly some background:
The ephemeral apps feature allows users to launch and try apps without
installing in Chrome. We do this by installing them transiently in extension
system, caching them for a short period of time, and then garbage collecting
them if they are unused. This transient install is only done for V2 apps (V1
apps are not supported and we don't bother installing the crx of hosted apps if
launched ephemerally).

By design, cached ephemeral apps do not have background activity (unlike
installed apps) - events and messages to them are blocked. This is analogous to
closing a browser tab.

The problem that this patch addresses:
I'm seeing some V2 apps triggering Chrome background mode and causing the "New
background app added" notification bubble to  appear when they are launched. I
think this is caused by the presence of the push messaging permission in the
manifest.

While the ephemeral app is running, we still want push messaging to work, but
once the app closes, it no longer receives messages. So I'd like to suppress the
notification and prevent the apps from triggering background mode.

atwilson: Please review this patch.
dimich: Is my reasoning correct that the push messaging permission in the V2 app
is the cause of triggering background mode?
Thanks all, especially if you read the entire message :)

[Dmitry Titov, 2014-07-11 08:16:00]

Background mode that you are disabling here is not a thing that lets "app to run
in the background". App won't run in the background - once its windows are
closed, the app will be killed in few seconds if there are no ongoing operations
with JS callbacks.

Instead, what "background mode" does is it keeps Chrome from exiting when all
tabs are closed, and it also causes Chrome to start up silently when user logs
into OS. This is done so that Chrome could keep a network connection to be able
to receive the push message and start up the app.

By doing what you are doing, you are not disabling the app to receive push
message when it is closed. If the user keeps Chrome running (keeping an open tab
for example), the app will still continue to receive the push messages (each one
will start the app's background page again). When (and if) Chrome is finally
closed, the app will stop receiving those.

So the effect will be likely a semi-random receiving of push messages (for
Googlers who keep Chrome running all the time there will be no difference, while
other users will sometimes get push messages and sometimes not). I doubt this is
the behavior you are after. I think we shoudl completely disable push
functionality for 'ephemeral apps', because it usually is useless to receive
them "while app is running" - they are typically used to wake up the app and
inform it about something that happens on the server when it is -not- running.

I think you should deny the pushMessaging permission itself form the app that is
ephemeral. Preferably letting user to know, since this can completely brake the
app.

[tmdiep, 2014-07-12 07:16:13]

On 2014/07/11 08:16:00, Dmitry Titov wrote:
> Background mode that you are disabling here is not a thing that lets "app to
run
> in the background". App won't run in the background - once its windows are
> closed, the app will be killed in few seconds if there are no ongoing
operations
> with JS callbacks.
> 
> ...
> 
> I think you should deny the pushMessaging permission itself form the app that
is
> ephemeral. Preferably letting user to know, since this can completely brake
the
> app.

Sorry for the confusion, I should have been more clear in my wording due to the
overload of the term 'background'.

I was aware that this patch has nothing to do with apps running in the
background. Another patch prevents the event/lazy background pages of inactive
cached ephemeral apps from being loaded:
https://codereview.chromium.org/118033002
In this second patch (review in progress), ephemeral apps will be disabled
altogether after their event page has been destroyed:
https://codereview.chromium.org/344543006
Both of these implementations prevent push messages from re-loading the event
page of an ephemeral app after it has been killed off the first time.

What I wanted to achieve in this CL was to:
- Prevent ephemeral apps that use push messaging from triggering Chrome's
background mode.
- Prevent the "New background app added" notification from appearing when the
ephemeral app is launched.
Basically because the apps will not receive the messages anyway.

As to whether we should allow ephemeral apps to use push messaging - we don't
want existing apps to break, so prefer to enable the functionality at least
while the app is running. After they are closed, we would like them to be
completely inactive while they are cached.

I've updated the description of this CL. Please let me know whether it achieves
its purpose. Thanks.

[Dmitry Titov, 2014-07-14 03:26:36]

I see you reasoning, thanks for adding a good explanation!

Apparently, I have reservations about the feature itself (as it effectively
creates yet another 'type' of apps which will likely have to be consciously
designed to actually work well in 'ephemeral' mode). Note that in some apps the
push is 'additional perk', while in others it's the main functionality (so once
ephemeral instance is killed, user does not receive incoming messages for
example). What is the plan on making user realize that when the app window is
closed, it will be 'silently uninstalled' in a few seconds? 

Having said that, I don't see an issue with your code except the note below.
Feel free to look at it and resubmit!

https://codereview.chromium.org/383113002/diff/1/chrome/browser/background/ba...
File chrome/browser/background/background_application_list_model.cc (right):

https://codereview.chromium.org/383113002/diff/1/chrome/browser/background/ba...
chrome/browser/background/background_application_list_model.cc:319: // be
reached for platforms apps with the push messaging permission, so
Also this can be reached by apps with "background' permission specified. We have
a whitelist for packaged apps that can have this permission. Also, this point
can be reached in other cases - like hosted apps with background mode.

I'd move this check to be the first and modified the comment to say something
like "nothing that is ephemeral can be background"...

[tmdiep, 2014-07-14 04:25:37]

On 2014/07/14 03:26:36, Dmitry Titov wrote:
> Apparently, I have reservations about the feature itself (as it effectively
> creates yet another 'type' of apps which will likely have to be consciously
> designed to actually work well in 'ephemeral' mode).

I understand and agree with your concerns. We've tried to make the ephemeral
mode back-compatible with existing apps as much as possible so that app
developers don't have to worry about catering for another mode too much. For
example, all APIs are available and (almost) all permissions are granted.

> What is the plan on making user realize that when the app window is
> closed, it will be 'silently uninstalled' in a few seconds? 

You might be interested in this discussion here:
https://code.google.com/p/chromium/issues/detail?id=339249
In summary, we discussed options, the consensus was that we should wait to see
if this proves to be a problem. Feel free to express your opinions in that bug.

https://codereview.chromium.org/383113002/diff/1/chrome/browser/background/ba...
File chrome/browser/background/background_application_list_model.cc (right):

https://codereview.chromium.org/383113002/diff/1/chrome/browser/background/ba...
chrome/browser/background/background_application_list_model.cc:319: // be
reached for platforms apps with the push messaging permission, so
On 2014/07/14 03:26:35, Dmitry Titov wrote:
> Also this can be reached by apps with "background' permission specified. We
have
> a whitelist for packaged apps that can have this permission. Also, this point
> can be reached in other cases - like hosted apps with background mode.
> 
> I'd move this check to be the first and modified the comment to say something
> like "nothing that is ephemeral can be background"...

Thanks for the suggestion. I've moved the check up and modified the comment.

We don't install the crx of hosted ephemeral apps as there is not much value in
doing so. When launched, we just navigate to their launch url in the browser. V1
apps cannot be ephemeral. So we shouldn't reach this point here for anything
other than platform apps, but it doesn't hurt to test all cases.

[Dmitry Titov, 2014-07-14 20:35:07]

LGTM

[Andrew T Wilson (Slow), 2014-07-23 13:34:32]

LGTM if you add the test requested.

https://codereview.chromium.org/383113002/diff/20001/chrome/browser/backgroun...
File chrome/browser/background/background_application_list_model_unittest.cc
(right):

https://codereview.chromium.org/383113002/diff/20001/chrome/browser/backgroun...
chrome/browser/background/background_application_list_model_unittest.cc:372:
service->PromoteEphemeralApp(ephemeral.get(), false /*from sync*/);
Ideally, we'd also have a BackgroundModeManagerTest for this case (bubble is
displayed when ephemeral app is promoted). Can you add this?

[tmdiep, 2014-07-25 05:26:07]

Thanks for the review. I've added a unit test for the bubble.

On 2014/07/23 13:34:32, Andrew T Wilson wrote:
> LGTM if you add the test requested.
> 
>
https://codereview.chromium.org/383113002/diff/20001/chrome/browser/backgroun...
> File chrome/browser/background/background_application_list_model_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/383113002/diff/20001/chrome/browser/backgroun...
> chrome/browser/background/background_application_list_model_unittest.cc:372:
> service->PromoteEphemeralApp(ephemeral.get(), false /*from sync*/);
> Ideally, we'd also have a BackgroundModeManagerTest for this case (bubble is
> displayed when ephemeral app is promoted). Can you add this?

[tmdiep, 2014-07-25 06:26:26]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-07-25 06:27:18]

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

[commit-bot: I haz the power, 2014-07-25 08:16:54]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[commit-bot: I haz the power, 2014-07-25 09:07:32]

Change committed as 285539