Closed Bug 958709 Opened 6 years ago Closed 6 years ago

swiping away webapp from Recents screen doesn't stop it

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 31
Tracking Status
firefox29 --- fixed
firefox30 --- fixed
firefox31 --- fixed
fennec - ---

People

(Reporter: myk, Assigned: jhugman)

Details

(Whiteboard: [WebRuntime])

Attachments

(2 files, 1 obsolete file)

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.
Component: Webapp Runtime → Web Apps
Product: Firefox → Firefox for Android
QA Contact: aaron.train
tracking-fennec: --- → ?
tracking-fennec: ? → -
Myk, maybe this should be triaged by webapps team?
Flags: needinfo?(myk)
(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.
Flags: needinfo?(myk)
Priority: -- → P1
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.
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.
Assignee: nobody → jhugman
Whiteboard: [WebRuntime]
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).
Flags: needinfo?(wjohnston)
Flags: needinfo?(mhaigh)
Flags: needinfo?(mark.finkle)
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.
Flags: needinfo?(wjohnston)
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.
Flags: needinfo?(mark.finkle)
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.
Attached patch bug-958709-1.patch (obsolete) — Splinter Review
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
Attachment #8393848 - Flags: review?(mhaigh)
Attachment #8393848 - Flags: review?(mark.finkle)
Comment on attachment 8393848 [details] [diff] [review]
bug-958709-1.patch

hg add TaskKiller.java ?
Attachment #8393848 - Attachment is obsolete: true
Attachment #8393848 - Flags: review?(mhaigh)
Attachment #8393848 - Flags: review?(mark.finkle)
Attachment #8393864 - Flags: review?(mhaigh)
Attachment #8393864 - Flags: review?(mark.finkle)
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.
Attachment #8393864 - Flags: review?(mark.finkle) → review+
Looks good - suggest that we wrap that warning in a #ifdef
Flags: needinfo?(mhaigh)
Attachment #8393864 - Flags: review?(mhaigh) → review+
https://hg.mozilla.org/mozilla-central/rev/b7015e01d136
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
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
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.
(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.
[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:
Attachment #8397821 - Flags: approval-mozilla-beta?
Attachment #8397821 - Flags: approval-mozilla-aurora?
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Attachment #8397821 - Flags: approval-mozilla-beta?
Attachment #8397821 - Flags: approval-mozilla-beta+
Attachment #8397821 - Flags: approval-mozilla-aurora?
Attachment #8397821 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 989095
You need to log in before you can comment on or make changes to this bug.