1382637 - Audio/video does not stop recording once firefox is closed

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Audio/Video, defect, P2)

Description

[Bogdan Surd, QA [:bsurd, NI]]

Device:  
 - Nexus 5 (Android 6.0.1);

Steps to reproduce:
 1. Go to https://opentokrtc.com and create a meeting;
 2. With another device or from the computer join the same meeting;
 3. Open android app view menu and close the app.

Expected result:
 As soon as the app is closed audio/video recording should stop.

Actual result:
 Audio/video recording is not stopped after Fennec is closed.

Notes:
 To fix this issue the user has to re-open FF and close it again.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

How do you know the recording hasn't stopped?

Comment 2

[Bogdan Surd, QA [:bsurd, NI]]

Hello James,

I've created a session on my mobile device and connected to the computer, after closing the app on the mobile device I could still see/hear the stream from the device even tho it was closed.

Comment 3

[Bogdan Surd, QA [:bsurd, NI]]

Recording with the issue: http://recordit.co/RoDt92iFIk

On the left side is Firefox on desktop and on the right a recording from the Nexus 5 (Android 6.0.1).

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Apparently swiping the app away did not kill Firefox. Maybe that's intentional by Android if the app has the camera open? Or maybe Firefox is somehow screwing it up? Dylan can you investigate a bit?

Comment 5

[Dylan Roeh (:droeh) (he/him)]

So the culprit here is that Android is keeping us around as a cached process -- kill the cached process and the video/audio stops recording. Unfortunately there doesn't appear to be a way to tell Android not to cache the process when we swipe to kill, and more forceful approaches (eg killing the process in BrowserApp.onDestroy()) cause other, worse problems.

I'm trying to figure out if there's something else we can do here, but so far I've got nothing.

Comment 6

[Dylan Roeh (:droeh) (he/him)]

Attachment: Zombify tabs in BrowserApp.onDestroy()

This just zombifies all tabs when BrowserApp.onDestroy() is called. It stops video/audio from recording, but we still end up with a dead tab the next time we open Fennec. Unfortunately this is probably the best we can do at the moment.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ea03fc7dc4b
Zombify all tabs in BrowserApp.onDestroy() to stop video/audio recording when Fennec is swipe-to-killed. r=snorp

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/8ea03fc7dc4b

Comment 9

[Aki Sasaki (not active)]

https://hg.mozilla.org/projects/date/rev/8ea03fc7dc4bbe6c3af73d83f149c293df7f264c
Bug 1382637 - Zombify all tabs in BrowserApp.onDestroy() to stop video/audio recording when Fennec is swipe-to-killed. r=snorp

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 11

[Dylan Roeh (:droeh) (he/him)]

Unfortunately this definitely can't go to beta as-is, it looks like there's some fallout: bug 1389054. When I have a patch that addresses that issue I'll nominate both for beta.

Comment 12

[Dylan Roeh (:droeh) (he/him)]

Reopening this as I ended up backing out the patch due to bug 1389054. I'll keep exploring options here, but unfortunately Android's swipe-to-kill behavior isn't very conducive to what we want to do here.

Comment 17

[Brian Peiris]

I think this is particularly problematic because when you swipe the app away from the "recent applications" menu, the usual "Camera an microphone are on" notification disappears and it never comes back.

Here are my steps to repro on a Moto G5 using Firefox Nightly 64:

1. Open Nightly on Android and visit https://appr.tc
2. Create and join a new room
3. Open Firefox on desktop and join the same room
4. Video and audio streaming begins as expected.
5. Press the "recent applications" button on the phone
6. Swipe Nightly away.

At this point the notification informing you that "Camera and microphone are on" disappears, but Nightly is still streaming your video and audio to the desktop.

7. Re-open Nightly on the phone.

Now Nightly is restored with the tab still open, except the tab is blank and there is still no indicator notification but the tab is still streaming audio and video to desktop.

8. Tap on the tab selector button in Nightly.
9. Close the AppRTC tab.

Now the tab is supposedly closed, but Nightly is _still_ streaming video and audio to the desktop with no indicator notification in the Android notification tray.

10. Lastly, open the recent applications menu and swipe Nightly away.

Nightly will now finally stop streaming audio and video to the desktop.

Comment 18

[Brian Peiris]

Forgot to mention my Moto G5 was running Android 7.0

Comment 19

[Nils Ohlmeier [:drno]]

(In reply to Brian Peiris from comment #17)
> 6. Swipe Nightly away.
> 
> At this point the notification informing you that "Camera and microphone are
> on" disappears, but Nightly is still streaming your video and audio to the
> desktop.

I'm not familiar enough with the Android world to exactly know what the expectation is from an app which got swiped away. If that is being used by some people to switch between apps one could argue that the audio/video streaming should continue. If that is the case then the cam/mic notifications should NOT disappear to make it clear to the user.
 
> 7. Re-open Nightly on the phone.
> 
> Now Nightly is restored with the tab still open, except the tab is blank and
> there is still no indicator notification but the tab is still streaming
> audio and video to desktop.
> 
> 8. Tap on the tab selector button in Nightly.
> 9. Close the AppRTC tab.
> 
> Now the tab is supposedly closed, but Nightly is _still_ streaming video and
> audio to the desktop with no indicator notification in the Android
> notification tray.

So here we should stop sending and release cam/mic. What is different on Android that closing the tab doesn't destroy the PeerConnections and release the cam/mic.

Comment 20

[Brian Peiris]

Yeah, fair point about the user expectations. 

I agree that all of these concerns can be allayed if we can continue displaying the indicator notification in all scenarios where your video and audio is still being streamed across the network.

Comment 22

[Chris Peterson [:cpeterson]]

Dylan, is this video recording bug still an issue in recent Fennec versions? Does it also affect GeckoView?

Comment 23

[Dylan Roeh (:droeh) (he/him)]

(In reply to Chris Peterson [:cpeterson] from comment #22)
> Dylan, is this video recording bug still an issue in recent Fennec versions?
> Does it also affect GeckoView?

It still effected Fennec last time I checked, but I tested this in GV some months ago and couldn't reproduce, so we should be fine there.

Comment 24

[Chris Peterson [:cpeterson]]

(In reply to Dylan Roeh (:droeh) from comment #23)
> (In reply to Chris Peterson [:cpeterson] from comment #22)
> > Dylan, is this video recording bug still an issue in recent Fennec versions?
> > Does it also affect GeckoView?
> 
> It still effected Fennec last time I checked, but I tested this in GV some
> months ago and couldn't reproduce, so we should be fine there.

OK. gv64=unaffected

Comment 25

[Chris Peterson [:cpeterson]]

Dylan, can you reproduce this issue in GV example or the Reference Browser?

Comment 26

[Dylan Roeh (:droeh) (he/him)]

(In reply to Chris Peterson [:cpeterson] from comment #25)

Dylan, can you reproduce this issue in GV example or the Reference Browser?

The test site was complaining about bandwidth issues and only audio was working, but that disconnected correctly when I swipe-to-kill GVE. I think this is Fennec-specific.

Comment 27

[Chris Peterson [:cpeterson]]

(In reply to Dylan Roeh (:droeh) from comment #26)

The test site was complaining about bandwidth issues and only audio was working, but that disconnected correctly when I swipe-to-kill GVE. I think this is Fennec-specific.

Thanks. I'll remove the [geckoview] whiteboard label.

Comment 28

[Jan Henning [:JanH]]

I think this is the same scenario as bug 1494748, i.e. swiping doesn't actually kill the app process if there's a service running, if some other activity belonging to the app is in the task switcher, etc.

As such, in principle the same could also happen to pure GeckoView apps. The GVE presumably isn't affected because it only has a single activity in the first place and doesn't launch any services or anything else fancy, anyway.
But if that is indeed the case, I'm not sure either whether there's an appropriate fix at the GeckoView level, or whether the embedding app needs to take care of this - I guess we should wait and see if any of our more complex GeckoView embedders run into similar issues.

As for Fennec, the problem with the original patch (bug 1389054) was almost certainly an earlier manifestation of bug 1494748. As such and with that bug finally fixed for good, it might be worth revisiting that approach again.

Comment 29

[Dylan Roeh (:droeh) (he/him)]

(In reply to Jan Henning [:JanH] from comment #28)

I think this is the same scenario as bug 1494748, i.e. swiping doesn't actually kill the app process if there's a service running, if some other activity belonging to the app is in the task switcher, etc.

Agreed.

As such, in principle the same could also happen to pure GeckoView apps. The GVE presumably isn't affected because it only has a single activity in the first place and doesn't launch any services or anything else fancy, anyway.
But if that is indeed the case, I'm not sure either whether there's an appropriate fix at the GeckoView level, or whether the embedding app needs to take care of this - I guess we should wait and see if any of our more complex GeckoView embedders run into similar issues.

Yeah, when I said Fennec-specific I should really say that it's something that isn't exactly a GV bug, or at least we don't think we have a reasonable approach to fix this in GV itself (after multiple conversations with snorp).

As for Fennec, the problem with the original patch (bug 1389054) was almost certainly an earlier manifestation of bug 1494748. As such and with that bug finally fixed for good, it might be worth revisiting that approach again.

Good point! I'll unbitrot that after the weekend and check it out. I think it still has some shortcomings (particularly for people running with "Don't Keep Activities" enabled), but it's certainly worth reconsidering.

Comment 30

[Dylan Roeh (:droeh) (he/him)]

Attachment: Zombify tabs in onDestroy()

As Jan suggested, his patch for bug 1494748 makes my original patch for this bug viable. Tested on opentokrtc.com; killing Fennec stops audio/video recording as desired and reopening Fennec works correctly.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03609832ffdc
Zombify tabs in onDestroy to ensure video/audio recording ends when Fennec is swipe-to-killed. r=snorp

Comment 32

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/03609832ffdc

Comment 33

[miralobontiu]

Hi,
Verified as fixed on the latest Nightly 66.0a1 (2019-01-28) and Beta 66.0b3, with HTC 10 (Android 8.0), and Nexus 5 (Android 6.0.1).

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f515f2a68f6f
Backing out patch due to breakage (see: 1532161, 1532157). r=me

Comment 35

[Dylan Roeh (:droeh) (he/him)]

Backed this out because I have too much GeckoView on my plate at the moment to investigate bug 1532161 and bug 1532157. Jan, if you're interested in this or have any ideas about the fallout from my patch, feel free to take it over.

Comment 36

[Jan Henning [:JanH]]

Reason for the NI: The backout needs uplifting to 66, too.

Other than that, the solution to bug 1523157 is quite straightforward and I was planning to fix it in the near future. Bug 1523161 on the other hand could be a bit more involved - I think it might possibly be related to how GeckoView (at least for Fennec) hides the web content until it receives a first-paint notification, and it could be that simultaneously restoring the activity and restoring a tab interferes with that. Maybe I can take a look, but regarding the latter bug I can't promise anything soon, either.

Comment 37

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 9038577 [details] [diff] [review]
Zombify tabs in onDestroy()

Marking original patch obsolete for clarity.

Comment 38

[Dylan Roeh (:droeh) (he/him)]

Attachment: Backout patch for breakage

Backout of original patch.

Comment 39

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 9040743 [details] [diff] [review]
Backout patch for breakage

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1382637

User impact if declined

Some breakage if the user has "Don't keep activities" enabled (see bug 1523161 and bug 1523157)

Is this code covered by automated tests?

No

Has the fix been verified in Nightly?

No

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

Backs out a patch that caused breakage and reverts to original code.

String changes made/needed

Comment 40

[Dylan Roeh (:droeh) (he/him)]

(In reply to Jan Henning [:JanH] from comment #36)

Reason for the NI: The backout needs uplifting to 66, too.

Other than that, the solution to bug 1523157 is quite straightforward and I was planning to fix it in the near future. Bug 1523161 on the other hand could be a bit more involved - I think it might possibly be related to how GeckoView (at least for Fennec) hides the web content until it receives a first-paint notification, and it could be that simultaneously restoring the activity and restoring a tab interferes with that. Maybe I can take a look, but regarding the latter bug I can't promise anything soon, either.

Thanks, I forgot we'd crossed a release boundary there. And yeah, bug 1523161 seems likely to be the more difficult of the two bugs, and unfortunately I think it's bad enough that we can't really land this patch without having a fix for it. Thanks again for all your help!

Comment 41

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9040743 [details] [diff] [review]
Backout patch for breakage

Sounds like backout is needed. OK for beta 6.

Comment 42

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b511d2df882c7fdbd3929608860affb914944245

Comment 43

[Stefan Deiac]

Hello, I tested this issue using steps from comment 0 and comment 17 on Beta 66.0b5 using OnePlus 5T (Android 9) and Google Pixel (Android 9) and is still reproducible.

Comment 44

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.