932816 - Unable to close the app if webrtc is being used

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Adrian Tamas (:AdrianT)]

Aurora 27.0a2 2013-09-29/ Nightly 28.0a1 2013-09-30
Samsung Galaxy Tab 2 (Android 4.1)/ LG Nexus 4 (Android 4.2.2)

Steps to reproduce:
1) Load http://mozilla.github.com/webrtc-landing/gum_test.html and share camera and microphone
2) Minimize the app and close it from the Android app manager
3) Open the app again

Expected results:
The app is closed and when it is reopened loads about:home

Actual results:
The app never closes and it's just maximized. The notification of camera and microphone in use is never removed

Comment 1

[Aaron Train [:aaronmt]]

This is reproducible and is also a leaking the camera again. After attempting to swipe off the browser, my console floods with:

V/WEBRTC-JC(17889): preview frame length 518400 context-2029162496
V/SecCameraCoreManager(  246): __data_cb

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Gian-Carlo - can you look at this?
blassey: doesn't killing from the app manager actually kill?  Is it merely a "please-shut-down" request?  Even if it's just a request, we should exit.  Do we still have a Quit command hidden anywhere we could try while in a gum test?

Comment 3

[Adrian Tamas (:AdrianT)]

Closing the app by force killing it or using the "Quit" add-on releases the camera and microphone and correctly closes the app

Comment 4

[Aaron Train [:aaronmt]]

I noticed in 25, swiping off the browser will remove will the 'Camera is on' notification, in 26 and 27, swiping the browser off will retain the 'Camera is on' notification.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

>close it from the Android app manager

What does this mean exactly?

Comment 6

[Aaron Train [:aaronmt]]

Comment #4, meant 28 and 27. 26 and 25 are unaffected as far as I can tell (I am merely activating the camera on the basic gUM test and then swiping the application off the recent list). In 27 and 28, the notification persists while in 25 and 26, the notification is cleared.

Comment 7

[Gian-Carlo Pascutto [:gcp]]

FWIW the icon also changed from a camera to a Fennec icon at some point, which also seems undesirable to me.

Comment 8

[Aaron Train [:aaronmt]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #7)
> FWIW the icon also changed from a camera to a Fennec icon at some point,
> which also seems undesirable to me.

Filed bug 933275

Comment 9

[Gian-Carlo Pascutto [:gcp]]

The first bad revision is:
changeset:   148539:3181933b1780
user:        Federico Paolinelli <[e-mail]>
date:        Tue Sep 24 13:53:55 2013 -0700
summary:     Bug 815202 - Add Pause and Cancel actions to download notifications. r=wesj

Comment 10

[Gian-Carlo Pascutto [:gcp]]

It looks like that changeset effectively backed out Bug 882136?!

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch

I yanked this because it didn't fix the real problem before. Didn't realize it was still helpful :)

Comment 12

[Brian Nicholson (:bnicholson)]

Comment on attachment 825733 [details] [diff] [review]
Patch

Review of attachment 825733 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, the flow here was really confusing -- short summary back in https://bugzilla.mozilla.org/show_bug.cgi?id=885783#c22. The way things are now, WebRTC notifications act the same as download notifications (which I was actually pushing for in that comment), but they probably shouldn't be as persistent.

Comment 13

[Wesley Johnston (:wesj)]

Shoot. We need to be careful here then. Downloads are also going through this flow now.

Maybe we need to not do this for progress notifications... Or maybe we're better with just a separate flag.

Comment 14

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #13)
> Shoot. We need to be careful here then. Downloads are also going through
> this flow now.

Good point, though this actually may not be a bad thing if it works correctly. The whole reason we added this foreground notification logic to downloads was because we couldn't think of a way to clean up notifications during a kill, and they would get permanently stuck after Fennec died (especially bad for private browsing). Even though it's kind of a hack, we have a way to do that now, so maybe cancelling in-progress downloads and allowing the browser to close is what we want?

On the other hand, this won't work very well with "Don't keep activities" since Gecko stays alive and the download continues, even if we remove the notifications. This is one of the reasons we could really use a native download service -- it would eliminate all of these issues by killing the Gecko-download dependency.

Question about your patch: will clearAll remove only in-progress notifications, or will it also clear finished ones? I think we want to remove only the former, so if clearAll actually clears all, we'll probably need a different fix.

Comment 15

[Wesley Johnston (:wesj)]

clearAll removes anything, including "ongoing" ones like the webrtc notifications or downloads.

Comment 16

[Federico Paolinelli]

Comment on attachment 825733 [details] [diff] [review]
Patch

Review of attachment 825733 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks good. However, as you and bnicholson discussed in the previous comments, if we add this, we will fix the webrtc bug but we will also change the current behaviour for download notifications. 
I would say that what we could do is to add a "never close me" flag to download notifications to re-enable the previous behaviour (downloads will go through this new flow, their notifications won't be killed as before), and then try to fix the well known issues related to ongoing downloads in another bug. Exploiting the side effect we are introducing here feels a bit too hacky.

This is enforced by about what Bryan says about "Don't keep activities". If I understood correctly (which might be far from the case), when the activity gets destroyed, the onDestroy method is called, all the notifications are removed and then the NotificationService calls stopForeground. In any case, that would not result in an immediate shutdown of the service, which might be kept around for a while (and gecko and its downloads with it)

Comment 17

[Federico Paolinelli]

Attachment: bug-932816-fix

Comment 18

[Federico Paolinelli]

Attachment: bug-932816-fix

Just noticed that I left a "FEDE" trace in the previous patch.
Anyway, this one adds a "persistent" flag that avoids download notification to be wiped out when clearAll() is called

Comment 19

[Wesley Johnston (:wesj)]

Comment on attachment 827038 [details] [diff] [review]
bug-932816-fix

Review of attachment 827038 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/NotificationHelper.java
@@ +287,5 @@
> +
> +        boolean persistent = message.optBoolean(PERSISTENT_ATTR);
> +        // We add only not persistent notifications to the list since we want to purge only
> +        // them when geckoapp is destroyed.
> +        if (!persistent && !mShowing.contains(id)) {

Lets add a comment up by mShowing as well, that says that it only contains notifications we want to purge when GeckoApp is destroyed (i.e. non-persistent ones).

Comment 20

[Federico Paolinelli]

Attachment: bug-932816-fix

Renamed mShowing to mClearableNotifications

Comment 21

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/ca25c3516cff

Comment 22

[Carsten Book [:Tomcat]]

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

Comment 23

[Aaron Train [:aaronmt]]

We need this uplifted.

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

Someone please nominate this for uplift

Comment 25

[Wesley Johnston (:wesj)]

Comment on attachment 827633 [details] [diff] [review]
bug-932816-fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 815202
User impact if declined: Have to stop webrtc to kill fennec
Testing completed (on m-c, etc.): landed on mc this week. Tested by QA
Risk to taking this patch (and alternatives if risky): Pretty low risk. Putting back some deleted code. A bit tricky because we also had to support the current path for downloads (Fennec can't be killed while downloads are in progress)
String or IDL/UUID changes made by this patch: None

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Aurora doesn't have Notifications.jsm. This will need a branch-specific patch for uplift.

Comment 27

[Wesley Johnston (:wesj)]

Attachment: Aurora Patch

Updated for Aurora. Seemed pretty straight forward. Still building to test though :)

Comment 28

[Federico Paolinelli]

Comment on attachment 833016 [details] [diff] [review]
Aurora Patch

Looks good.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b5eabc6c76dd

Comment 30

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 27 Beta 1 (2013-12-11)
Device: LG Nexus 4
OS: Android 4.2.2