958709 - swiping away webapp from Recents screen doesn't stop it

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Description

[Myk Melez [:myk] [@mykmelez]]

Swiping away a webapp from the Recents screen in Android doesn't stop it, although it should, since users expect an app to stop when you swipe it away from that screen.

This is particularly noticeable with apps that play music, like many games.  When you swipe the app away, and it continues to run (in the background), the music continues to play.

I'm not sure if this is new behavior introduced by synthetic APKs or existed before that feature was introduced, but either way, we should fix it, so swiping away an app behaves the way users expect.

Comment 1

[Lucas Rocha (:lucasr)]

Myk, maybe this should be triaged by webapps team?

Comment 2

[Myk Melez [:myk] [@mykmelez]]

(In reply to Lucas Rocha (:lucasr) from comment #1)
> Myk, maybe this should be triaged by webapps team?

Yes!  We've been doing ad-hoc triages, and we don't have one coming up yet, but I'll triage this now and then set up a session to look at the other bugs that have been filed since our last triage.

Comment 3

[James Hugman [:jhugman] [@jhugman]]

What happens when an app is swiped away in Recent Apps? http://www.howtogeek.com/169549/what-exactly-happens-when-you-swipe-an-android-app-from-the-recent-apps-list/

So this would be possible for the APK determine it had been swiped away, then communicate with Fennec (intent, perhaps?) which would do something to the gecko process and relevant task/process. 

Using an intent would be the easiest way, but it's a little heavy weight, and registration of the intent would have to be done at app launch, on a per app basis. (mainly because we may have more than one fennec app installed, and it would be weird to put up an activity chooser in response to swiping an application away).

Suggest this is a P2.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Martyn, James, and I discussed this today and agreed to leave it as a P1 for now, given the impact on the user experience (especially when you swipe away an app that plays music, and the app keeps on playing music).  But we'll reconsider if it turns out to be significantly more difficult or scarier than we think.

Comment 5

[James Hugman [:jhugman] [@jhugman]]

Now identified a way of doing this. 

We have to register a BroadcastReceiver in Fennec's AndroidManifest.xml. Registering it dynamically appears unreliable.

The synth APK needs to run a Service whose only function is to listen for onTaskRemoved events. Once one is detected it sends out the intent.

Questions:

 * what intent/filter characteristics are sensible here. None of the standard actions seem appropriate. Suggest action = org.mozilla.webapps.TASK_REMOVED, category = android.intent.category.DEFAULT, extras: packageName. For reference, the launch intent uses ACTION_VIEW with mimetype: application/webapp.
 * this particular design allows any app to send intents to kill any webapp. How should we stop this being abused? I can think of three ways of doing this:

1. Do nothing. The attacker can only transmit a kill command.
2. Use custom permissions. The permission would be declared in Fennec, and asked for by synthetized APKs. I'm not sure what the permission prompt looks like in the absence of Fennec.
3. Transmit a nonce (a random number, used only once) at startActivity(). Use that nonce to validate the kill command.

The same mechanism may end up being used for when we start catching Intents and translating them to WebActivities.

 * The current design flow is likely to be somewhat sucky in the context of multiple runtimes being installed. (the user will have to chose between the runtimes when swiping away the app from the recent apps list).

Comment 6

[Wesley Johnston (:wesj)]

Sounds like a good plan to me.

If I had to vote on the kill fix, I'd say we just use #1. Off the top of my head, force killing one of our apps doesn't seem like something I'm worried about (yet...). If we eventually do fix it, I'd vote for the permission set.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

If I understand this issue correctly, the APK is killed but the Gecko/Runtime process is not. We are looking for a way to communicate to Gecko/Runtime that the app was killed and then we can shutdown the Runtime.

Hopefully we find that the approach is not to complicated, works on most versions of Android and could be reused for other purposes (like WebActivities).

I am also OK with #1 (do nothing) for now.

The UX around having multiple Runtimes (Nightly, Aurora, Beta or Release) installed sounds... odd. Showing an activity chooser when swiping away an app might be confusing, but time will tell.

Comment 8

[James Hugman [:jhugman] [@jhugman]]

mfinkle: yes, exactly right. 

The odd UX around having multiple apps is definitely odd, and is more likely fixed by establishing a return intent/broadcast receiver when at Webapp-install time (so the APK can know which and remember the ComponentName of the runtime).

As per mfinkle and wesj, proceeding with this bug with the "Do nothing" option.

Comment 9

[James Hugman [:jhugman] [@jhugman]]

Attachment: bug-958709-1.patch

Catching intent as proposed. 

Refactored process killing from EventListener.uninstall method.

The intent is being sent by the apk-factory-library in the synth APK. See PR: https://github.com/mozilla/apk-factory-library/pull/2

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8393848 [details] [diff] [review]
bug-958709-1.patch

hg add TaskKiller.java ?

Comment 11

[James Hugman [:jhugman] [@jhugman]]

Attachment: Added TaskKiller for the win.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8393864 [details] [diff] [review]
Added TaskKiller for the win.

>diff --git a/mobile/android/base/webapp/TaskKiller.java b/mobile/android/base/webapp/TaskKiller.java

>+        } else {
>+            Log.w(LOGTAG, "Asked to kill " + packageName + " but this runtime (" + context.getPackageName() + ") doesn't know about it.");

This might be a little "leaky" but I don't think the package name is an issue. Just wanted to point out that we do want to minimize leaking too much info to logcat.

Seems OK to me. Still think Martyn should have a look though.

Comment 13

[Martyn Haigh (:mhaigh)]

Looks good - suggest that we wrap that warning in a #ifdef

Comment 14

[Myk Melez [:myk] [@mykmelez]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7015e01d136

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b7015e01d136

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

Another option that might work is to use a bound service [1]. Basically, the runtime would bind to a service inside the synth apk [2]. The runtime can then detect when the binding is disconnected (i.e. the apk is killed) through the ServiceConnection interface [3] or IBinder interface [4]. I think this method is more secure because it doesn't use a callback intent. As a bonus, it also gives us a secure communication channel between the runtime and the service running in the apk (for WebActivities and such).

[1] http://developer.android.com/guide/components/bound-services.html
[2] http://developer.android.com/guide/components/bound-services.html#Binding
[3] http://developer.android.com/reference/android/content/ServiceConnection.html#onServiceDisconnected%28android.content.ComponentName%29
[4] http://developer.android.com/reference/android/os/IBinder.html#linkToDeath%28android.os.IBinder.DeathRecipient,%20int%29

Comment 17

[James Hugman [:jhugman] [@jhugman]]

jchen: This is an awesome idea! I wish I'd thought of it!

> As a bonus, it also gives us a secure communication channel between the runtime 
> and the service running in the apk (for WebActivities and such).
It would be perfect for the job at hand. 

For WebActivities: as a secondary channel perhaps, but as a primary channel will need to use straight intent + activity (service + service worker, yes please). WebActivities will more than likely need to be able to wake/launch the webapp (therefore Gecko). Binding to the service has a dependency on the runtime already running.

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to James Hugman [:jhugman] [@jhugman] from comment #17)
> jchen: This is an awesome idea! I wish I'd thought of it!

Thanks! :)

> For WebActivities: as a secondary channel perhaps, but as a primary channel
> will need to use straight intent + activity (service + service worker, yes
> please). WebActivities will more than likely need to be able to wake/launch
> the webapp (therefore Gecko). Binding to the service has a dependency on the
> runtime already running.

Ah that's true. I guess it can be used for any continued communication after the intent has already been sent.

Comment 19

[James Hugman [:jhugman] [@jhugman]]

Attachment: Added subject line suitable for uplift.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Missing functionality/leaky abstraction in bug #895599 	 
User impact if declined: 
This patch replaces a very non-obvious workaround that would need to be publicised by developer evangelism and documentation.
Testing completed (on m-c, etc.): 
Landed on m-c two days ago.
Risk to taking this patch (and alternatives if risky): 
None. 
String or IDL/UUID changes made by this patch:

Comment 20

[Aaron Train [:aaronmt]]

This does not work work for me on trunk (Nightly 03/27), Nexus 5 (Android 4.4.2).

Start an app: (adb shell ps | grep mozilla):

u0_a331   9020  182   1160304 114564 ffffffff 00000000 S org.mozilla.fennec:org.mozilla.fennec.Webapp0

Swipe away the running app, re-run ps: 

u0_a331   9020  182   1147444 152840 ffffffff 00000000 S org.mozilla.fennec:org.mozilla.fennec.Webapp0

Comment 21

[Myk Melez [:myk] [@mykmelez]]

(In reply to Aaron Train [:aaronmt] from comment #20)
> This does not work work for me on trunk (Nightly 03/27), Nexus 5 (Android
> 4.4.2).

The change that landed here is to the Fennec code.  We still need to land PR #2 <https://github.com/mozilla/apk-factory-library/pull/2> in the apk-factory-library repository and regenerate APKs for this to work.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7a26beae0e47
https://hg.mozilla.org/releases/mozilla-beta/rev/408d8017ce4b