Audio/video does not stop recording once firefox is closed

REOPENED
Unassigned

Status

()

defect
P2
normal
Rank:
12
REOPENED
2 years ago
2 months ago

People

(Reporter: bsurd, Unassigned)

Tracking

55 Branch
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec?, geckoview64 unaffected, geckoview65 unaffected, geckoview66 wontfix, firefox54 unaffected, firefox55 wontfix, firefox56 wontfix, firefox57 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 wontfix, firefox67 affected)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

2 years ago
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.
How do you know the recording hasn't stopped?
Flags: needinfo?(bogdan.surd)
Reporter

Comment 2

2 years ago
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.
Flags: needinfo?(bogdan.surd)
Reporter

Comment 3

2 years ago
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).
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?
Flags: needinfo?(droeh)
tracking-fennec: ? → +
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.
Flags: needinfo?(droeh)
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.
Assignee: nobody → droeh
Attachment #8894563 - Flags: review?(snorp)
Attachment #8894563 - Flags: review?(snorp) → review+

Comment 7

2 years ago
Pushed by droeh@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8ea03fc7dc4b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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
Please nominate this for Beta approval when you get a chance.
Flags: needinfo?(droeh)
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.
Flags: needinfo?(droeh)
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 57 → ---
Duplicate of this bug: 1425380
Duplicate of this bug: 1426693
Duplicate of this bug: 1436335
Duplicate of this bug: 1437874

Comment 17

8 months ago
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

8 months ago
Forgot to mention my Moto G5 was running Android 7.0
(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

8 months ago
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.
Assignee: droeh → nobody
Assignee: nobody → droeh
QA Contact: jyavenard
Assignee: droeh → nobody
Assignee: nobody → droeh
QA Contact: jyavenard
Rank: 12
Priority: -- → P2
Duplicate of this bug: 1492261
Dylan, is this video recording bug still an issue in recent Fennec versions? Does it also affect GeckoView?
Flags: needinfo?(droeh)
(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.
Flags: needinfo?(droeh)
(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

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

Flags: needinfo?(droeh)

(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.

Flags: needinfo?(droeh)

(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.

Whiteboard: [geckoview]

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.

(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.

Posted patch Zombify tabs in onDestroy() (obsolete) — Splinter Review

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.

Attachment #8894563 - Attachment is obsolete: true
Attachment #9038577 - Flags: review?(snorp)
Attachment #9038577 - Flags: review?(snorp) → review+

Comment 31

4 months ago
Pushed by droeh@mozilla.com:
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

4 months ago
bugherder
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66

Updated

4 months ago
Depends on: 1523157

Updated

4 months ago
Depends on: 1523161

Comment 33

4 months ago

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).

Status: RESOLVED → VERIFIED

Comment 34

4 months ago
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f515f2a68f6f
Backing out patch due to breakage (see: 1532161, 1532157). r=me

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.

Assignee: droeh → nobody

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.

Status: VERIFIED → REOPENED
Flags: needinfo?(droeh)
Hardware: ARM → All
Resolution: FIXED → ---
Comment on attachment 9038577 [details] [diff] [review]
Zombify tabs in onDestroy()

Marking original patch obsolete for clarity.
Attachment #9038577 - Attachment is obsolete: true

Backout of original patch.

Attachment #9040743 - Flags: review+

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

Flags: needinfo?(droeh)
Attachment #9040743 - Flags: approval-mozilla-beta?

(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 on attachment 9040743 [details] [diff] [review]
Backout patch for breakage

Sounds like backout is needed. OK for beta 6.
Attachment #9040743 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 43

4 months ago

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.

Target Milestone: Firefox 66 → ---
See Also: → 1506957
You need to log in before you can comment on or make changes to this bug.